Lazy revCount - #15772
Conversation
7ce7143 to
838ee97
Compare
|
I once implemented lazy attrs/revcounts (https://github.com/edolstra/nix/compare/98624325311300b1373c61df565fd49f4325b55c..a68da5fc4b8fe7ae6b406385a26f2b83c7550d10), but it turned out to be useless, because fingerprint computation (needed for the eval cache) requires the revcount strictly. So we don't actually gain anything from the laziness. |
|
@edolstra, there's the roberth@6491749 commit in this branch though. It does kinda make sense to me. |
|
I have confirmed it to work by temporarily putting an exception right at the start of |
|
Hm is nixpkgs in your case a git+ input? I think we still have the eager revCount there because we serialise the whole lock file string instead of making an attribute set with thunks that would get passed to call-flake.nix |
| git -C "$TEST_ROOT/shallow-build-parent" add flake.nix | ||
| git -C "$TEST_ROOT/shallow-build-parent" commit -m "add flake" | ||
| git clone --depth 1 "file://$TEST_ROOT/shallow-build-parent" "$TEST_ROOT/shallow-build-clone" | ||
| rm -rf "$TEST_ROOT/shallow-build-parent" | ||
| nix build --dry-run "git+file://$TEST_ROOT/shallow-build-clone" |
There was a problem hiding this comment.
Hm does the lock file not get serialised in this case? I'm a bit confused why revCount would not be computed in this case...
There was a problem hiding this comment.
The short of it is that the root node does not get locked the way inputs do.
When used as a proper input, it gets locked and serialized to JSON before being taken apart again by call-flake.nix.
So that kills the laziness for inputs.
Whether that's a good thing or not is debatable. It can be argued both ways:
- Storing is scalable: computing it and storing it in the lock means we could trust the value in dependency lock files (like Reuse input lock files #7730 presumably would), limiting the number of
revCountcomputations to direct dependencies instead of a significant portion of the wholeinputsclosure. - Computing it lazily is perfect as long as
revCountis not accessed. Nixpkgs doesn't use it.
Wildcard solution: model it as a build trace so that it can be substituted. (benevolent IFD)
EDIT: IMO the real solution is for fetchTree to take a page from GraphQL and require all output attributes to be explicitly requested. This idea would then extend to certain attributes when it comes to flake inputs. In other words, revCount would be opt-in.
Memoizes a `fun<T()>`. Useful because "call by need" has its use cases outside the evaluator too. Motivating use case: libfetchers lazy attributes.
Extend the Attr variant with a LazyAttr alternative: a deferred computation that is only evaluated when the attribute value is needed. This lets fetchers defer expensive work (like revCount) until an expression demands it. The existing getters and serialization transparently force any lazy attrs.
The "revCount" attribute is now emitted as a lazy attribute if libfetchers were to return it as such; as will be done in next commit.
revCount requires an expensive walk of the entire commit graph. Now it's wrapped in a LazyAttr so the cost is only paid when the Nix expression actually accesses `.revCount`. This also fixes fetching from shallow clones with fetchGit. Previously the eager revCount computation failed the whole fetch, but now `.outPath` and some other attributes succeed, while `.revCount` access fails (as expected for shallow repos).
The flake eval cache fingerprint eagerly forced revCount via getRevCount(), which defeats lazy revCount on shallow clones. Since revCount is functionally determined by rev (already part of the fingerprint), we only need to include its *presence* in the fingerprint, not its value. Note that before these changes, Nix had a bug where it could produce a wrong revcount on shallow clones. It arguably should have inferred `shallow = true;` but it didn't and I'm not changing that behavior either. Leaving it at its default is more "pure" in a sense, but a case could certainly be made to let the CLI infer the value that works automagically. (Purity in the CLI is a spectrum; system attribute selection could be argued to be impure too, for instance.)
Unfortunately this is not the case, since users can specify arbitrary values for |
23b2f61 to
732c739
Compare
It was fine for trustworthy revCounts, but technically the functional dependency described does not always hold up, or we can not trust it to. - buggy tarball providers - merge conflicts in lock files or other kinds of chaos in "user space"
Fixed in 220ccc7 |
xokdvium
left a comment
There was a problem hiding this comment.
Makes sense to me. Maybe it's slightly hacky, but seems worth it. The general mechanism of transforming a fun into a thunk could be factored out in the future however. Not worth the trouble for now though.
|
I think this PR broke "nix flake update --commit-lock-file" for git+https inputs as the comparison in lockfile.cc was tripped by the lazy revCount being evaluated on one side and not on the other side. Which means I got lots of updates like this: Since my C++ knowledge is none existent I asked an LLM to try to figure something out and in the end this came out: diff --git a/src/libflake/lockfile.cc b/src/libflake/lockfile.cc
index 75127521b..ffd4ec8f5 100644
--- a/src/libflake/lockfile.cc
+++ b/src/libflake/lockfile.cc
@@ -364,11 +364,23 @@ std::ostream & operator<<(std::ostream & stream, const Node::Edge & edge)
return stream;
}
+/* Locked refs can differ only in `revCount`, which is a lazy computation
+ in a freshly resolved git input but a concrete integer in a lock file.
+ revCount is derived from `rev`, so ignore it when comparing. */
+static bool lockedRefsEqual(const FlakeRef & a, const FlakeRef & b)
+{
+ auto ia = a.input.attrs;
+ auto ib = b.input.attrs;
+ ia.erase("revCount");
+ ib.erase("revCount");
+ return a.subdir == b.subdir && ia == ib;
+}
+
static bool equals(const Node::Edge & e1, const Node::Edge & e2)
{
if (auto n1 = std::get_if<0>(&e1))
if (auto n2 = std::get_if<0>(&e2))
- return (*n1)->lockedRef == (*n2)->lockedRef;
+ return lockedRefsEqual((*n1)->lockedRef, (*n2)->lockedRef);
if (auto f1 = std::get_if<1>(&e1))
if (auto f2 = std::get_if<1>(&e2))
return *f1 == *f2;No idea if this breaks caching in some other place or majorly breaks something, so please take it with a grain of salt, but it fixes my imminent issue and now I think it should be safe to compare as git commits cannot be freely reordered without changing the hash, so revCount cannot change without the commit changing. |
|
Hm, comparison should probably force those attributes as needed. I assumed that would be the case, but apparently not. Do you have a reproducer? |
|
It's pretty easy to reproduce: Basically run nix flake update --commit-lock-file in this repo https://gitea.c3d2.de/c3d2/nix-config |
|
This is an example commit https://gitea.c3d2.de/c3d2/nix-config/commit/014474926831036656ac5a323884cf34524d281a with the broken behaviour. |
|
Tentatively put up #16430 with the fix. Really silly bug |
Motivation
revCount, so let's only fetch it when needed.(or when serializing the lock)
nix build git+file:///path/to/repowould gogetAccessorFromWorkdirand fail to throw the revcount error, returning a number that's too low instead. I'm unsure whether the CLI should infershallow = true;, but I'm open to the idea if users need it. For now I've kept it simple and "pure" for what that's worth.Context
Written partially at OceanSprint 2026
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.