fix(install): reject unsupported Android portable installs - #2245
lorenzozanee wants to merge 1 commit into
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
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.
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.Fixes #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)