-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eval: add NIX_VALIDATE_EVAL_NONDETERMINISM
know for attrset comparison
#8711
Conversation
This change adds an extra knob to detect nin-determinism in attrset comparison. It follows real problem discovered by Vladimir Kryachko in current `nixpkgs` that fails evaluation non-deterministically based on internal state of the eval: This works: $ nix-instantiate -E 'let pkgs = import <nixpkgs> {}; in pkgs.nix' /nix/store/6gzxax0rl0k1n3hg0s22jnj6c1c0aj3b-nix-2.15.1.drv This fails: $ nix-instantiate -E 'let pkgs = import <nixpkgs> {}; in let aaaaaaaaaaa = { bbbbbbbbbbbbbb.extensions = []; }; in pkgs.nix' error: assertion '(final).hasSharedLibraries' failed Note that `let aaaaaaaaaaa = { ... }` binding should not be able to affect evaluation of `pkgs.nix`. This happens because attrsets are implementing lazy attrset comparison. The change implements eager comparison when `NIX_VALIDATE_EVAL_NONDETERMINISM` is set to ease debugging such failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an extremely useful tool. I have added this patch to my local overlay. I think it should be merged. It made fixing NixOS/nixpkgs#244045 really easy.
Stack traces produced with NIX_VALIDATE_EVAL_NONDETERMINISM
are much, much more useful than those produced without it. Of course, this comes at the expense of significantly slower evaluation. But for debugging purposes this is totally worth it.
Only one minor nitpick: technically Nix's equality is deterministic, but it is deterministically lazy according to a traversal of attribute names. What the NIX_VALIDATE_EVAL_NONDETERMINISM
flag does is make equality strict in all the values corresponding to keys in the intersection of the attrsets' names.
Maybe NIX_EQUALITY_NOTLAZY
would be a more accurate name for what it does.
Yeah, I guess it's a matter of perspective. Probably "spooky action at a distance" would be a better description. Otherwise it's hard to explain evaluation rules that explains why these two differ in result without involving symbol table for the whole interpreter session:
Or even this:
vs
I would say it's not a deterministic evaluation. |
I wonder what would be the performance impact of actually making this deterministic. If you want to go fast you shouldn't do too many deep comparisons anyway...
The pointer equality hack has its own problems, but those could be alleviated by forcing the value. I don't think we need to consider that now. |
I apologize, I overlooked that part. Yes the fact that unused entries in the lexical scope affect the results is completely nuts.
I think cppnix already does. Tvix does, and had to do so in order to match cppnix's behavior (and pass its test suite). Speaking of which, tvix does not emulate the scope-sensitive cppnix behavior described here. Paging @sternenseemann ... |
Pretty sure that all implementations already do this. Not by sorting the attributes on comparison, but by always having a sorted representation of the attribute set in memory already (in cppnix this is a sorted vector of pairs, in tvix this is a B-tree map). |
Our implementation sorts using a string interning id rather than lexicographically, so the sort order is non-deterministic. |
cppnix = c++ nix, this here project.
Seems like this has changed since 2.3, where the interned strings were compared by the interned string, not the interning ID. Not sure how the whole machinery (esp. attribute lookups) works here at the moment then. |
Note that you can trigger this behavior without using
|
This change adds an extra knob to detect nin-determinism in attrset comparison. It follows real problem discovered by Vladimir Kryachko in current
nixpkgs
that fails evaluation non-deterministically based on internal state of the eval:This works:
This fails:
Note that
let aaaaaaaaaaa = { ... }
binding should not be able to affect evaluation ofpkgs.nix
.This happens because attrsets are implementing lazy attrset comparison. The change implements eager comparison when
NIX_VALIDATE_EVAL_NONDETERMINISM
is set to ease debugging such failures.Motivation
Context
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.