nix-tools: share the git-fetch cache between plan2nix and stack2nix (#1689) - #2544
Open
hamishmack wants to merge 5 commits into
Open
nix-tools: share the git-fetch cache between plan2nix and stack2nix (#1689)#2544hamishmack wants to merge 5 commits into
hamishmack wants to merge 5 commits into
Conversation
Plan2Nix.Cache and Stack2nix.Cache were byte-for-byte "identical" copies
(per the stale comment atop the former) that had since drifted apart:
* Plan2Nix.Cache stored a "NOHASH" placeholder for empty hashes so the
whitespace-delimited cache format still round-trips through `words`,
but read/wrote lazily.
* Stack2nix.Cache forced reads/writes strictly (<$!!> / $!) but wrote a
raw empty string for missing hashes, which collapses a field under
`words` and corrupts the cache line for local packages.
Replace both with a single library module NixTools.Cache that keeps the
union of the correct behaviours: the NOHASH placeholder AND the strict IO.
Repoint Stack2nix and Plan2Nix at it, delete the two old modules, and
update the cabal module lists (plan-to-nix already depends on the library).
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates the git-fetch cache implementation used by plan-to-nix and stack-to-nix into a single shared library module (NixTools.Cache) to prevent format drift and cache corruption (notably around missing hashes and strict IO).
Changes:
- Introduce
NixTools.Cacheas the unified cache implementation (NOHASH placeholder + strict read/write). - Repoint
Plan2NixandStack2nixto importNixTools.Cacheinstead of tool-specific cache modules. - Update
nix-tools.cabalto exposeNixTools.Cacheand remove the duplicated module wiring.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| nix-tools/nix-tools/lib/NixTools/Cache.hs | New shared cache module; merges correct behaviors from both prior implementations. |
| nix-tools/nix-tools/plan2nix/Plan2Nix.hs | Switch cache import to the shared NixTools.Cache. |
| nix-tools/nix-tools/lib/Stack2nix.hs | Switch cache import to the shared NixTools.Cache. |
| nix-tools/nix-tools/nix-tools.cabal | Expose NixTools.Cache from the library; drop the duplicate module from plan-to-nix’s other-modules. |
| nix-tools/nix-tools/lib/Stack2nix/Cache.hs | Removes the now-redundant cache module implementation. |
Comments suppressed due to low confidence (1)
nix-tools/nix-tools/lib/NixTools/Cache.hs:29
readCacheuses a non-exhaustive pattern match intoTuple, so any malformed/blank cache line will throw aPatternMatchFailand abort the entire read. Making this total removes a-Wallwarning and yields a clearer error if it ever happens (behaviour still fails fast).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review follow-up on #2544. `readCache`'s `toTuple` only matched a six-field line, so a blank or truncated cache line (e.g. from an interrupted `appendCache`) raised `PatternMatchFail`. Because this module now reads strictly the failure is caught by `cacheHits`, but the whole cache is then discarded rather than just the bad line, and `-Wall` flagged the incomplete match. Use `mapMaybe` and skip lines we cannot parse: the cache is advisory, so a skipped entry only means we fetch it again. Also silences the `-Wunused-matches` warning for the unused `sha256` binder in `cacheHits`, so the new shared module compiles clean under `-Wall`. Verified with `nix develop` + `cabal build -w ghc lib:nix-tools plan-to-nix stack-to-nix`: both -Wincomplete-patterns and -Wunused-matches warnings for NixTools/Cache.hs are gone (they were present before this commit).
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.
Summary
Plan2Nix.CacheandStack2nix.Cachewere supposed to be identical (thePlan2Nix copy literally opened with
-- Note: this is identical to Stack2nix.Cache)but had drifted apart in a way that mattered:
NOHASHplaceholder for empty hashes, so thewhitespace-delimited cache format still round-trips through
words— but itread and wrote the file lazily.
<$!!>/$!) — butwrote a raw empty string for a missing hash. Under
wordsan empty fieldsimply disappears, so a local-package cache line loses a column and no longer
parses back to the 6-tuple: latent cache corruption.
This externalises the logic into a single library module
NixTools.Cachethat keeps the union of the correct behaviours — the
NOHASHplaceholder andthe strict IO — then repoints both
Stack2nixandPlan2Nixat it and deletesthe two old modules.
plan-to-nixalready depends on thenix-toolslibrary, sono new dependency is needed.
Changes
nix-tools/nix-tools/lib/NixTools/Cache.hs(merged, more-defensive impl).lib/Stack2nix/Cache.hsandplan2nix/Plan2Nix/Cache.hs.import NixTools.Cacheinlib/Stack2nix.hsandplan2nix/Plan2Nix.hs.NixTools.Cachein the library (replacingStack2nix.Cache);drop
Plan2Nix.Cachefromplan-to-nix'sother-modules.Testing
The public API and type signatures are identical to both old modules
(
readCache/appendCache/cacheHits), and both call sites only import(appendCache, cacheHits), so they compile unchanged — this is asignature-preserving merge. I typechecked the new module standalone with GHC
(
ghc -fno-code -Wall): it compiles; the only warnings (non-exhaustivetoTuple, unusedsha256in thecacheHitscomprehension) are presentverbatim in both original modules, so there is no new warning/regression.
A full from-source
nix-toolsbuild (needsghc-9.6+ the hnix closure) wasnot run in this environment.
Release caveat: haskell.nix consumes
nix-toolsas a prebuilt static releasebinary, so this change reaches haskell.nix builds only once a new
nix-toolsrelease is cut.
Closes #1689.