difftastic: deterministic parser link order for reproducible builds - #283
Conversation
difftastic's build.rs compiles its 11 vendored tree-sitter parsers with `parsers.par_iter().for_each(|p| p.build())` (rayon). Each cc::Build::compile() emits its `cargo:rustc-link-lib` directive when it finishes, so under parallel compilation the parser static libs link in non-deterministic completion order. The parser tables are large (~99 MB of .rodata), so reshuffling their link order moves everything — producing distributed, same-size differences across .text/.rodata/.data/.rela.dyn build-to-build. codegen-units=1 and CONST_RANDOM_SEED can't fix this; it's link order, not codegen. Serialize parser compilation (par_iter -> iter) so the link directives are emitted in deterministic source order. Verified byte-identical across two from-scratch builds (repro-verify). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesDeterministic Parser Compilation Patch
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/difftastic/build.sh (1)
9-18: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider enhancing verification to confirm the original pattern was removed.
The current grep verification only checks that
parsers.iter().for_eachexists after patching, but doesn't verify thatparsers.par_iter().for_eachwas actually removed. If upstream changes the code structure so the sed pattern no longer matches, butparsers.iter().for_eachhappens to exist elsewhere (e.g., in a comment or different context), the verification would incorrectly pass.Adding a second check would make the verification more robust and explicit:
🔍 Enhanced verification
sed -i 's/parsers\.par_iter()\.for_each/parsers.iter().for_each/' build.rs grep -q 'parsers.iter().for_each' build.rs || { echo "ERROR: difftastic par_iter->iter patch did not apply — build.rs changed" >&2; exit 1; } +! grep -q 'parsers\.par_iter()\.for_each' build.rs || { echo "ERROR: original par_iter pattern still present after patch" >&2; exit 1; }This verifies both that the desired pattern exists AND the undesired pattern is absent, making the patch verification bidirectional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/difftastic/build.sh` around lines 9 - 18, The current verification only confirms that the desired pattern `parsers.iter().for_each` exists after the sed command, but does not verify that the original undesired pattern `parsers.par_iter().for_each` was actually removed. Add a second grep check after the existing grep verification to explicitly confirm that `parsers.par_iter().for_each` is no longer present in build.rs. Use a negated grep check (grep with appropriate flags to fail if the pattern is found) to ensure the original pattern has been removed, making the verification bidirectional and more robust to upstream code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/difftastic/build.sh`:
- Around line 9-18: The current verification only confirms that the desired
pattern `parsers.iter().for_each` exists after the sed command, but does not
verify that the original undesired pattern `parsers.par_iter().for_each` was
actually removed. Add a second grep check after the existing grep verification
to explicitly confirm that `parsers.par_iter().for_each` is no longer present in
build.rs. Use a negated grep check (grep with appropriate flags to fail if the
pattern is found) to ensure the original pattern has been removed, making the
verification bidirectional and more robust to upstream code changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf0e665e-9ee2-4c05-b042-c383be777f39
📒 Files selected for processing (1)
packages/difftastic/build.sh
What
Makes the
difftasticpackage build byte-reproducibly with a one-wordbuild.rschange.Root cause
difftastic'sbuild.rscompiles its 11 vendored tree-sitter parsers in parallel:Each
cc::Build::compile()prints itscargo:rustc-link-libdirective when it finishes, so under parallel compilation the parser static libs link in non-deterministic completion order. Those parser tables are large (~99 MB of.rodata), so reshuffling their link order relocates everything — yielding distributed, same-size differences across.text/.rodata/.data/.rela.dynbetween builds (measured ~16% of.text, ~8% of.rodata).This sits above the codegen layer, so the existing
-C codegen-units=1+CONST_RANDOM_SEED(from #254) couldn't fix it — it's link order, not codegen or compile-time RNG.Fix
Serialize parser compilation (
par_iter()→iter()) so thecargo:rustc-link-libdirectives are emitted in deterministic source order. Applied as a grep-guardedsedinbuild.sh.Verification
repro-verify difftastic(two from-scratch builds +repro-check diff): byte-identical ✅ (was: ~9.5% of bytes differing before the fix).minimal checkgreen.🤖 Generated with Claude Code
Summary by CodeRabbit