Motivation

There are two changes going on here: changing BuildResult and changing the goal state machine logic.

Introduce Worker::makeGoal

This takes a DerivedPath so the caller doesn't need to care about which sort of goal does what.

BuildResult changes

Make BuildResult be like it was before, and instead made KeyedBuildResult be a subclass wit the path. Only buildPathsWithResults returns KeyedBuildResults, everything else just becomes like it was before, where the "key" is unambiguous from context.

The idea here is that storing the path when it is already unambiguous from context denormalizes our data model, making two sources of truth that must be kept in sync. This isn't ideal.

This is especially notable for DerivationGoal, where two DerivedPath requests may correspond to the same DerivationGoal. Before, the BuildResult of that goal (stored, then returned) would just be the first one to create that goal. That is non-deterministic!

KeyedBuildResult is useful in the return type of KeyedBuildResult, but this is because it is conceptually returning an mapping of DerivedPath keys to BuildResult values. We could, for example, also use std::map<BuildResult, BuildResult> for this purpose (and probably also have the arguments be a std::set not std::vector for symmetry), in which case we would here also not need KeyedBuildResult.

State machine changes

The needRestart, retrySubstitution, and retriedSubstitution bools are replaced with enums. The enum cases and conditional logic are exhaustively documented to try to make the code easier to understand.

An especially thorny case with the old code was that needRestart was set to true if the outputs changed for a goal that had progressed to building, but needRestart would never be read again. This is an in fact a good thing operationally, as a build will produce all outputs and so restarting is not necessary. But this (a) being how it worked, and (b) being a good thing was quite obfuscated just reading the code!

The new code and comments call out this case explicitly.

Introduce SingleDrvOutputs

In many cases we are dealing with a collection of realisations, they are all outputs of the same derivation. In that case, we don't need "derivation hashes modulos" to be part of our map key, because the output names alone will be unique. Those hashes are still part of the realisation proper, so we aren't loosing any information, we're just "normalizing our schema" by narrowing the "primary key".

Besides making our data model a bit "tighter" this allows us to avoid a double for loop in DerivationGoal::waiteeDone. The inner for loop was previously just to select the output we cared about without knowing its hash. Now we can just select the output by name directly.

Context

Protocol non-changes

Note that neither protocol is changed as part of this: we are still transferring DrvOutputs over the wire for BuildResults. I would only consider revising this once #6223 is merged, and we can mention protocol versions inside factored-out serialization logic. Until then it is better not change anything because it would come a the cost of code reuse.

Initializers

Unfortunately, we need to avoid C++20 strictness on designated initializers.

(BTW https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2287r1.html
this offers some new syntax for this use-case. Hopefully this will be
adopted and we can eventually use it.)

No having that yet, maybe it would be better to not make KeyedBuildResult a subclass to just avoid this.

History of this PR

In #6311 (comment), I thought that since derivation goals' wanted outputs can "grow" due to overlapping dependencies (See DerivationGoal::addWantedOutputs, called by Worker::makeDerivationGoalCommon), the previous bug fix had an unfortunate side effect of causing more pointless rebuilds.

In particular, I was worried about this situation:

1. Goal made from DerivedPath::Built { foo, OutputsSpec::Names { a } }.

2. Goal gives up on on substituting, starts building.

3. Goal made from DerivedPath::Built { foo, OutputsSpec::Names { b } }, in fact is just modified original goal.

4. Though the goal had gotten as far as building, so all outputs were going to be produced, addWantedOutputs no longer knows that and so the goal is flagged to be restarted.

This might sound far-fetched with input-addressed drvs, where we usually basically have all our goals "planned out" before we start doing anything, but with CA derivation goals and especially RFC 92, where drv resolution means goals are created after some building is completed, it would be more likely to happen.

@edolstra, on the other hand, thought this was not an issue because even though needsRestart = true would be set, it would never be read again. Eelco might be right, but nonetheless this is still very confusing storing "needs restart" but in fact neither needing nor wanting a restart!

So the first thing I did was restore the clearing of wantedOutputs we used to do, and then filter the outputs in buildPathsWithResults to only get the ones we care about.

The KeyedBuildResult change technically was already needed due to the "modified derivational goal with second request" issue described in the motivation section, but become more urgent with this modification of wantedOutputs.

Eelco however (if I recalled correctly) in real time meeting (so no paper trail) also didn't like modifying wantedOutputs upon falling back to doing a build (as we did before), because we should only modify it in response to new requests --- actual wants --- and not because we are "incidentally" building all the outptus beyond what may have been requested.

That's a fair point, and the alternative is to replace the boolean soup with proper enums: Instead of modifying wantedOuputs som more, we'll modify needsRestart to indicate we are passed the need. That lead me to the bool -> enum changes, and then I did the substitution ones in addition too for consistency.

"Keyed" data types in general.

I think separating the "primary key" field(s) from the other fields is good practical in general. (I would like to do the same thing for ValidPathInfo, for example.) Besides the example in the motivation, there are probably other cases where we would like to do std::map<Key, ThingWithoutKey> where the "without keys" is necessary to not contain duplicate keys and just precludes the possibility of those duplicate keys being out of sync.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests - tests/nixos/*
• documentation in the manual
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or incompatible change: updated release notes