This reverts commit 5e3986f59c. This
un-implements RFC 92 but fixes the critical bug #9052 which many people
are hitting. This is a decent stop-gap until a minimal reproduction of
that bug is found and a proper fix can be made.
Mostly fixed#9052, but I would like to leave that issue open until we
have a regression test, so I can then properly fix the bug (unbreaking
RFC 92) later.
(cherry picked from commit 8440afbed756254784d9fea3eaab06649dffd390)
The `-c` flag belongs to `sh` not `nix shell`. As it stands, the command errors with:
```
$ nix shell nixpkgs#gnumake --command sh --command "cd src && make"
sh: --command: invalid option
```
https://github.com/NixOS/nix/pull/8276 was good for readability, but it missed this since that PR used a find/replace script.
The Derivation parser and old ATerm unfortunately leaves few ways to get
nice errors when an old version of Nix encounters a new version of the
format. The most likely scenario for this to occur is with a new client
making a derivation that the old daemon it is communicating with cannot
understand.
The extensions we just created for dynamic derivation deps will add a
version field, solving the problem going forward, but there is still the
issue of what to do about old versions of Nix up to now.
The solution here is to carefully catch the bad error from the daemon
that is likely to indicate this problem, and add some extra context to
it.
There is another "Ugly backwards compatibility hack" in
`remote-store.cc` that also works by transforming an error.
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
We use the same nested map representation we used for goals, again in
order to save space. We might someday want to combine with `inputDrvs`,
by doing `V = bool` instead of `V = std::set<OutputName>`, but we are
not doing that yet for sake of a smaller diff.
The ATerm format for Derivations also needs to be extended, in addition
to the in-memory format. To accomodate this, we added a new basic
versioning scheme, so old versions of Nix will get nice errors. (And
going forward, if the ATerm format changes again the errors will be even
better.)
`parsedStrings`, an internal function used as part of parsing
derivations in A-Term format, used to consume the final `]` but expect
the initial `[` to already be consumed. This made for what looked like
unbalanced brackets at callsites, which was confusing. Now it consumes
both which is hopefully less confusing.
As part of testing, we also created a unit test for the A-Term format for
regular non-experimental derivations too.
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Apply suggestions from code review
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
- Don't assert: Derivation ATerms are not necessarily produced by Nix,
and parsers should always throw graceful errors
- Improve error message from `static void except(..)`, shows both what
we expected and what we actually got.
The intention is that we backport it, and then hopefully a few people
might get slightly better errors if they try out new experimental drv
files (for RFC 92) with an old version of Nix.
To avoid dealing with an optional `drvPath` (because we might not know
it yet) everywhere, make an `CreateDerivationAndRealiseGoal`. This goal
just builds/substitutes the derivation file, and then kicks of a build
for that obtained derivation; in other words it does the chaining of
goals when the drv file is missing (as can already be the case) or
computed (new case).
This also means the `getDerivation` state can be removed from
`DerivationGoal`, which makes the `BasicDerivation` / in memory case and
`Derivation` / drv file file case closer together.
The map type is factored out for clarity, and because we will soon hvae
a second use for it (`Derivation` itself).
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
We're about to split up `DerivationGoal` a bit. At that point
`makeDerivationGoal` will mean something more specific than it does
today. (Perhaps a future rename will make this clearer.)
On the other hand, the more public `Worker::makeGoal` function will
continue to work exactly as before. So by moving some call sites to use
that instead, we preemptively avoid issues in the next step.
This function is now trivial enough that it doesn't need to exist.
`EvalState` can still be initialized with a custom search path, but we
don't have a need to mutate the search path after it has been
constructed, and I don't see why we would need to in the future.
Fixes#8229
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>
If you have a URL that needs to be percent-encoded, such as
`http://localhost:8181/test/+3d.tar.gz`, and try to lock that in a Nix
flake such as the following:
{
inputs.test = { url = "http://localhost:8181/test/+3d.tar.gz"; flake = false; };
outputs = { test, ... }: {
t = builtins.readFile test;
};
}
running `nix flake metadata` shows that the input URL has been
incorrectly double-encoded (despite the flake.lock being correctly
encoded only once):
[...snip...]
Inputs:
└───test: http://localhost:8181/test/%252B3d.tar.gz?narHash=sha256-EFUdrtf6Rn0LWIJufrmg8q99aT3jGfLvd1//zaJEufY%3D
(Notice the `%252B`? That's just `%2B` but percent-encoded again)
With this patch, the double-encoding is gone; running `nix flake
metadata` will show the proper URL:
[...snip...]
Inputs:
└───test: http://localhost:8181/test/%2B3d.tar.gz?narHash=sha256-EFUdrtf6Rn0LWIJufrmg8q99aT3jGfLvd1//zaJEufY%3D
---
As far as I can tell, this happens because Nix already percent-encodes
the URL and stores this as the value of `inputs.asdf.url`.
However, when Nix later tries to read this out of the eval state as a
string (via `getStrAttr`), it has to run it through `parseURL` again to
get the `ParsedURL` structure.
Now, this itself isn't a problem -- the true problem arises when using
`ParsedURL::to_string` later, which then _re-escapes the path_. It is
at this point that what would have been `%2B` (`+`) becomes `%252B`
(`%2B`).
Without the change build with `-D_GLIBCXX_ASSERTIONS` exposes testsuite
assertion:
$ gdb src/libexpr/tests/libnixexpr-tests
Reading symbols from src/libexpr/tests/libnixexpr-tests...
(gdb) break __glibcxx_assert_fail
(gdb) run
(gdb) bt
in std::__glibcxx_assert_fail(char const*, int, char const*, char const*)@plt () from /mnt/archive/big/git/nix/src/libexpr/libnixexpr.so
in std::basic_string_view<char, std::char_traits<char> >::operator[] (this=0x7fffffff56c0, __pos=4)
at /nix/store/r74fw2j8rx5idb0w8s1s6ynwwgs0qmh9-gcc-14.0.0/include/c++/14.0.0/string_view:258
in nix::SearchPath::Prefix::suffixIfPotentialMatch (this=0x7fffffff5780, path=...) at src/libexpr/search-path.cc:15
in nix::SearchPathElem_suffixIfPotentialMatch_partialPrefix_Test::TestBody (this=0x555555a17540) at src/libexpr/tests/search-path.cc:62
As string sizes are usigned types `(a - b) > 0` effectively means
`a != b`. While the intention should be `a > b`.
The change fixes test suite pass.
In the Nix language, given a drv path, we should be able to construct
another string referencing to one of its output. We can do this today
with `(import drvPath).output`, but this only works for derivations we
already have.
With dynamic derivations, however, that doesn't work well because the
`drvPath` isn't yet built: importing it like would need to trigger IFD,
when the whole point of this feature is to do "dynamic build graph"
without IFD!
Instead, what we want to do is create a placeholder value with the right
string context to refer to the output of the as-yet unbuilt derivation.
A new primop in the language, analogous to `builtins.placeholder` can be
used to create one. This will achieve all the right properties. The
placeholder machinery also will match out the `outPath` attribute for CA
derivations works.
In 60b7121d2c we added that type of
placeholder, and the derived path and string holder changes necessary to
support it. Then in the previous commit we cleaned up the code
(inspiration finally hit me!) to deduplicate the code and expose exactly
what we need. Now, we can wire up the primop trivally!
Part of RFC 92: dynamic derivations (tracking issue #6316)
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
`EvalState::mkSingleDerivedPathString` previously contained its own
inverse (printing, rather than parsing) in order to validate what was
parsed. Now that is pulled out into its own separate function:
`EvalState::coerceToSingleDerivedPath`.
In additional that pulled out logic is deduplicated with
`EvalState::mkOutputString` via `EvalState::mkOutputStringRaw`, which is
itself deduplicated (and generalized) with
`DownstreamPlaceholder::mkOutputStringRaw`.
All these changes make the unit tests simpler.
(We would ideally write more unit tests for `mkSingleDerivedPathString`
`coerceToSingleDerivedPath` directly, but we cannot yet do that because
the IO in reading the store path won't work when the dummy store cannot
hold anything. Someday we'll have a proper in-memory store which will
work for this.)
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
std::move(state->data) and data.empty() were called in a loop, and
could run with no other threads intervening. Accessing moved objects
is undefined behavior, and could cause a crash.
Virtual methods are no longer valid once the derived destructor has
run. This means the compiler is free to optimize them to be
non-virtual.
Found using clang-tidy
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>
This enables nix to correctly report what will be fetched in the case
that everything is a cache hit.
Note however that if an intermediate build of something which is not
cached could still cause products to end up being substituted if the
intermediate build results in a CA path which is in the cache.
Fixes#8615.
Signed-off-by: Peter Waller <p@pwaller.net>
When receiving a stream of NARs through the ssh-ng protocol, an already
existing path would cause the NAR archive to not be read in the stream,
resulting in trying to parse the NAR as a ValidPathInfo. This results in
the error message:
error: not an absolute path: 'nix-archive-1'
Fixes#6253
Usually this problem is avoided by running QueryValidPaths before
AddMultipleToStore, but can arise when two parallel nix processes gets
the same response from QueryValidPaths. This makes the problem more
prominent when running builds in parallel.
When loading a derivation from a JSON, malformed input would trigger
cryptic "assertion failed" errors. Simply replacing calls to `operator []`
with calls to `.at()` was not enough, as this would cause json.execptions
to be printed verbatim.
Display nice error messages instead and give some indication where the
error happened.
*Before:*
```
$ echo 4 | nix derivation add
error: [json.exception.type_error.305] cannot use operator[] with a string argument with number
$ nix derivation show nixpkgs#hello | nix derivation add
Assertion failed: (it != m_value.object->end()), function operator[], file /nix/store/8h9pxgq1776ns6qi5arx08ifgnhmgl22-nlohmann_json-3.11.2/include/nlohmann/json.hpp, line 2135.
$ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add
error: [json.exception.type_error.302] type must be string, but is object
$ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add
error: [json.exception.type_error.302] type must be object, but is string
```
*After:*
```
$ echo 4 | nix derivation add
error: Expected JSON of derivation to be of type 'object', but it is of type 'number'
$ nix derivation show nixpkgs#hello | nix derivation add
error: Expected JSON object to contain key 'name' but it doesn't
$ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add
error: Expected JSON value to be of type 'string' but it is of type 'number'
$ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add
error:
… while reading key 'outputs'
error: Expected JSON value to be of type 'object' but it is of type 'string'
```
I haven't checked when this was exactly introduced, but on Nix 2.16 I
realized that the additional lines inserted when using `--precise` are
completely separated from the tree:
nix why-depends /nix/store/ccgr4faaxys39s091qridxg1947lggh4-evcxr-0.14.2 /nix/store/b7hvml0m3qmqraz1022fwvyyg6fc1vdy-gcc-12.2.0 --precise --extra-experimental-features nix-command
/nix/store/ccgr4faaxys39s091qridxg1947lggh4-evcxr-0.14.2
→ /nix/store/lcf37pgp3rgww67v9x2990hbfwx96c1w-gcc-wrapper-12.2.0
→ /nix/store/b7hvml0m3qmqraz1022fwvyyg6fc1vdy-gcc-12.2.0
└───bin/evcxr: …':'}.PATH=${PATH/':''/nix/store/lcf37pgp3rgww67v9x2990hbfwx96c1w-gcc-wrapper-12.2.0/bin'':'/':'}…
└───bin/cpp: …k disable=SC2193.[[ "/nix/store/b7hvml0m3qmqraz1022fwvyyg6fc1vdy-gcc-12.2.0/bin/cpp" = *++ ]] &&…
This is apparently because `std::cout` is buffered and flushed in the
end whereas the rest of the output isn't. The fix is rather simple, just
use `logger->cout` as it's already the case for the rest of the code.
This way we also don't need to insert additional newlines in the `hits`
map since that's something the logger takes care of.
Also added a small test to make sure that the layout of this is somehow
tested to reduce the risk of further regressions here.
Avoid duplicated code, and also avoid "on the fly" path construction
(which makes it harder to keep track of which paths we use).
The factored out code doesn't create the Nix state dir anymore, but this
is fine because other in nix-env and nix-channel does:
- nix-channel: Line 158 in this commit
- nix-env: Line 1407 in this commit
Special-casing the file name is rather ugly, so we shouldn't do
that. So now any {file,http,https} URL is handled by
TarballInputScheme, except for non-flake inputs (i.e. inputs that have
the attribute `flake = false`).
It was initially unclear to me which of these are temporary state for
the verify paths computation, and which of these are the results of that
computation to be used in the rest of the function. Now, it is clear,
and enforced.
We don't care about non-store-paths in there (things like `.links`, are,
in fact, allowed). So let's just skip them up front and be more strongly
typed.
Over the last year or so I've run into several use cases where I need to
parse and/or serialize URLs for use by `builtins.fetchTree` or
`builtins.getFlake`, largely in order to produce _lockfile-like_ files
for lang2nix frameworks or tools which use `nix` internally to drive
builds.
I've gone through the painstaking process of emulating
`nix::FlakeRef::fromAttrs` and `nix::parseFlakeRef` several times with
mixed success; but these are difficult to create and even harder to
maintain if I hope to stay aligned with changes to the real
parser/serializer.
I understand why adding new `builtins` isn't something we want to do
flagrantly. I'm recommending this addition simply because I keep
encountering use cases where I need to parse/serialize these URIs in
`nix` expressions, and I want a reliable solution.
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: John Ericson <git@JohnEricson.me>
Will need to do subclass-specific implementations in the next commit.
This isn't because there will be multiple variations of the daemon
protocol (whew!) but because different clients pick and choose different
parts to use.
This makes it more useful. In general, the derivation will be in one
store, and the realisation info is in another.
This also helps us avoid duplication. See how `resolveDerivedPath` is
now simpler because it uses `queryPartialDerivationOutputMap`. In #8369
we get more flavors of derived path, and need more code to resolve them
all, and this problem only gets worse.
The fact that we need a new method to deal with the multiple dispatch is
unfortunate, but this generally relates to the fact that `Store` is a
sub-par interface, too bulky/unwieldy and conflating separate concerns.
Solving that is out of scope of this PR.
This is part of the RFC 92 work. See tracking issue #6316
the original change broke many pre-existing anchor links.
also change formatting of the constants listing slightly:
- the type should not be part of the anchor
- add highlight to the "impure only" note
Robert Hensing
Christina Rust
Yorick
Maximilian Bosch