build: pin -C metadata so bun's crates keep stable symbol names - #33339
build: pin -C metadata so bun's crates keep stable symbol names#33339robobun wants to merge 5 commits into
Conversation
Cargo folds the -C metadata of every dependency into a unit's own, and rustc hashes that value into the StableCrateId that v0 symbol names carry. A dependency-edge edit anywhere below a crate therefore renames every symbol that crate defines. bun_runtime has direct deps on ~100 workspace crates, so it was renamed by nearly every commit, while leaves like bun_core never moved. Add a RUSTC_WORKSPACE_WRAPPER that replaces cargo's value with one derived only from the unit's identity (package, version, target, crate types, features). Registry crates keep cargo's hash; only workspace members are wrapped. Symbols stay unique because rustc mixes the crate name into StableCrateId independently of -C metadata.
|
Updated 10:22 PM PT - Jul 4th, 2026
✅ @robobun, your commit b2f2d8db7f37fb2bcf11824589916fdb00c33948 passed in 🧪 To try this PR locally: bunx bun-pr 33339That installs a local version of the PR into your bun-33339 --bun |
|
Status: ready for review. Reproduced by re-applying the exact dependency edit that rotated it between the two canary builds (
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a Rust host shim that rewrites ChangesMetadata shim and build wiring
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@scripts/build/rust.ts`:
- Around line 214-234: The `rustc_metadata_shim` rule is running before the
cross-toolchain repair path, so a partially installed pinned toolchain can fail
during the shim build and block `rust_build_cross`. Update the flow so the
`rustup toolchain install --force ...` repair happens before any
`metadataShim`-triggered build, or apply the same toolchain-availability guard
around the `rustc_metadata_shim` step. Use the existing `rustc_metadata_shim`
rule and the `rust_build_cross` / `metadataShim` dependency path to place the
fix without changing the build intent.
In `@test/internal/rustc-metadata-shim.test.ts`:
- Around line 51-73: Add test coverage for the compact `-Cmetadata=<hash>`
spelling handled by `rewrite_metadata` so the shim’s one-argument rewrite path
is exercised, not just the existing `-C metadata=<hash>` form. Update the tests
in `rustc-metadata-shim.test.ts`—especially the `unit`, `argvFor`, and
`metadataOf` cases—to include both accepted spellings and assert they normalize
the same way through `rewrite_metadata`/`argvFor`.
- Around line 94-198: The behavioral tests in describe.skipIf(rustc === null)
are recompiling the same shim for every case, which is redundant. Move the
compileShim call out of each test into a shared setup such as beforeAll, keep
the per-test tempDir fixture isolation, and reuse the compiled shim path in
argvFor/metadataOf-driven assertions. Ensure the shared shim is only built once
from compileShim and then passed into each test that currently invokes it.
- Around line 96-106: The subprocess helpers are leaving pipes unread, which can
deadlock child processes when buffers fill. Update both `compileShim` and
`argvFor` to drain all piped streams concurrently by awaiting stdout, stderr,
and exit together, following the repo convention used in subprocess tests. Make
sure the fix is applied at the helper level so every test using `compileShim` or
`argvFor` benefits from drained `proc.stdout` and `proc.stderr`.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: eca9dd4e-7f8d-410a-9487-e4043418a1d8
📒 Files selected for processing (4)
scripts/build/CLAUDE.mdscripts/build/rust.tsscripts/build/rustc-metadata-shim.rstest/internal/rustc-metadata-shim.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The pin only has to identify the compilation unit; rustc mixes the crate name into StableCrateId on top of it, no two workspace packages share a name, and cargo wraps workspace members only. So CARGO_PKG_NAME is enough and the wrapper needs no argument parsing at all. Also run the shim's bare rustc through the same rustup toolchain repair rust_build_cross does: it is the build's first rustup-proxy call, and a partially installed toolchain has no host rust-std for it to link against. Share that prefix between the two rules instead of duplicating it. Test: fix the ninja output name assertion on Windows hosts (exeSuffix), drain both subprocess pipes, and compile the wrapper once per suite.
|
Cut the diff down in 8a21b11 after feedback that the first pass was over-engineered: 412 added lines → 285, and the wrapper itself 152 → 72. The pin is now just the package name. Re-verified the same way, on a debug linux-x64 build, by adding
All four workspace crates sampled still get distinct ids, On the review comments:
Windows Unread stdio pipes — fixed; both helpers now drain stdout and stderr alongside Shim recompiled per test — fixed; it's compiled once in Untested |
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 `@test/internal/rustc-metadata-shim.test.ts`:
- Around line 80-141: The three behavioral tests in the rustc metadata shim
suite are independent and currently run sequentially even though they only call
argvFor after beforeAll finishes setting up shim and fakeRustc. Update the test
declarations under describe.skipIf(rustc === null) to run concurrently (prefer
test.concurrent or describe.concurrent where appropriate) so the
subprocess-spawning cases can execute in parallel without shared mutable state.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8ee042f5-8af8-4137-975e-b5ced4d8b4c7
📒 Files selected for processing (3)
scripts/build/rust.tsscripts/build/rustc-metadata-shim.rstest/internal/rustc-metadata-shim.test.ts
They only read `shim`/`fakeRustc`, which beforeAll finishes writing before any of them start. test/CLAUDE.md asks for this whenever tests in a file spawn processes.
|
CI is green on the pin: all 16 Worth noting that #33302 landed on main while this was open, which makes the payoff concrete rather than hypothetical. It links with export function shouldGenerateOrderFile(cfg: Config): boolean {
if (!orderFileEligible(cfg)) return false;
if (!cfg.canary) return true;
return /\[generate symbol order\]/i.test(process.env.BUILDKITE_MESSAGE ?? "");
}So the common path links against an order file produced by an earlier build. Every |
There was a problem hiding this comment.
Earlier feedback is addressed and I found no new issues, but this inserts a wrapper into every workspace rustc invocation and reshapes the toolchain-repair path in rust.ts — build-infrastructure with cross-platform reach that's worth a human sign-off.
Extended reasoning...
Overview
This PR adds a RUSTC_WORKSPACE_WRAPPER (scripts/build/rustc-metadata-shim.rs, 72 lines of Rust compiled by a bare rustc at build time) that rewrites cargo's -C metadata=<hash> to metadata=bun.<CARGO_PKG_NAME> for every workspace crate, so v0 symbol names stay stable across dependency-graph edits. scripts/build/rust.ts gains a new ninja rule for the shim, hoists the rustup toolchain install --force repair into a shared prefix used by both the shim rule and rust_build_cross, sets RUSTC_WORKSPACE_WRAPPER in cargo's env, and threads the compiled shim as an implicit input of every cargo edge. A one-line entry is added to scripts/build/CLAUDE.md, and test/internal/rustc-metadata-shim.test.ts covers both the ninja-graph wiring and the wrapper's argv rewriting.
Security risks
None apparent. The wrapper only runs at build time, only reads CARGO_PKG_NAME from the environment cargo sets, and execs the real rustc with a rewritten argv. No network, no untrusted input, no change to shipped-binary behavior.
Level of scrutiny
Moderate-to-high. This is not runtime code, but it sits in the path of every workspace rustc invocation across all 16 build lanes and changes how symbol disambiguators are derived. The correctness argument rests on cargo/rustc internals (that RUSTC_WORKSPACE_WRAPPER applies only to workspace members, that cargo always emits -C and metadata=… as two args, that no two workspace packages share a name, that rustc mixes the crate name into StableCrateId independently). The refactor also moves the toolchain-repair prefix and changes how the rustc_metadata_shim rule is shell-wrapped on Windows. All of this is well-reasoned in the PR description and comments, and CI is reported green on all lanes — but it's the kind of build-system design decision a maintainer should confirm.
Other factors
- All prior review feedback (mine on the Windows
.exeregex and undrained stderr; CodeRabbit's on the toolchain-repair ordering, pipe draining, single-compilebeforeAll, andtest.concurrent) is resolved in commits 8a21b11 and cc95357, and the current diff reflects those fixes. - The bug-hunting pass on the latest revision found nothing.
- The one-time cost (first build after merge recompiles all workspace crates because the wrapper path enters cargo's fingerprint) is called out in the description.
- The change is not simple/mechanical: it introduces a new build-time executable, a new ninja rule, and a behavioral dependency on cargo's argv shape — outside the "approve without human review" bar.
CI's test lanes run a downloaded binary, and the rustc they find is a rustup proxy: the first call downloads a channel (40s, progress on stderr) and on Windows writes shim.exe where the test looked for shim. Neither belongs in a test lane — the build-rust lanes already prove the wrapper compiles and cargo runs through it on every platform. Point the behavioural tests at the wrapper emitRust() drops in the build directory, next to the bun under test, and skip when it isn't there.
|
CI caught a real problem in the test, not the pin — fixed in 85704c8. The behavioural tests compiled the wrapper with whatever They now drive the wrapper No coverage lost that matters: the 16 The other red lane in that build, |
A debug or ASAN bun writes benign noise there (CLAUDE.md says so). The pipe is still drained so it can't fill; stderr now only surfaces in the message when the wrapper exits non-zero.
1 participant
bun_runtime's Rust symbol disambiguator changes on nearly every build, so a symbol likedoes not exist under that name in the next build. Nothing is wrong with the shipped binary, but anything keyed on symbol names across two builds silently loses the crate: reusing an lld
--symbol-ordering-filefrom an earlier build, sccache hits, and symbolicating an old profile against a new binary.Cause
Cargo's
-C metadatafor a unit mixes in the sorted-C metadataof every one of its dependencies (compute_metadataincompilation_files.rs), and rustc hashes-C metadatainto theStableCrateIdthat every v0 symbol carries. So a dependency-edge edit anywhere below a crate renames every symbol that crate defines.bun_runtimehas direct dependencies on ~100 workspace crates, so it is renamed by almost every commit.bun_core,bun_bunfigandbun_js_parsersit near the bottom of the graph and so never move. It is notbuild.rs, the git sha, or the workspace version: RUSTFLAGS and the profile are deliberately excluded from-C metadata(cargo hashes them into-C extra-filenameonly), and-C metadatais computed before any build script runs.Concretely, between two adjacent canary builds
bun_sql_jscdropped itsbun_wyhashdependency. That rotatedbun_sql_jsc's-C metadata, which rotatedbun_runtime's andbun_rust's.Fix
A
RUSTC_WORKSPACE_WRAPPER(scripts/build/rustc-metadata-shim.rs) replaces cargo's value with one derived only from the unit's own identity:That is everything in cargo's hash that identifies the compilation unit (package, version, target, crate types, features), minus the dependency hash that churns. Symbols stay unique because rustc mixes the crate name into
StableCrateIdindependently of-C metadata, and no two workspace packages share a name.Notes on the shape of the fix:
_WORKSPACE_, not plainRUSTC_WRAPPER. Cargo applies it to workspace members only, which is exactly the set whose names we care about; registry crates (where two versions of one crate name can coexist) keep cargo's collision-proof hash.cargo clippysets this variable itself, so it keeps working.-C metadatait is handed, so an extra one from RUSTFLAGS does not shadow cargo's — verified, it produces a third value that still rotates.-C extra-filenameis left alone, so cargo's on-disk artifact names stay unique across dependency-graph states.rustcrather than cargo: the wrapper has to exist before cargo runs, so it can't be a workspace member, and one host.rsis cheaper than a second cargo invocation.rustcsits next tocargo, the same assumptionfindRustup()already makes.One-time cost: the wrapper's path is part of cargo's fingerprint for workspace members, so the first build after this lands recompiles bun's own crates.
Verification
Re-applying the exact
bun_sql_jscdependency edit on a debug linux-x64 build:bun_runtimeCsduDwerHUqXv→CscY7r8IXUlgZbun_sql_jscCs9HsfIadT9o5→Cs1w60FSEQOrUbun_coreCsbzcMvzFcEb5→CsbzcMvzFcEb5Commands
bun bdlinks and runs, and the binary carries exactly onebun_runtimedisambiguator.test/internal/rustc-metadata-shim.test.tscovers the ninja wiring (everywhere) and the wrapper's rewriting (where rustc is available): two different cargo metadata hashes collapse to the same pinned value, distinct units keep distinct values,-C extra-filenameand every other argument pass through, and an invocation with no-C metadata(cargo's-vV/--printprobes) is untouched.