build: order smoke_test and dsymutil after strip - #36139
Conversation
The smoke_test and dsymutil rule commands wrap through cfg.jsRuntime (process.execPath). When bun on PATH resolves into the build directory, that path is the strip output itself (build/release/bun). Both edges depended only on the linked exe, so ninja could schedule them concurrently with strip, and the wrapper exec would fail with 'Permission denied' on the half-written file. Adds the stripped exe as an order-only input to both edges whenever strip runs. Covers all three build modes (full, link-only, rust-and-link).
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused build-graph ordering fix; the two inline notes are non-blocking nits.
Extended reasoning...
Overview
Adds the stripped executable as an order-only ninja input to the smoke_test and dsymutil build edges so they can't run concurrently with strip. Threads a new strippedExe parameter through emitSmokeTest / emitDsymutil at all three linking call sites (full, link-only, rust-and-link) in scripts/build/bun.ts, exports both helpers for test access, and adds test/internal/build-post-link-ordering.test.ts asserting the emitted edge shape for release, debug, and darwin-cross configs.
Security risks
None. This only changes ninja dependency-graph metadata in the build scripts; no runtime code, no user input, no auth/crypto surface.
Level of scrutiny
Low-to-moderate. It's build tooling, not shipped runtime — a mistake would surface as a build failure rather than a user-facing bug. The mechanism (order-only || inputs) is the ninja-correct model for "must exist first but content isn't read", and I confirmed BuildNode.orderOnlyInputs is already supported in scripts/build/ninja.ts and rendered as || <path> in the format the tests assert. emitDsymutil taking strippedExe: string (non-optional) is safe because every call site is inside if (shouldStrip(cfg)) where strippedExe is always defined; emitSmokeTest correctly keeps it optional and only spreads orderOnlyInputs when present, so debug builds emit an unchanged edge.
Other factors
The PR description gives a clear root cause (jsRuntime = process.execPath resolving to the strip output) with before/after ninja -t query output, and explicitly considered/rejected two alternatives. All sibling call sites are covered — I checked there are no other emitSmokeTest/emitDsymutil callers. The two inline findings are both nits: the test helper's continuation-unwrap regex is latent (the asserted edges are well under the 120-char wrap threshold) and the mockToolchain duplication follows an established pattern in test/internal/. Neither affects correctness of the fix.
|
Updated 1:52 PM PT - Jul 27th, 2026
✅ @robobun, your commit bd2e594baedf1ff1e4f385947e6f36ad28462330 passed in 🧪 To try this PR locally: bunx bun-pr 36139That installs a local version of the PR into your bun-36139 --bun |
The strip/dsymutil/phony/smoke-test sequence was open-coded in all three linking modes (full, link-only, rust-and-link); the partial fix in #30539 missed one of them. Fold it into a single emitPostLink() so any future mode gets the ordering for free, and retarget the test at the helper all call sites use instead of the leaf functions. Also fix the ninja-continuation unwrap regex in the test helper to match what wrapLongLine actually emits (space, $, newline, 4+ spaces).
There was a problem hiding this comment.
LGTM — the order-only dep is the right ninja model, and folding the three call sites into emitPostLink removes the duplication that let #30539 miss one.
What was reviewed:
- Refactor is behavior-preserving: strip/dsymutil/phony/smoke-test emit in the same order and under the same
shouldStrip/cfg.darwinguards as before, at all three call sites. orderOnlyInputsis a realBuildNodefield andemitDsymutil's newstrippedExe: stringparam is always defined at its only call site (insideif (shouldStrip)).- Both prior nits are resolved — the unwrap regex now matches
wrapLongLine()'s output, and themockToolchaindedup is reasonably deferred. - Test's
buildEdge(out, "phony")first-match reliance checked: thebunphony is emitted beforecheckin the debug case, and onlycheckexists in the darwin-cross case.
Extended reasoning...
Overview
Two files: scripts/build/bun.ts extracts the post-link sequence (strip → dsymutil → bun phony → smoke test) from three linking modes (emitBun, emitLinkOnly, emitRustAndLink) into a single exported emitPostLink(), and threads the strip output through as an order-only input on the smoke_test and dsymutil build edges. test/internal/build-post-link-ordering.test.ts is new and asserts the exact ninja edge text for release (order-only || bun present), debug (absent, plus the bun phony), and darwin-cross (dsymutil ordered, smoke test short-circuits to a check phony).
Security risks
None. This is configure-time ninja graph emission in scripts/; no runtime code, no user input, no network, no crypto.
Level of scrutiny
Low-to-moderate. Build infrastructure only — a mistake here fails the build loudly rather than shipping a broken binary. The refactor is mechanical (three copies → one helper) and I diffed each old block against emitPostLink's body: same guards, same call order, same phony logic. The only semantic addition is the orderOnlyInputs: [strippedExe] on two n.build() calls, which is the textbook ninja idiom for "must exist first, don't rebuild me on change" and matches the PR's stated intent.
Other factors
- The PR description explains both the mechanism (
cfg.jsRuntimecan be the strip output) and why alternatives (rejecting the config, dropping the wrapper) are worse. - Test coverage hits the variant matrix REVIEW.md asks for: strip-on / strip-off, and the darwin-only
dsymutilpath via a cross-config. The.skipIf(isMacOS)on the darwin test is justified in-comment (native macOSresolveConfigprobesxcode-select). - I confirmed
orderOnlyInputsexists onBuildNode(scripts/build/ninja.ts:66) and thatphony()emitsbuild <name>: phony <deps>matching the test's.toBe()strings. - Both of my earlier inline nits are resolved: the continuation-unwrap regex is now
/ \$\n +/g→" "(matcheswrapLongLine()), and themockToolchain()duplication is acknowledged as a follow-up — reasonable scope call for a focused fix. - No CODEOWNERS entry covers
scripts/.
2 participants
Problem
When
bunon PATH resolves into the build directory (e.g.build/releaseis first on PATH, soprocess.execPath == /path/to/build/release/bun),cfg.jsRuntimegets baked into thesmoke_testanddsymutilninja rule commands as the stream.ts wrapper. But those build edges only declarebun-profileas an input, not the strippedbun, so ninja is free to run strip and the wrapper concurrently (both depend only on the link output). The wrapper then execsbuild/release/bunwhile strip is writing it:Generated ninja before this change:
Fix
Extract an
emitPostLink()helper that owns the strip / dsymutil /bunphony / smoke-test sequence for every linking mode (full, link-only, rust-and-link), and have it add the stripped exe as an order-only input to thesmoke_testanddsymutiledges. After:Ninja now serializes strip before those edges; the wrapper never sees a half-written file. Order-only (not implicit) because the edges don't read the stripped binary's content, they only need it to not be mid-write. Debug builds (no strip) are unchanged.
The sequence was previously open-coded in all three modes; #30539's partial fix missed
emitRustAndLink. Folding it into one helper means any future mode gets the ordering for free. The refactor produces byte-identicalbuild.ninjaoutput for every profile.Why this is the right fix
Refusing a
jsRuntimeinsidebuildDirat configure time would break any environment where the onlybunis the build output (no~/.bun/bin/bun,build/releaseon PATH). Dropping the stream.ts wrapper would lose the Windows console buffering compensation. Declaring the actual dependency is the ninja-correct model and costs ~1s of serialization on release builds.Verification
New
test/internal/build-post-link-ordering.test.tsasserts the edge shape throughemitPostLinkfor release (order-only dep present), debug (absent,bunphony instead), and darwin-target configs (dsymutil ordered too).Note: the fix is in
scripts/, notsrc/, so the automated fail-before check (which stashessrc/ packages/only) cannot revert it; the before/after ninja graph above is the proof.Related
#30539 carries a partial version of this fix (smoke_test only, two of three call sites) bundled with an unrelated bundler change; this standalone PR supersedes that portion so it can land independently.
no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.