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 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe function probes for native armhf execution, prefers a packaged qemu-arm binfmt descriptor or an enabled kernel binfmt entry, and only writes/imports and enables 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 |
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
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
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? |
|
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 :) |
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.Codex (iav#117 P1 round 1) also flagged: the hand-rolled descriptor overwrites a packaged
/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.Assisted-by: Claude:claude-opus-4.7
Summary by CodeRabbit