Skip to content

Fix assorted bugs found in code review (v2/v1 builders, parser, modules, overlays, hix, nix-tools) - #2610

Open
hamishmack wants to merge 7 commits into
masterfrom
hkm/tier1-bug-fixes
Open

Fix assorted bugs found in code review (v2/v1 builders, parser, modules, overlays, hix, nix-tools)#2610
hamishmack wants to merge 7 commits into
masterfrom
hkm/tier1-bug-fixes

Conversation

@hamishmack

@hamishmack hamishmack commented Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Bugs found during a review of the codebase. They are grouped into one PR to keep CI runs down, with one commit per area.

v2 builder

  • builtins.toFile rejects text that references a derivation output. Per-package configureOptions/ghcOptions containing a store path, and darwin framework dirs, therefore failed eval. writeText is now used only when the text has derivation context, so the hashes of context-free text are unchanged.
  • An exe slice that did not produce its binary now fails. Previously it cached a placeholder script that exits 1.
  • --<prog>-option= is no longer re-emitted under configure-options:. cabal hashes that field into pkgHashConfigureScriptArgs and only passes it to ./configure, so the slice's UnitId diverged from plan-nix's. These options still reach the program via <prog>-options:.

v1 builder

  • make-config-files: the fallback used when ghc.cachedDeps is missing pointed at the GHC store path.
  • shellFor: the propagateBuildInputs typo dropped the user's propagatedBuildInputs.
  • check: passthru no longer inherits attributes that may not exist (cleanSrc, configFiles). The comment about doCrossCheck now matches what it does; its behaviour is unchanged.

Eval / modules

  • cabal-project-parser: a commented-out -- --sha256: line corrupted tag, and URLs containing -- were truncated. Includes a unit test.
  • cleanCabalComponent: hs-source-dirs: src also matched src-gen/, and package.yaml was dropped for packages in a subdirectory.
  • hackage-quirks: lsp-test and postgrest replaced the generated cabal.project with packages: ., which dropped the package from the plan.

Overlays / hix / flake

  • hix: --supportedSystems, --overlays and --config were ignored unless --projectArgs was also passed.
  • flake devShells: GHC < 9.6 is now excluded, because those compiler attributes throw and break nix flake check.
  • wine iserv and test wrappers:
    • quote "$@"
    • tolerate a clash between libX.dll and X.dll, and a glob that matches nothing
    • respect WINEPREFIX
  • windows: removed the mfpr override, a typo for mpfr that has never applied.

nix-tools

These fixes take effect once a new static nix-tools release is published.

Expected hash changes

  • Every v2 exe slice.
  • v2 packages that have <prog>-options.
  • Windows cross TH builds.

Testing

  • Every changed .nix file parses.
  • The unit tests pass.
  • Targeted evals for each fix.
  • nix-tools plan-to-nix and stack-to-nix build.

Changes after review

The changes below are in fa93f0d, made after a /code-review pass; Copilot couldn't review because of quota.

  • Dropped the haddock dependency fix. With profiling enabled it would pull in .profiled.haddock, and it built a haddock derivation for every dependency, even those with doHaddock = false. It will come in a separate PR.
  • Dropped the allComponent keep-flag merge. Nothing reads it, and it could conflict with user definitions.
  • textFile is now in haskellLib and is also used for the cabal file in comp-v2-builder.
  • The devShells filter now uses tryEval, so it follows traceWarnOld.
  • The Wine iserv wrapper is back to the hermetic WINEPREFIX=$TMP.

@hamishmack
hamishmack requested a balanced review from Copilot October 26, 2026 04:01
Copilot AI reviewed Oct 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@hamishmack
v2: fix toFile context failure, exe placeholder, duplicated prog options
8a502ab 
* build-cabal-slice: `builtins.toFile` rejects text that references a
  derivation output, so per-package `configureOptions`/`ghcOptions`
  mentioning a store path (or darwin framework `extra-lib-dirs`) failed
  eval.  Use `writeText` only when the text carries derivation context;
  context-free text keeps `toFile` and identical hashes.
* comp-v2-builder: fail when an exe slice did not produce its binary
  instead of caching a placeholder script that exits 1.
* configure-args: stop re-emitting `--<prog>-option=` under
  `configure-options:`.  cabal hashes that field into
  `pkgHashConfigureScriptArgs` and only passes it to ./configure, so it
  forked the slice UnitId from plan-nix without reaching the program.
  These options already round-trip via `<prog>-options:`.
@hamishmack
v1 builder: fix haddock deps, cachedDeps fallback, shellFor typo
c6f4b53 
* haddock-builder: propagate the dependencies' haddock derivations
  (`docsConfigFiles.libDeps`) so the docs package DB has working
  `haddock-interfaces`/`haddock-html` for cross-package links
  (regressed in cfd757a).  Also pass backpack `instantiations` and
  `prebuilt-depends` as the build derivation does.
* make-config-files: the fallback when `ghc.cachedDeps` is missing
  interpolated the GHC store path rather than the `cachedDeps` drv.
* shell-for: `propagateBuildInputs` typo silently dropped the user's
  `propagatedBuildInputs`.
* check: don't inherit `cleanSrc` (never present) or `configFiles`
  (only with `keepConfigFiles`) into passthru; correct the stale
  comment about `doCrossCheck`.
@hamishmack
Fix cabal.project comment parsing, component source filtering, allCom…
6f522d5 
…ponent keep flags

* cabal-project-parser: only treat whole-line or whitespace-preceded
  `--` as a comment.  A commented-out `-- --sha256:` line corrupted the
  preceding `tag`, and URLs containing `--` were truncated.  Add a unit
  test.
* clean-cabal-component: match source/include/data dirs on path
  boundaries (`src` no longer pulls in `src-gen`), and keep
  `package.yaml` for packages in a subdirectory.
* package.nix: the `keep*` flags were `//`-ed onto a `mkMerge` value and
  ignored by the module system; make them part of the merge.
* hackage-quirks: lsp-test and postgrest replaced the generated
  `cabal.project` with `packages: .`, dropping the package from the
  plan; put their flags in `cabalProjectLocal` instead.
@hamishmack
Fix hix input overrides, flake devShells, wine wrappers
b5599df 
* hix: `--supportedSystems`, `--overlays` and `--config` were ignored
  unless `--projectArgs` was also given.
* flake: exclude GHC < 9.6 from devShells; those compilers throw and
  broke `nix flake check`.
* mingw_w64: quote "$@" in the wine iserv and test wrappers, don't
  abort on `libX.dll`/`X.dll` clashes or an unmatched glob, respect a
  caller's WINEPREFIX, drop dead assignments.
* windows: remove the `mfpr` override (a typo for mpfr that never
  applied).
@hamishmack
nix-tools: fix fetchWith deadlock on large output and crash on empty …
5cf71ba 
…output

fetchWith now reads the prefetch output before waiting for the process
(avoids a pipe-buffer deadlock) and handles empty output without a
partial pattern.  Applied to both nix-tools and the nix-tools-sh mirror.
(The cache-reader fix originally in this commit is superseded by
NixTools.Cache from #2544.)
@hamishmack
Address review: drop haddock change, shared textFile, devShell filter
82fb596 
* haddock-builder: revert to master.  Propagating the deps' `.haddock`
  derivations also pulled in `.profiled.haddock` when profiling is
  enabled, and forced a haddock derivation per dependency even with
  `doHaddock = false`.  Needs a separate design.
* Move `textFile` to haskellLib (using `pkgs.writeText`, matching the
  neighbouring `source-repo-block`) and use it for the cabal file in
  comp-v2-builder too.
* package.nix: remove the `allComponent` keep* merge.  comp-builder reads
  keep* from the component being built, not `allComponent`, so the merge
  was dead and could conflict with user definitions.
* mingw_w64: keep the hermetic `WINEPREFIX=$TMP` for iserv.
* flake: filter devShells with `tryEval` on the compiler version so the
  cut-off tracks `traceWarnOld` rather than a hardcoded regex.
@hamishmack
nix-tools-sh: mirror the fetchWith fix
4dc1104

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hamishmack