Repository navigation
build: remove the dead ELF regular-LTO summary fix-up (and the llvm-tools rustup component) - #41020
Conversation
…ols rustup component Every cross-language-LTO platform links ThinLTO with per-CGU Rust bitcode since #34782, and rustLtoLinkInputs() has returned its input unchanged ever since (crossLangLto implies lto, and it bailed on cfg.lto). Delete it, the rust_lto_fix rule, rust-lto-fix-cli.ts and the workaround entry, and stop listing llvm-tools in rust-toolchain.toml — nothing else used it, and it was the one rustup component a BUN_TOOLCHAIN_RUST build could not supply. Config.rustSysroot / Host.rustTriple lose their only reader and go too. No-Verification-Needed: build-script dead code; debug/release/link-only configures and the scripts/build tests are unchanged
|
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 (14)
💤 Files with no reviewable changes (9)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughChangesThe Rust build pipeline stops installing Rust build pipeline
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change removes an unused LTO fix-up and the unnecessary llvm-tools dependency without changing verified build configuration results; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline cleanup nits, I verified the core behavioral claim: crossLangLto = lto && … at config.ts:807, so the deleted rustLtoLinkInputs()'s early-return guard cfg.lto || !cfg.crossLangLto was always true — the function was the identity on every config and no ninja edge could reference rust_lto_fix. The removal itself is safe; what remains is finishing the dead-code sweep.
Extended reasoning...
The inline findings are all completeness nits on the dead-code sweep (leftover rustSysroot/rustHostTriple return fields in findRustLld(), three more mock Toolchain literals in test/internal/source-lints/ and build-rust-toolchain-probe.test.ts, and a now-unused join import). None of them speak to whether removing the fix-up itself is behaviorally safe, so I checked that directly: config.ts:807 sets const crossLangLto = lto && !(windows && host.os === "windows"), which means crossLangLto implies lto, and the deleted function's guard if (cfg.lto || !cfg.crossLangLto || …) return rustObjects therefore always short-circuited to identity. The rust_lto_fix rule registration was gated on cfg.crossLangLto && !cfg.darwin && !cfg.windows, but since no n.build edge could ever be emitted for it, the rule was dead in every generated build.ninja. A repo-wide grep confirms no remaining references to rust_lto_fix, rust-lto-fix, bun_runtime.lto.o, or llvm-tools outside tools.ts and the test files already flagged inline.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
test/internal/source-lints/build-rust.test.ts— nit: stalerustSysroot/rustHostTripleonToolchain-typed object literals — sweep:rustSysroot:|rustHostTriple:in test/internal/. The PR dropped these fields fromToolchainand updated four mock factories, but three more remain (source-lints/build-rust.test.ts, source-lints/webkit-prebuilt-url.test.ts, source-lints/windows-cross-config.test.ts); TypeScript's excess-property check on object literals will flag eachreturn { …, rustSysroot: undefined, rustHostTriple: undefined }: Toolchain. Fix: delete the two lines from every remaining mockToolchainliteral. [also at: scripts/build/config.ts:430 - RemovingrustSysroot/rustHostTriplefromToolchainupdated fourtest/internal/*.test.tsmock factories but…]Extended reasoning...
Toolchainin scripts/build/config.ts no longer declaresrustSysrootorrustHostTriple. The three test files construct object literals directly typed asToolchain(function mockToolchain(): Toolchain { return { … } }/const mockToolchain: Toolchain = { … }), which triggers TypeScript's excess-property check —tscreports "Object literal may only specify known properties, and 'rustSysroot' does not exist in type 'Toolchain'". On the base branch these compiled cleanly; after merge any typecheck coveringtest/internal/source-lints/fails, and the code contradicts REVIEW.md's "delete dead code in the same PR that makes it dead / update every constructor site". The four sibling files the PR did touch (build-codegen-declared-outputs, build-debug-info-flags, build-post-link-ordering, macos-cross-config) show the intended fix.Verification: nit — The
Toolchaininterface in scripts/build/config.ts dropsrustSysrootandrustHostTriple(diff removes lines at former :443-448), but three test files still put those fields on fresh object literals directly typed asToolchain: - test/internal/source-lints/build-rust.test.ts:38const mockToolchain: Toolchain = { …, rustSysroot: undefined, rustHostTriple: undefined, … }(lines… -
🟡
scripts/build/rust.ts— nit: thejoinimport fromnode:pathis now dead — its only call site wasjoin(cfg.rustSysroot, "lib", "rustlib", cfg.host.rustTriple, "bin")inside the deletedrustLtoLinkInputs(); every remainingjoin(in this file isArray.prototype.join. Fix: dropjoinfrom thenode:pathimport list (leavedirnameandresolve, both still used).Extended reasoning...
Base rust.ts line 886 was the sole path-style
join()call. After this PR, greppingscripts/build/rust.tsfor a barejoin((not preceded by.) yields zero hits — lines 159/680/779/809/835 are all<array>.join(sep).dirname(line 786) andresolve(lines 174/183/194/744/789) remain live.scripts/build/tsconfig.jsondoes not setnoUnusedLocals, so tsc won't flag it, but REVIEW.md requires deleting dead code in the same PR that makes it dead; this import became dead precisely becauserustLtoLinkInputs()was removed.Verification: nit — scripts/build/rust.ts:28 still reads
import { dirname, join, resolve } from "node:path";. Grepping the post-diff file for a bare (non-member)joinyields only line 28 itself; every otherjoin(occurrence (lines 159, 680, 779, 809, 835) is<array>.join(...). The sole path-join()call site wasjoin(cfg.rustSysroot, "lib", "rustlib", cfg.host.rustTriple, "bin")inside the…
| // rust-lld: optional alternative linker for cross-language LTO when | ||
| // rustc's bundled LLVM is newer than clang's. See findRustLld(). | ||
| const { rustLld, rustLlvmVersion, rustSysroot, rustHostTriple } = findRustLld(os); | ||
| const { rustLld, rustLlvmVersion } = findRustLld(os); |
There was a problem hiding this comment.
🟡 nit (optional): findRustLld() still declares and returns rustSysroot/rustHostTriple (lines ~660-664, 738, 748) but its sole caller resolveLlvmToolchain() now destructures only { rustLld, rustLlvmVersion }, leaving those return fields dead — REVIEW.md requires deleting code in the same PR that makes it dead. Fix: drop the two fields from findRustLld()'s return type and returned objects (keep the local sysroot/rustHostTriple computations — they still feed the gcc-ld/ld.lld path), and update test/internal/build-rust-toolchain-probe.test.ts:46-53's toEqual accordingly.
Extended reasoning...
Before this change resolveLlvmToolchain() destructured rustSysroot/rustHostTriple from findRustLld() and stamped them onto Toolchain; the diff removes that destructure (line 575) and drops them from the returned Pick<Toolchain, ...> (lines 417-418) and the returned object (lines 602-603), so nothing outside findRustLld() reads them anymore. findRustLld() still computes sysroot and parses rustHostTriple — both are needed internally to build the lib/rustlib/<triple>/bin path for locating rust-lld — but exposing them in the return signature is now dead surface. The PR description's own "Verified" section claims the workaround cleanup is complete, and REVIEW.md's "Delete dead code in the same PR that makes it dead" applies. No user-visible failure; this is code hygiene.
Verification: nit — The dead return fields are real and were made dead by this diff. findRustLld() at scripts/build/tools.ts:656 still declares rustSysroot: string | undefined (line 660) and rustHostTriple: string | undefined (line 662) in its return type, and populates them in the returned objects at lines 664, 738, and 748. The sole caller, resolveLlvmToolchain() at line 575, was changed in this…
|
@robobun fix the comment s |
|
Two of the three review nits are already in main: The last one, the dead |
…ld() (#41043) Follow-up to #41020. It finishes the last open review nit from that PR. ### Problem - `findRustLld()` in `scripts/build/tools.ts:656` still declares and returns `rustSysroot` and `rustHostTriple`. Its only caller, `resolveLlvmToolchain()` (`tools.ts:575`), reads `rustLld` and `rustLlvmVersion` only since #41020 dropped the two fields from `Toolchain`. - The two return fields have no reader. `test/internal/build-rust-toolchain-probe.test.ts` was the last one, and it only used them as a window into the probe. ### Fix - Drop `rustSysroot` and `rustHostTriple` from the return type, from `none`, and from the two return sites. The local `sysroot` and `rustHostTriple` values stay. They build the `lib/rustlib/<host>/bin` path that locates rust-lld. - The probe test now observes the pin through the real output. The fake sysroot has a `gcc-ld/ld.lld` only under `sysroot-for-<channel>/lib/rustlib/host-for-<channel>/`. With the pin, `findRustLld("linux")` returns that path. Without it, the fake rustc answers `…-for-unset`, the path does not exist, and `rustLld` is `undefined`. - Verified: `test/internal/build-rust-toolchain-probe.test.ts` passes. With `RUSTUP_TOOLCHAIN` removed from the probe env in `tools.ts`, it fails with `rustLld: undefined`. The eight `test/internal` build-script tests pass (55 tests). `tsc -p scripts/build` reports the same six pre-existing errors as main. ### Background - `findRustLld()` runs at configure time. It asks `rustc --print sysroot` and `rustc -vV` for the sysroot and the host triple, then looks for `<sysroot>/lib/rustlib/<host>/bin/gcc-ld/ld.lld`. The build links with that lld when rustc's LLVM is newer than clang's. - `RUSTUP_TOOLCHAIN` pins the rustup proxy to the channel from `rust-toolchain.toml`. Without the pin, a proxy run in the repo root installs every component and target the file lists (about 2.4 GB). The probe test exists to keep that pin in place. <details><summary>Notes</summary> - The other two nits from the #41020 review are already in main: `scripts/build/rust.ts` no longer imports `join`, and the three `test/internal/source-lints/` mock `Toolchain` literals no longer set `rustSysroot`/`rustHostTriple`. - The `tempDir` file map accepts a function value that receives `{ root }`. The fake rustc uses it to print an absolute sysroot path, so `rustLld` matches `join(String(dir), ldLld)` exactly. - Commands run: `USE_SYSTEM_BUN=1 bun test` on the eight `test/internal` build-script files, `bun bd test test/internal/build-rust-toolchain-probe.test.ts`, `bunx tsc --noEmit -p scripts/build/tsconfig.json` on this branch and on main, `bunx prettier --check` on both files. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/internal/build-rust-toolchain-probe.test.ts <!-- robobun:evidence:end -->
2 participants
What
Deletes
scripts/build/rust-lto-fix-cli.ts, therust_lto_fixninja rule,rustLtoLinkInputs()and its three call sites inbun.ts, therustc-no-regular-lto-summaryentry inworkarounds.ts, andllvm-toolsfromrust-toolchain.toml's components.Config.rustSysroot/Host.rustTriple(and theToolchainfields feeding them) lose their only reader and go too.Why
The fix-up re-emitted rustc's fat, summary-less LTO bitcode with a regular-LTO summary so lld wouldn't abort with "inconsistent LTO Unit splitting". Since #34782 every cross-language-LTO platform links ThinLTO with per-CGU Rust bitcode (
CARGO_PROFILE_RELEASE_LTO=off), andrustLtoLinkInputs()has been the identity ever since:crossLangLto = lto && …inconfig.ts, and it returned early oncfg.lto || !cfg.crossLangLto. Its own comment says "Delete this function once confirmed"; the workaround entry'scleanupspells out exactly this diff.The practical motivation:
llvm-toolswas the one thing aBUN_TOOLCHAIN_RUSTbuild (#40884 — a rustc sysroot without rustup) could not supply, since the fix-up self-healed withrustup component add llvm-tools.Verified
--configure-onlyfor debug, release andci-link-onlyunchanged (no edge ever referencedrust_lto_fix).grepfinds no remaining reference tollvm-tools/rust-lto-fix/bun_runtime.lto.oin scripts, bootstrap, Dockerfiles, CI or docs;findRustLld()usesgcc-ld/ld.lldfrom therustccomponent, notllvm-tools.tsc -p scripts/buildhas no new errors;test/internal/build-*.test.ts+macos-cross-config.test.ts: same results as main.