fix(rootfs/qemu-static): use direct armhf exec probe instead of broken arch-test gate#9820
fix(rootfs/qemu-static): use direct armhf exec probe instead of broken arch-test gate#9820iav wants to merge 3 commits into
Conversation
iav
requested review from
ColorfulRhino and
schwar3kat
and removed request for
a team
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughProbe for native armhf execution; if absent, prefer a packaged ChangesQEMU ARMhf native capability probe and setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/rootfs/qemu-static.sh (1)
232-233: 💤 Low valueHardcode
qemu-arminstead of inheritingwanted_archfrom caller scope.This function's name pins the target arch (
armhf_target), and the only call site (line 187) enters whenwanted_arch == "arm". Relying on the caller's loop variable here couples the helper to a particular invocation context without any guard — a future caller or refactor could silently produceqemu-with no arch suffix. Using the literal string also matches the heredoc above (line 222) which is already arm-specific.♻️ Suggested change
- run_host_command_logged update-binfmts --import "qemu-${wanted_arch}" - run_host_command_logged update-binfmts --enable "qemu-${wanted_arch}" + run_host_command_logged update-binfmts --import "qemu-arm" + run_host_command_logged update-binfmts --enable "qemu-arm"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/rootfs/qemu-static.sh` around lines 232 - 233, The helper function armhf_target currently calls run_host_command_logged with "qemu-${wanted_arch}", inheriting wanted_arch from caller scope; change these two calls to use the literal "qemu-arm" instead of interpolating wanted_arch so the helper is self-contained and matches the arm-specific heredoc and intent. Update both run_host_command_logged update-binfmts --import and --enable invocations to use "qemu-arm" and ensure no other references inside armhf_target rely on wanted_arch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 232-233: The helper function armhf_target currently calls
run_host_command_logged with "qemu-${wanted_arch}", inheriting wanted_arch from
caller scope; change these two calls to use the literal "qemu-arm" instead of
interpolating wanted_arch so the helper is self-contained and matches the
arm-specific heredoc and intent. Update both run_host_command_logged
update-binfmts --import and --enable invocations to use "qemu-arm" and ensure no
other references inside armhf_target rely on wanted_arch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f4f347d-a3de-47de-91fe-fa6e149fa4d8
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
iav
force-pushed
the
fix/native-armhf-detection
branch
2 times, most recently
from
934a2f0 to
d5b76d4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/rootfs/qemu-static.sh (1)
218-226: 💤 Low valueProbe path is gated solely on the cross-toolchain loader, missing fallback for foreign architecture setups.
The probe at line 222 checks
/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3(installed bygcc-arm-linux-gnueabihf). If this cross-toolchain package is absent—possible in pre-prepared hosts or manual foreign architecture setups—the probe fails silently and falls through to write a hand-rolledqemu-armdescriptor and force emulation, defeating this PR's optimization.In normal builds,
gcc-arm-linux-gnueabihfis installed during host preparation when the target includes armhf, so this is not a typical correctness issue. However, hosts with armhf added as a foreign dpkg architecture (without the cross-toolchain) could use the alternative loader path/lib/arm-linux-gnueabihf/ld-linux-armhf.so.3to detect native capability without additional packages. Consider probing both paths to support this setup, or at minimum document thegcc-arm-linux-gnueabihfdependency at the call site so users debugging slow builds can identify whether they need to install it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/rootfs/qemu-static.sh` around lines 218 - 226, The armhf kernel probe relies only on /usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3 via the armhf_probe check and will miss hosts that have armhf as a foreign dpkg architecture where the loader lives at /lib/arm-linux-gnueabihf/ld-linux-armhf.so.3; update the probe logic used around the armhf_probe variable to check both paths ("/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3" and "/lib/arm-linux-gnueabihf/ld-linux-armhf.so.3") and invoke the same executable --help test for whichever exists, falling back to the existing qemu-arm descriptor logic only if neither path yields an executable; alternatively add a clear comment/log entry (via display_alert) documenting the gcc-arm-linux-gnueabihf dependency if you prefer not to add the second-path probe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 218-226: The armhf kernel probe relies only on
/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3 via the armhf_probe check and
will miss hosts that have armhf as a foreign dpkg architecture where the loader
lives at /lib/arm-linux-gnueabihf/ld-linux-armhf.so.3; update the probe logic
used around the armhf_probe variable to check both paths
("/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3" and
"/lib/arm-linux-gnueabihf/ld-linux-armhf.so.3") and invoke the same executable
--help test for whichever exists, falling back to the existing qemu-arm
descriptor logic only if neither path yields an executable; alternatively add a
clear comment/log entry (via display_alert) documenting the
gcc-arm-linux-gnueabihf dependency if you prefer not to add the second-path
probe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 688ab1df-44ba-4174-9a39-f917a6a0ebf8
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
iav
force-pushed
the
fix/native-armhf-detection
branch
from
d5b76d4 to
4be6288
Compare
|
Re: nitpick "Probe path is gated solely on the cross-toolchain loader, missing fallback for foreign architecture setups." Verified: The "absent" case the nitpick raises applies only when the operator sets |
|
Should we merge this after 05 release? |
igorpecovnik
added
08
|
I think it could be done earlier. |
|
Depends how well is this tested? I want as less surprises as possible when hitting the "build all" button. |
|
Indeed — my testing surface doesn't cover CI or the big builder fleet. Agree on holding the merge until builder hiccups can't threaten a release cut. |
Yep. After release we can afford to have broken CI for days while before its really not needed :) |
iav
force-pushed
the
fix/native-armhf-detection
branch
from
4be6288 to
53cb297
Compare
iav
force-pushed
the
fix/native-armhf-detection
branch
4 times, most recently
from
765d575 to
1aed440
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/functions/rootfs/qemu-static.sh (1)
261-274: 💤 Low valueConsider adding verification after hand-write path.
Unlike the packaged descriptor path (lines 239-244), there's no verification that
qemu-armactually became enabled in the kernel after import/enable. Whileset -ecatches command failures,update-binfmts --enablecould succeed without the kernel entry actually becoming enabled (e.g., if binfmt_misc is in an unexpected state).♻️ Optional: Add verification for consistency
run_host_command_logged update-binfmts --import qemu-arm run_host_command_logged update-binfmts --enable qemu-arm + if [[ ! -e /proc/sys/fs/binfmt_misc/qemu-arm ]] || + [[ "$(head -n1 /proc/sys/fs/binfmt_misc/qemu-arm 2> /dev/null)" != "enabled" ]]; then + exit_with_error "Hand-written qemu-arm descriptor could not be enabled. Check binfmt_misc state." + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/rootfs/qemu-static.sh` around lines 261 - 274, The hand-written qemu-arm descriptor path writes /usr/share/binfmts/qemu-arm and runs update-binfmts --import/--enable but lacks post-enable verification; after run_host_command_logged update-binfmts --enable qemu-arm add a check using update-binfmts --display qemu-arm or by reading /proc/sys/fs/binfmt_misc/qemu-arm (or /proc/sys/fs/binfmt_misc/status) to confirm the entry is present and marked enabled, and if the check fails retry or log an error and exit non-zero so the failure is not silently ignored (reference the created file /usr/share/binfmts/qemu-arm and the commands update-binfmts --import qemu-arm and update-binfmts --enable qemu-arm).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 276-279: The debug block is passing shell operators as literal
args to arch-test (run_host_command_logged arch-test "||" true); update the
if-body so the shell operator applies to the command exit instead of being
passed to arch-test—call run_host_command_logged arch-test and place || true
after that invocation (i.e., run_host_command_logged arch-test || true), keeping
the existing SHOW_DEBUG check and display_alert call unchanged.
---
Nitpick comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 261-274: The hand-written qemu-arm descriptor path writes
/usr/share/binfmts/qemu-arm and runs update-binfmts --import/--enable but lacks
post-enable verification; after run_host_command_logged update-binfmts --enable
qemu-arm add a check using update-binfmts --display qemu-arm or by reading
/proc/sys/fs/binfmt_misc/qemu-arm (or /proc/sys/fs/binfmt_misc/status) to
confirm the entry is present and marked enabled, and if the check fails retry or
log an error and exit non-zero so the failure is not silently ignored (reference
the created file /usr/share/binfmts/qemu-arm and the commands update-binfmts
--import qemu-arm and update-binfmts --enable qemu-arm).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23911c9e-9472-4915-8474-2947dc33f7b5
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
iav
force-pushed
the
fix/native-armhf-detection
branch
from
1aed440 to
1d6d472
Compare
…n arch-test gate `arch-test arm` probes ARMv5 EABI, divergent from kernel COMPAT (armhf EABI v5+), and returns failure on modern aarch64 hosts (Ubuntu Noble 6.8 / Ampere Altra) with a fully working CONFIG_COMPAT path — routing mmdebstrap through qemu-user emulation at ~10× slowdown. Replace with a 4-tier detection: prefer the packaged `/usr/share/binfmts/qemu-arm` descriptor if present, trust an already-enabled kernel registration, otherwise probe COMPAT directly by exec'ing `ld-linux-armhf.so.3`, falling back to the hand-rolled descriptor only when none applies. Assisted-by: Claude:claude-opus-4.7
iav
force-pushed
the
fix/native-armhf-detection
branch
from
42eb08d to
c456ae4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 209-218: In prepare_host_binfmt_qemu_cross_arm64_host_armhf_target
the current ordering runs "${armhf_probe}" --help while qemu-arm may still be
enabled, causing the probe to succeed via qemu and then disable qemu-arm
incorrectly; change the flow so you first disable or mask
/proc/sys/fs/binfmt_misc/qemu-arm (using the same mechanism that writes to
/proc/sys/fs/binfmt_misc/qemu-arm) before invoking armhf_probe, then run
"${armhf_probe}" --help; if the probe fails, restore the previous qemu-arm state
(undo the disable) and continue using qemu; use the existing display_alert calls
(e.g., display_alert "Disabling qemu-arm..." and the "Host kernel can run armhf
natively..." path) and the same return semantics so behavior remains consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 477e0c1c-20b7-4958-805f-61729a15aeb1
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/functions/rootfs/qemu-static.sh (1)
209-218:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDisable
qemu-armbefore probing native armhf.This still probes
ld-linux-armhf.so.3while/proc/sys/fs/binfmt_misc/qemu-armmay be enabled. In that state the probe can succeed via qemu, and then Line 214 disables qemu-arm and returns as if native COMPAT worked. On a no-COMPAT host that turns a working qemu path into a later failure. Save the oldqemu-armstate, disable it before the probe, and restore it if the probe fails; also don’t swallow a failed toggle here.🔧 Minimal fix sketch
+ local qemu_arm_was_enabled=0 + if [[ "${prefer_native}" == "yes" ]] && [[ -x "${armhf_probe}" ]]; then + if [[ -e /proc/sys/fs/binfmt_misc/qemu-arm ]] && + [[ "$(head -n1 /proc/sys/fs/binfmt_misc/qemu-arm 2> /dev/null)" == "enabled" ]]; then + qemu_arm_was_enabled=1 + display_alert "Disabling qemu-arm to probe native COMPAT for armhf" "temporarily bypassing binfmt_misc" "info" + echo 0 > /proc/sys/fs/binfmt_misc/qemu-arm || exit_with_error "Failed to disable qemu-arm before native armhf probe" + fi + + if "${armhf_probe}" --help > /dev/null 2>&1; then + display_alert "Host kernel can run armhf natively (CONFIG_COMPAT)" "no qemu-arm setup needed" "debug" + return 0 + fi + + if (( qemu_arm_was_enabled )); then + echo 1 > /proc/sys/fs/binfmt_misc/qemu-arm || exit_with_error "Failed to restore qemu-arm after native armhf probe" + fi + fi - - if [[ "${prefer_native}" == "yes" ]] && [[ -x "${armhf_probe}" ]] && - "${armhf_probe}" --help > /dev/null 2>&1; then - if [[ -e /proc/sys/fs/binfmt_misc/qemu-arm ]] && - [[ "$(head -n1 /proc/sys/fs/binfmt_misc/qemu-arm 2> /dev/null)" == "enabled" ]]; then - display_alert "Disabling qemu-arm to use native COMPAT for armhf" "≈10× faster than qemu emulation" "info" - echo 0 > /proc/sys/fs/binfmt_misc/qemu-arm 2> /dev/null || true - else - display_alert "Host kernel can run armhf natively (CONFIG_COMPAT)" "no qemu-arm setup needed" "debug" - fi - return 0 - fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/rootfs/qemu-static.sh` around lines 209 - 218, The probe can succeed via qemu-arm currently because you disable qemu-arm only after probing; instead, save the current state of /proc/sys/fs/binfmt_misc/qemu-arm (e.g., read its first line or existence), disable it before running the probe "${armhf_probe}" --help, then if the probe succeeds treat COMPAT as available; if the probe fails restore the saved qemu-arm state (and check the write succeeded) and do not swallow any failure (propagate/log the error instead of silently returning 0). Update the block that references prefer_native, armhf_probe and /proc/sys/fs/binfmt_misc/qemu-arm so toggling qemu-arm is reversible and failures to toggle are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 209-218: The probe can succeed via qemu-arm currently because you
disable qemu-arm only after probing; instead, save the current state of
/proc/sys/fs/binfmt_misc/qemu-arm (e.g., read its first line or existence),
disable it before running the probe "${armhf_probe}" --help, then if the probe
succeeds treat COMPAT as available; if the probe fails restore the saved
qemu-arm state (and check the write succeeded) and do not swallow any failure
(propagate/log the error instead of silently returning 0). Update the block that
references prefer_native, armhf_probe and /proc/sys/fs/binfmt_misc/qemu-arm so
toggling qemu-arm is reversible and failures to toggle are handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c892c189-2506-4ea0-afe1-c284fd476c58
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
iav
force-pushed
the
fix/native-armhf-detection
branch
2 times, most recently
from
1c66025 to
12f0371
Compare
…registration When CONFIG_COMPAT runs armhf binaries natively on aarch64 (≈10× faster than qemu-user emulation), but qemu-arm is registered in kernel binfmt_misc, the previous "trust existing setup" check returned early and the build went through qemu anyway. Empirically: helios4 build 41 min via qemu vs 19 min native COMPAT on droid (RK3399). Reorder probes: 1. Snapshot initial qemu-arm state. 2. COMPAT probe must run with qemu-arm OFF — kernel binfmt_misc routes armhf exec through qemu first when enabled, making the probe a liar about native capability. Temp-disable before probe; on probe failure (no COMPAT, e.g. Apple Silicon under HVF where AArch32@EL0 SIGILL's) restore qemu-arm and fall through to qemu setup. Toggle errors are not swallowed. 3. Trust-existing path validates via arch-test arm — the `enabled` flag alone does not attest that the interpreter actually runs (stale path, removed package); without this, broken registrations passed setup and failed later in chroot. 4. _verify_qemu_arm_executes helper called after each successful import/enable/repair branch, so qemu issues fail fast at host-prepare time instead of mid-rootfs-build. The descriptor itself is left intact — only the active flag is toggled. Gated by ARMBIAN_PREFER_NATIVE_ARMHF=no for explicit opt-out. Behaviour preserved on hosts without COMPAT. Assisted-by: Claude:claude-opus-4.7
…uilds Spell out the practical effect of CONFIG_COMPAT + COMPAT_VDSO + ARM64_32BIT_EL0: an arm64 host running such a kernel can execute armhf userspace natively, so rootfs/chroot steps run native instead of via qemu-user-static (~10× faster). Cross-reference the rootfs probe and the ARMBIAN_PREFER_NATIVE_ARMHF switch, and note the Apple-M-series exception (no AArch32@EL0 in silicon). Assisted-by: Claude:claude-opus-4.7
iav
force-pushed
the
fix/native-armhf-detection
branch
from
752bb11 to
dbd7005
Compare
2 participants
Problem
prepare_host_binfmt_qemu_cross_arm64_host_armhf_target(added in6755e9190, 2024-12-28, for Apple Silicon) gates onarch-test arm. On success → skip qemu-arm import; on failure → write a hand-rolled/usr/share/binfmts/qemu-armmagic with interpreter/usr/bin/qemu-arm-staticand enable it.This is broken on most modern aarch64 hosts:
arch-test armprobes ARMv5 EABI. Current toolchains and the kernel's COMPAT layer target armhf (EABI v5+); the two surfaces diverge.arch-test armreturns failure even withCONFIG_COMPAT=yand a fully-working COMPAT path. The gate then unconditionally routes every aarch64 host through the Apple-Silicon import branch — qemu-arm gets registered, mmdebstrap and chroot apt-get / dpkg --configure / customize_image run through qemu-user-static emulation, ~10× slower than native.update-binfmts --enable qemu-armat function entry could re-enable a pre-existing registration even on hosts the operator deliberately wanted to keep without qemu./usr/share/binfmts/qemu-arm(resolute'sqemu-user-binfmtships/usr/bin/qemu-armwithout-static) → enable fails → armhf builds break on resolute Apple-Silicon-like hosts.Empirically: helios4
BUILD_MINIMAL=yes RELEASE=trixie PREFER_DOCKER=yesfinishes in 1:07 min on Hetzner CAX21 when qemu-arm stays unregistered (mmdebstrap native via binfmt_elf), vs ~10× longer when emulation routes mmdebstrap.Fix
Replace the single
arch-test armgate with a 4-tier detection that picks the safest path on each host:/usr/share/binfmts/qemu-armexists (packaged byqemu-user-binfmton resolute, or by qemu-user-static install):update-binfmts --enable qemu-arm— loads the packaged descriptor into the kernel. Preserves the packaged interpreter path. Hard-error if enable fails (descriptor present but broken — host needs operator attention)./proc/sys/fs/binfmt_misc/qemu-armexists without descriptor: trust ifenabled, hard-error if disabled (no descriptor to re-enable from).exec /usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3 --help(ld-linux-armhf is shipped bygcc-arm-linux-gnueabihf, an armbian aarch64 host build dep). Authoritative test ofCONFIG_COMPATfunctionality. If succeeds → kernel runs armhf natively → no qemu setup needed./usr/share/binfmts/qemu-armmagic with/usr/bin/qemu-arm-static, import, enable — the original behaviour, now only reached when actually needed.Also removed the unconditional
update-binfmts --enable qemu-arm "|| true"at function entry — its job is subsumed by step (1).Empirical validation
opts_n+=("COMPAT" "COMPAT_VDSO" "ARM64_32BIT_EL0")):ld-linux-armhf.so.3execs withExec format error; probe correctly fails → step (4) fires./usr/share/binfmts/qemu-armwith/usr/bin/qemu-arm-static): step (1) early-returns, descriptor md5 unchanged before/after, kernel entry left enabled.Test plan
qemu-user-binfmtpackage — covered by inspection (step 1 enables packaged descriptor). No resolute Apple Silicon hardware available; happy to defer to community verification.Summary by CodeRabbit
Bug Fixes
Refactor
Style