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>
This method does *not* create a new type of goal. We instead just make
`DerivationGoal` more sophisticated, which is much easier to do now that
`DerivationBuildingGoal` has been split from it (and so many fields are
gone, or or local variables instead).
This avoids the need for a secondarily trampoline goal that interacted
poorly with `addWantedOutputs`. That, I hope, will mean the bugs from
before do not reappear.
There may in fact be a reason to introduce such a trampoline in the
future, but it would only happen in conjunction with getting rid of
`addWantedOutputs`.
Restores the functionality (and tests) that was reverted in
f4f28cdd0e.
Cherry-pick https://gerrit.lix.systems/c/lix/+/2100
This change fixes a potential concurrency failure when accessing random
which is not thread safe.
Co-authored-by: Lily Ballard <lily@ballards.net>
Try to make `DerivationGoal` care less whether we're working from an
in-memory derivation or not.
It's a clean-up in its own right, but it will also help with other
cleanups under the umbrella of #12628.
