Restore dynamic derivations!

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
f4f28cd.

Author: Ericson2314

src/libstore/build/derivation-building-goal.cc

@@ -296,7 +296,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
 
             // FIXME wanted outputs
             auto resolvedDrvGoal = worker.makeDerivationGoal(
-                pathResolved, OutputsSpec::All{}, buildMode);
+                makeConstantStorePathRef(pathResolved), OutputsSpec::All{}, buildMode);
             {
                 Goals waitees{resolvedDrvGoal};
                 co_await await(std::move(waitees));
@@ -338,7 +338,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
 
                       throw Error(
                           "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/realisation)",
-                          worker.store.printStorePath(resolvedDrvGoal->drvPath), outputName);
+                          resolvedDrvGoal->drvReq->to_string(worker.store), outputName);
                     }();
 
                     if (!drv->type().isImpure()) {

src/libstore/build/derivation-goal.cc

@@ -24,16 +24,16 @@
 
 namespace nix {
 
-DerivationGoal::DerivationGoal(const StorePath & drvPath,
+DerivationGoal::DerivationGoal(ref<const SingleDerivedPath> drvReq,
     const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
     : Goal(worker, loadDerivation())
-    , drvPath(drvPath)
+    , drvReq(drvReq)
     , wantedOutputs(wantedOutputs)
     , buildMode(buildMode)
 {
     name = fmt(
         "building of '%s' from .drv file",
-        DerivedPath::Built { makeConstantStorePathRef(drvPath), wantedOutputs }.to_string(worker.store));
+        DerivedPath::Built { drvReq, wantedOutputs }.to_string(worker.store));
     trace("created");
 
     mcExpectedBuilds = std::make_unique<MaintainCount<uint64_t>>(worker.expectedBuilds);
@@ -43,16 +43,16 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath,
 
 DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv,
     const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
-    : Goal(worker, haveDerivation())
-    , drvPath(drvPath)
+    : Goal(worker, haveDerivation(drvPath))
+    , drvReq(makeConstantStorePathRef(drvPath))
     , wantedOutputs(wantedOutputs)
     , buildMode(buildMode)
 {
     this->drv = std::make_unique<Derivation>(drv);
 
     name = fmt(
         "building of '%s' from in-memory derivation",
-        DerivedPath::Built { makeConstantStorePathRef(drvPath), drv.outputNames() }.to_string(worker.store));
+        DerivedPath::Built { drvReq, drv.outputNames() }.to_string(worker.store));
     trace("created");
 
     mcExpectedBuilds = std::make_unique<MaintainCount<uint64_t>>(worker.expectedBuilds);
@@ -61,13 +61,24 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation
 }
 
 
+static StorePath pathPartOfReq(const SingleDerivedPath & req)
+{
+    return std::visit(
+        overloaded{
+            [&](const SingleDerivedPath::Opaque & bo) { return bo.path; },
+            [&](const SingleDerivedPath::Built & bfd) { return pathPartOfReq(*bfd.drvPath); },
+        },
+        req.raw());
+}
+
+
 std::string DerivationGoal::key()
 {
     /* Ensure that derivations get built in order of their name,
        i.e. a derivation named "aardvark" always comes before
        "baboon". And substitution goals always happen before
        derivation goals (due to "b$"). */
-    return "b$" + std::string(drvPath.name()) + "$" + worker.store.printStorePath(drvPath);
+    return "b$" + std::string(pathPartOfReq(*drvReq).name()) + "$" + drvReq->to_string(worker.store);
 }
 
 
@@ -99,18 +110,37 @@ Goal::Co DerivationGoal::loadDerivation() {
         /* The first thing to do is to make sure that the derivation
            exists.  If it doesn't, it may be created through a
            substitute. */
-
-        if (buildMode != bmNormal || !worker.evalStore.isValidPath(drvPath)) {
-            Goals waitees{upcast_goal(worker.makePathSubstitutionGoal(drvPath))};
+        if (auto optDrvPath = [this]() -> std::optional<StorePath> {
+                if (buildMode != bmNormal)
+                    return std::nullopt;
+
+                auto drvPath = StorePath::dummy;
+                try {
+                    drvPath = resolveDerivedPath(worker.store, *drvReq);
+                } catch (MissingRealisation &) {
+                    return std::nullopt;
+                }
+                auto cond = worker.evalStore.isValidPath(drvPath) || worker.store.isValidPath(drvPath);
+                return cond ? std::optional{drvPath} : std::nullopt;
+            }()) {
+            trace(
+                fmt("already have drv '%s' for '%s', can go straight to building",
+                    worker.store.printStorePath(*optDrvPath),
+                    drvReq->to_string(worker.store)));
+        } else {
+            trace("need to obtain drv we want to build");
+            Goals waitees{worker.makeGoal(DerivedPath::fromSingle(*drvReq))};
             co_await await(std::move(waitees));
         }
 
-        trace("loading derivation");
+        trace("outer load and build derivation");
 
         if (nrFailed != 0) {
-            co_return done(BuildResult::MiscFailure, {}, Error("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath)));
+            co_return amDone(ecFailed, Error("cannot build missing derivation '%s'", drvReq->to_string(worker.store)));
         }
 
+        StorePath drvPath = resolveDerivedPath(worker.store, *drvReq);
+
         /* `drvPath' should already be a root, but let's be on the safe
            side: if the user forgot to make it a root, we wouldn't want
            things being garbage collected while we're busy. */
@@ -129,13 +159,13 @@ Goal::Co DerivationGoal::loadDerivation() {
             }
         }
         assert(drv);
-    }
 
-    co_return haveDerivation();
+        co_return haveDerivation(drvPath);
+    }
 }
 
 
-Goal::Co DerivationGoal::haveDerivation()
+Goal::Co DerivationGoal::haveDerivation(StorePath drvPath)
 {
     trace("have derivation");
 
@@ -221,11 +251,11 @@ Goal::Co DerivationGoal::haveDerivation()
 
     {
         /* Check what outputs paths are not already valid. */
-        auto [allValid, validOutputs] = checkPathValidity();
+        auto [allValid, validOutputs] = checkPathValidity(drvPath);
 
         /* If they are all valid, then we're done. */
         if (allValid && buildMode == bmNormal) {
-            co_return done(BuildResult::AlreadyValid, std::move(validOutputs));
+            co_return done(drvPath, BuildResult::AlreadyValid, std::move(validOutputs));
         }
     }
 
@@ -262,7 +292,7 @@ Goal::Co DerivationGoal::haveDerivation()
     assert(!drv->type().isImpure());
 
     if (nrFailed > 0 && nrFailed > nrNoSubstituters && !settings.tryFallback) {
-        co_return done(BuildResult::TransientFailure, {},
+        co_return done(drvPath, BuildResult::TransientFailure, {},
             Error("some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ",
                 worker.store.printStorePath(drvPath)));
     }
@@ -271,16 +301,16 @@ Goal::Co DerivationGoal::haveDerivation()
 
     if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) {
         needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
-        co_return haveDerivation();
+        co_return haveDerivation(std::move(drvPath));
     }
 
-    auto [allValid, validOutputs] = checkPathValidity();
+    auto [allValid, validOutputs] = checkPathValidity(drvPath);
 
     if (buildMode == bmNormal && allValid) {
-        co_return done(BuildResult::Substituted, std::move(validOutputs));
+        co_return done(drvPath, BuildResult::Substituted, std::move(validOutputs));
     }
     if (buildMode == bmRepair && allValid) {
-        co_return repairClosure();
+        co_return repairClosure(std::move(drvPath));
     }
     if (buildMode == bmCheck && !allValid)
         throw Error("some outputs of '%s' are not valid, so checking is not possible",
@@ -303,7 +333,7 @@ struct value_comparison
 };
 
 
-Goal::Co DerivationGoal::repairClosure()
+Goal::Co DerivationGoal::repairClosure(StorePath drvPath)
 {
     assert(!drv->type().isImpure());
 
@@ -313,7 +343,7 @@ Goal::Co DerivationGoal::repairClosure()
        that produced those outputs. */
 
     /* Get the output closure. */
-    auto outputs = queryDerivationOutputMap();
+    auto outputs = queryDerivationOutputMap(drvPath);
     StorePathSet outputClosure;
     for (auto & i : outputs) {
         if (!wantedOutputs.contains(i.first)) continue;
@@ -371,11 +401,11 @@ Goal::Co DerivationGoal::repairClosure()
             throw Error("some paths in the output closure of derivation '%s' could not be repaired",
                 worker.store.printStorePath(drvPath));
     }
-    co_return done(BuildResult::AlreadyValid, assertPathValidity());
+    co_return done(drvPath, BuildResult::AlreadyValid, assertPathValidity(drvPath));
 }
 
 
-std::map<std::string, std::optional<StorePath>> DerivationGoal::queryPartialDerivationOutputMap()
+std::map<std::string, std::optional<StorePath>> DerivationGoal::queryPartialDerivationOutputMap(const StorePath & drvPath)
 {
     assert(!drv->type().isImpure());
 
@@ -391,7 +421,7 @@ std::map<std::string, std::optional<StorePath>> DerivationGoal::queryPartialDeri
     return res;
 }
 
-OutputPathMap DerivationGoal::queryDerivationOutputMap()
+OutputPathMap DerivationGoal::queryDerivationOutputMap(const StorePath & drvPath)
 {
     assert(!drv->type().isImpure());
 
@@ -407,7 +437,7 @@ OutputPathMap DerivationGoal::queryDerivationOutputMap()
 }
 
 
-std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
+std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity(const StorePath & drvPath)
 {
     if (drv->type().isImpure()) return { false, {} };
 
@@ -422,7 +452,7 @@ std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
     }, wantedOutputs.raw);
     SingleDrvOutputs validOutputs;
 
-    for (auto & i : queryPartialDerivationOutputMap()) {
+    for (auto & i : queryPartialDerivationOutputMap(drvPath)) {
         auto initialOutput = get(initialOutputs, i.first);
         if (!initialOutput)
             // this is an invalid output, gets catched with (!wantedOutputsLeft.empty())
@@ -487,16 +517,17 @@ std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
 }
 
 
-SingleDrvOutputs DerivationGoal::assertPathValidity()
+SingleDrvOutputs DerivationGoal::assertPathValidity(const StorePath & drvPath)
 {
-    auto [allValid, validOutputs] = checkPathValidity();
+    auto [allValid, validOutputs] = checkPathValidity(drvPath);
     if (!allValid)
         throw Error("some outputs are unexpectedly invalid");
     return validOutputs;
 }
 
 
 Goal::Done DerivationGoal::done(
+    const StorePath & drvPath,
     BuildResult::Status status,
     SingleDrvOutputs builtOutputs,
     std::optional<Error> ex)

src/libstore/build/entry-points.cc

@@ -30,7 +30,7 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod
         if (i->exitCode != Goal::ecSuccess) {
 #ifndef _WIN32 // TODO Enable building on Windows
             if (auto i2 = dynamic_cast<DerivationGoal *>(i.get()))
-                failed.insert(printStorePath(i2->drvPath));
+                failed.insert(i2->drvReq->to_string(*this));
             else
 #endif
             if (auto i2 = dynamic_cast<PathSubstitutionGoal *>(i.get()))

src/libstore/build/worker.cc

@@ -54,11 +54,11 @@ std::shared_ptr<G> Worker::initGoalIfNeeded(std::weak_ptr<G> & goal_weak, Args &
 }
 
 std::shared_ptr<DerivationGoal> Worker::makeDerivationGoalCommon(
-    const StorePath & drvPath,
+    ref<const SingleDerivedPath> drvReq,
     const OutputsSpec & wantedOutputs,
     std::function<std::shared_ptr<DerivationGoal>()> mkDrvGoal)
 {
-    std::weak_ptr<DerivationGoal> & goal_weak = derivationGoals[drvPath];
+    std::weak_ptr<DerivationGoal> & goal_weak = derivationGoals.ensureSlot(*drvReq).value;
     std::shared_ptr<DerivationGoal> goal = goal_weak.lock();
     if (!goal) {
         goal = mkDrvGoal();
@@ -71,18 +71,18 @@ std::shared_ptr<DerivationGoal> Worker::makeDerivationGoalCommon(
 }
 
 
-std::shared_ptr<DerivationGoal> Worker::makeDerivationGoal(const StorePath & drvPath,
+std::shared_ptr<DerivationGoal> Worker::makeDerivationGoal(ref<const SingleDerivedPath> drvReq,
     const OutputsSpec & wantedOutputs, BuildMode buildMode)
 {
-    return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr<DerivationGoal> {
-        return std::make_shared<DerivationGoal>(drvPath, wantedOutputs, *this, buildMode);
+    return makeDerivationGoalCommon(drvReq, wantedOutputs, [&]() -> std::shared_ptr<DerivationGoal> {
+        return std::make_shared<DerivationGoal>(drvReq, wantedOutputs, *this, buildMode);
     });
 }
 
 std::shared_ptr<DerivationGoal> Worker::makeBasicDerivationGoal(const StorePath & drvPath,
     const BasicDerivation & drv, const OutputsSpec & wantedOutputs, BuildMode buildMode)
 {
-    return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr<DerivationGoal> {
+    return makeDerivationGoalCommon(makeConstantStorePathRef(drvPath), wantedOutputs, [&]() -> std::shared_ptr<DerivationGoal> {
         return std::make_shared<DerivationGoal>(drvPath, drv, wantedOutputs, *this, buildMode);
     });
 }
@@ -118,10 +118,7 @@ GoalPtr Worker::makeGoal(const DerivedPath & req, BuildMode buildMode)
 {
     return std::visit(overloaded {
         [&](const DerivedPath::Built & bfd) -> GoalPtr {
-            if (auto bop = std::get_if<DerivedPath::Opaque>(&*bfd.drvPath))
-                return makeDerivationGoal(bop->path, bfd.outputs, buildMode);
-            else
-                throw UnimplementedError("Building dynamic derivations in one shot is not yet implemented.");
+            return makeDerivationGoal(bfd.drvPath, bfd.outputs, buildMode);
         },
         [&](const DerivedPath::Opaque & bo) -> GoalPtr {
             return makePathSubstitutionGoal(bo.path, buildMode == bmRepair ? Repair : NoRepair);
@@ -130,25 +127,45 @@ GoalPtr Worker::makeGoal(const DerivedPath & req, BuildMode buildMode)
 }
 
 
+template<typename K, typename V, typename F>
+static void cullMap(std::map<K, V> & goalMap, F f)
+{
+    for (auto i = goalMap.begin(); i != goalMap.end();)
+        if (!f(i->second))
+            i = goalMap.erase(i);
+        else ++i;
+}
+
+
 template<typename K, typename G>
 static void removeGoal(std::shared_ptr<G> goal, std::map<K, std::weak_ptr<G>> & goalMap)
 {
     /* !!! inefficient */
-    for (auto i = goalMap.begin();
-         i != goalMap.end(); )
-        if (i->second.lock() == goal) {
-            auto j = i; ++j;
-            goalMap.erase(i);
-            i = j;
-        }
-        else ++i;
+    cullMap(goalMap, [&](const std::weak_ptr<G> & gp) -> bool {
+        return gp.lock() != goal;
+    });
+}
+
+template<typename K>
+static void removeGoal(std::shared_ptr<DerivationGoal> goal, std::map<K, DerivedPathMap<std::weak_ptr<DerivationGoal>>::ChildNode> & goalMap);
+
+template<typename K>
+static void removeGoal(std::shared_ptr<DerivationGoal> goal, std::map<K, DerivedPathMap<std::weak_ptr<DerivationGoal>>::ChildNode> & goalMap)
+{
+    /* !!! inefficient */
+    cullMap(goalMap, [&](DerivedPathMap<std::weak_ptr<DerivationGoal>>::ChildNode & node) -> bool {
+        if (node.value.lock() == goal)
+            node.value.reset();
+        removeGoal(goal, node.childMap);
+        return !node.value.expired() || !node.childMap.empty();
+    });
 }
 
 
 void Worker::removeGoal(GoalPtr goal)
 {
     if (auto drvGoal = std::dynamic_pointer_cast<DerivationGoal>(goal))
-        nix::removeGoal(drvGoal, derivationGoals);
+        nix::removeGoal(drvGoal, derivationGoals.map);
     else if (auto drvBuildingGoal = std::dynamic_pointer_cast<DerivationBuildingGoal>(goal))
         nix::removeGoal(drvBuildingGoal, derivationBuildingGoals);
     else if (auto subGoal = std::dynamic_pointer_cast<PathSubstitutionGoal>(goal))
@@ -297,7 +314,7 @@ void Worker::run(const Goals & _topGoals)
         topGoals.insert(i);
         if (auto goal = dynamic_cast<DerivationGoal *>(i.get())) {
             topPaths.push_back(DerivedPath::Built {
-                .drvPath = makeConstantStorePathRef(goal->drvPath),
+                .drvPath = goal->drvReq,
                 .outputs = goal->wantedOutputs,
             });
         } else