I think that it's not very helpful to get "cached failures" in a wrong
`flake.nix`. This can be very confusing when debugging a Nix expression.
See for instance NixOS/nixpkgs#118115.
In fact, the eval cache allows a forced reevaluation which is used for
e.g. `nix eval`.
This change makes sure that this is the case for `nix build` as well. So
rather than
λ ma27 [~/Projects/exp] → ../nix/outputs/out/bin/nix build -L --rebuild --experimental-features 'nix-command flakes'
error: cached failure of attribute 'defaultPackage.x86_64-linux'
the evaluation of already-evaluated (and failed) attributes looks like
this now:
λ ma27 [~/Projects/exp] → ../nix/outputs/out/bin/nix build -L --rebuild --experimental-features 'nix-command flakes'
error: attribute 'hell' missing
at /nix/store/mrnvi9ss8zn5wj6gpn4bcd68vbh42mfh-source/flake.nix:6:35:
5|
6| packages.x86_64-linux.hello = nixpkgs.legacyPackages.x86_64-linux.hell;
| ^
7|
(use '--show-trace' to show detailed location information)
When working on some more complex Nix code, there are sometimes rather
unhelpful or misleading error messages, especially if coerce-errors are
thrown.
This patch is a first steps towards improving that. I'm happy to file
more changes after that, but I'd like to gather some feedback first.
To summarize, this patch does the following things:
* Attrsets (a.k.a. `Bindings` in `libexpr`) now have a `Pos`. This is
helpful e.g. to identify which attribute-set in `listToAttrs` is
invalid.
* The `Value`-struct has a new method named `determinePos` which tries
to guess the position of a value and falls back to a default if that's
not possible.
This can be used to provide better messages if a coercion fails.
* The new `determinePos`-API is used by `builtins.concatMap` now. With
that change, Nix shows the exact position in the error where a wrong
value was returned by the lambda.
To make sure it's still obvious that `concatMap` is the problem,
another stack-frame was added.
* The changes described above can be added to every other `primop`, but
first I'd like to get some feedback about the overall approach.
* The position of the `name`-attribute appears in the trace.
* If e.g. `meta` has no `outPath`-attribute, a `cannot coerce set to
string` error will be thrown where `pos` points to `name =` which is
highly misleading.
If there were many top-level goals (which are not destroyed until the
very end), commands like
$ nix copy --to 'ssh://localhost?remote-store=/tmp/nix' \
/run/current-system --no-check-sigs --substitute-on-destination
could fail with "Too many open files". So now we do some explicit
cleanup from amDone(). It would be cleaner to separate goals from
their temporary internal state, but that would be a bigger refactor.
This avoids an ambiguity where the `StorePathWithOutputs { drvPath, {}
}` could mean "build `brvPath`" or "substitute `drvPath`" depending on
context.
It also brings the internals closer in line to the new CLI, by
generalizing the `Buildable` type is used there and makes that
distinction already.
In doing so, relegate `StorePathWithOutputs` to being a type just for
backwards compatibility (CLI and RPC).
These are by no means part of the notion of a store, but rather are
things that happen to use stores. (Or put another way, there's no way
we'd make them virtual methods any time soon.) It's better to move them
out of that too-big class then.
Also, this helps us remove StorePathWithOutputs from the Store interface
altogether next commit.
Alyssa Ross
Maximilian Bosch
Robert Hensing