eval: add NIX_VALIDATE_EVAL_NONDETERMINISM know for attrset comparison#8711
eval: add NIX_VALIDATE_EVAL_NONDETERMINISM know for attrset comparison#8711trofi wants to merge 1 commit intoNixOS:masterfrom trofi:validate-eval-non-determinism
NIX_VALIDATE_EVAL_NONDETERMINISM know for attrset comparison#8711Conversation
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.
ghost
left a comment
There was a problem hiding this comment.
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
nixpkgsthat 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_NONDETERMINISMis set to ease debugging such failures.Motivation
Context
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/teststests/nixos/*Priorities
Add 👍 to pull requests you find important.