Repository navigation
difftastic: deterministic parser link order for reproducible builds - #283
Merged
Merged
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>
Contributor
📝 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 |
Contributor
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
mitodrummer
approved these changes
Jun 22, 2026
motdotla
pushed a commit
to motdotla/pkgs
that referenced
this pull request
Aug 27, 2026
…ox#283) (gominimal#415) * ci: add report-only corresponding-source check (inbox#283 / inbox#53) For every copyleft package (classified from license_spdx via prefix match on expression tokens), verify its exact source archive is durably present on the public mirror - the corresponding-source offer that distributing copyleft binaries from the cache obligates. Presence is probed with unauthenticated HTTPS HEADs against the public bucket (fixed host, charset-validated paths, no secrets); exactness rides on the build's own sha256 pin. Doubled slashes are normalized to match minimal's gs:// fetcher (graphviz's build.ncl has a literal // that builds fine). First full-catalog run: 127 copyleft packages -> 89 mirrored / 0 mirror-missing / 38 unmirrored (ffmpeg, ghostscript+grafana both AGPL, jdk, gcc-arm-none-eabi, libvips, x264/x265, ...) - the concrete gominimal#283 work list. Runs on PRs (changed packages), dispatch, and a weekly schedule: mirror objects can vanish independently of any PR, and that silent drift is the main thing to catch. Seventh sibling of the report-only check family; MUST NOT be required. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * corresponding-source: recognize out-of-band mirrored sources + probe fixes The inbox#283 backfill uploads archives to the mirror WITHOUT touching build.ncl (repointing changes spec_hashes -> rebuild cascades; URLs get repointed lazily at natural version bumps). Teach the check to probe the conventional mirror path (github -> owner/repo/basename, else flat basename) for upstream-URL packages and report 'mirrored (out-of-band)' instead of falsely flagging them unmirrored. Probe fixes caught by the backfill itself: underscore was missing from the path charset (jdk's OpenJDK21U-jdk_x64_... failed validation before probing), and a failed probe in the out-of-band branch collapsed into 'unmirrored' instead of 'probe-unknown'. Full-catalog state after backfill: 127/127 copyleft packages mirrored (89 by URL, 38 out-of-band), 0 missing, 0 unmirrored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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