It's hard to tell if I changed any behavior, but if I did, I think I
made it better, because now we explicitly move stuff out of the chroot
(if we were going to) before trying to delete the chroot.
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.
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.
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.
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 is just more honest, since we downcasted it to `LocalStore` in many
places. We had the downcast before because it wasn't needed in the hook
case, just the local building case, but now that `DerivationBuilder` is
separated and just does the building case, we have formalized the
boundary where the single downcast should occur.
No derivation goal type has a notion of variable wanted outputs any
more. They either want them all, or they just care about a single
output, in which case we would just store this information for the one
output in question.
This leads to a use-after free, because staticOutputHashes returns a temporary
object that dies before we can do a `return *mOutputHash`.
This is most likely the cause for random failures in Hydra [1].
[1]: https://hydra.nixos.org/build/305091330/nixlog/2
The problem with old code was that it used getUri for both the `diskCache`
as well as logging. This is really bad because it mixes the textual human
readable representation with the caching.
Also using getUri for the cache key is really problematic for the S3 store,
since it doesn't include the `endpoint` in the cache key, so it's totally broken.
This starts separating the logging / cache concerns by introducing a
`getHumanReadableURI` that should only be used for logging. The caching
logic now instead uses `getReference().render(/*withParams=*/false)` exclusively.
This would need to be fixed in follow-ups, because that's really fragile and
broken for some store types (but it was already broken before).
Move output result filtering logic and assert just into the branch where
it is not obviously a no op / meeting the assertion.
Add a comment too, while we are at it.
Now that `DerivationGoal::checkPathValidity` is legible, we can see that
it only sets `outputKnown`, and doesn't read it. Likewise, with
co-routines, we don't have tiny scopes that make local variables
difficult. Between these two things, we can simply have
`checkPathValidity` return what it finds, rather than mutate some state,
and update everyting to use local variables.
The same transformation could probably be done to the other derivation
goal types (which currently, unfortunately, contain their own
`checkPathValidity`s, though they are diverging, and we hope and believe
that they continue to diverge).
`Store::queryPartialDerivationOutputMap` is nothing but checking
statically-known output paths, and then `Store::queryRealisation`, and
we were doing both of those things already. Inline that and simplify,
again taking advantage of the fact that we only care about one output.
Instead of parsing a structured attrs at some later point, we parsed it
right away when parsing the A-Term format, and likewise serialize it to
`__json = <JSON dump>` when serializing a derivation to A-Term.
The JSON format can directly contain the JSON structured attrs without
so encoding it, so we just do that.
* It is tough to contribute to a project that doesn't use a formatter,
* It is extra hard to contribute to a project which has configured the formatter, but ignores it for some files
* Code formatting makes it harder to hide obscure / weird bugs by accident or on purpose,
Let's rip the bandaid off?
Note that PRs currently in flight should be able to be merged relatively easily by applying `clang-format` to their tip prior to merge.
The use of R"(...)" added a bunch of unnecessary whitespace, e.g.
error:
Unable to start any build;
either increase '--max-jobs' or enable remote builds.
For more information run 'man nix.conf' and search for '/machines'.
Now we get
error: Unable to start any build; either increase '--max-jobs' or enable remote builds.
For more information run 'man nix.conf' and search for '/machines'.
This is just a code cleanup; it should not be behavior change.
`addWantedOutputs` is removed by introducing `DerivationTrampolineGoal`.
`DerivationGoal` now only tracks a single output, and is back to
tracking a plain store path `drvPath`, not a deriving path one. Its
`addWantedOutputs` method is gone. These changes will allow subsequent
PRs to simplify it greatly.
Because the purpose of each goal is back to being immutable, we can also
once again make `Goal::buildResult` a public field, and get rid of the
`getBuildResult` method. This simplifies things also.
`DerivationTrampolineGoal` is, as the nane is supposed to indicate, a
cheap "trampoline" goal. It takes immutable sets of wanted outputs, and
just kicks of `DerivationGoal`s for them. Since now "actual work" is
done in these goals, it is not wasteful to have separate ones for
separate sets of outputs, even if those outputs (and the derivations
they are from) overlap.
This design is described in more detail in the doc comments on the goal
types, which I've now greatly expanded.
---
This separation of concerns will make it possible for future work on
issues like #11928, and to continue the path of having more goal types,
but each goal type does fewer things (issue #12628).
---
This commit in some sense reverts
f4f28cdd0e, but that one kept around
`addWantedOutputs`. I am quite sure it was having two layers of goals
with `addWantedOutputs` that caused the issues --- restarting logic like
`addWantedOutputs` has is very tempermental! In this version of the
change, we have *zero* layers of `addWantedOutputs` --- no goal type
needs it, or otherwise has a mutable objective --- and so I think this
change is safe.
Co-authored-by: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com>
