ci(nightly): drop the musl linker override so rustc keeps the link (#683) - #717
ci(nightly): drop the musl linker override so rustc keeps the link (#683)#717logbie wants to merge 1 commit into
Conversation
CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER=musl-gcc hands the musl link to musl-gcc, a spec-file wrapper around the host gcc whose specs select musl's dynamic loader. Under it the build can silently emit a binary with a PT_INTERP segment while still going green, and -C target-feature=+crt-static cannot win the link back. That is exactly the condition #616 was closed to prevent. Rust's x86_64-unknown-linux-musl target ships its own self-contained musl libc and static-links by default, so removing the override moves the build toward rustc's default rather than away from it. CC_ stays: cc-rs genuinely needs it to compile aws-lc-sys for the musl target. The behaviour is currently benign on blacksmith-8vcpu-ubuntu-2404, so this is a latent dependency on an unpinned property of the runner image rather than a live breakage. The "Assert the binaries are statically linked" step (PT_INTERP absence, both binaries) remains the detector and is unchanged. Closes #683
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe nightly Linux musl build continues to use ChangesLinux musl build
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This is a localized nightly CI configuration change with no application-code impact; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # cc-rs needs this so aws-lc-sys' C and assembly compile for the musl | ||
| # target. It is deliberately NOT paired with a matching | ||
| # CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER. That variable would hand | ||
| # the link to musl-gcc - a spec-file wrapper around the host gcc whose | ||
| # specs select musl's DYNAMIC loader - so the build silently emits a | ||
| # binary carrying a PT_INTERP segment, and `-C target-feature=+crt-static` | ||
| # cannot win the link back once musl-gcc is driving it. Rust's | ||
| # x86_64-unknown-linux-musl target ships its own self-contained musl and | ||
| # static-links by default, so the correct move is to leave the link to | ||
| # rustc rather than take it away. Dropping that override is wfl#683; the | ||
| # regression it prevents is wfl#616 reappearing. The `Assert the binaries | ||
| # are statically linked` step below is what proves this held. | ||
| CC_x86_64_unknown_linux_musl: musl-gcc |
There was a problem hiding this comment.
🔍 Removing the linker override changes which compiler drives the link, and the stack-size link-arg must still pass through
With CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER gone, rustc falls back to its default linker driver (cc, i.e. the host gcc on the Blacksmith image) combined with the self-contained musl objects it ships. That path must still accept the -C link-arg=-Wl,-z,stack-size=8388608 rustflag from the cfg(target_os = "linux") block in .cargo/config.toml:10-11, which the musl target matches — the parser depends on that 8 MB stack. Host cc passes -Wl,... through unchanged, so the argument survives, but nothing in CI asserts the resulting stack-size program header, so a silent regression here would only show up as deep-recursion parser failures at runtime rather than as a red build. Worth considering an assertion on the PT_GNU_STACK/stack-size value alongside the existing PT_INTERP check.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # cc-rs needs this so aws-lc-sys' C and assembly compile for the musl | ||
| # target. It is deliberately NOT paired with a matching | ||
| # CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER. That variable would hand | ||
| # the link to musl-gcc - a spec-file wrapper around the host gcc whose | ||
| # specs select musl's DYNAMIC loader - so the build silently emits a | ||
| # binary carrying a PT_INTERP segment, and `-C target-feature=+crt-static` | ||
| # cannot win the link back once musl-gcc is driving it. Rust's | ||
| # x86_64-unknown-linux-musl target ships its own self-contained musl and | ||
| # static-links by default, so the correct move is to leave the link to | ||
| # rustc rather than take it away. Dropping that override is wfl#683; the | ||
| # regression it prevents is wfl#616 reappearing. The `Assert the binaries | ||
| # are statically linked` step below is what proves this held. |
There was a problem hiding this comment.
🔍 No PR-triggered lane exercises this job, so the change is unverified until the next nightly
build-linux exists only in nightly.yml and is gated on check-for-changes.outputs.should_build == 'true', so no pull-request event can run it. The Assert the binaries are statically linked step is therefore the sole detector and it first executes after merge. If the default-linker path fails outright (e.g. host cc cannot find the self-contained musl objects), the nightly build breaks rather than degrading, which is the safer failure mode, but the merge is effectively unvalidated at review time.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Adjusts the Linux musl nightly build workflow to rely on rustc’s default musl linking behavior (static by default) by removing a harmful linker override that can silently produce binaries with a PT_INTERP segment.
Changes:
- Removed
CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER: musl-gccfrom thebuild-linuxjob environment. - Added an explanatory comment documenting why
CC_x86_64_unknown_linux_muslis required (forcc-rs/aws-lc-sys) while the linker override must not be reintroduced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13abffc9a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # rustc rather than take it away. Dropping that override is wfl#683; the | ||
| # regression it prevents is wfl#616 reappearing. The `Assert the binaries | ||
| # are statically linked` step below is what proves this held. |
There was a problem hiding this comment.
The build-linux job was checked and is only exercised by this scheduled/manually dispatched nightly workflow, so the unchanged static-link and Debian smoke checks do not validate this new linker selection until after it reaches main. If rustc's inferred self-contained setup cannot link the musl-gcc-built native archives on the runner, the first full nightly fails after merge; moreover, the commit's stated R0 classification is incorrect because deleting the linker override changes executable build behavior. Add a non-publishing PR or disposable staging lane that builds both binaries through this path and runs the existing PT_INTERP and Debian checks before relying on the default linker.
AGENTS.md reference: AGENTS.md:L129-L137
Useful? React with 👍 / 👎.
|
CI is complete and fully green: 18 substantive checks pass, 2 skipped ( Notably green: The verification limit stated in the description is unchanged and worth repeating before merge: none of these 18 checks exercise One scheduling note: the last three nightlies were designed no-change skips, because Posted by the WFL repo warden (automated maintenance pass). |
|
Stale-PR check, 2026-08-19 warden pass. This PR is now ~48h idle with no blocker on it — 18 substantive checks pass, 2 skipped ( The reason for the nudge rather than silence is that the situation around it has changed. That matters specifically for this PR. The defect it fixes — Next action: maintainer merge. There is no author-side work left. Once merged, Posted by the WFL repo warden (automated triage pass). |
|
Still open and still green — 24h since the last note, and this is the fourth pass flagging it. Rather than repeat yesterday's status, here is the one thing I could check today that nobody had checked yet: whether the three open warden PRs can be merged as a batch. They can. Test-merged locally against
#717 and #718 are the two that share And the combined tree is lint-clean. Current state of all three: each is Why this matters beyond tidiness: the scheduled nightly has now taken five consecutive designed no-change skips (08-16 → 08-20) because No action needed from the author; this is a maintainer merge decision. Deliberately posted once, here, rather than duplicated onto #716 and #718 — the finding is about the queue, not about any single PR. Posted by the WFL repo warden (automated triage pass). |
Fixes #683.
nightly.yml'sbuild-linuxjob sets bothCC_x86_64_unknown_linux_muslandCARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKERtomusl-gcc. The second is unnecessary and actively harmful:musl-gccis a spec-file wrapper around the hostgcc, and its specs select musl's dynamic loader. On an image where that holds, the job silently produces a binary with aPT_INTERPsegment while still going green — the exact condition #616 was closed to prevent — and-C target-feature=+crt-staticcannot win the link back oncemusl-gccis driving it.Rust's
x86_64-unknown-linux-musltarget ships its own self-contained musl libc and static-links by default, so removing the override moves the build toward rustc's default rather than away from it.What changed
One deleted line, plus a comment recording why the pairing must not come back.
TARGET: x86_64-unknown-linux-musl CC_x86_64_unknown_linux_musl: musl-gcc - CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_LINKER: musl-gccCC_is kept deliberately —cc-rsneeds it to compileaws-lc-sys' C and assembly for the musl target, and that is the one dependency in the graph with a musl story to get wrong.Deliberately not included
The issue also floats adding
CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_RUSTFLAGS: "-C target-feature=+crt-static". I left it out. It is redundant with the target default, and it introduces a second rustflags source that has to union correctly with thecfg(target_os = "linux")block in.cargo/config.tomlcarrying the 8 MBstack-sizelink-arg the parser depends on. The issue author verified that union empirically, but it turns a pure deletion into a change with a non-obvious interaction, and the parser's stack budget is not worth risking for redundancy. The comment states the intent instead. Happy to add it if you'd rather have it explicit.Risk and verification
Risk class R0 — CI mechanics, no behavioural change to the language. No
src/change, noTestPrograms/exposure, no version touched. Pertesting.mdthis needs no manufactured failing test: the existing check is the test.Verified locally:
actionlint1.7.7 onnightly.yml: zero findings, identical tomainbefore the change.build-linux.envnow resolves to exactly{TARGET, CC_x86_64_unknown_linux_musl, CARGO_PROFILE_RELEASE_DEBUG}.MUSL_LINKERreference remains anywhere in the tree outside the explanatory comment.What I could not verify, stated plainly:
build-linuxruns only innightly.yml, and there is no PR-triggered musl lane —.github/workflows/verify-linux-musl.ymlis registered but absent from the tree. So PR CI cannot exercise this job, and the authoritative proof is theAssert the binaries are statically linkedstep (PT_INTERPabsence on bothwflandwfl-lsp) on the first full nightly after merge. That step is unchanged and is the correct detector.I did not try to prove it by dispatching
nightly.ymlfrom this branch, because thereleasejob is guarded only byshould_build == 'true'with nogithub.refcondition — it holdscontents: writeand read-modify-writes the nightly release plus the Spaces rolling pointers,SHA256SUMSandstatus.json. A branch dispatch would have published unmerged artifacts over the real nightly. That gap is written up in #683 and deserves its own fix.Since the next nightly is the verification, it is worth a glance at tomorrow's
build-linuxlog after this merges. IfPT_INTERPever does appear, the assertion fails loudly and this is trivially revertible.Opened by the WFL repo warden (automated maintenance pass). Not merged by me — CI and a human decide.
Summary by CodeRabbit