ghc: deterministic link order + package.cache (reproducible Haskell) - #277
Conversation
GHC's code output is already reproducible; the residual is metadata ordering from hash-set iteration of package/UnitId collections: 1. link-arg/DT_NEEDED/.dynstr order — backport of upstream GHC #26838 / MR !15453 (sort preload1 in compiler/GHC/Unit/State.hs); Debian ships this exact patch for 9.10.3. 2. ghc-pkg package.cache — sort the .conf list before serialize in utils/ghc-pkg/Main.hs (no upstream fix; sort already imported). Both guarded by grep so a future GHC source change fails the build loudly. Fixing the compiler propagates to all downstream Haskell (stack, HLS). DRAFT: Debian-proven + guarded, but not yet local build-twice-verified (GHC is a multi-hour build_cost_multiple=6 build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesGHC Build Reproducibility Patches
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
| ghc_pkg_main=utils/ghc-pkg/Main.hs | ||
| grep -q 'confs = map (path </>) $ filter (".conf" `isSuffixOf`) fs' "$ghc_pkg_main" \ | ||
| || { echo "ERROR: ghc-pkg package.cache patch target not found in $ghc_pkg_main — GHC source changed." >&2; exit 1; } |
There was a problem hiding this comment.
Guard gap: Unlike patch (1) which explicitly adds sort to the Data.List import via sed, patch (2) relies on the assumption that sort is already in scope in Main.hs. The grep guard here only verifies the target line exists — it doesn't verify sort is importable.
If sort happens NOT to be in scope (e.g., a future GHC version reorganizes imports in ghc-pkg/Main.hs), the build would fail with a Haskell compile error deep into the multi-hour Hadrian build rather than failing early at this guard.
Consider adding a guard like:
grep -q 'import Data.List' "$ghc_pkg_main" \
|| { echo "ERROR: Data.List (sort) not imported in $ghc_pkg_main — add sort import." >&2; exit 1; }Or, more robustly, mirror the patch (1) pattern: explicitly sed in a sort import if it's not already present, so the patch is self-contained regardless of upstream import state.
|
Good catch, thanks — agreed, the patch-(2) guard checks the target line but not that I'll add the fail-fast import guard to harden against a future GHC reorg — e.g. |
Per review on #277: patch (2) relies on 'sort' being in scope in utils/ghc-pkg/Main.hs. 9.10.3 imports it (Main.hs:78), but add a grep guard so a future GHC import reorg fails fast here rather than deep in the multi-hour Hadrian build. Output-neutral; verified result unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅ Verified reproducible. Overnight build-twice (aarch64, 8 CPU/28G) came back byte-identical: 6602/6602 files ( |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ghc/build.sh (2)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
tar -xfas required by the packaging guideline.The extraction command uses
tar -xof; the guideline explicitly requirestar -xffor manual tarball extraction inbuild.sh.As per coding guidelines, “When extracting tarballs manually, add 'tar' to build_deps and use 'tar -xf' in build.sh.”
🤖 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/ghc/build.sh` at line 5, The tar extraction command in the build.sh file currently uses the tar -xof flags, but according to packaging guidelines, it should use tar -xf instead. Change the tar command that extracts the ghc-${MINIMAL_ARG_VERSION}-src.tar.xz archive from tar -xof to tar -xf to comply with the required packaging standard.Source: Coding guidelines
65-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing required deterministic C/C++ toolchain flags for this build path.
The script invokes GHC/Hadrian build steps that include C/C++ compilation, but required deterministic flags are not exported in this script (
CFLAGS/CXXFLAGS,LDFLAGS,ARFLAGS). Add the mandated exports before configure/build.As per coding guidelines, “For C/C++ builds, apply compiler flags: CFLAGS/CXXFLAGS with '-ffile-prefix-map=$(pwd)=/builddir -gno-record-gcc-switches', LDFLAGS with '-Wl,--build-id=none', and ARFLAGS='Drc'.”
🤖 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/ghc/build.sh` around lines 65 - 89, The build script is missing required deterministic C/C++ toolchain flags that need to be exported before the configure and hadrian build steps. Add export statements for CFLAGS, CXXFLAGS, LDFLAGS, and ARFLAGS before the ./configure call (which configures GHC). Set CFLAGS and CXXFLAGS to include '-ffile-prefix-map=$(pwd)=/builddir -gno-record-gcc-switches', LDFLAGS to '-Wl,--build-id=none', and ARFLAGS to 'Drc' to ensure deterministic builds and proper toolchain behavior for the GHC/Hadrian compilation.Source: Coding guidelines
🤖 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.
Inline comments:
In `@packages/ghc/build.sh`:
- Around line 12-39: The build.sh script applies reproducibility patches to GHC
source code but does not include verification that these patches actually
achieve byte-for-byte reproducibility. After the sed commands that apply patches
(2) to both $ghc_state and $ghc_pkg_main files, add a verification step that
builds the package twice to separate output directories and compares them
byte-for-byte using diff or similar tools to ensure the resulting binaries and
artifacts are identical. If the comparison fails, the script should report an
error indicating the reproducibility goal was not met.
---
Outside diff comments:
In `@packages/ghc/build.sh`:
- Line 5: The tar extraction command in the build.sh file currently uses the tar
-xof flags, but according to packaging guidelines, it should use tar -xf
instead. Change the tar command that extracts the
ghc-${MINIMAL_ARG_VERSION}-src.tar.xz archive from tar -xof to tar -xf to comply
with the required packaging standard.
- Around line 65-89: The build script is missing required deterministic C/C++
toolchain flags that need to be exported before the configure and hadrian build
steps. Add export statements for CFLAGS, CXXFLAGS, LDFLAGS, and ARFLAGS before
the ./configure call (which configures GHC). Set CFLAGS and CXXFLAGS to include
'-ffile-prefix-map=$(pwd)=/builddir -gno-record-gcc-switches', LDFLAGS to
'-Wl,--build-id=none', and ARFLAGS to 'Drc' to ensure deterministic builds and
proper toolchain behavior for the GHC/Hadrian compilation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a7515bd-b817-4f9f-bf39-2ce3aa112d2d
📒 Files selected for processing (1)
packages/ghc/build.sh
| # Reproducibility: GHC iterates package/UnitId collections in hash-set order, | ||
| # making the linker arg / DT_NEEDED / .dynstr order in every binary AND the | ||
| # ghc-pkg package.cache non-deterministic. Code (.text/.rodata) is already | ||
| # byte-identical; this is pure ORDERING. Sort the two collections. | ||
| # | ||
| # (1) Link order — backport of upstream GHC #26838 / MR !15453 (merged for | ||
| # 10.0.1; NOT in 9.10.x; Debian ships this exact patch for 9.10.3). Restores | ||
| # the sorted-by-UnitId order GHC <= 9.6 had. | ||
| ghc_state=compiler/GHC/Unit/State.hs | ||
| grep -q 'import Data.List ( intersperse, partition, sortBy, isSuffixOf, sortOn )' "$ghc_state" \ | ||
| && grep -q 'let preload1 = nonDetKeysUniqMap (filterUniqMap (isJust . uv_explicit) vis_map)' "$ghc_state" \ | ||
| || { echo "ERROR: GHC link-order patch targets not found in $ghc_state — GHC source changed; revisit the #26838 backport." >&2; exit 1; } | ||
| sed -i 's/import Data.List ( intersperse, partition, sortBy, isSuffixOf, sortOn )/import Data.List ( intersperse, partition, sortBy, isSuffixOf, sortOn, sort )/' "$ghc_state" | ||
| sed -i 's/let preload1 = nonDetKeysUniqMap (filterUniqMap (isJust . uv_explicit) vis_map)/let preload1 = sort $ nonDetKeysUniqMap (filterUniqMap (isJust . uv_explicit) vis_map)/' "$ghc_state" | ||
|
|
||
| # (2) ghc-pkg package.cache — sort the .conf list before it is read + serialized | ||
| # so the post-build `ghc-pkg recache` emits a byte-identical cache regardless | ||
| # of filesystem readdir order. (No upstream fix exists; `sort` already | ||
| # imported in Main.hs.) | ||
| ghc_pkg_main=utils/ghc-pkg/Main.hs | ||
| grep -q 'confs = map (path </>) $ filter (".conf" `isSuffixOf`) fs' "$ghc_pkg_main" \ | ||
| || { echo "ERROR: ghc-pkg package.cache patch target not found in $ghc_pkg_main — GHC source changed." >&2; exit 1; } | ||
| # The `sort $` below needs `sort` in scope; 9.10.3's Main.hs imports it (line 78), | ||
| # but guard it so a future GHC import reorg fails here, not deep in the build. | ||
| grep -qE 'import Data.List \(.*\bsort\b' "$ghc_pkg_main" \ | ||
| || { echo "ERROR: 'sort' not imported in $ghc_pkg_main — patch (2) requires it (add a sort import)." >&2; exit 1; } | ||
| sed -i 's#confs = map (path </>) $ filter (".conf" `isSuffixOf`) fs#confs = map (path </>) $ sort $ filter (".conf" `isSuffixOf`) fs#' "$ghc_pkg_main" | ||
|
|
There was a problem hiding this comment.
Add the required build-twice reproducibility verification step.
This script improves ordering determinism, but it still does not perform the required “build twice and compare two $OUTPUT_DIR trees byte-for-byte” verification step in build.sh. Please add that explicit check before considering this reproducibility work complete.
As per coding guidelines, “Verify reproducibility by building the package twice and comparing the two $OUTPUT_DIR trees for byte-for-byte identity.”
🤖 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/ghc/build.sh` around lines 12 - 39, The build.sh script applies
reproducibility patches to GHC source code but does not include verification
that these patches actually achieve byte-for-byte reproducibility. After the sed
commands that apply patches (2) to both $ghc_state and $ghc_pkg_main files, add
a verification step that builds the package twice to separate output directories
and compares them byte-for-byte using diff or similar tools to ensure the
resulting binaries and artifacts are identical. If the comparison fails, the
script should report an error indicating the reproducibility goal was not met.
Source: Coding guidelines
What
Makes
ghc(GHC 9.10.3) reproducible — and, because the fix is in the compiler/linker, it makes every downstream Haskell package (stack, haskell-language-server, …) reproducible too.Diagnosis
GHC's functional output is already reproducible —
.text/.rodata/.dataare byte-identical across builds (even the 71 MBlibHSghc.so). The entire residual is metadata ordering, from GHC iterating package/UnitId collections in hash-set order (nonDetEltsUFM), in two places:DT_NEEDED/.dynstrordering in every linked binary (also what makesstack's identically-sized functions land at different addresses).ghc-pkg recache→package.cacheserialized in unsortedreaddirorder.-fobject-determinismis not the lever here (codegen is already deterministic).Fix (two source patches, applied before the Hadrian build)
sort $onpreload1incompiler/GHC/Unit/State.hs). Merged upstream for 10.0.1; not in 9.10.x; Debian ships this exact patch for 9.10.3 (Debian #1125305). It restores the sorted-by-UnitId order GHC ≤ 9.6 had — behavior-restoring, low-risk.package.cache— sort the.conflist before it's read/serialized inutils/ghc-pkg/Main.hs(sortalready imported). No upstream fix exists; trivial and additive. The build's existing post-installghc-pkg recachethen emits a byte-identical cache.Each patch is guarded by a
grepthat fails the build loudly (in seconds) if a future GHC moves the target, rather than silently regressing.Status — DRAFT pending verification
The patches are high-confidence (Debian-proven for this exact version, applied behind fail-fast guards) but not yet locally build-twice-verified — a from-scratch GHC build is multi-hour (
build_cost_multiple=6), ×2 for a repro check. Marking draft until that verify (or CI / a beefy builder) confirms byte-identical output +ghc --version.When minimal moves to GHC ≥ 10.0.1, drop patch (1) (it lands upstream); keep (2) (still unfixed upstream).
🤖 Generated with Claude Code
Summary by CodeRabbit