Repository navigation
fix(install): reject unsupported Android portable installs - #2245
Conversation
Signed-off-by: lorenzozanee <wyz0707@proton.me>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you — this is the right fix and the right size. On Termux the installer currently downloads the portable asset, which then dies with "Bad system call" — it is statically linked glibc, and it makes system calls Android's seccomp filter rejects; stopping before the download, with a message that says why, is exactly what should happen. Detecting on three independent signals (uname -o, TERMUX_VERSION, the $PREFIX path) is thorough, guarding it behind OS = linux keeps every other platform untouched, and the test is nicely built — stubbing curl to exit 99 proves "rejected before download" rather than merely "rejected".
Three small things, and then I would like to merge it:
1. The test will never run as it stands. scripts/test.sh does not glob the tests/ directory — it lists each shell contract test explicitly (bash "$ROOT/tests/test_build_dir_safety.sh" and so on, around lines 200–270), and every one of them is named test_*.sh with underscores. tests/test-install-android-guard.sh is neither listed nor named to the convention, so no lane would ever execute it, and a guard nobody runs protects nothing. Please rename it to tests/test_install_android_guard.sh and add one line for it next to the other contract tests in scripts/test.sh. (It would be the first installer test in that list — a good precedent.)
2. Fixes #1202 → Refs #1202, please. #1202 is much wider than this: it also covers the gaps in building from source under Termux and an ENOENT on the temporary database that is still unexplained. Merging with Fixes would auto-close all of that. This PR resolves the first failure mode in that issue — cleanly — and should say so without closing the rest.
3. One sentence in the error message promises slightly more than we can. "Build from source with scripts/build.sh in Termux" reads as a supported path; the reporter on #1202 got it mostly working, but with open problems, and Termux support is an undecided question here. Something like
error: Android/Termux cannot run the Linux portable binary (static glibc; Android kills it with "Bad system call").
Building from source in Termux may work but is not a supported platform yet - see issue #1202.
tells the user the truth and gives them the thread where the workarounds are collected. Your test greps for Android/Termux, so it keeps passing.
Two things that are ours, not yours:
- The red
test-msanyou saw earlier wasdaemon_bootstrap_fails_fast_when_daemon_dies_at_publication— a daemon test that has nothing to do withinstall.sh. I re-ran it on the identical commit and it passed, which makes it a nondeterministic test on our side; it is recorded as such and will not be held against this PR. lint / lintis currently red onmainitself for an unrelated reason, and a fix is in flight (#2257). If you rebase after that lands you will see a clean run.
A note on process, since you are new here: force-pushing your own PR branch is completely fine, and keep the Signed-off-by on any amended commit. Thanks again — small, careful fixes to the installer are some of the most valuable contributions this project gets, because that script is the first thing every new user runs.
… its verdict daemon_bootstrap_fails_fast_when_daemon_dies_at_publication went red on the MSan lane of three PRs that touch no daemon code at all (DeusData#2245, DeusData#2158, DeusData#2140) with tests/test_daemon_bootstrap.c:1116: ASSERT(daemon_named_cause) and green on a rerun of the identical commit. Cause, attributed 2026-09-20. The test forks a real daemon host whose record publication fails with ENOSPC, and asserts that the client names that cause instead of waiting out its 30 s startup deadline. But before it listens, the host SHA-256s its own executable image, and the test runner is ~450 MB: ~3 s natively, 20-32 s under MSan (48 CI samples, median 25.8 s). The spawn override returned as soon as it had forked, so the client's 30 s clock raced the host's hash. On a slow run the deadline fired first, the reaper SIGKILLed a host that had not failed yet, and the log had no listen_failed line to find. Nothing was wrong with the product: with a 28 s artificial delay the client does learn the recorded ENOSPC cause. The spawn override now reaps the forked host before it returns. The host writes its start-failure record and only then releases its lifetime reservation and exits, so "record on disk, reservation released" is a stable state pinned by construction when the client takes its first look -- and that first look is guaranteed: the client's post-spawn wait loop runs at least once even past the deadline, and its first failure check after a spawn is unthrottled. How long the host takes no longer matters. No production code changes; the assertions are unchanged. A regression to the pre-DeusData#1828 behaviour (a client that ignores the record) still fails the unchanged assertions: its message no longer names the failure, the errno and the path. Proof, one build with temporary env-switched instrumentation that is not part of this commit (a sleep in the forked host before it starts, and a switch back to the old fire-and-forget spawn): spawn forked host delayed daemon_bootstrap suite synchronous no 28 passed old (async) 31 s 27 passed, 1 failed: ASSERT(log_read) synchronous 31 s 28 passed The old spawn fails under the delay one assertion earlier than CI did (log_read, not daemon_named_cause): the injected sleep sits before the host opens its log, whereas in CI the host was killed mid-hash with the log already open. Same mechanism -- the client's deadline beating the host -- and the 2026-09-20 attribution reproduced the CI message itself with a delay placed after the log is opened. The final tree, without the instrumentation: daemon_bootstrap 28 passed, three runs in a row. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
… its verdict daemon_bootstrap_fails_fast_when_daemon_dies_at_publication went red on the MSan lane of three PRs that touch no daemon code at all (DeusData#2245, DeusData#2158, DeusData#2140) with tests/test_daemon_bootstrap.c:1116: ASSERT(daemon_named_cause) and green on a rerun of the identical commit. Cause, attributed 2026-09-20. The test forks a real daemon host whose record publication fails with ENOSPC, and asserts that the client names that cause instead of waiting out its 30 s startup deadline. But before it listens, the host SHA-256s its own executable image, and the test runner is ~450 MB: ~3 s natively, 20-32 s under MSan (48 CI samples, median 25.8 s). The spawn override returned as soon as it had forked, so the client's 30 s clock raced the host's hash. On a slow run the deadline fired first, the reaper SIGKILLed a host that had not failed yet, and the log had no listen_failed line to find. Nothing was wrong with the product: with a 28 s artificial delay the client does learn the recorded ENOSPC cause. The spawn override now reaps the forked host before it returns. The host writes its start-failure record and only then releases its lifetime reservation and exits, so "record on disk, reservation released" is a stable state pinned by construction when the client takes its first look -- and that first look is guaranteed: the client's post-spawn wait loop runs at least once even past the deadline, and its first failure check after a spawn is unthrottled. How long the host takes no longer matters. No production code changes; the assertions are unchanged. A regression to the pre-DeusData#1828 behaviour (a client that ignores the record) still fails the unchanged assertions: its message no longer names the failure, the errno and the path. Proof, one build with temporary env-switched instrumentation that is not part of this commit (a sleep in the forked host before it starts, and a switch back to the old fire-and-forget spawn): spawn forked host delayed daemon_bootstrap suite synchronous no 28 passed old (async) 31 s 27 passed, 1 failed: ASSERT(log_read) synchronous 31 s 28 passed The old spawn fails under the delay one assertion earlier than CI did (log_read, not daemon_named_cause): the injected sleep sits before the host opens its log, whereas in CI the host was killed mid-hash with the log already open. Same mechanism -- the client's deadline beating the host -- and the 2026-09-20 attribution reproduced the CI message itself with a delay placed after the log is opened. The final tree, without the instrumentation: daemon_bootstrap 28 passed, three runs in a row. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
… its verdict daemon_bootstrap_fails_fast_when_daemon_dies_at_publication went red on the MSan lane of three PRs that touch no daemon code at all (DeusData#2245, DeusData#2158, DeusData#2140) with tests/test_daemon_bootstrap.c:1116: ASSERT(daemon_named_cause) and green on a rerun of the identical commit. Cause, attributed 2026-09-20. The test forks a real daemon host whose record publication fails with ENOSPC, and asserts that the client names that cause instead of waiting out its 30 s startup deadline. But before it listens, the host SHA-256s its own executable image, and the test runner is ~450 MB: ~3 s natively, 20-32 s under MSan (48 CI samples, median 25.8 s). The spawn override returned as soon as it had forked, so the client's 30 s clock raced the host's hash. On a slow run the deadline fired first, the reaper SIGKILLed a host that had not failed yet, and the log had no listen_failed line to find. Nothing was wrong with the product: with a 28 s artificial delay the client does learn the recorded ENOSPC cause. The spawn override now reaps the forked host before it returns. The host writes its start-failure record and only then releases its lifetime reservation and exits, so "record on disk, reservation released" is a stable state pinned by construction when the client takes its first look -- and that first look is guaranteed: the client's post-spawn wait loop runs at least once even past the deadline, and its first failure check after a spawn is unthrottled. How long the host takes no longer matters. No production code changes; the assertions are unchanged. A regression to the pre-DeusData#1828 behaviour (a client that ignores the record) still fails the unchanged assertions: its message no longer names the failure, the errno and the path. Proof, one build with temporary env-switched instrumentation that is not part of this commit (a sleep in the forked host before it starts, and a switch back to the old fire-and-forget spawn): spawn forked host delayed daemon_bootstrap suite synchronous no 28 passed old (async) 31 s 27 passed, 1 failed: ASSERT(log_read) synchronous 31 s 28 passed The old spawn fails under the delay one assertion earlier than CI did (log_read, not daemon_named_cause): the injected sleep sits before the host opens its log, whereas in CI the host was killed mid-hash with the log already open. Same mechanism -- the client's deadline beating the host -- and the 2026-09-20 attribution reproduced the CI message itself with a delay placed after the log is opened. The final tree, without the instrumentation: daemon_bootstrap 28 passed, three runs in a row. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
DeusData
added
bug
|
Thanks again for the focused Android guard. Following up on the existing review: the test needs registration in the shell runner, the issue link should avoid auto-closing the broader #1202, and the error text should describe Termux source builds as unsupported. Those are the outstanding requested changes in this thread. |
|
Thanks again for the Android install guard. The recorded next revision still needs shell-runner registration, a non-closing link to the broader #1202, and error wording that calls Termux source builds unsupported. Please let us know when that is ready. |
Every shell contract test under tests/ is named test_*.sh with underscores. A pure rename, so history and blame follow the file. Refs DeusData#1202 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
scripts/test.sh lists every shell contract test explicitly and does not glob tests/, so an unlisted test never ran on any leg. Register tests/test_install_android_guard.sh as Step 0x2, right after Step 0x, in the same two-line shape as its neighbours. Give the two PATH stubs (uname, curl) the same shebang every other stub under tests/ uses, so they are proper executables on every leg instead of relying on the shell fallback for files without one. Refs DeusData#1202 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The guard told users to build from source with scripts/build.sh in Termux, which reads as a supported path. Source builds under Termux still have open problems tracked in DeusData#1202, so the message now says why the portable binary cannot run there and that a source build may work but is not supported yet, pointing at DeusData#1202. The test still matches Android/Termux in the new message. Refs DeusData#1202 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Brings the branch up to current main (it was 2061 commits behind) so the new Step 0x2 runs inside the current scripts/test.sh. No conflicts. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Point CBM_DOWNLOAD_URL, the documented download-base override in install.sh, at a closed loopback port. If a curl stub were ever missed on some leg, the download would then fail locally instead of fetching a real release. Check that the uname stub is the one PATH resolves before running install.sh. Otherwise the test would quietly exercise the real host instead of an Android one and prove nothing. Refs DeusData#1202 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @lorenzozanee - this is exactly the right fix for the first failure in #1202, and it was right in substance from the start. Stopping before the download, instead of letting Termux users fetch a binary that Android then kills with "Bad system call", is the behaviour we want. Detecting on three independent signals (uname -o, TERMUX_VERSION and the Termux PREFIX path) makes it robust, keeping it behind OS = linux leaves every other platform untouched, and stubbing curl to exit 99 is what lets the test prove "rejected before download" rather than just "rejected". You allowed maintainer edits, so we finished the small points from our review ourselves and you do not need to do anything more. What each commit on top of yours does:
We also changed "Fixes #1202" to "Refs #1202" in the description: #1202 also covers source builds under Termux, which stay open, and this PR cleanly resolves the first failure mode in it. We checked it locally: the test passes, and with the guard disabled it fails at Step 0x2 because the installer reaches the (stubbed) download. If you push to this branch again, please pull first, since it now carries these commits on top of yours. Thanks again - the installer is the first thing every new user runs, so careful fixes like this one matter a lot. |
|
Merged in 9c19b5d. Thank you again, @lorenzozanee! Termux users now get a clear message before anything is downloaded, instead of a binary that Android kills with "Bad system call", and the guard test runs on every leg as Step 0x2. #1202 stays open for source builds under Termux, which this PR cleanly separates out. Much appreciated. |
What does PR do?
On Android/Termux,
install.shpreviously selected and downloaded an incompatible Linux portable binary. Detect Android/Termux, report the supported alternatives, and stop before download. Add a regression test that verifies the downloader is not invoked.Refs #1202
Checklist
git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)