Make `KeyedBuildResult`, `BuildResult` like before, and fix bug another way

In https://github.com/NixOS/nix/pull/6311#discussion_r834863823, I
realized 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 paticular, we have this situation:

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

2. Goal gives on on substituting, starts building.

3. Goal made from `DerivedPath::Built { foo, {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
is more likely to happen.

So the first thing to do 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.

But fix also has its own side effect in that the `DerivedPath` in the
`BuildResult` in `DerivationGoal` cannot be trusted; it is merely the
*first* `DerivedPath` for which this goal was originally created.

To remedy this, I made `BuildResult` be like it was before, and instead
made `KeyedBuildResult` be a subclass wit the path. Only
`buildPathsWithResults` returns `KeyedBuildResult`s, everything else
just becomes like it was before, where the "key" is unambiguous from
context.

I think separating the "primary key" field(s) from the other fields is
good practical in general anyways. (I would like to do the same thing
for `ValidPathInfo`.) Among other things, it allows constructions like
`std::map<Key, ThingWithKey>` where doesn't contain duplicate keys and
just precludes the possibility of those duplicate keys being out of
sync.

We might leverage the above someday to overload `buildPathsWithResults`
to take a *set* of return a *map* per the above.

-----

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.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

1 files changed, 23 insertions, 0 deletions

diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc
index d59b94797..13b2e509a 100644
--- a/src/libstore/build/goal.cc
+++ b/src/libstore/build/goal.cc
@@ -11,6 +11,29 @@ bool CompareGoalPtrs::operator() (const GoalPtr & a, const GoalPtr & b) const {
 }
 
 
+BuildResult Goal::getBuildResult(const DerivedPath & req) {
+    BuildResult res { buildResult };
+
+    if (auto pbp = std::get_if<DerivedPath::Built>(&req)) {
+        auto & bp = *pbp;
+
+        /* Because goals are in general shared between derived paths
+           that share the same derivation, we need to filter their
+           results to get back just the results we care about.
+         */
+
+        for (auto it = res.builtOutputs.begin(); it != res.builtOutputs.end();) {
+            if (bp.outputs.contains(it->first.outputName))
+                ++it;
+            else
+                it = res.builtOutputs.erase(it);
+        }
+    }
+
+    return res;
+}
+
+
 void addToWeakGoals(WeakGoals & goals, GoalPtr p)
 {
     if (goals.find(p) != goals.end())