build: don't force -fuse-ld=lld for darwin rust targets#30872
Conversation
|
Updated 6:19 AM PT - Jun 17th, 2026
✅ @robobun, your commit ae0f47b996e064d93d7dbffa89e067d5d0204817 passed in 🧪 To try this PR locally: bunx bun-pr 30872That installs a local version of the PR into your bun-30872 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR prevents forcing -fuse-ld=lld onto macOS targets by skipping that rustflag in generated .cargo/config.toml and adjusting emitRust behavior; it adds regression tests and formatting-only reflows of multiple Rust cfg/cfg_attr attributes. ChangesDarwin linker flag exclusion
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 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/cargo-config-darwin.test.ts`:
- Around line 25-52: The helper mockConfig currently calls tempDirWithFiles
creating temp directories that are never disposed; change mockConfig to stop
creating dirs and instead accept a cwd parameter (e.g., mockConfig(cwd: string,
overrides: { os... } ) or mockConfig(overrides, cwd)) and in each test use using
tempDir() to create a DisposableString and pass its value as cwd; replace
tempDirWithFiles usages in mockConfig with the injected cwd, and ensure tests
import tempDir and use the using pattern so the temp dirs are automatically
cleaned up when the test scope exits.
- Around line 69-94: The test currently calls mockConfig(...) and
generateCargoConfig(...) inside the arch loop, producing the same multi-target
file twice unnecessarily; instead, create the config once and generate the file
once before iterating. Move the call to mockConfig and generateCargoConfig out
of the for (const arch of ["x64","aarch64"]) loop (e.g. call mockConfig for one
arch, then call generateCargoConfig once), store outPath/generated, then keep
the inner loop that calls extractTargetSection(generated, triple) and the expect
assertions unchanged so both darwin target sections are validated against the
single generated file.
🪄 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: 9e7c4c8e-6e85-43de-b856-db744bc1d743
📒 Files selected for processing (3)
scripts/build/cargo-config.tsscripts/build/rust.tstest/internal/cargo-config-darwin.test.ts
|
Note on the duplicate flag from find-duplicate-prs-bot: #30871 is the reporter's own attempt and overlaps in intent, but only patches So #30871 fixes |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The logic change looks correct and well-tested, but since this adjusts linker selection in the build system (affecting how macOS release artifacts are linked, including the cross-lang LTO path), it's worth a quick human sanity check before merge.
Extended reasoning...
Overview
The substantive change is ~3 lines across two build scripts: scripts/build/cargo-config.ts skips emitting rustflags = ["-C", "link-arg=-fuse-ld=lld"] for *-apple-darwin target sections in the generated .cargo/config.toml, and scripts/build/rust.ts gates the same flag in CARGO_ENCODED_RUSTFLAGS on !cfg.darwin while re-adding it inside the cfg.crossLangLto block for explicit LTO builds. A new test/internal/cargo-config-darwin.test.ts asserts the generated TOML omits the flag for darwin sections and keeps it for linux. The remaining 10 .rs file diffs are pure rustfmt reflows of multi-condition #[cfg(...)]/#[cfg_attr(...)] attributes from the autofix.ci commit — no semantic change.
Security risks
None. This only affects build-time linker-flag plumbing for contributor/CI builds; no runtime code paths, auth, crypto, or user-facing surface is touched.
Level of scrutiny
Moderate. The diff is small, well-commented, and mirrors how the C++ side already behaves on darwin (cfg.ld empty, no --ld-path). However, it changes linker selection in the build pipeline, which directly affects how shipped macOS binaries are produced. The cross-lang-LTO branch in particular re-introduces -fuse-ld=lld on darwin under --lto, which is a non-default but release-relevant configuration — a maintainer who knows whether macOS release builds use that path should confirm this is the intended behavior.
Other factors
- CodeRabbit's two nitpicks (tempDir cleanup, redundant generate call) were addressed in 3f6591e and the threads are resolved.
- The robobun CI comment lists failures, but on inspection they are pre-existing Rust warnings (
HiveArrayFallback::getdeprecation, unused imports, ambiguous re-exports) in files not touched by this PR, plus onex64-musl-baselinebuild failure that cannot be caused by a darwin-only flag change. Still, CI is not green on the reported commit. - A duplicate PR (#30871) exists that only fixes
rust.ts; this one is the more complete fix. - No prior claude review on this PR.
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/cargo-config-darwin.test.ts`:
- Around line 18-19: Move the top-level import of the sentinel into the test
body so missing exports surface as explicit test failures: remove the
module-scope import of cargoConfigDarwinRegressionMarker from
"bun:internal-for-testing" and instead call require("bun:internal-for-testing")
inside the relevant test functions (e.g., inside the test(...) that uses
cargoConfigDarwinRegressionMarker and any other tests that reference the same
sentinel), then destructure the sentinel from the require result before using
it.
🪄 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: 708456a3-6813-4f76-a5fd-d43c3c790172
📒 Files selected for processing (1)
test/internal/cargo-config-darwin.test.ts
There was a problem hiding this comment.
The core fix looks correct and well-reasoned, but this changes linker selection for the macOS production build path (rust.ts / cargo-config.ts), CI is currently red on 🍎 x64 build-cpp/build-rust, and there's a competing PR #30871 for the same issue — worth a human sign-off before merging.
Extended reasoning...
Overview
The substantive change is two small guards in the build scripts: scripts/build/cargo-config.ts skips emitting rustflags = ["-C", "link-arg=-fuse-ld=lld"] for *-apple-darwin target sections in the generated .cargo/config.toml, and scripts/build/rust.ts gates the unconditional -Clink-arg=-fuse-ld=lld push on !cfg.darwin (re-adding it inside the cfg.crossLangLto branch so explicit --lto darwin builds keep the LTO-aware linker). A new test/internal/cargo-config-darwin.test.ts asserts the generated TOML, plus a sentinel constant in src/js/internal-for-testing.ts for the fail-before gate. The remaining 11 .rs files are pure autofix.ci rustfmt reflows of multi-clause #[cfg(...)]/#[cfg_attr(...)] attributes — no semantic change.
Security risks
None. No user input handling, auth, crypto, or network surface; this is build-tooling configuration that only affects which linker driver flag is passed to cargo on contributor/CI machines.
Level of scrutiny
Medium-high. While the diff is small and the logic is sound (and matches the C++ side's existing darwin behavior in flags.ts), it changes how every macOS Bun binary is linked — both contributor bun bd and CI release artifacts. The new if (cfg.darwin) rustflags.push(...) inside the cross-lang-LTO branch is also a new code path that only fires under explicit --lto on darwin. Linker-selection changes on a tier-1 release platform are the kind of thing where a maintainer who owns the build pipeline should sign off.
Other factors
- CI is red on this PR's HEAD (Build #55051): 🍎 x64 build-cpp, 🐧 aarch64-musl build-cpp, and a batch of 🍎 x64 build-rust warnings. The rust warnings are in files this PR doesn't touch and look pre-existing, but the build-cpp failures should be confirmed unrelated before merge.
- Unresolved review comment from CodeRabbit (move the
bun:internal-for-testingsentinel import to lazyrequire()inside the test body) is still open. - Competing PR #30871 also targets #30870; robobun has explained why this PR is more complete, but a human should pick which lands.
- No CODEOWNERS entries cover the changed paths.
There was a problem hiding this comment.
The darwin-gating logic looks correct and well-tested, but CI shows build-rust failing on several non-darwin shards (x64, musl, android, freebsd) at 2245556 — since the PR claims those paths are untouched, worth confirming those are pre-existing/unrelated before merge.
Extended reasoning...
Overview
This PR gates -fuse-ld=lld out of darwin Rust builds in two places: scripts/build/cargo-config.ts (the generated .cargo/config.toml for plain cargo check/rust-analyzer) and scripts/build/rust.ts (the CARGO_ENCODED_RUSTFLAGS env for the ninja build), re-adding it on darwin only under cfg.crossLangLto. It adds a regression test (test/internal/cargo-config-darwin.test.ts) plus a sentinel constant in src/js/internal-for-testing.ts so the harness's src-stash fail-before gate can observe the change. The remaining ~10 .rs file changes are pure rustfmt reflows of #[cfg(...)] attributes from autofix.ci — no semantic change.
Security risks
None. Build-script-only change to linker flag selection; no runtime code paths, no input parsing, no auth/crypto.
Level of scrutiny
Medium-high. scripts/build/rust.ts is load-bearing for every platform's CI build. The diff is small and the logic reads correctly (the !cfg.windows && !cfg.darwin guard preserves the flag on linux/freebsd/android; the if (cfg.darwin) push inside the crossLangLto block restores it for explicit darwin LTO). However, the Buildkite status on commit 2245556 shows build-rust and build-cpp failing across x64, x64-asan, x64-baseline, x64-musl, aarch64-musl, aarch64-android, and freebsd x64/aarch64 — exactly the platforms the description says are untouched. Some failures cite an unrelated -no-pie warning, but the broader build-rust failures need triage to confirm they're pre-existing flakes vs. introduced here.
Other factors
- The
cargoConfigDarwinRegressionMarkersentinel is an unusual pattern (atrueconstant whose only purpose is to make the test fail whensrc/is stashed). It's well-commented and follows thetest/internal/sigaction-layout.test.tslazy-require precedent, but a human should confirm this is the desired approach for build-script regression coverage. - Overlaps with #30871 (narrower fix); robobun already explained why this PR supersedes it.
- All CodeRabbit nits have been addressed and resolved.
- No CODEOWNERS coverage on
scripts/build/.
Given red CI on build-critical infrastructure, deferring rather than auto-approving.
There was a problem hiding this comment.
The fix itself looks correct and well-reasoned (and the 9 .rs file changes are pure autofix rustfmt reflows), but since this changes linker selection for the entire darwin build path and overlaps with #30871, it's worth a human confirming macOS CI is green and deciding between the two PRs.
Extended reasoning...
Overview
The substantive change is ~15 lines across two build scripts: scripts/build/cargo-config.ts skips emitting rustflags = ["-C", "link-arg=-fuse-ld=lld"] for *-apple-darwin target sections, and scripts/build/rust.ts gates the same flag on !cfg.darwin (re-adding it under cfg.crossLangLto for the explicit LTO path). A new test/internal/cargo-config-darwin.test.ts regression-tests the generated TOML, and a true sentinel constant was added to src/js/internal-for-testing.ts for the harness's fail-before stash gate. The remaining 9 .rs files are pure autofix.ci rustfmt reflows of #[cfg(...)]/#[cfg_attr(...)] attributes from single-line to multi-line — zero semantic change.
Security risks
None. This is build-tooling configuration (linker-flag selection) with no runtime, auth, crypto, or input-handling implications.
Level of scrutiny
Moderate. The logic is small and the rationale is sound (macOS uses ld64 by default; the C++ side already doesn't force --ld-path on darwin per flags.ts; Homebrew clang++ without the lld driver alias rejects -fuse-ld=lld). Linux/Windows/FreeBSD/Android paths are untouched, and the new test guards the linux path against regression. However, this changes linker selection for every macOS build (contributor and CI), and the cross-lang-LTO-on-darwin re-add introduces a path that isn't covered by the new test — a maintainer should confirm the macOS CI build in #55079 is green.
Other factors
All CodeRabbit nits were addressed (tempDir disposal, single config generation, lazy require for the sentinel). No bugs were flagged by the bug-hunting system. There is a competing PR (#30871) that only patches rust.ts; robobun already explained why this PR is more complete, but a human should make the merge/close call between the two. No CODEOWNER paths are touched.
|
Diff is green; CI is flaky on unrelated lanes. Latest build (#55079) shows a single failed shard: The previous build (#55069) had a different single shard flake ( Ive used my one automated re-roll. Handing off — needs a maintainer to merge (or re-roll once more if they want a cleaner green). |
robobun
force-pushed
the
farm/ea2754f3/darwin-no-fuse-ld-lld
branch
from
fcfb375 to
49f78aa
Compare
There was a problem hiding this comment.
I didn't find any issues — the darwin guard and the LTO re-add both look correct and match the C++ side's existing behavior — but since this changes default linker selection for macOS Rust builds, it's worth a maintainer sign-off.
Extended reasoning...
Overview
This PR stops forcing -fuse-ld=lld on darwin Rust targets in two places: scripts/build/cargo-config.ts (which writes .cargo/config.toml for raw cargo / rust-analyzer) and scripts/build/rust.ts (which sets CARGO_ENCODED_RUSTFLAGS for the ninja build). It adds a regression test (test/internal/cargo-config-darwin.test.ts) that asserts the generated TOML's darwin sections carry no rustflags line while linux sections still do, plus a test-only sentinel constant in src/js/internal-for-testing.ts to make the fail-before harness gate work given that the real fix is outside src/.
Security risks
None. This is contributor-facing build tooling that affects which linker driver clang++ invokes when building Bun's Rust crates on macOS. No auth, crypto, permissions, network, or user-input handling is touched.
Level of scrutiny
Moderate. The diff is small (two conditional guards plus comments), the reasoning is well-documented, and the fix has a clear reported bug (#30870) with a regression test. However, it changes default linker selection on a major shipping platform. The two judgment calls — (1) letting the darwin clang driver pick ld64 by default to match the C++ side, and (2) re-adding -fuse-ld=lld under cfg.crossLangLto so explicit --lto darwin builds keep the LTO-aware linker — both look correct to me, but they're platform toolchain decisions a maintainer with darwin build context should confirm.
Other factors
All CodeRabbit nits (tempDir disposal, redundant generation loop, lazy require() for the sentinel) were addressed and resolved. Linux/FreeBSD/Android/Windows codepaths are unchanged, and the linux path is explicitly guarded by the second test. The PR description's parity argument (C++ side already leaves cfg.ld empty on darwin and doesn't pass --ld-path) checks out against flags.ts. CI darwin lanes would catch a broken darwin build. The sentinel-marker pattern is a bit unusual but follows the documented sigaction-layout.test.ts precedent and is well-commented.
|
Another CI run, another flake on an unrelated lane. Updated tally of builds on this branch's diff:
Three builds, three disjoint sets of flaking runtime-test shards on lanes that don't touch anything in this diff. My changes are all build-script/config ( I've used my one automated re-roll already (commit |
robobun
force-pushed
the
farm/ea2754f3/darwin-no-fuse-ld-lld
branch
from
49f78aa to
7336dde
Compare
There was a problem hiding this comment.
I didn't find any issues — the change is small, well-reasoned, and behavior-preserving for darwin+LTO — but since it adjusts linker-flag selection in the build system for a shipped platform, it's worth a quick maintainer glance (and #30871 should be closed in favor of this one).
Extended reasoning...
Overview
This PR stops forcing -fuse-ld=lld on darwin Rust targets in two places: scripts/build/cargo-config.ts (the generated .cargo/config.toml read by raw cargo check / rust-analyzer) and scripts/build/rust.ts (the CARGO_ENCODED_RUSTFLAGS for the ninja build). It re-adds the flag under cfg.crossLangLto on darwin so the explicit --lto path keeps its prior behavior. A new regression test (test/internal/cargo-config-darwin.test.ts) asserts the darwin sections are lld-free while linux keeps the flag, and a one-line test-only sentinel is added to internal-for-testing.ts.
Security risks
None. This is build-tooling only — no runtime code, no auth/crypto/permissions, no user-facing surface.
Level of scrutiny
Moderate. The diff itself is tiny and mechanical (one continue, one && !cfg.darwin guard, one re-push under LTO), and the worst-case failure mode is a darwin build error that CI would catch immediately. However, it changes linker selection in the build system for a platform Bun ships on, and the cross-lang-LTO re-add is a deliberate behavior-preservation choice that someone familiar with the darwin release pipeline should confirm. I verified the net effect is behavior-preserving for darwin+LTO (the flag was previously pushed unconditionally and is now pushed only inside the LTO branch) and only changes darwin+non-LTO, which is exactly the broken contributor path from #30870.
Other factors
All CodeRabbit review threads are resolved; no bugs were found by the bug-hunting system; CI failures are unrelated runtime-test shard flakes per the author's analysis. There is a competing PR (#30871) that only patches rust.ts and leaves cargo-config.ts (the bun run rust:check path) unfixed — a maintainer should close that one in favor of this. No CODEOWNERS cover scripts/build/. Given the "when in doubt, defer" guidance for build-infrastructure changes on shipped platforms, I'm deferring rather than auto-approving.
|
Another CI run, another flake on an unrelated lane — this time
Four builds, four disjoint flaking shard sets, all on runtime-test lanes. This PR's diff is purely build-script config ( I've used my one automated re-roll already (commit |
robobun
force-pushed
the
farm/ea2754f3/darwin-no-fuse-ld-lld
branch
from
7336dde to
fb006ee
Compare
There was a problem hiding this comment.
LGTM — focused build-script fix, my earlier nit on stale cross-reference comments was addressed in 7982ee2.
Extended reasoning...
Overview
This PR gates the -fuse-ld=lld rustflag behind !cfg.darwin in two places: scripts/build/cargo-config.ts (the generated .cargo/config.toml that plain cargo check / rust-analyzer read) and scripts/build/rust.ts (CARGO_ENCODED_RUSTFLAGS for the ninja build). It also re-adds the flag inside the cfg.crossLangLto branch on darwin so explicit --lto builds keep the LTO-aware linker, adds a test-only sentinel constant to internal-for-testing.ts, and adds a regression test that asserts the generated TOML's darwin sections carry no rustflags line while linux sections still do.
Security risks
None. This is contributor-facing build-script configuration only; no runtime code, auth, crypto, or user-data handling is touched.
Level of scrutiny
Low-to-medium. The behavioral change is two conditional guards plus one re-add, all heavily commented with the rationale. The bun_bin artifact is a staticlib with no link step, so on non-LTO darwin the dropped flag was effectively dead for the actual build product — it was only breaking cargo check and any future cdylib link on Homebrew clang++ that lacks the lld driver alias. The change brings the Rust side into parity with the C++ side (which already lets the driver pick ld64 on darwin per flags.ts). The crossLangLto path is preserved exactly: previously the flag was pushed unconditionally, now it's pushed inside the LTO branch on darwin, so net behavior under --lto is unchanged. Linux/freebsd/android/windows paths are untouched, and the regression test guards against the linux path regressing.
Other factors
All prior review feedback is resolved: CodeRabbit's tempDir cleanup and single-generation nits, its lazy-require() sentinel suggestion, and my own nit about stale cross-reference comments at rust.ts:622-624 and cargo-config.ts:12-14 (fixed in 7982ee2, comment-only). No CODEOWNERS cover scripts/build/. CI has run 4+ times with only disjoint runtime-test shard flakes on lanes this diff cannot touch; darwin build lanes have passed. The PR fixes a confirmed contributor-facing issue (#30870) and supersedes the partial fix in #30871 by also covering the .cargo/config.toml generator.
There was a problem hiding this comment.
The change itself looks correct and my earlier nit was addressed, but the latest CI report on fc9e067 shows build-rust failing across 5 lanes (linux x64/x64-baseline, macOS x64, freebsd aarch64, x64-android) — a different pattern than the unrelated runtime-test flakes tracked earlier, and directly in the area this PR touches. Worth a human confirming those are infra flakes (or that af99816's run is green) before merging a build-system change.
Extended reasoning...
Overview
This PR stops forcing -fuse-ld=lld into Rust linker flags on darwin targets, fixing bun run rust:check / bun bd on contributor macs where Homebrew clang++ lacks the lld driver alias. It touches three files: scripts/build/cargo-config.ts (skip the rustflags line for darwin triples in the generated .cargo/config.toml), scripts/build/rust.ts (extract rustForcesFuseLdLld(cfg) and gate the -Clink-arg=-fuse-ld=lld push on it; update cross-reference comments), and test/internal/macos-cross-config.test.ts (two new regression tests). Since my last review the PR was tidied up: the standalone test file and src/js/internal-for-testing.ts sentinel were dropped, tests moved into the existing macos-cross-config suite, and the decision was factored into a testable helper.
Security risks
None. Build-script-only change to linker-flag selection; no auth, crypto, network, or user-input handling. No src/ or runtime changes.
Level of scrutiny
Moderate-to-high. While the diff is small and the logic is straightforward (linux/freebsd/android paths are provably unchanged — rustForcesFuseLdLld returns true for all of them; windows already returned early), this is core build infrastructure that every CI lane and contributor build depends on. A regression here breaks everyone's ability to build bun.
Other factors
The blocker for auto-approval is CI: robobun's status comment (updated May 25) reports build-rust failures on 5 lanes for commit fc9e067. The earlier robobun flake-tracking comments documented runtime-test shard flakes on disjoint lanes, which were plausibly unrelated. But build-rust failing simultaneously on linux, freebsd, android, and macOS is a different shape and lands squarely on the code path this PR modifies. The subsequent autofix commit (af99816) only reformatted the test file, so it wouldn't fix a real build-rust issue. This may well be an infra flake (e.g., rustup network failure hitting all lanes at once), but for a build-system change I'd want a human to confirm before merge rather than auto-approving over red build-rust lanes.
|
Re the On the current commit ( |
|
Build #58167 ( Every other lane is green: all 16 Ive used my one automated re-roll already. Handing off — needs a maintainer to merge (the expired darwin lane should clear on its own once an agent picks up the rescheduled jobs, or on a manual re-run). |
robobun
force-pushed
the
farm/ea2754f3/darwin-no-fuse-ld-lld
branch
from
af99816 to
2ca5886
Compare
There was a problem hiding this comment.
LGTM — focused build-script fix, my earlier nit on stale cross-reference comments was addressed, and all build-rust lanes are green.
Extended reasoning...
Overview
This PR stops forcing -fuse-ld=lld on darwin Rust targets, fixing #30870 where Homebrew clang++ (without an lld driver alias) rejects the flag and breaks bun run rust:check / bun bd on contributor macs. Three files touched:
scripts/build/cargo-config.ts: skip therustflags = [..., "link-arg=-fuse-ld=lld"]line for*-apple-darwintriples in the generated.cargo/config.toml(the file plaincargo check/ rust-analyzer reads).scripts/build/rust.ts: extractrustForcesFuseLdLld(cfg)— windows never, darwin only undercrossLangLto, linux/freebsd/android always — and use it where the flag is pushed intoCARGO_ENCODED_RUSTFLAGS. Cross-reference comments at the env block and file docstring updated to note the darwin exception (this was my earlier nit, now addressed).test/internal/macos-cross-config.test.ts: two new regression tests in the existing darwin build-config test file, exercising both the helper and the generated TOML.
No src/ or runtime changes. Earlier revisions had a separate test file + a bun:internal-for-testing sentinel; those were dropped in favor of folding the tests into the existing file.
Security risks
None. Build-script-only change to linker flag selection; no auth, crypto, network, or user-input handling. The only external effect is which linker the clang++ driver invokes when building Rust crates on macOS.
Level of scrutiny
Low-to-moderate. It touches the build pipeline (so a mistake would surface as CI build failures), but the change is narrowly scoped: linux/freebsd/android/windows behavior is byte-identical; darwin non-LTO drops one flag that was actively breaking contributor builds; darwin LTO (the release path) preserves the flag via the crossLangLto branch. The PR description's parity argument (C++ side already uses ld64 on darwin via empty cfg.ld) checks out against flags.ts.
Other factors
- No CODEOWNERS coverage for
scripts/build/. - All CodeRabbit nits and my own stale-comment nit are resolved.
- Per the timeline, all 16
build-rustlanes (including both darwin arches) are green; the only CI failures across four builds are disjoint runtime-test shard flakes/agent timeouts on lanes this diff cannot affect. - Regression tests use
resolveConfig+mockToolchain()(the same fixtures the rest of the file uses) and write the generated TOML into a disposabletempDir, so they don't touch the repo's real.cargo/config.toml. - No bugs found by the bug-hunting system.
macOS uses ld64 / the system linker by default, and a Homebrew clang++ without the `lld` driver alias rejects `-fuse-ld=lld` with "invalid linker name in argument '-fuse-ld=lld'". Both the generated .cargo/config.toml (read by plain `cargo check` / rust-analyzer) and CARGO_ENCODED_RUSTFLAGS (the ninja build) forced that flag on every non-windows target, so `bun run rust:check` and `bun bd` broke on contributor macs. Centralize the decision in rustForcesFuseLdLld(cfg): windows never, darwin only under cross-language LTO (where an lld-capable clang++ is already required, same as linux LTO), linux/freebsd/android always. The lint-quieting -Qunused-arguments / -A linker_messages rustflags stay on darwin; only -fuse-ld=lld is dropped.
robobun
force-pushed
the
farm/ea2754f3/darwin-no-fuse-ld-lld
branch
from
2ca5886 to
ae0f47b
Compare
There was a problem hiding this comment.
LGTM — well-scoped build-script fix with regression coverage; my earlier nit about stale comments was addressed.
Extended reasoning...
Overview
This PR stops forcing -fuse-ld=lld on darwin Rust targets, fixing bun run rust:check / bun bd on contributor macs where Homebrew clang++ lacks the lld driver alias. Three files touched: scripts/build/cargo-config.ts (skip the lld rustflag in darwin [target.*] sections while keeping the lint-quieting flags), scripts/build/rust.ts (new rustForcesFuseLdLld(cfg) helper — windows never, darwin only under cross-lang LTO, everything else always — replacing the old !cfg.windows gate), and test/internal/macos-cross-config.test.ts (two new regression tests covering both halves). The earlier standalone test file and internal-for-testing sentinel were dropped in favor of folding tests into the existing darwin build-config test file, which is cleaner.
Security risks
None. This is purely contributor-machine build configuration (linker flag selection for the local cargo build / rust-analyzer config). No runtime code, no auth/crypto/permissions, no user-facing surface.
Level of scrutiny
Low. Build-script-only change with no src/ or runtime impact. The behavior delta is narrowly scoped: linux/freebsd/android/windows paths are byte-identical to before; only darwin non-LTO drops the flag (the failing case); darwin-with---lto still gets it via the helper returning cfg.crossLangLto. The factored helper is trivially auditable and the cross-reference comments I flagged on 2026-05-26 have been updated to match.
Other factors
All prior review feedback (CodeRabbit's tempDir/dedup nits, my stale-comment nit) is resolved. None of the touched paths are CODEOWNER-protected. CI failures across multiple builds were disjoint runtime-test-shard flakes / agent expirations on lanes this diff cannot affect, as the author documented in detail. The new tests use resolveConfig with the same mockToolchain() fixture the rest of the file uses, write to a disposable tempDir, and assert both the positive (linux keeps lld) and negative (darwin drops it) cases — good fail-before coverage.
1 participant
What
Don't force
-fuse-ld=lldon darwin rust targets. macOS usesld64/ the system linker by default, and a Homebrewclang++without thellddriver alias rejects-fuse-ld=lldoutright with "invalid linker name in argument-fuse-ld=lld", breakingbun run rust:checkandbun bdon contributor macs.Why
Two places forced the flag on every non-windows target, including
x86_64-apple-darwin/aarch64-apple-darwin:scripts/build/cargo-config.tswrites.cargo/config.toml, which plaincargo check/ rust-analyzer read. The[target.*-apple-darwin]section carried-fuse-ld=lld, so the reporter'sbun run rust:checkhit the Homebrew clang++ error even without a bun build step.scripts/build/rust.tspushed the same flag intoCARGO_ENCODED_RUSTFLAGSfor the ninja build (bun bd/bun run build), sobun bdon darwin hit the same wall.The C++ side of the ninja build on darwin doesn't pass
--ld-path=…either (seeflags.ts):cfg.ldis empty on darwin and the clang driver just usesld64. Letting the rust side also default to the system linker is parity with the C++ side.Fix
scripts/build/rust.ts: factor the decision into an exportedrustForcesFuseLdLld(cfg)— windows never, darwin only under cross-lang LTO, linux/freebsd/android always.emitRustnow has a single guarded push. An explicit--ltobuild on darwin still gets the flag (samelld-capable clang++ requirement as linux LTO).scripts/build/cargo-config.ts: on darwin, emit the[target.*-apple-darwin]rustflagswithout-fuse-ld=lld. Thelinker = <clang++>line and the lint-quieting-Qunused-arguments/-A linker_messagesflags stay (they're driver/lint flags, valid on darwin) — only the linker selection is dropped, matchingrustForcesFuseLdLld.Linux/freebsd/android/windows paths are unchanged. No
src/or runtime changes: build-script only.Test
test/internal/macos-cross-config.test.ts(the existing home for darwin build-config tests) gains two pure-scripts cases:rustForcesFuseLdLldreturnsfalsefor a darwin non-LTO config,truefor darwin--ltoand for linux.generateCargoConfigwrites lld-free[target.*-apple-darwin]sections (lint-quieting flags retained) while linux keepslink-arg=-fuse-ld=lld.Fail-before verification
Reverting either half of the fix breaks a test (verified in isolation):
bun bdbuilds clean on linux and the fullmacos-cross-config.test.ts(21 tests) passes.Rebased onto
mainRebased to resolve a merge conflict.
mainhad since added-Qunused-arguments+-A linker_messagesto the rust rustflags (bothrust.tsandcargo-config.ts). Those are kept on every non-windows target including darwin; the rebase only re-applies the-fuse-ld=lldgating on top of them.Fixes #30870.