-
Notifications
You must be signed in to change notification settings - Fork 5k
build: pin -C metadata so bun's crates keep stable symbol names #33339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
5
commits into
main
Choose a base branch
from
farm/a27e62b9/pin-rustc-metadata
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+280
−10
Open
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7e98ff5
build: pin -C metadata so bun's crates keep stable symbol names
robobun 8a21b11
build: simplify the -C metadata pin to the package name
robobun cc95357
test: run the shim's subprocess cases concurrently
robobun 85704c8
test: drive the wrapper the build produced instead of compiling one
robobun b2f2d8d
test: don't assert the wrapper's stderr is empty
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| //! `RUSTC_WORKSPACE_WRAPPER`: replace cargo's `-C metadata` with the package | ||
| //! name, so bun's crates keep the same v0 symbol names from build to build. | ||
| //! | ||
| //! Cargo mixes the `-C metadata` of every dependency into a unit's own, and | ||
| //! rustc hashes that into the `StableCrateId` every v0 symbol carries — the | ||
| //! `Cs…` in `_RNvNtNtCs4EG9u9StnXu_11bun_runtime3cli7command15boot_standalone`. | ||
| //! A dependency-edge edit *anywhere below a crate* therefore renames every | ||
| //! symbol that crate defines. `bun_runtime` has ~100 direct workspace deps, so | ||
| //! it was renamed by nearly every commit, and anything matching symbol names | ||
| //! across two builds silently lost it: lld `--symbol-ordering-file` reuse, | ||
| //! sccache hits, symbolicating an old profile against a new binary. | ||
| //! | ||
| //! rustc hashes *every* `-C metadata` it is handed, so an extra one from | ||
| //! RUSTFLAGS can't shadow cargo's — rewriting it here is the only lever. | ||
| //! | ||
| //! The package name alone keeps symbols unique: rustc mixes the crate name into | ||
| //! `StableCrateId` on top of this, no two workspace packages share a name, and | ||
| //! cargo applies this wrapper to workspace members only — registry crates, where | ||
| //! two versions of one name can coexist, keep cargo's hash. | ||
|
|
||
| use std::env; | ||
| use std::ffi::OsString; | ||
| use std::process::Command; | ||
|
|
||
| fn main() { | ||
| let mut argv = env::args_os().skip(1); | ||
| let Some(rustc) = argv.next() else { | ||
| eprintln!("rustc-metadata-shim: expected rustc's path as the first argument"); | ||
| std::process::exit(1); | ||
| }; | ||
|
|
||
| // Cargo emits `-C` and `metadata=<hash>` as two arguments, and nothing else | ||
| // it passes starts with `metadata=`. Invocations with none at all (cargo's | ||
| // `-vV` / `--print` probes) fall through untouched. | ||
| let pin = format!( | ||
| "metadata=bun.{}", | ||
| env::var("CARGO_PKG_NAME").unwrap_or_default() | ||
| ); | ||
| let args: Vec<OsString> = argv | ||
| .map(|arg| match arg.to_str() { | ||
| Some(a) if a.starts_with("metadata=") => OsString::from(pin.as_str()), | ||
| _ => arg, | ||
| }) | ||
| .collect(); | ||
|
|
||
| run(&rustc, &args) | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| fn run(rustc: &OsString, args: &[OsString]) -> ! { | ||
| use std::os::unix::process::CommandExt; | ||
| let err = Command::new(rustc).args(args).exec(); | ||
| eprintln!( | ||
| "rustc-metadata-shim: cannot exec {}: {err}", | ||
| rustc.to_string_lossy() | ||
| ); | ||
| std::process::exit(1) | ||
| } | ||
|
|
||
| #[cfg(not(unix))] | ||
| fn run(rustc: &OsString, args: &[OsString]) -> ! { | ||
| match Command::new(rustc).args(args).status() { | ||
| Ok(status) => std::process::exit(status.code().unwrap_or(1)), | ||
| Err(err) => { | ||
| eprintln!( | ||
| "rustc-metadata-shim: cannot run {}: {err}", | ||
| rustc.to_string_lossy() | ||
| ); | ||
| std::process::exit(1) | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| /** | ||
| * Regression tests for the `-C metadata` pin — scripts/build/rustc-metadata-shim.rs | ||
| * and its wiring in scripts/build/rust.ts. | ||
| * | ||
| * Cargo folds the `-C metadata` of every dependency into a unit's own, and rustc | ||
| * hashes that into the `Cs…` disambiguator every v0 symbol carries. Without the | ||
| * wrapper, a dependency-edge edit anywhere below a crate renames every symbol | ||
| * that crate defines — `bun_runtime`, with ~100 direct workspace deps, was | ||
| * renamed by nearly every commit. | ||
| * | ||
| * The graph assertion is configure-time only and runs everywhere. The | ||
| * behavioural ones compile the wrapper, so they need a rust toolchain. | ||
| */ | ||
| import { beforeAll, describe, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, tempDir } from "harness"; | ||
| import { join } from "node:path"; | ||
|
|
||
| import { resolveConfig, type Toolchain } from "../../scripts/build/config.ts"; | ||
| import { Ninja } from "../../scripts/build/ninja.ts"; | ||
| import { emitRust, registerRustRules } from "../../scripts/build/rust.ts"; | ||
|
|
||
| const shimSource = join(import.meta.dirname, "..", "..", "scripts", "build", "rustc-metadata-shim.rs"); | ||
|
|
||
| test("the wrapper is built before cargo and reaches its env", () => { | ||
| /** A fully-populated fake toolchain — resolveConfig never spawns any of these. */ | ||
| const toolchain: Toolchain = { | ||
| cc: "/fake/llvm/bin/clang", | ||
| cxx: "/fake/llvm/bin/clang++", | ||
| clangVersion: "21.1.8", | ||
| clangResourceDir: "/fake/llvm/lib/clang/21", | ||
| ar: "/fake/llvm/bin/llvm-ar", | ||
| ranlib: "/fake/llvm/bin/llvm-ranlib", | ||
| ld: "/fake/llvm/bin/ld.lld", | ||
| ld64Lld: "/fake/llvm/bin/ld64.lld", | ||
| rustLld: undefined, | ||
| rustLlvmVersion: "22.1.4", | ||
| rustSysroot: undefined, | ||
| rustHostTriple: undefined, | ||
| strip: "/fake/bin/strip", | ||
| llvmStrip: "/fake/llvm/bin/llvm-strip", | ||
| dsymutil: "/fake/llvm/bin/dsymutil", | ||
| bun: "/fake/bin/bun", | ||
| jsRuntime: "/fake/bin/bun", | ||
| esbuild: "/fake/bin/esbuild", | ||
| ccache: undefined, | ||
| cmake: "/fake/bin/cmake", | ||
| cargo: "/fake/rust/bin/cargo", | ||
| cargoHome: "/fake/.cargo", | ||
| rustupHome: "/fake/.rustup", | ||
| msvcLinker: undefined, | ||
| rc: undefined, | ||
| mt: undefined, | ||
| nasm: undefined, | ||
| }; | ||
| const cfg = resolveConfig({ os: "linux", arch: "x64", buildType: "Debug" }, toolchain); | ||
| const n = new Ninja({ buildDir: cfg.buildDir }); | ||
| registerRustRules(n, cfg); | ||
| emitRust(n, cfg, { codegenInputs: [], codegenOrderOnly: [], rustSources: [], vendorStamps: [] }); | ||
| // ninja wraps long build lines with a trailing `$`; join them back up. | ||
| const graph = n.toString().replace(/\$\n\s+/g, " "); | ||
|
|
||
| // `.exe` on a Windows host: the wrapper is a host executable, and the host | ||
| // isn't the linux target asked for above. | ||
| expect(graph).toMatch(/^build rustc-metadata-shim(\.exe)?: rustc_metadata_shim \S*rustc-metadata-shim\.rs\b/m); | ||
| // `_WORKSPACE_`, not plain RUSTC_WRAPPER: registry crates keep cargo's | ||
| // collision-proof hash, only our own crates get a pinned one. | ||
| expect(graph).toContain("--env=RUSTC_WORKSPACE_WRAPPER="); | ||
| expect(graph).not.toContain("--env=RUSTC_WRAPPER="); | ||
|
|
||
| const cargoEdge = graph.split("\n").find(line => line.startsWith("build ") && line.includes("libbun_rust.a:")); | ||
| expect(cargoEdge).toBeDefined(); | ||
| // Implicit inputs come after the `|` — cargo can't run before the wrapper exists. | ||
| expect(cargoEdge!.split("|")[1]).toContain("rustc-metadata-shim"); | ||
| }); | ||
|
|
||
| // Compiling the wrapper needs rustc. It sits next to cargo on both rustup and | ||
| // distro installs — the same assumption registerRustRules() makes. | ||
| const rustc = Bun.which("rustc"); | ||
|
|
||
| describe.skipIf(rustc === null)("rustc metadata shim", () => { | ||
| let shim = ""; | ||
| let fakeRustc = ""; | ||
|
|
||
| beforeAll(async () => { | ||
| // Not disposed: bun:test has no `using` for suite-scoped fixtures, and the | ||
| // OS reaps its own temp dir. | ||
| const dir = String( | ||
| tempDir("rustc-metadata-shim", { "fake-rustc.ts": `console.log(process.argv.slice(2).join("\\n"))` }), | ||
| ); | ||
| shim = join(dir, "shim"); | ||
| fakeRustc = join(dir, "fake-rustc.ts"); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [rustc!, "--edition", "2024", "-Copt-level=0", "-o", shim, shimSource], | ||
| env: bunEnv, | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect({ stdout, stderr, exitCode }).toEqual({ stdout: "", stderr: "", exitCode: 0 }); | ||
| }); | ||
|
|
||
| /** Run the wrapper; returns the argv it would have handed the real rustc. */ | ||
| async function argvFor(args: string[], pkg = "bun_runtime") { | ||
| await using proc = Bun.spawn({ | ||
| // The wrapper's first argument is always rustc's path — that's cargo's contract. | ||
| cmd: [shim, bunExe(), fakeRustc, ...args], | ||
| env: { ...bunEnv, CARGO_PKG_NAME: pkg }, | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); | ||
| return stdout.split("\n").filter(Boolean); | ||
| } | ||
|
|
||
| const unit = ["--crate-name", "bun_runtime", "--crate-type", "lib", "--target", "x86_64-unknown-linux-gnu"]; | ||
|
|
||
| test("two dependency-graph states collapse to the same metadata", async () => { | ||
| // The only difference is cargo's dependency hash — exactly what moves when a | ||
| // crate anywhere below this one gains or loses a dependency. `extra-filename` | ||
| // must survive: it keys cargo's on-disk artifact names. | ||
| const before = [...unit, "-C", "extra-filename=-4e6e51e9e0da5e6b", "-C", "metadata=4e6e51e9e0da5e6b", "lib.rs"]; | ||
| const after = [...unit, "-C", "extra-filename=-9c1d0ab7735ffd12", "-C", "metadata=9c1d0ab7735ffd12", "lib.rs"]; | ||
|
|
||
| expect(await argvFor(before)).toEqual( | ||
| before.map(arg => (arg.startsWith("metadata=") ? "metadata=bun.bun_runtime" : arg)), | ||
| ); | ||
| expect(await argvFor(after)).toEqual( | ||
| after.map(arg => (arg.startsWith("metadata=") ? "metadata=bun.bun_runtime" : arg)), | ||
| ); | ||
| }); | ||
|
|
||
| test("distinct packages keep distinct metadata", async () => { | ||
| expect(await argvFor([...unit, "-C", "metadata=aaaa"], "bun_core")).toContain("metadata=bun.bun_core"); | ||
| }); | ||
|
|
||
| test("an invocation without -C metadata passes through untouched", async () => { | ||
| // cargo probes the wrapper with `-vV` / `--print` before it compiles anything. | ||
| expect(await argvFor(["-vV"])).toEqual(["-vV"]); | ||
| expect(await argvFor(["--print", "cfg"])).toEqual(["--print", "cfg"]); | ||
| }); | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.