Don't copy flakes to the store unnecessarily (maybe 3rd time's the charm) - #15711
Conversation
github-actions
Bot
added
new-cli
|
Lol, we have a pre-existing UAF in the C API with readOnlyMode.... Why was it even done in such a sucky way. |
59d7d81 to
be29086
Compare
|
@xokdvium do you have numbers for evaluating nixpkgs on this? |
|
On my machine I got a 3x improvement for a fresh instance of nixpkgs (github:nixos/nixpkgs) when it's not already in the tarball-cache and not pre-hashed. So 30s -> 10s wall time, though a good chunk of that is just downloading the tarball. With a local nixpkgs checkout and a warm page cache I got |
|
Scratch that, it's not necessary - this PR doesn't have the issue with store path rewriting. |
This was some leftovers from detnix cherry-pick, we ban string contexts in getFlake and have always.
be29086 to
7163570
Compare
|
@Mic92, fixed both of the issues (the global lstat cache was dropped in dirFd accessor PRs, storePath issue is fixed here). |
7163570 to
0b2b331
Compare
This builds on top of #14050 to actually make flakes get lazily copied to the store. This repurposes a slightly less lazy (but also more deterministic) approach than determinate nix has taken. We do still pay to cost of hashing an input once to compute the store path and narHash daemon-client-side. This could be improved in follow-ups in case we don't actually need to check the narHash (like during local development). We need certain backwards compatibility hacks for getFlake with a discarded string context, those are similar to what detnix does. See: DeterminateSystems#422 See: DeterminateSystems#402 Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
0b2b331 to
891ef14
Compare
|
will test this in my own fork now: Mic92/dotfiles#5528 |
| panic(fmt( | ||
| "hashed store path computed by the evaluator ('%1%') does not match what was computed when copying to the store ('%2%'), this is a bug", | ||
| store->printStorePath(path), | ||
| store->printStorePath(storePath))); | ||
| } | ||
| } |
There was a problem hiding this comment.
It occurs to me that this should probably be a hash mismatch error instead...
There was a problem hiding this comment.
Yup, I hit this assert when modifying a dirty tree while stuff was being evaluated. Will put up a PR making this an error instead. It's not that big of an issue of course, since that's a trade off we must make if we want to avoid copying stuff to the store eagerly.
Backports the lazy trees variant upstream merged to master (NixOS/nix#15711 plus its two post-merge fixes, NixOS/nix#15950 and NixOS/nix#16078) onto the 2.34.7 base as patches 0024-0027, and gates the behavior behind a new off-by-default `lazy-trees` eval setting (patch 0028). With the setting off, mountInput() copies eagerly exactly like the base and the ensure functions return immediately; with it on, flake inputs mount at their final content-addressed store paths (hashed via dry run, so paths, hashes, and lock files are unchanged by construction) and only materialize when something forces them. Both sources named in the issue were examined: edolstra's lazy-trees-v2 (NixOS/nix#13225, random virtual store paths plus on-demand devirtualization) was closed unmerged on 2026-07-16 after roberth's determinism objection, pointing at #15711 as the mergeable subset; and Determinate's fork shipped lazy trees via staged rollouts since 3.8.0, but its current v2.35.1 tree has dropped the random-virtual-path implementation entirely and rides the same upstream mechanism vendored here. The pushback (roberth's impurity objection and rope-string lazy-hashing alternative, the toString-without-context wrinkle) is documented in the patch 0025 and 0028 bodies and the site update page. packages/site/src/lib/updates/nix-fork-lazy-trees-setting.svx Closes #3645 (PR authored by an AI agent via Claude Code, Claude Opus 4.5)
Backports the lazy trees variant upstream merged to master (NixOS/nix#15711 plus its two post-merge fixes, NixOS/nix#15950 and NixOS/nix#16078) onto the 2.34.7 base as patches 0024-0027, and gates the behavior behind a new off-by-default `lazy-trees` eval setting (patch 0028). With the setting off, mountInput() copies eagerly exactly like the base and the ensure functions return immediately; with it on, flake inputs mount at their final content-addressed store paths (hashed via dry run, so paths, hashes, and lock files are unchanged by construction) and only materialize when something forces them. Both sources named in the issue were examined: edolstra's lazy-trees-v2 (NixOS/nix#13225, random virtual store paths plus on-demand devirtualization) was closed unmerged on 2026-07-16 after roberth's determinism objection, pointing at #15711 as the mergeable subset; and Determinate's fork shipped lazy trees via staged rollouts since 3.8.0, but its current v2.35.1 tree has dropped the random-virtual-path implementation entirely and rides the same upstream mechanism vendored here. The pushback (roberth's impurity objection and rope-string lazy-hashing alternative, the toString-without-context wrinkle) is documented in the patch 0025 and 0028 bodies and the site update page. packages/site/src/lib/updates/nix-fork-lazy-trees-setting.svx Closes #3645 (PR authored by an AI agent via Claude Code, Claude Opus 4.5)
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nixpkgs-multiverse-every-version-that-ever-existed/79490/6 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nixpkgs-multiverse-every-version-that-ever-existed/79490/26 |
4 participants
Motivation
3rd time's the charm!
This builds on top of #14050 to
actually make flakes get lazily copied to the store. This repurposes
a slightly less lazy (but also more deterministic) approach than
determinate nix has taken. We do still pay to cost of hashing an input
once to compute the store path and narHash daemon-client-side. This
could be improved in follow-ups in case we don't actually need to check
the narHash (like during local development).
We need certain backwards compatibility hacks for getFlake with a discarded string
context, those are similar to what detnix does.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.