Rework derivation input resolution

I refactored the way that input resolution works in `DerivationGoal`. To
be honest, it is probably unclear to the reader whether this new way is
better or worse. I suppose *intrinsic* motivation, I can say that

- the more structured use of `inputGoal` (a local variable) is better
  than the shotgrun approach with `inputDrvOutputs`

- A virtual `waiteeDone` was a hack, and now it's gone.

However, the *real* motivation of this is not the above things, but that
it is needed for my mammoth refactor fixing NixOS#11897 and NixOS#11928.

It is nice that this step could come first, rather than making that
refactor even bigger.

Author: Ericson2314

src/libstore/build/derivation-creation-and-realisation-goal.cc

@@ -4,7 +4,7 @@
 namespace nix {
 
 DerivationCreationAndRealisationGoal::DerivationCreationAndRealisationGoal(
-    ref<SingleDerivedPath> drvReq, const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
+    ref<const SingleDerivedPath> drvReq, const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
     : Goal(worker, DerivedPath::Built{.drvPath = drvReq, .outputs = wantedOutputs})
     , drvReq(drvReq)
     , wantedOutputs(wantedOutputs)

src/libstore/build/derivation-creation-and-realisation-goal.hh

@@ -30,7 +30,7 @@ struct DerivationCreationAndRealisationGoal : public Goal
     /**
      * How to obtain a store path of the derivation to build.
      */
-    ref<SingleDerivedPath> drvReq;
+    ref<const SingleDerivedPath> drvReq;
 
     /**
      * The path of the derivation, once obtained.
@@ -67,7 +67,7 @@ struct DerivationCreationAndRealisationGoal : public Goal
     BuildMode buildMode;
 
     DerivationCreationAndRealisationGoal(
-        ref<SingleDerivedPath> drvReq,
+        ref<const SingleDerivedPath> drvReq,
         const OutputsSpec & wantedOutputs,
         Worker & worker,
         BuildMode buildMode = bmNormal);

src/libstore/build/derivation-goal.cc

@@ -309,6 +309,18 @@ Goal::Co DerivationGoal::haveDerivation()
 }
 
 
+/**
+ * Used for `inputGoals` local variable below
+ */
+struct value_comparison
+{
+    template <typename T>
+    bool operator()(const ref<T> & lhs, const ref<T> & rhs) const {
+        return *lhs < *rhs;
+    }
+};
+
+
 /* At least one of the output paths could not be
    produced using a substitute.  So we have to build instead. */
 Goal::Co DerivationGoal::gaveUpOnSubstitution()
@@ -317,19 +329,22 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
        is no need to restart. */
     needRestart = NeedRestartForMoreOutputs::BuildInProgressWillNotNeed;
 
-    /* The inputs must be built before we can build this goal. */
-    inputDrvOutputs.clear();
+    std::map<ref<const SingleDerivedPath>, GoalPtr, value_comparison> inputGoals;
+
     if (useDerivation) {
-        std::function<void(ref<SingleDerivedPath>, const DerivedPathMap<StringSet>::ChildNode &)> addWaiteeDerivedPath;
+        std::function<void(ref<const SingleDerivedPath>, const DerivedPathMap<StringSet>::ChildNode &)> addWaiteeDerivedPath;
 
-        addWaiteeDerivedPath = [&](ref<SingleDerivedPath> inputDrv, const DerivedPathMap<StringSet>::ChildNode & inputNode) {
-            if (!inputNode.value.empty())
-                addWaitee(worker.makeGoal(
+        addWaiteeDerivedPath = [&](ref<const SingleDerivedPath> inputDrv, const DerivedPathMap<StringSet>::ChildNode & inputNode) {
+            if (!inputNode.value.empty()) {
+                auto g = worker.makeGoal(
                     DerivedPath::Built {
                         .drvPath = inputDrv,
                         .outputs = inputNode.value,
                     },
-                    buildMode == bmRepair ? bmRepair : bmNormal));
+                    buildMode == bmRepair ? bmRepair : bmNormal);
+                inputGoals.insert_or_assign(inputDrv, g);
+                addWaitee(std::move(g));
+            }
             for (const auto & [outputName, childNode] : inputNode.childMap)
                 addWaiteeDerivedPath(
                     make_ref<SingleDerivedPath>(SingleDerivedPath::Built { inputDrv, outputName }),
@@ -417,15 +432,32 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
             [&](const DerivationType::Impure &) {
                 return true;
             }
-        }, drvType.raw);
+        }, drvType.raw)
+            /* no inputs are outputs of dynamic derivations */
+            || std::ranges::any_of(
+                fullDrv.inputDrvs.map.begin(),
+                fullDrv.inputDrvs.map.end(),
+                [](auto & pair) { return !pair.second.childMap.empty(); });
 
         if (resolveDrv && !fullDrv.inputDrvs.map.empty()) {
             experimentalFeatureSettings.require(Xp::CaDerivations);
 
             /* We are be able to resolve this derivation based on the
                now-known results of dependencies. If so, we become a
                stub goal aliasing that resolved derivation goal. */
-            std::optional attempt = fullDrv.tryResolve(worker.store, inputDrvOutputs);
+            std::optional attempt = fullDrv.tryResolve(worker.store,
+                [&](ref<const SingleDerivedPath> drvPath, const std::string & outputName) -> std::optional<StorePath> {
+                    auto mEntry = get(inputGoals, drvPath);
+                    if (!mEntry) return std::nullopt;
+
+                    auto buildResult = (*mEntry)->getBuildResult(DerivedPath::Built{drvPath, OutputsSpec::Names{outputName}});
+                    if (!buildResult.success()) return std::nullopt;
+
+                    auto i = get(buildResult.builtOutputs, outputName);
+                    if (!i) return std::nullopt;
+
+                    return i->outPath;
+                });
             if (!attempt) {
               /* TODO (impure derivations-induced tech debt) (see below):
                  The above attempt should have found it, but because we manage
@@ -456,54 +488,31 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
             co_return resolvedFinished();
         }
 
-        std::function<void(const StorePath &, const DerivedPathMap<StringSet>::ChildNode &)> accumInputPaths;
-
-        accumInputPaths = [&](const StorePath & depDrvPath, const DerivedPathMap<StringSet>::ChildNode & inputNode) {
-            /* Add the relevant output closures of the input derivation
-               `i' as input paths.  Only add the closures of output paths
-               that are specified as inputs. */
-            auto getOutput = [&](const std::string & outputName) {
-                /* TODO (impure derivations-induced tech debt):
-                   Tracking input derivation outputs statefully through the
-                   goals is error prone and has led to bugs.
-                   For a robust nix, we need to move towards the `else` branch,
-                   which does not rely on goal state to match up with the
-                   reality of the store, which is our real source of truth.
-                   However, the impure derivations feature still relies on this
-                   fragile way of doing things, because its builds do not have
-                   a representation in the store, which is a usability problem
-                   in itself. When implementing this logic entirely with lookups
-                   make sure that they're cached. */
-                if (auto outPath = get(inputDrvOutputs, { depDrvPath, outputName })) {
-                    return *outPath;
+        /* If we get this far, we know no dynamic drvs inputs */
+
+        for (auto & [depDrvPath, depNode] : fullDrv.inputDrvs.map) {
+            for (auto & outputName : depNode.value) {
+                /* Don't need to worry about `inputGoals`, because
+                   impure derivations are always resolved above. Can
+                   just use DB. This case only happens in the (older)
+                   input addressed and fixed output derivation cases. */
+                auto outMap = [&]{
+                    for (auto * drvStore : { &worker.evalStore, &worker.store })
+                        if (drvStore->isValidPath(depDrvPath))
+                            return worker.store.queryDerivationOutputMap(depDrvPath, drvStore);
+                    assert(false);
+                }();
+
+                auto outMapPath = outMap.find(outputName);
+                if (outMapPath == outMap.end()) {
+                    throw Error(
+                        "derivation '%s' requires non-existent output '%s' from input derivation '%s'",
+                        worker.store.printStorePath(drvPath), outputName, worker.store.printStorePath(depDrvPath));
                 }
-                else {
-                    auto outMap = [&]{
-                        for (auto * drvStore : { &worker.evalStore, &worker.store })
-                            if (drvStore->isValidPath(depDrvPath))
-                                return worker.store.queryDerivationOutputMap(depDrvPath, drvStore);
-                        assert(false);
-                    }();
-
-                    auto outMapPath = outMap.find(outputName);
-                    if (outMapPath == outMap.end()) {
-                        throw Error(
-                            "derivation '%s' requires non-existent output '%s' from input derivation '%s'",
-                            worker.store.printStorePath(drvPath), outputName, worker.store.printStorePath(depDrvPath));
-                    }
-                    return outMapPath->second;
-                }
-            };
 
-            for (auto & outputName : inputNode.value)
-                worker.store.computeFSClosure(getOutput(outputName), inputPaths);
-
-            for (auto & [outputName, childNode] : inputNode.childMap)
-                accumInputPaths(getOutput(outputName), childNode);
-        };
-
-        for (auto & [depDrvPath, depNode] : fullDrv.inputDrvs.map)
-            accumInputPaths(depDrvPath, depNode);
+                worker.store.computeFSClosure(outMapPath->second, inputPaths);
+            }
+        }
     }
 
     /* Second, the input sources. */
@@ -1532,35 +1541,4 @@ Goal::Done DerivationGoal::done(
     return amDone(buildResult.success() ? ecSuccess : ecFailed, std::move(ex));
 }
 
-
-void DerivationGoal::waiteeDone(GoalPtr waitee, ExitCode result)
-{
-    Goal::waiteeDone(waitee, result);
-
-    if (!useDerivation || !drv) return;
-    auto & fullDrv = *dynamic_cast<Derivation *>(drv.get());
-
-    std::optional info = tryGetConcreteDrvGoal(waitee);
-    if (!info) return;
-    const auto & [dg, drvReq] = *info;
-
-    auto * nodeP = fullDrv.inputDrvs.findSlot(drvReq.get());
-    if (!nodeP) return;
-    auto & outputs = nodeP->value;
-
-    for (auto & outputName : outputs) {
-        auto buildResult = dg.get().getBuildResult(DerivedPath::Built {
-            .drvPath = makeConstantStorePathRef(dg.get().drvPath),
-            .outputs = OutputsSpec::Names { outputName },
-        });
-        if (buildResult.success()) {
-            auto i = buildResult.builtOutputs.find(outputName);
-            if (i != buildResult.builtOutputs.end())
-                inputDrvOutputs.insert_or_assign(
-                    { dg.get().drvPath, outputName },
-                    i->second.outPath);
-        }
-    }
-}
-
 }

src/libstore/build/derivation-goal.hh

@@ -82,13 +82,6 @@ struct DerivationGoal : public Goal
      */
     OutputsSpec wantedOutputs;
 
-    /**
-     * Mapping from input derivations + output names to actual store
-     * paths. This is filled in by waiteeDone() as each dependency
-     * finishes, before `trace("all inputs realised")` is reached.
-     */
-    std::map<std::pair<StorePath, std::string>, StorePath> inputDrvOutputs;
-
     /**
      * See `needRestart`; just for that field.
      */
@@ -335,8 +328,6 @@ struct DerivationGoal : public Goal
         SingleDrvOutputs builtOutputs = {},
         std::optional<Error> ex = {});
 
-    void waiteeDone(GoalPtr waitee, ExitCode result) override;
-
     StorePathSet exportReferences(const StorePathSet & storePaths);
 
     JobCategory jobCategory() const override {

src/libstore/build/goal.hh

@@ -429,7 +429,7 @@ public:
 
     void addWaitee(GoalPtr waitee);
 
-    virtual void waiteeDone(GoalPtr waitee, ExitCode result);
+    void waiteeDone(GoalPtr waitee, ExitCode result);
 
     virtual void handleChildOutput(Descriptor fd, std::string_view data)
     {

src/libstore/build/worker.cc

@@ -45,7 +45,7 @@ Worker::~Worker()
 
 
 std::shared_ptr<DerivationCreationAndRealisationGoal> Worker::makeDerivationCreationAndRealisationGoal(
-    ref<SingleDerivedPath> drvReq,
+    ref<const SingleDerivedPath> drvReq,
     const OutputsSpec & wantedOutputs,
     BuildMode buildMode)
 {
@@ -601,17 +601,4 @@ GoalPtr upcast_goal(std::shared_ptr<DerivationGoal> subGoal)
     return subGoal;
 }
 
-std::optional<std::pair<std::reference_wrapper<const DerivationGoal>, std::reference_wrapper<const SingleDerivedPath>>> tryGetConcreteDrvGoal(GoalPtr waitee)
-{
-    auto * odg = dynamic_cast<DerivationCreationAndRealisationGoal *>(&*waitee);
-    if (!odg) return std::nullopt;
-    /* If we failed to obtain the concrete drv, we won't have created
-       the concrete derivation goal. */
-    if (!odg->concreteDrvGoal) return std::nullopt;
-    return {{
-        std::cref(*odg->concreteDrvGoal),
-        std::cref(*odg->drvReq),
-    }};
-}
-
 }

src/libstore/build/worker.hh

@@ -37,21 +37,6 @@ GoalPtr upcast_goal(std::shared_ptr<DerivationGoal> subGoal);
 
 typedef std::chrono::time_point<std::chrono::steady_clock> steady_time_point;
 
-/**
- * The current implementation of impure derivations has
- * `DerivationGoal`s accumulate realisations from their waitees.
- * Unfortunately, `DerivationGoal`s don't directly depend on other
- * goals, but instead depend on `DerivationCreationAndRealisationGoal`s.
- *
- * We try not to share any of the details of any goal type with any
- * other, for sake of modularity and quicker rebuilds. This means we
- * cannot "just" downcast and fish out the field. So as an escape hatch,
- * we have made the function, written in `worker.cc` where all the goal
- * types are visible, and use it instead.
- */
-
-std::optional<std::pair<std::reference_wrapper<const DerivationGoal>, std::reference_wrapper<const SingleDerivedPath>>> tryGetConcreteDrvGoal(GoalPtr waitee);
-
 /**
  * A mapping used to remember for each child process to what goal it
  * belongs, and comm channels for receiving log data and output
@@ -218,7 +203,7 @@ public:
      */
 private:
     std::shared_ptr<DerivationCreationAndRealisationGoal> makeDerivationCreationAndRealisationGoal(
-        ref<SingleDerivedPath> drvPath,
+        ref<const SingleDerivedPath> drvPath,
         const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal);
     std::shared_ptr<DerivationGoal> makeDerivationGoalCommon(
         const StorePath & drvPath, const OutputsSpec & wantedOutputs,