Conversation
…eleted Ninja re-runs an edge when an input is newer than its outputs or when the command line changed. A file that disappeared from an edge's input list is neither, so after deleting a src/js module (or a .bind.ts, a scraped .rs file, a bake or bun-error source) the reconfigured edge was up to date and the generated output kept the deleted file's code until some surviving input was edited. emitCppBind already handled this for cppbind by writing cxx-sources.txt at configure time with writeIfChanged and listing it as an implicit input. Move that into sourceListFile() and use it for bundle-modules, bindgen, generate-host-exports, bake-codegen and bun-error as well. Steps that pass their file list on the command line (generate-classes, bindgenv2, build-fallbacks) already re-run through ninja's command-line tracking.
|
Warning Review limit reached
Next review available in: 13 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 (3)
Comment |
|
Status: reproduced and fixed, waiting on CI.
|
|
Updated 10:19 AM PT - Aug 13th, 2026
✅ @robobun, your commit 085f257e807573a6030c720674fdd928e36dd2a2 passed in 🧪 To try this PR locally: bunx bun-pr 38038That installs a local version of the PR into your bun-38038 --bun |
There was a problem hiding this comment.
LGTM — generalizes the existing cxx-sources.txt pattern to the five other glob-driven codegen steps so ninja re-runs them when an input is deleted.
What was reviewed:
sourceListFile()is a byte-for-byte extraction of the inlineemitCppBindlogic (same pathcxx-sources.txt, same repo-relative/forward-slash/newline-terminated format), so existing trees don't spuriously re-run cppbind.- The five new manifests are pure additions to
implicitInputs;writeIfChangedkeeps mtime stable on unchanged reconfigures, and the helper's ownmkdirSynccovers the first-configure case now that it runs beforeemitCppBind. - The test follows the existing
source-lintsmockToolchain/resolveConfig pattern (windows-cross-config.test.ts), reads only the realpackages/bun-error/package.jsonfrom the repo, and spawns nothing.
Extended reasoning...
Overview
This PR fixes a build-system incrementality gap: on a warm tree, deleting a file that a codegen step globs (e.g. rm src/js/internal/foo.ts) did not re-run that step, leaving stale generated code in the binary until some surviving input was edited. It touches only scripts/build/codegen.ts (build tooling, ~50 lines net), scripts/build/CLAUDE.md (one sentence), and adds a new hermetic test in test/internal/source-lints/.
The fix extracts the existing inline cxx-sources.txt write from emitCppBind into a shared sourceListFile() helper and applies it as an implicit input to five more steps: emitJsModules, emitBindgen, emitHostExports, emitBakeCodegen, emitBunError. Six previously-private emitters and the Ctx type are exported so the test can drive them without going through emitCodegen() (which would spawn bun for bindgenv2 list-outputs).
Security risks
None. This is configure-time build tooling that writes text manifests into the build directory and adds ninja edge inputs. It does not touch runtime code, network, auth, or anything that ships in the binary.
Level of scrutiny
Low-to-moderate. Build-system-only; a bug here could at worst cause spurious codegen re-runs (mitigated by writeIfChanged + restat=1) or, in the other direction, leave the incrementality gap unfixed. CI does clean builds, so it can't affect release artifacts. The mechanism is not new — it is the exact pattern emitCppBind has used for cxx-sources.txt all along, generalized to a helper. I verified the refactored emitCppBind produces the same filename and content format as before, so existing trees don't churn.
Other factors
- The PR description is unusually thorough: it includes real
ninja -d explaintranscripts demonstrating the bug and the fix, a synthetic ninja check confirming the command-line-carrying steps (generate-classes, bindgenv2, build-fallbacks) are unaffected, andtsc --noEmitverification. - The test is hermetic (no ninja, no compiler, no subprocess), uses
tempDir, and follows the established mockToolchain/resolveConfig pattern fromwindows-cross-config.test.tsin the same directory. It asserts both structure (each edge declares its manifest as an implicit input, manifest content matches the fed files) and behavior (mtime stable across an unchanged reconfigure; only the changed manifest is rewritten when a file is dropped). The author reports it fails withscripts/stashed and with only the manifest wiring removed. - The one repo-file dependency the test picks up transitively (
packages/bun-error/package.jsonviaemitBunInstall) exists and is a stable build input. - No prior human or bot reviews to defer to; only a CodeRabbit rate-limit notice.
There was a problem hiding this comment.
LGTM — generalizes the existing cxx-sources.txt manifest pattern to the five other glob-driven codegen steps so ninja re-runs them when an input is deleted.
What was reviewed:
sourceListFile()is a byte-for-byte extraction of the oldemitCppBindinline logic (samecxx-sources.txtpath/format, so warm trees don't re-run cppbind);mkdirSyncnow fires earlier viaemitBunErrorbut was never the first codegenDir writer anyway.- Manifests are added as implicit inputs only — the codegen/esbuild rules don't read
$in, so command lines are unchanged and no-op reconfigures stayno work to do(verified viawriteIfChangedinfs.tsand the mtime assertion in the second test). - The new test follows the existing
windows-cross-config.test.tspattern (mock toolchain +resolveConfig) and lives insource-lints/per that dir's README; it pins both the edge wiring and the write-only-when-changed invariant.
Extended reasoning...
Overview
This PR fixes a build-system incrementality gap: deleting a file that a codegen step globs (e.g. rm src/js/internal/foo.ts) did not re-run that step on a warm tree, because ninja only re-runs an edge when an input is newer or the command line changed — a file that vanished from the input list is neither. The fix extracts the existing cxx-sources.txt manifest logic from emitCppBind into a sourceListFile() helper and applies it to the five other affected steps (emitJsModules, emitBindgen, emitHostExports, emitBakeCodegen, emitBunError). Each writes codegen/<name>-sources.txt at configure time via writeIfChanged and lists it as an implicit input, so a deletion becomes a manifest rewrite that ninja can see. Three files touched: scripts/build/codegen.ts (+62/-24), a new test in test/internal/source-lints/, and a one-line scripts/build/CLAUDE.md doc update.
Security risks
None. This is configure-time build-graph generation; it does not touch runtime code, user input, network, auth, or anything shipped in the binary. The only new I/O is writing five small text files under the build directory at configure time.
Level of scrutiny
Low-to-medium. The mechanism is not new — emitCppBind has done exactly this for cxx-sources.txt all along, and the helper preserves that path and content byte-for-byte (verified: same relative(cfg.cwd, p).replace(/\\\\/g, '/') + trailing newline), so existing trees keep their mtime and don't spuriously re-run cppbind. The five new manifests are pure additions to implicitInputs; the codegen/esbuild rules use $args rather than $in, so command lines are untouched and the only observable effect is the intended one. The PR description includes real ninja -d explain transcripts demonstrating both the bug and the fix, plus a synthetic check confirming the command-line-carrying steps (generate-classes, bindgenv2, node-fallbacks) already handle deletion via ninja's command-hash tracking and correctly do not need a manifest.
Other factors
- The test is well-placed (
test/internal/source-lints/per its README: build-script unit tests that never touch the bun binary) and follows the establishedwindows-cross-config.test.tspattern of mockToolchain+resolveConfig. It emits the six steps into a temp dir, parses the ninja text back, and asserts (a) each edge declares its manifest as an implicit input, (b) every line in each manifest is also a tracked edge input (so edits still re-run as before), (c) an unchanged reconfigure leaves every manifest's content and mtime untouched, and (d) dropping one JS source rewrites onlyjs-sources.txt. Hermetic, no subprocess, usestempDirwithusing. - Exporting
Ctxand the six emitters is scoped to internal build scripts (not public API) and is justified in a comment:emitCodegen()as a whole spawnsbunfor bindgenv2list-outputs, which the test needs to avoid. - The one-time cost (five steps re-run once on trees configured before this change, when their manifests first appear) is disclosed in the description and is the expected behavior for any new implicit input.
- CI reports +0.0 KB on every platform binary, consistent with a change that only affects when codegen re-runs, not what it produces.
Problem
rm src/js/internal/foo.ts && bun bdleavesInternalModuleRegistry+enum.h(and the module's code) in the binary until some surviving input of the same step is edited. Reproduced on linux-x64 with a probe module: after removing it,ninja -d explain codegen/InternalModuleRegistry+enum.hreportsno work to doand the header still containsInternalZzProbe(transcript below).scripts/build/codegen.tslist the globbed files as inputs and nothing carries the input set. Ninja re-runs an edge when an input is newer than its outputs or the command line changed; after a deletion, configure regeneratesbuild.ninjawith a shorter input list, the command is unchanged, and the outputs are newer than every remaining input, so the edge is up to date.emitJsModules(bundle-modules readdirssrc/js),emitBindgen(bindgen.ts scans for.bind.ts),emitHostExports(scrapessrc/runtime+src/jsc.rsfiles),emitBakeCodegenandemitBunError(bundles; which files exist decides how imports resolve).emitCppBindalready handled it by writingcodegen/cxx-sources.txtat configure time and listing it as an implicit input.emitGeneratedClasses,emitBindgenV2andemitNodeFallbackspass the file list on the command line, so a deletion changes the command and ninja re-runs them ("command line changed"); checked with a synthetic ninja file. The cargo edge is also left alone: a.rsdeletion that matters comes with an edit to the file that declared the module, and cargo's own fingerprinting decides what recompiles.Fix
codegen.ts: the cxx-sources.txt logic becomessourceListFile(cfg, name, files), which writescodegen/<name>-sources.txtwithwriteIfChangedand returns the path.emitCppBinduses it (same path and content as before, so existing trees do not re-run cppbind), and the five steps above list their manifest as an implicit input:js-sources.txt,bindgen-sources.txt,host-exports-sources.txt,bake-sources.txt,bun-error-sources.txt.bun bdalways does). The codegen rule hasrestat = 1and the scripts usewriteIfNotChanged, so a set change whose output is unchanged re-runs the step and nothing downstream, as withcxx-sources.txttoday.test/internal/source-lints/build-codegen-source-lists.test.ts(the source-lints dir per its README: build-script unit test, no bun binary involved): emits the six steps into a temp build dir with made-up source lists and checks that each edge has its manifest as an implicit input listing exactly the files the edge is fed (for host-exports, only the.rsfiles in the scrape scope), that an unchanged reconfigure rewrites none of them (content and mtime), and that dropping one JS module rewrites onlyjs-sources.txt. Passes withbun bd test; fails withscripts/stashed, and also fails (js-sources.txtmissing) with only the manifest wiring removed and the exports kept. To make the steps reachable withoutemitCodegen()(which spawns bun for bindgenv2'slist-outputs, several seconds per call under a debug build),Ctxand the six emitters are exported.bun bd(explainnamesjs-sources.txt) and the module is gone; a reconfigure with no changes is stillno work to do; removing a.bind.tsdirties the bindgen edge the same way (transcripts below).bunx tsc --noEmit -p scripts/build/tsconfig.jsonreports no errors incodegen.ts; the rest oftest/internal/source-lints/and the existingtest/internal/build-*tests still pass underbun bd test.scripts/build/CLAUDE.md: the "Add a codegen step" entry says when a step needs a manifest.Background
.ninja_log. The declared inputs come frombuild.ninja, which configure regenerates from a fresh glob on everybun bd; a file that is no longer in the list is simply not stat'd, so its removal is invisible unless something else records that the list changed.| fileon a build line): tracked for dirtiness exactly like an explicit input, but not passed to the command as$in. The codegen and esbuild rules do not use$inat all, which is why the list itself is not on the command line and why the manifest goes in this slot.writeIfChanged(scripts/build/fs.ts): configure-time write that skips the write when the content is identical, so the file's mtime only moves when the content does.restat = 1on the codegen rule: after the command runs, ninja re-stats the outputs and prunes downstream edges whose inputs did not actually change, and records the newest input mtime so an input that triggered a no-op run does not trigger it again.Two neighbouring gaps found on the way are filed separately and not changed here: the bundle-modules edge does not list
src/jsc/modules/NativeModuleList.horsrc/js/builtins/BunBuiltinNames.h, which its scripts read, and the PCH picking up a regenerated header one build late is #37992.Probe transcripts (linux-x64, build/debug, target = codegen/InternalModuleRegistry+enum.h)
Unfixed:
Fixed (same sequence):
Fixed, bindgen (dry run after moving
src/jsc/bindgen_test.bind.tsaway and reconfiguring):Manifests written on this tree after the change (
cxx-sources.txtkept its old mtime, the content is unchanged):Synthetic check that a list carried on the command line already re-runs (the generate-classes / bindgenv2 / build-fallbacks shape): with
command = ls $in > /dev/null && echo out > $out, dropping an input givesninja explain: command line changed for out; with the list not in the command,no work to do.[stamp-90s] gate passed · iteration 1 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file