As I complained in
https://github.com/NixOS/nix/pull/6784#issuecomment-1421777030 (a
comment on the wrong PR, sorry again!), #6693 introduced a second
completions mechanism to fix a bug. Having two completion mechanisms
isn't so nice.
As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef`
to `std::string` when collecting flake refs. Now it is `FlakeRefs`
again.
The underlying issue that sought to work around was that completion of
arguments not at the end can still benefit from the information from
latter arguments.
To fix this better, we rip out that change and simply defer all
completion processing until after all the (regular, already-complete)
arguments have been passed.
In addition, I noticed the original completion logic used some global
variables. I do not like global variables, because even if they save
lines of code, they also obfuscate the architecture of the code.
I got rid of them moved them to a new `RootArgs` class, which now has
`parseCmdline` instead of `Args`. The idea is that we have many argument
parsers from subcommands and what-not, but only one root args that owns
the other per actual parsing invocation. The state that was global is
now part of the root args instead.
This did, admittedly, add a bunch of new code. And I do feel bad about
that. So I went and added a lot of API docs to try to at least make the
current state of things clear to the next person.
--
This is needed for RFC 134 (tracking issue #7868). It was very hard to
modularize `Installable` parsing when there were two completion
arguments. I wouldn't go as far as to say it is *easy* now, but at least
it is less hard (and the completions test finally passed).
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Change-Id: If18cd5be78da4a70635e3fdcac6326dbfeea71a5
(cherry picked from commit 67eb37c1d0de28160cd25376e51d1ec1b1c8305b)
These now have equivalents in the standard lib in C++20. This change was
performed with a custom clang-tidy check which I will submit later.
Executed like so:
ninja -C build && run-clang-tidy -checks='-*,nix-*' -load=build/libnix-clang-tidy.so -p .. -fix ../tests | tee -a clang-tidy-result
Change-Id: I62679e315ff9e7ce72a40b91b79c3e9fc01b27e9
While preparing PRs like #9753, I've had to change error messages in
dozens of code paths. It would be nice if instead of
EvalError("expected 'boolean' but found '%1%'", showType(v))
we could write
TypeError(v, "boolean")
or similar. Then, changing the error message could be a mechanical
refactor with the compiler pointing out places the constructor needs to
be changed, rather than the error-prone process of grepping through the
codebase. Structured errors would also help prevent the "same" error
from having multiple slightly different messages, and could be a first
step towards error codes / an error index.
This PR reworks the exception infrastructure in `libexpr` to
support exception types with different constructor signatures than
`BaseError`. Actually refactoring the exceptions to use structured data
will come in a future PR (this one is big enough already, as it has to
touch every exception in `libexpr`).
The core design is in `eval-error.hh`. Generally, errors like this:
state.error("'%s' is not a string", getAttrPathStr())
.debugThrow<TypeError>()
are transformed like this:
state.error<TypeError>("'%s' is not a string", getAttrPathStr())
.debugThrow()
The type annotation has moved from `ErrorBuilder::debugThrow` to
`EvalState::error`.
(cherry picked from commit c6a89c1a1659b31694c0fbcd21d78a6dd521c732)
Change-Id: Iced91ba4e00ca9e801518071fb43798936cbd05a
Also be more consistent with quotes around attribute paths
(cherry picked from commit 9404ce36e4edd1df12892089bdab1ceb7d4d7a97)
Change-Id: Iaa80073b4a07a6ffef106a3c12ecd02b4f6f67aa
Types converted:
- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`
Existing ones mostly conforming the pattern cleaned up:
- `ContentAddressMethod`
- `ContentAddressWithReferences`
The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think #8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.
See that issue (#7479) for details on the general goal.
`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
We want to be able to write down `foo.drv^bar.drv^baz`:
`foo.drv^bar.drv` is the dynamic derivation (since it is itself a
derivation output, `bar.drv` from `foo.drv`).
To that end, we create `Single{Derivation,BuiltPath}` types, that are
very similar except instead of having multiple outputs (in a set or
map), they have a single one. This is for everything to the left of the
rightmost `^`.
`NixStringContextElem` has an analogous change, and now can reuse
`SingleDerivedPath` at the top level. In fact, if we ever get rid of
`DrvDeep`, `NixStringContextElem` could be replaced with
`SingleDerivedPath` entirely!
Important note: some JSON formats have changed.
We already can *produce* dynamic derivations, but we can't refer to them
directly. Today, we can merely express building or example at the top
imperatively over time by building `foo.drv^bar.drv`, and then with a
second nix invocation doing `<result-from-first>^baz`, but this is not
declarative. The ethos of Nix of being able to write down the full plan
everything you want to do, and then execute than plan with a single
command, and for that we need the new inductive form of these types.
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
* nix flake check: improve error message if overlay is not a lambda
Suppose you have an overlay like this
{
inputs = { /* ... */ };
outputs = { flake-utils, ... }: flake-utils.lib.eachDefaultSystem
(system: {
overlays.default = final: prev: {
};
});
}
then `nix flake check` (correctly) fails because `overlays` are supposed
to have the structure `overlays.<name> = final: prev: exp`. However, the
error-message is a little bit counter-intuitive:
error: overlay does not take an argument named 'final'
While one might guess where the error actually comes from because the
trace above says `… while checking the overlay 'overlays.x86_64-linux'`
this is still pretty confusing because it complains about an argument
not being named `final` even though that's evidently the case.
With this change, the error-message actually makes it clear what's
wrong:
[ma27@carsten:~/Projects/nix/tmp]$ nix flake check --extra-experimental-features 'nix-command flakes' path:$(pwd)
error:
… while checking flake output 'overlays'
at /nix/store/clgblnxx003hyrq8qkz5ab6kgqkck6qc-source/flake.nix:4:5:
3| outputs = { ... }: {
4| overlays.x86_64-linux.snens = final: prev: {
| ^
5| kek = throw "snens";
… while checking the overlay 'overlays.x86_64-linux'
at /nix/store/clgblnxx003hyrq8qkz5ab6kgqkck6qc-source/flake.nix:4:5:
3| outputs = { ... }: {
4| overlays.x86_64-linux.snens = final: prev: {
| ^
5| kek = throw "snens";
error: overlay is not a lambda, but a set instead
Motivation
`PathSet` is not correct because string contexts have other forms
(`Built` and `DrvDeep`) that are not rendered as plain store paths.
Instead of wrongly using `PathSet`, or "stringly typed" using
`StringSet`, use `std::std<StringContextElem>`.
-----
In support of this change, `NixStringContext` is now defined as
`std::std<StringContextElem>` not `std:vector<StringContextElem>`. The
old definition was just used by a `getContext` method which was only
used by the eval cache. It can be deleted altogether since the types are
now unified and the preexisting `copyContext` function already suffices.
Summarizing the previous paragraph:
Old:
- `value/context.hh`: `NixStringContext = std::vector<StringContextElem>`
- `value.hh`: `NixStringContext Value::getContext(...)`
- `value.hh`: `copyContext(...)`
New:
- `value/context.hh`: `NixStringContext = std::set<StringContextElem>`
- `value.hh`: `copyContext(...)`
----
The string representation of string context elements no longer contains
the store dir. The diff of `src/libexpr/tests/value/context.cc` should
make clear what the new representation is, so we recommend reviewing
that file first. This was done for two reasons:
Less API churn:
`Value::mkString` and friends did not take a `Store` before. But if
`NixStringContextElem::{parse, to_string}` *do* take a store (as they
did before), then we cannot have the `Value` functions use them (in
order to work with the fully-structured `NixStringContext`) without
adding that argument.
That would have been a lot of churn of threading the store, and this
diff is already large enough, so the easier and less invasive thing to
do was simply make the element `parse` and `to_string` functions not
take the `Store` reference, and the easiest way to do that was to simply
drop the store dir.
Space usage:
Dropping the `/nix/store/` (or similar) from the internal representation
will safe space in the heap of the Nix programming being interpreted. If
the heap contains many strings with non-trivial contexts, the saving
could add up to something significant.
----
The eval cache version is bumped.
The eval cache serialization uses `NixStringContextElem::{parse,
to_string}`, and since those functions are changed per the above, that
means the on-disk representation is also changed.
This is simply done by changing the name of the used for the eval cache
from `eval-cache-v4` to eval-cache-v5`.
----
To avoid some duplication `EvalCache::mkPathString` is added to abstract
over the simple case of turning a store path to a string with just that
string in the context.
Context
This PR picks up where #7543 left off. That one introduced the fully
structured `NixStringContextElem` data type, but kept `PathSet context`
as an awkward middle ground between internal `char[][]` interpreter heap
string contexts and `NixStringContext` fully parsed string contexts.
The infelicity of `PathSet context` was specifically called out during
Nix team group review, but it was agreeing that fixing it could be left
as future work. This is that future work.
A possible follow-up step would be to get rid of the `char[][]`
evaluator heap representation, too, but it is not yet clear how to do
that. To use `NixStringContextElem` there we would need to get the STL
containers to GC pointers in the GC build, and I am not sure how to do
that.
----
PR #7543 effectively is writing the inverse of a `mkPathString`,
`mkOutputString`, and one more such function for the `DrvDeep` case. I
would like that PR to have property tests ensuring it is actually the
inverse as expected.
This PR sets things up nicely so that reworking that PR to be in that
more elegant and better tested way is possible.
Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>
This introduces the SourcePath type from lazy-trees as an abstraction
for accessing files from inputs that may not be materialized in the
real filesystem (e.g. Git repositories). Currently, however, it's just
a wrapper around CanonPath, so it shouldn't change any behaviour. (On
lazy-trees, SourcePath is a <InputAccessor, CanonPath> tuple.)
`legacyPackages` of nixpkgs trigger eval errors in `hasContent`, causing
the whole `legacyPackages` being skipped. We should treat it as
has-content in that case.
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.
However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.
This isn't so clear to use.
What this commit does is make those command classes like the others,
with richer `run` functions.
Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:
- `prepare` and `load` are gone entirely! One command just hands just
hands off to the next.
- `useDefaultInstallables` because `defaultInstallables`. This takes
over `prepare` for the one case that needs it, and provides enough
flexiblity to handle `nix repl`'s idiosyncratic migration.
- We can use `ref` instead of `std::shared_ptr`. The former must be
initialized (so it is like Rust's `Box` rather than `Option<Box>`,
This expresses the invariant that the installable are in fact
initialized much better.
This is possible because since we just have local variables not
fields, we can stop worrying about the not-yet-initialized case.
- Fewer lines of code! (Finally I have a large refactor that makes the
number go down not up...)
- `nix repl` is now implemented in a clearer way.
The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.
To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.
This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.
The "diagnostic ignored `-Woverloaded-virtual`" pragma helps because C++
doesn't like our many `run` methods. In our case, we don't mind the
shadowing it all --- it is *intentional* that the derived class only
provides a `run` method, and doesn't call any of the overridden `run`
methods.
Helps with https://github.com/NixOS/rfcs/pull/134
For frameworks it's important that structures are as lazy as possible
to prevent infinite recursions, performance issues and errors that
aren't related to the thing to evaluate. As a consequence, they have
to emit more attributes than strictly (sic) necessary.
However, these attributes with empty values are not useful to the user
so we omit them.
Prior to this change, we had a bunch of ad-hoc string manipulation code
scattered around. This made it hard to figure out what data model for
string contexts is.
Now, we still store string contexts most of the time as encoded strings
--- I was wary of the performance implications of changing that --- but
whenever we parse them we do so only through the
`NixStringContextElem::parse` method, which handles all cases. This
creates a data type that is very similar to `DerivedPath` but:
- Represents the funky `=<drvpath>` case as properly distinct from the
others.
- Only encodes a single output, no wildcards and no set, for the
"built" case.
(I would like to deprecate `=<path>`, after which we are in spitting
distance of `DerivedPath` and could maybe get away with fewer types, but
that is another topic for another day.)
Allow `nix build flake1 flake2 --update-input <Tab>` to complete the
inputs of both flakes.
Also do tilde expansion so that `nix build ~/flake --update-input <Tab>`
works.
jade
eldritch horrors
Maximilian Bosch
Robert Hensing
Yorick