Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions scripts/build/bun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ export function emitBun(n: Ninja, cfg: Config, sources: Sources): BunOutput {
// Must run BEFORE stripping could discard sections it needs (we don't
// strip bun-profile itself, only copy → bun, so this is safe).
if (cfg.darwin) {
dsym = emitDsymutil(n, cfg, exe, exeName);
dsym = emitDsymutil(n, cfg, exe, exeName, strippedExe);
}
}

Expand All @@ -542,7 +542,7 @@ export function emitBun(n: Ninja, cfg: Config, sources: Sources): BunOutput {
// ASAN binaries to run from subprocesses (shadow memory layout conflict
// with ELF_ET_DYN_BASE, see sanitizers/856). We try with setarch first,
// fall back to direct invocation.
emitSmokeTest(n, cfg, exe, exeName);
emitSmokeTest(n, cfg, exe, exeName, strippedExe);

return { exe, strippedExe, dsym, deps, codegen, rustObjects, objects: allObjects };
}
Expand Down Expand Up @@ -656,10 +656,10 @@ function emitLinkOnly(n: Ninja, cfg: Config): BunOutput {
let dsym: string | undefined;
if (shouldStrip(cfg)) {
strippedExe = emitStrip(n, cfg, exe, flags.stripflags);
if (cfg.darwin) dsym = emitDsymutil(n, cfg, exe, exeName);
if (cfg.darwin) dsym = emitDsymutil(n, cfg, exe, exeName, strippedExe);
}
if (strippedExe === undefined) n.phony("bun", [exe]);
emitSmokeTest(n, cfg, exe, exeName);
emitSmokeTest(n, cfg, exe, exeName, strippedExe);

return {
exe,
Expand Down Expand Up @@ -737,10 +737,10 @@ function emitRustAndLink(n: Ninja, cfg: Config, sources: Sources): BunOutput {
let dsym: string | undefined;
if (shouldStrip(cfg)) {
strippedExe = emitStrip(n, cfg, exe, flags.stripflags);
if (cfg.darwin) dsym = emitDsymutil(n, cfg, exe, exeName);
if (cfg.darwin) dsym = emitDsymutil(n, cfg, exe, exeName, strippedExe);
}
if (strippedExe === undefined) n.phony("bun", [exe]);
emitSmokeTest(n, cfg, exe, exeName);
emitSmokeTest(n, cfg, exe, exeName, strippedExe);

return {
exe,
Expand All @@ -758,8 +758,22 @@ function emitRustAndLink(n: Ninja, cfg: Config, sources: Sources): BunOutput {
* errors, the build failed — typically means a link-time issue that the
* linker didn't catch (missing symbol only referenced at init, ICU ABI
* mismatch, etc.).
*
* `strippedExe`: the strip output (release builds only). Added as an
* order-only input so this rule never runs while strip is mid-write: the
* rule command's `cfg.jsRuntime` wrapper is `process.execPath`, which can
* BE the strip output when `bun` on PATH resolves into the build directory
* (build/release/bun). Without the ordering, ninja runs strip and this edge
* concurrently (both depend only on `exe`) and the wrapper exec fails with
* "Permission denied" on the half-written file.
*/
function emitSmokeTest(n: Ninja, cfg: Config, exe: string, exeName: string): void {
export function emitSmokeTest(
n: Ninja,
cfg: Config,
exe: string,
exeName: string,
strippedExe: string | undefined,
): void {
// Skip when the binary can't run on this host (different os/arch/abi) —
// `ninja check` becomes a no-op alias for the exe.
if (!cfg.canRunOnHost) {
Expand Down Expand Up @@ -804,6 +818,7 @@ function emitSmokeTest(n: Ninja, cfg: Config, exe: string, exeName: string): voi
outputs: [stamp],
rule: "smoke_test",
inputs: [exe],
...(strippedExe !== undefined ? { orderOnlyInputs: [strippedExe] } : {}),
});

// Phony target — `ninja check` runs the smoke test.
Expand Down Expand Up @@ -860,8 +875,11 @@ function emitStrip(n: Ninja, cfg: Config, inputExe: string, stripflags: string[]
* Runs dsymutil on bun-profile (which has full DWARF). The .dSYM lets you
* symbolicate crash logs from the stripped `bun` — lldb/Instruments find
* it automatically by UUID.
*
* `strippedExe` is order-only for the same reason as emitSmokeTest: the
* `cfg.jsRuntime` wrapper may be the strip output itself.
*/
function emitDsymutil(n: Ninja, cfg: Config, inputExe: string, exeName: string): string {
export function emitDsymutil(n: Ninja, cfg: Config, inputExe: string, exeName: string, strippedExe: string): string {
assert(cfg.darwin, "dsymutil is darwin-only");
assert(cfg.dsymutil !== undefined, "dsymutil not found in toolchain");

Expand Down Expand Up @@ -892,6 +910,7 @@ function emitDsymutil(n: Ninja, cfg: Config, inputExe: string, exeName: string):
outputs: [out],
rule: "dsymutil",
inputs: [inputExe],
orderOnlyInputs: [strippedExe],
});

return out;
Expand Down
128 changes: 128 additions & 0 deletions test/internal/build-post-link-ordering.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/**
* Regression tests for ninja ordering of post-link steps (strip, smoke test,
* dsymutil) in scripts/build/bun.ts.
*
* The smoke_test and dsymutil rule commands are wrapped through
* `cfg.jsRuntime` (= process.execPath). When `bun` on PATH resolves inside the
* build directory, that path is the strip output itself (build/release/bun),
* and without an ordering edge ninja will run strip and the wrapper exec
* concurrently, failing with "Permission denied" on the half-written file.
*
* These exercise the ninja-emission logic only (no compiler or ninja needed),
* so they run on every host.
*/
import { describe, expect, test } from "bun:test";
import { isMacOS, tempDir } from "harness";
import { join, resolve } from "node:path";

import { emitDsymutil, emitSmokeTest } from "../../scripts/build/bun.ts";
import { resolveConfig, type Config, type PartialConfig, type Toolchain } from "../../scripts/build/config.ts";
import { Ninja } from "../../scripts/build/ninja.ts";

/** A fully-populated fake toolchain; resolveConfig never spawns any of these. */
function mockToolchain(overrides: Partial<Toolchain> = {}): Toolchain {

Check warning on line 23 in test/internal/build-post-link-ordering.test.ts

View check run for this annotation

Claude / Claude Code Review

mockToolchain() is the fourth copy of this helper in test/internal/

This is the fourth copy of `mockToolchain()` in `test/internal/` — near-identical helpers already exist in `macos-cross-config.test.ts`, `source-lints/webkit-prebuilt-url.test.ts`, and `source-lints/windows-cross-config.test.ts`. The copies are already drifting (this one had to add `hostCc`/`hostCxx` that the others lack), so it may be worth extracting a shared `mockToolchain()` into something like `test/internal/build-helpers.ts` and importing it in all four places. Non-blocking — test-only, an
Comment thread
robobun marked this conversation as resolved.
return {
cc: "/fake/llvm/bin/clang",
cxx: "/fake/llvm/bin/clang++",
hostCc: undefined,
hostCxx: undefined,
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: undefined,
cargoHome: undefined,
rustupHome: undefined,
msvcLinker: undefined,
rc: undefined,
mt: undefined,
nasm: undefined,
...overrides,
};
}

/**
* Resolve a host-targeted config: no os/arch override, so `canRunOnHost` is
* true and emitSmokeTest emits the real rule (not the phony short-circuit).
*/
function hostConfig(partial: PartialConfig, buildDir: string): Config {
return resolveConfig(
{ buildDir, ...partial },
// jsRuntime = the strip output: what resolveToolchain() produces when
// `bun` on PATH resolves into build/release/.
mockToolchain({ jsRuntime: join(buildDir, "bun") }),
);
}

/** Find one build-edge line in the generated ninja text (continuations unwrapped). */
function buildEdge(ninja: string, rule: string): string {
const flat = ninja.replace(/\$\n {2}/g, "");
const line = flat.split("\n").find(l => l.startsWith("build ") && l.includes(`: ${rule} `));
if (line === undefined) throw new Error(`no '${rule}' edge in ninja output:\n${ninja}`);
return line;
}

Check warning on line 77 in test/internal/build-post-link-ordering.test.ts

View check run for this annotation

Claude / Claude Code Review

buildEdge() unwrap regex doesn't match Ninja's actual continuation format

The unwrap regex `/\$\n {2}/g` doesn't match what `wrapLongLine()` in `scripts/build/ninja.ts` actually emits — continuations are ` $` + newline + **4** spaces, so a wrapped `foo $\n bar` would unwrap to `foo bar` (3 stray spaces) and the exact `.toBe()` assertions below would fail with a confusing whitespace diff. Currently latent (these edges are ~51-73 chars, under the 120-char wrap threshold), but `/ \$\n {4}/g` → `" "` would make the helper match its own docstring.
Comment thread
robobun marked this conversation as resolved.

describe("post-link ninja ordering", () => {
test("release smoke_test is ordered after strip", () => {
using dir = tempDir("build-post-link", {});
const buildDir = String(dir);
const cfg = hostConfig({ buildType: "Release" }, buildDir);
expect(cfg.canRunOnHost).toBe(true);

const n = new Ninja({ buildDir });
const exe = resolve(buildDir, `bun-profile${cfg.exeSuffix}`);
const strippedExe = resolve(buildDir, `bun${cfg.exeSuffix}`);
emitSmokeTest(n, cfg, exe, "bun-profile", strippedExe);

// strip writes `bun`; the smoke_test wrapper execs cfg.jsRuntime
// (= `bun` here). Without `|| bun` ninja schedules them concurrently
// and the wrapper sees a half-written file.
expect(buildEdge(n.toString(), "smoke_test")).toBe(
`build bun-profile.smoke-test-passed: smoke_test bun-profile${cfg.exeSuffix} || bun${cfg.exeSuffix}`,
);
});

test("debug smoke_test has no strip dep (nothing to order against)", () => {
using dir = tempDir("build-post-link", {});
const buildDir = String(dir);
const cfg = hostConfig({ buildType: "Debug", assertions: true }, buildDir);

const n = new Ninja({ buildDir });
const exe = resolve(buildDir, `bun-debug${cfg.exeSuffix}`);
emitSmokeTest(n, cfg, exe, "bun-debug", undefined);

expect(buildEdge(n.toString(), "smoke_test")).toBe(
`build bun-debug.smoke-test-passed: smoke_test bun-debug${cfg.exeSuffix}`,
);
});

// Cross-config path only: on macOS, resolveConfig({ os: "darwin" }) probes
// xcode-select for the real SDK, which belongs to the native test above.
// The ordering logic is identical to the smoke_test case.
test.skipIf(isMacOS)("darwin release dsymutil is ordered after strip", () => {
using dir = tempDir("build-post-link", {});
const buildDir = String(dir);
const cfg = resolveConfig({ os: "darwin", arch: "aarch64", buildType: "Release", buildDir }, mockToolchain());

const n = new Ninja({ buildDir });
const exe = resolve(buildDir, "bun-profile");
const strippedExe = resolve(buildDir, "bun");
emitDsymutil(n, cfg, exe, "bun-profile", strippedExe);

expect(buildEdge(n.toString(), "dsymutil")).toBe("build bun-profile.dSYM: dsymutil bun-profile || bun");
});
});
Loading