From 9ff8998805c76d472838f907e7e7cec53ba046f4 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 22 Jul 2026 08:18:28 +0000 Subject: [PATCH 1/2] build: stop git apply from silently skipping dep patches with a diff --git header fetch-cli's applyPatch runs git apply --no-index with cwd=vendor//, which is a subdirectory of the bun repo. git still discovers the enclosing repo, and for a patch with a 'diff --git a/... b/...' header treats the paths as toplevel-relative (git-apply(1): 'When running from a subdirectory in a repository, patched paths outside the directory are ignored'). The dep's src/foo.c is outside the vendor// prefix, so git prints 'Skipped patch ...' only under -v and exits 0 having changed nothing. applyPatch checked only the exit status, so the .ref stamp certified an unpatched tree. Set GIT_CEILING_DIRECTORIES=dirname(dest) so repo discovery stops above the dep dir (and drop inherited GIT_DIR/GIT_WORK_TREE which would bypass the ceiling). Add -v + LC_ALL=C and throw on 'Skipped patch' in stderr as a safety net. patches/libuv/win-poll-abort-with-disconnect.patch (added in #32488) has a diff --git header and was silently not being applied; with this change it applies. --- scripts/build/fetch-cli.ts | 35 +++++++- test/internal/dep-patch-apply.test.ts | 118 ++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 test/internal/dep-patch-apply.test.ts diff --git a/scripts/build/fetch-cli.ts b/scripts/build/fetch-cli.ts index 63d06d5fb8c4..cc5119cb2ddb 100644 --- a/scripts/build/fetch-cli.ts +++ b/scripts/build/fetch-cli.ts @@ -24,7 +24,7 @@ import { spawnSync } from "node:child_process"; import { createHash } from "node:crypto"; import { existsSync, readFileSync } from "node:fs"; import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; -import { basename, join } from "node:path"; +import { basename, dirname, join } from "node:path"; import { downloadWithRetry, extractTarGz, fetchPrebuilt } from "./download.ts"; import { BuildError, assert } from "./error.ts"; import { writeIfChanged } from "./fs.ts"; @@ -242,13 +242,32 @@ function normalizeLf(s: string): string { * so a CRLF-mangled checkout still applies cleanly. --no-index: dest/ is * not a git repo. --ignore-whitespace / --ignore-space-change: patches are * authored against upstream which may have different trailing whitespace. + * + * GIT_CEILING_DIRECTORIES stops git from discovering the enclosing bun + * repo. Without it, `--no-index` still runs setup_git_directory(), finds + * the bun repo, and for a patch with a `diff --git a/... b/...` header + * treats its paths as TOPLEVEL-relative ("When running from a subdirectory + * in a repository, patched paths outside the directory are ignored" — + * git-apply(1)): `src/foo.c` is outside the `vendor//` prefix, so git + * reports `Skipped patch '...'` (only under -v) and exits 0 having changed + * nothing — the .ref stamp then certifies an unpatched tree. Plain unified + * diffs (`--- a/X` / `+++ b/X` with no header) are resolved cwd-relative + * instead, which is why most of patches/ went unaffected. + * + * The skip check below is belt-and-suspenders against any remaining + * exit-0-but-skipped case. It needs -v (git only prints the skip message + * above normal verbosity) and LC_ALL=C (the message goes through gettext). */ -function applyPatch(dest: string, patchPath: string, patchBody: string): void { - const result = spawnSync("git", ["apply", "--ignore-whitespace", "--ignore-space-change", "--no-index", "-"], { +export function applyPatch(dest: string, patchPath: string, patchBody: string): void { + // Inherited GIT_DIR/GIT_WORK_TREE (git hooks, some CI wrappers) bypass + // ceiling-based discovery entirely and reintroduce the skip. + const { GIT_DIR: _d, GIT_WORK_TREE: _w, ...env } = process.env; + const result = spawnSync("git", ["apply", "-v", "--ignore-whitespace", "--ignore-space-change", "--no-index", "-"], { cwd: dest, input: normalizeLf(patchBody), stdio: ["pipe", "ignore", "pipe"], encoding: "utf8", + env: { ...env, GIT_CEILING_DIRECTORIES: dirname(dest), LC_ALL: "C" }, }); if (result.error) { @@ -264,6 +283,16 @@ function applyPatch(dest: string, patchPath: string, patchBody: string): void { hint: "The patch may be out of date with the pinned commit", }); } + + // Defense-in-depth: a file git decides is outside the worktree is + // skipped with exit 0. Nothing under patches/ is ever meant to be + // skipped — if it fires, GIT_CEILING_DIRECTORIES above didn't take. + if (result.stderr.includes("Skipped patch")) { + throw new BuildError(`git apply silently skipped a file:\n${result.stderr.trim()}`, { + file: patchPath, + hint: "The patch path resolved outside the dep source dir; see applyPatch() in fetch-cli.ts", + }); + } } // Only run if this file is the entry point (not imported as a module). diff --git a/test/internal/dep-patch-apply.test.ts b/test/internal/dep-patch-apply.test.ts new file mode 100644 index 000000000000..8589fa44275c --- /dev/null +++ b/test/internal/dep-patch-apply.test.ts @@ -0,0 +1,118 @@ +/** + * Regression tests for scripts/build/fetch-cli.ts::applyPatch. + * + * vendor// is a subdirectory of the bun repo's worktree. `git apply + * --no-index` still runs repo discovery, and from a subdirectory a patch + * with a `diff --git a/... b/...` header has its paths interpreted as + * TOPLEVEL-relative: "When running from a subdirectory in a repository, + * patched paths outside the directory are ignored" (git-apply(1)). The + * dep's `src/foo.c` is outside the `vendor//` prefix, so git says + * "Skipped patch '...'" (only under -v) and exits 0 having touched + * nothing. applyPatch used to check only the exit status, so such a patch + * was silently dropped and the .ref stamp certified an unpatched tree. + * + * These tests recreate the layout (git repo → subdir → extracted source) + * and drive applyPatch directly. + */ +import { describe, expect, test } from "bun:test"; +import { tempDir } from "harness"; +import { spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { applyPatch } from "../../scripts/build/fetch-cli.ts"; + +const ORIGINAL = "line one\nline two\nline three\n"; +const PATCHED_LINE = "line two (patched)"; + +/** A patch with a `diff --git` header, the form that tripped the bug. */ +const GIT_HEADER_PATCH = `diff --git a/src/file.c b/src/file.c +index 1111111..2222222 100644 +--- a/src/file.c ++++ b/src/file.c +@@ -1,3 +1,3 @@ + line one +-line two ++${PATCHED_LINE} + line three +`; + +/** The same change as a plain unified diff (what most of patches/ uses). */ +const PLAIN_PATCH = `--- a/src/file.c ++++ b/src/file.c +@@ -1,3 +1,3 @@ + line one +-line two ++${PATCHED_LINE} + line three +`; + +/** + * Recreate fetch-cli's environment: a git repo with an extracted dep + * source tree under vendor//. Returns the absolute dep dir (what + * fetch-cli passes as `dest`). + */ +function makeDepTree(): { dir: ReturnType; dest: string; target: string } { + const dir = tempDir("apply-patch", { + "vendor/mydep/src/file.c": ORIGINAL, + }); + // A real repo, so `git apply`'s setup_git_directory() finds a worktree + // above dest — the precondition for the toplevel-relative path rewrite. + const init = spawnSync("git", ["init", "-q"], { cwd: String(dir), encoding: "utf8" }); + if (init.status !== 0) throw new Error(`git init failed: ${init.stderr}`); + const dest = join(String(dir), "vendor", "mydep"); + return { dir, dest, target: join(dest, "src", "file.c") }; +} + +describe("applyPatch (scripts/build/fetch-cli.ts)", () => { + test("applies a diff --git patch from a repo subdirectory", () => { + const { dir, dest, target } = makeDepTree(); + using _ = dir; + + // Before the fix: git discovers the enclosing repo, treats "src/file.c" + // as toplevel-relative (outside the vendor/mydep/ prefix), prints + // "Skipped patch 'src/file.c'." only under -v, and exits 0 — applyPatch + // returns normally and the file is untouched. + applyPatch(dest, "test.patch", GIT_HEADER_PATCH); + + expect(readFileSync(target, "utf8")).toContain(PATCHED_LINE); + }); + + test("applies a plain unified diff from a repo subdirectory", () => { + const { dir, dest, target } = makeDepTree(); + using _ = dir; + + applyPatch(dest, "test.patch", PLAIN_PATCH); + + expect(readFileSync(target, "utf8")).toContain(PATCHED_LINE); + }); + + test("a patch that does not apply is reported as an error", () => { + const { dir, dest } = makeDepTree(); + using _ = dir; + + const bad = GIT_HEADER_PATCH.replace("-line two\n", "-does not exist\n"); + expect(() => applyPatch(dest, "test.patch", bad)).toThrow(/Patch failed/); + }); + + test("ignores inherited GIT_DIR / GIT_WORK_TREE", () => { + // A build kicked off from a git hook inherits GIT_DIR/GIT_WORK_TREE, + // which bypass GIT_CEILING_DIRECTORIES entirely. applyPatch must drop + // them so the ceiling takes effect. + const { dir, dest, target } = makeDepTree(); + using _ = dir; + + const saved = { GIT_DIR: process.env.GIT_DIR, GIT_WORK_TREE: process.env.GIT_WORK_TREE }; + process.env.GIT_DIR = join(String(dir), ".git"); + process.env.GIT_WORK_TREE = String(dir); + try { + applyPatch(dest, "test.patch", GIT_HEADER_PATCH); + expect(readFileSync(target, "utf8")).toContain(PATCHED_LINE); + } finally { + for (const [k, v] of Object.entries(saved)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + } + }); +}); From a6cd153f63c9ecf191d6cfe35a69bb4d64fdd3c1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 22 Jul 2026 08:47:08 +0000 Subject: [PATCH 2/2] Address review: invalidate stale libuv trees; tighten test assertions - Strip the diff --git / index header from win-poll-abort-with-disconnect.patch. The header change alters the patch-content hash, so computeSourceIdentity() produces a new .ref identity for libuv and existing vendor/libuv/ trees that were stamped against the silently-skipped patch are re-fetched. Also brings the patch in line with the other header-less entries under patches/. - Tighten the applyPatch tests to assert exact file contents with toBe() instead of toContain(), and trim the comment headers. --- .../win-poll-abort-with-disconnect.patch | 2 - test/internal/dep-patch-apply.test.ts | 48 ++++++------------- 2 files changed, 14 insertions(+), 36 deletions(-) diff --git a/patches/libuv/win-poll-abort-with-disconnect.patch b/patches/libuv/win-poll-abort-with-disconnect.patch index 92b49ffaeaa2..d22fef59a8ec 100644 --- a/patches/libuv/win-poll-abort-with-disconnect.patch +++ b/patches/libuv/win-poll-abort-with-disconnect.patch @@ -1,5 +1,3 @@ -diff --git a/src/win/poll.c b/src/win/poll.c -index ecc6cf3..361b0fc 100644 --- a/src/win/poll.c +++ b/src/win/poll.c @@ -115,7 +115,12 @@ static void uv__fast_poll_submit_poll_req(uv_loop_t* loop, uv_poll_t* handle) { diff --git a/test/internal/dep-patch-apply.test.ts b/test/internal/dep-patch-apply.test.ts index 8589fa44275c..5a7ab1736875 100644 --- a/test/internal/dep-patch-apply.test.ts +++ b/test/internal/dep-patch-apply.test.ts @@ -1,18 +1,8 @@ /** - * Regression tests for scripts/build/fetch-cli.ts::applyPatch. - * - * vendor// is a subdirectory of the bun repo's worktree. `git apply - * --no-index` still runs repo discovery, and from a subdirectory a patch - * with a `diff --git a/... b/...` header has its paths interpreted as - * TOPLEVEL-relative: "When running from a subdirectory in a repository, - * patched paths outside the directory are ignored" (git-apply(1)). The - * dep's `src/foo.c` is outside the `vendor//` prefix, so git says - * "Skipped patch '...'" (only under -v) and exits 0 having touched - * nothing. applyPatch used to check only the exit status, so such a patch - * was silently dropped and the .ref stamp certified an unpatched tree. - * - * These tests recreate the layout (git repo → subdir → extracted source) - * and drive applyPatch directly. + * Regression tests for scripts/build/fetch-cli.ts::applyPatch: `git apply` + * from a repo subdirectory treats `diff --git` patch paths as toplevel- + * relative and silently skips them with exit 0 (git-apply(1)). See the + * doc comment on applyPatch for the full mechanism. */ import { describe, expect, test } from "bun:test"; import { tempDir } from "harness"; @@ -23,7 +13,7 @@ import { join } from "node:path"; import { applyPatch } from "../../scripts/build/fetch-cli.ts"; const ORIGINAL = "line one\nline two\nline three\n"; -const PATCHED_LINE = "line two (patched)"; +const PATCHED = "line one\nline two (patched)\nline three\n"; /** A patch with a `diff --git` header, the form that tripped the bug. */ const GIT_HEADER_PATCH = `diff --git a/src/file.c b/src/file.c @@ -33,7 +23,7 @@ index 1111111..2222222 100644 @@ -1,3 +1,3 @@ line one -line two -+${PATCHED_LINE} ++line two (patched) line three `; @@ -43,21 +33,16 @@ const PLAIN_PATCH = `--- a/src/file.c @@ -1,3 +1,3 @@ line one -line two -+${PATCHED_LINE} ++line two (patched) line three `; -/** - * Recreate fetch-cli's environment: a git repo with an extracted dep - * source tree under vendor//. Returns the absolute dep dir (what - * fetch-cli passes as `dest`). - */ +/** git repo → vendor//src/file.c; returns the `dest` fetch-cli passes. */ function makeDepTree(): { dir: ReturnType; dest: string; target: string } { const dir = tempDir("apply-patch", { "vendor/mydep/src/file.c": ORIGINAL, }); - // A real repo, so `git apply`'s setup_git_directory() finds a worktree - // above dest — the precondition for the toplevel-relative path rewrite. + // Repo above dest is the precondition for the toplevel-relative rewrite. const init = spawnSync("git", ["init", "-q"], { cwd: String(dir), encoding: "utf8" }); if (init.status !== 0) throw new Error(`git init failed: ${init.stderr}`); const dest = join(String(dir), "vendor", "mydep"); @@ -69,13 +54,10 @@ describe("applyPatch (scripts/build/fetch-cli.ts)", () => { const { dir, dest, target } = makeDepTree(); using _ = dir; - // Before the fix: git discovers the enclosing repo, treats "src/file.c" - // as toplevel-relative (outside the vendor/mydep/ prefix), prints - // "Skipped patch 'src/file.c'." only under -v, and exits 0 — applyPatch - // returns normally and the file is untouched. + // Pre-fix: "Skipped patch 'src/file.c'." (only under -v), exit 0, file untouched. applyPatch(dest, "test.patch", GIT_HEADER_PATCH); - expect(readFileSync(target, "utf8")).toContain(PATCHED_LINE); + expect(readFileSync(target, "utf8")).toBe(PATCHED); }); test("applies a plain unified diff from a repo subdirectory", () => { @@ -84,7 +66,7 @@ describe("applyPatch (scripts/build/fetch-cli.ts)", () => { applyPatch(dest, "test.patch", PLAIN_PATCH); - expect(readFileSync(target, "utf8")).toContain(PATCHED_LINE); + expect(readFileSync(target, "utf8")).toBe(PATCHED); }); test("a patch that does not apply is reported as an error", () => { @@ -96,9 +78,7 @@ describe("applyPatch (scripts/build/fetch-cli.ts)", () => { }); test("ignores inherited GIT_DIR / GIT_WORK_TREE", () => { - // A build kicked off from a git hook inherits GIT_DIR/GIT_WORK_TREE, - // which bypass GIT_CEILING_DIRECTORIES entirely. applyPatch must drop - // them so the ceiling takes effect. + // These (set by git hooks) bypass GIT_CEILING_DIRECTORIES entirely. const { dir, dest, target } = makeDepTree(); using _ = dir; @@ -107,7 +87,7 @@ describe("applyPatch (scripts/build/fetch-cli.ts)", () => { process.env.GIT_WORK_TREE = String(dir); try { applyPatch(dest, "test.patch", GIT_HEADER_PATCH); - expect(readFileSync(target, "utf8")).toContain(PATCHED_LINE); + expect(readFileSync(target, "utf8")).toBe(PATCHED); } finally { for (const [k, v] of Object.entries(saved)) { if (v === undefined) delete process.env[k];