Aftet the previous simplifications, there is no reason to catch the
error and immediately return it with a `std::variant` --- just let the
caller catch it instead.
Instead of that funny business, the fixed output checks are not put in
`checkOutputs`, with the other (newer) output checks, where they also
better belong. The control flow is reworked (with comments!) so that
`checkOutputs` also runs in the `bmCheck` case.
Not only does this preserve existing behavior of `bmCheck`
double-checking fixed output hashes with less tricky code, it also makes
`bmCheck` better by also double-checking the other output checks, rather
than just assuming they pass if the derivation is deterministic.
It's fine to set these worker flags a little later in the control flow,
since we'll be sure to reach those points in the error cases. And doing
that is much nicer than having these tangled callbacks.
I originally made the callbacks to meticulously recreate the exact
behavior which I didn't quite understand. Now, thanks to cleaning up the
error handling, I do understand what is going on, so I can be confident
that this change is safe to make.
Instead of passing them around separately, or doing finicky logic in a
try-catch block to recover them, just make `BuildError` always contain a
status, and make it the thrower's responsibility to set it. This is much
more simple and explicit.
Once that change is done, split the `done` functions of `DerivationGoal`
and `DerivationBuildingGoal` into separate success and failure
functions, which ends up being easier to understand and hardly any
duplication.
Also, change the handling of failures in resolved cases to use
`BuildResult::DependencyFailed` and a new message. This is because the
underlying derivation will also get its message printed --- which is
good, because in general the resolved derivation is not unique. One dyn
drv test had to be updated, but CA (and dyn drv) is experimental, so I
do not mind.
Finally, delete `SubstError` because it is unused.
The commit says it was added for CA testing --- manual I assume, since
there is no use of this in the test suite. I don't think we need it any
more, and I am not sure whether it was ever supposed to have made it to
`master` either.
This reverts commit 2eec2f765a.
We currently just use this during the build of a derivation, but there is no
reason we wouldn't want to use it elsewhere, e.g. to check the outputs
of someone else's build after the fact.
Moreover, I like pulling things out of `DerivationBuilder` that are
simple and don't need access to all that state. While
`DerivationBuilder` is unix-only, this refactor also make the code more
portable "for free".
The header is private, at Eelco's request.
With the migration to /nix/var/nix/builds we now have failing builds
when the derivation name is too long.
This change removes the derivation name from the temporary build to have
a predictable prefix length:
Also see: https://github.com/NixOS/infra/pull/764
for context.
This allows us to replace some very hacky and not correct string
concatentation in `HttpBinaryCacheStore`. It will especially be useful
with #13752, when today's hacks started to cause problems in practice,
not just theory.
Also make `fixGitURL` returned a `ParsedURL`.
I need this for some `ParseURL` improvements, but I figure this is
better to send as its own PR.
I changed the tests willy-nilly to sometimes use
`std::list<std::string_view>` instead of `Strings` (which is
`std::list<std::string>`).
Co-Authored-By: Sergei Zimmerman <sergei@zimmerman.foo>
It is suppposed to be "post build" not "during the build" after all. Its
location now matches that for the hook case (see elsewhere in
`DerivationdBuildingGoal`).
It was in a try-catch before, and now it isn't, but I believe that it is
impossible for it to throw `BuildError`, which is sufficient for this
code motion to be correct.
Update src/libutil/windows/current-process.cc
Prefer `nullptr` over `NULL`
Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
Update src/libutil/unix/current-process.cc
Prefer C++ type casts
Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
Update src/libutil/windows/current-process.cc
Prefer C++ type casts
Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
Update src/libutil/unix/current-process.cc
Don't allocate exception
Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
It would have been nice to use std::views::enumerate here, but
it uses a signed difference type for the value_type:
> value_type = std::tuple<difference_type, ranges::range_value_t<Base>>
zip + iota has the same semantics as the code used to have, so there's
no behavior change here.
This is a nicer separation of concerns --- `DerivationBuilder` just
mounts the extra paths you tell it too, and the outside world is
responsible for making sure those extra paths make sense.
Since the closure only depends on global settings, and not
per-derivation information, we also have the option of moving this up
further and caching it across all local builds. (I only just realized
this after having done this refactor. I am not doing that change at this
time, however.)
Now, `DerivationBuilder` only concerns itself with `finalEnv` and
`extraFiles`, in straightforward unconditional code. All the fancy
desugaring logic is consolidated in `DerivationBuildingGoal`.
We should better share the pulled-out logic with `nix-shell`/`nix
develop`, which would fill in some missing features, arguably fixing
bugs.
I think this is a better separation of concerns. `DerivationBuilder`
doesn't need to to the final, query-heavy details about how these things
are constructed. It just operates on the level of "simple, stupid" files
and environment variables.
As much as I prefer rewriting the parsed rather than unparsed JSON for
elegance, this gets in the way of the separation of concerns that I am
trying to do.
As a practical matter, any rewriting that this did will also be done by
the second round of rewriting that remains below, so removing this code
should have no effect.
This is needed to rearrange include order, but I also think it is a good
thing anyways, as we seek to reduce the use of global settings variables
over time.
This avoids problems with older versions of Nix that don't put the
caches in WAL mode. That's generally not a problem, until you do something like
nix build --print-out-paths ... | cachix
which deadlocks because cachix tries to switch the caches to truncate
mode, which requires exclusive access. But the first process cannot
make progress because the cachix process isn't reading from the pipe.
With "truncate" mode, if we try to write to the database while another
process has an active write transaction, we'll block until the other
transaction finishes. This is a problem for the evaluation cache in
particular, since it uses long-running transactions.
WAL mode does not have this issue: it just returns "busy" right away,
so Nix will print
error (ignored): SQLite database '/home/eelco/.cache/nix/eval-cache-v5/...' is busy
and stop trying to write to the evaluation cache. (This was the
intended/original behaviour, see AttrDb::doSQLite().)
This systematizes the way our s3:// URLs are parsed in filetransfer.cc.
Yoinked out and refactored out of [1].
[1]: https://github.com/NixOS/nix/pull/13752
Co-authored-by: Bernardo Meurer Costa <beme@anthropic.com>
