Merge pull request #12567 from obsidiansystems/slightly-rework-drv-resolution

Rework derivation input resolution

src/libstore/build/derivation-goal.cc

@@ -322,6 +322,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()
@@ -330,19 +342,22 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
        is no need to restart. */
     needRestart = NeedRestartForMoreOutputs::BuildInProgressWillNotNeed;
 
-    /* The inputs must be built before we can build this goal. */
+    std::map<ref<const SingleDerivedPath>, GoalPtr, value_comparison> inputGoals;
-    inputDrvOutputs.clear();
-    if (useDerivation) {
-        std::function<void(ref<SingleDerivedPath>, const DerivedPathMap<StringSet>::ChildNode &)> addWaiteeDerivedPath;
 
-        addWaiteeDerivedPath = [&](ref<SingleDerivedPath> inputDrv, const DerivedPathMap<StringSet>::ChildNode & inputNode) {
+    if (useDerivation) {
-            if (!inputNode.value.empty())
+        std::function<void(ref<const SingleDerivedPath>, const DerivedPathMap<StringSet>::ChildNode &)> addWaiteeDerivedPath;
-                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 }),
@@ -430,7 +445,12 @@ 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);
@@ -438,7 +458,19 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
             /* 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
@@ -469,54 +501,31 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution()
             co_return resolvedFinished();
         }
 
-        std::function<void(const StorePath &, const DerivedPathMap<StringSet>::ChildNode &)> accumInputPaths;
+        /* If we get this far, we know no dynamic drvs inputs */
 
-        accumInputPaths = [&](const StorePath & depDrvPath, const DerivedPathMap<StringSet>::ChildNode & inputNode) {
+        for (auto & [depDrvPath, depNode] : fullDrv.inputDrvs.map) {
-            /* Add the relevant output closures of the input derivation
+            for (auto & outputName : depNode.value) {
-               `i' as input paths.  Only add the closures of output paths
+                /* Don't need to worry about `inputGoals`, because
-               that are specified as inputs. */
+                   impure derivations are always resolved above. Can
-            auto getOutput = [&](const std::string & outputName) {
+                   just use DB. This case only happens in the (older)
-                /* TODO (impure derivations-induced tech debt):
+                   input addressed and fixed output derivation cases. */
-                   Tracking input derivation outputs statefully through the
+                auto outMap = [&]{
-                   goals is error prone and has led to bugs.
+                    for (auto * drvStore : { &worker.evalStore, &worker.store })
-                   For a robust nix, we need to move towards the `else` branch,
+                        if (drvStore->isValidPath(depDrvPath))
-                   which does not rely on goal state to match up with the
+                            return worker.store.queryDerivationOutputMap(depDrvPath, drvStore);
-                   reality of the store, which is our real source of truth.
+                    assert(false);
-                   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
+                auto outMapPath = outMap.find(outputName);
-                   in itself. When implementing this logic entirely with lookups
+                if (outMapPath == outMap.end()) {
-                   make sure that they're cached. */
+                    throw Error(
-                if (auto outPath = get(inputDrvOutputs, { depDrvPath, outputName })) {
+                        "derivation '%s' requires non-existent output '%s' from input derivation '%s'",
-                    return *outPath;
+                        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);
+                worker.store.computeFSClosure(outMapPath->second, inputPaths);
-                    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);
     }
 
     /* Second, the input sources. */
@@ -1545,34 +1554,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());
-
-    auto * dg = dynamic_cast<DerivationGoal *>(&*waitee);
-    if (!dg) return;
-
-    auto * nodeP = fullDrv.inputDrvs.findSlot(DerivedPath::Opaque { .path = dg->drvPath });
-    if (!nodeP) return;
-    auto & outputs = nodeP->value;
-
-    for (auto & outputName : outputs) {
-        auto buildResult = dg->getBuildResult(DerivedPath::Built {
-            .drvPath = makeConstantStorePathRef(dg->drvPath),
-            .outputs = OutputsSpec::Names { outputName },
-        });
-        if (buildResult.success()) {
-            auto i = buildResult.builtOutputs.find(outputName);
-            if (i != buildResult.builtOutputs.end())
-                inputDrvOutputs.insert_or_assign(
-                    { dg->drvPath, outputName },
-                    i->second.outPath);
-        }
-    }
-}
-
 }

src/libstore/build/derivation-goal.hh

@@ -78,13 +78,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.
      */
@@ -331,8 +324,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

@@ -396,7 +396,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

@@ -552,4 +552,9 @@ GoalPtr upcast_goal(std::shared_ptr<DrvOutputSubstitutionGoal> subGoal)
     return subGoal;
 }
 
+GoalPtr upcast_goal(std::shared_ptr<DerivationGoal> subGoal)
+{
+    return subGoal;
+}
+
 }

src/libstore/derivations.cc

@@ -1052,49 +1052,36 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String
 
 std::optional<BasicDerivation> Derivation::tryResolve(Store & store, Store * evalStore) const
 {
-    std::map<std::pair<StorePath, std::string>, StorePath> inputDrvOutputs;
+    return tryResolve(
-
+        store,
-    std::function<void(const StorePath &, const DerivedPathMap<StringSet>::ChildNode &)> accum;
+        [&](ref<const SingleDerivedPath> drvPath, const std::string & outputName) -> std::optional<StorePath> {
-    accum = [&](auto & inputDrv, auto & node) {
+            try {
-        for (auto & [outputName, outputPath] : store.queryPartialDerivationOutputMap(inputDrv, evalStore)) {
+                return resolveDerivedPath(store, SingleDerivedPath::Built{drvPath, outputName}, evalStore);
-            if (outputPath) {
+            } catch (Error &) {
-                inputDrvOutputs.insert_or_assign({inputDrv, outputName}, *outputPath);
+                return std::nullopt;
-                if (auto p = get(node.childMap, outputName))
-                    accum(*outputPath, *p);
             }
-        }
+        });
-    };
-
-    for (auto & [inputDrv, node] : inputDrvs.map)
-        accum(inputDrv, node);
-
-    return tryResolve(store, inputDrvOutputs);
 }
 
 static bool tryResolveInput(
     Store & store, StorePathSet & inputSrcs, StringMap & inputRewrites,
     const DownstreamPlaceholder * placeholderOpt,
-    const StorePath & inputDrv, const DerivedPathMap<StringSet>::ChildNode & inputNode,
+    ref<const SingleDerivedPath> drvPath, const DerivedPathMap<StringSet>::ChildNode & inputNode,
-    const std::map<std::pair<StorePath, std::string>, StorePath> & inputDrvOutputs)
+    std::function<std::optional<StorePath>(ref<const SingleDerivedPath> drvPath, const std::string & outputName)> queryResolutionChain)
 {
-    auto getOutput = [&](const std::string & outputName) {
-        auto * actualPathOpt = get(inputDrvOutputs, { inputDrv, outputName });
-        if (!actualPathOpt)
-            warn("output %s of input %s missing, aborting the resolving",
-                outputName,
-                store.printStorePath(inputDrv)
-            );
-        return actualPathOpt;
-    };
-
     auto getPlaceholder = [&](const std::string & outputName) {
         return placeholderOpt
             ? DownstreamPlaceholder::unknownDerivation(*placeholderOpt, outputName)
-            : DownstreamPlaceholder::unknownCaOutput(inputDrv, outputName);
+            : [&]{
+                auto * p = std::get_if<SingleDerivedPath::Opaque>(&drvPath->raw());
+                // otherwise we should have had a placeholder to build-upon already
+                assert(p);
+                return DownstreamPlaceholder::unknownCaOutput(p->path, outputName);
+            }();
     };
 
     for (auto & outputName : inputNode.value) {
-        auto actualPathOpt = getOutput(outputName);
+        auto actualPathOpt = queryResolutionChain(drvPath, outputName);
         if (!actualPathOpt) return false;
         auto actualPath = *actualPathOpt;
         if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
@@ -1106,13 +1093,12 @@ static bool tryResolveInput(
     }
 
     for (auto & [outputName, childNode] : inputNode.childMap) {
-        auto actualPathOpt = getOutput(outputName);
-        if (!actualPathOpt) return false;
-        auto actualPath = *actualPathOpt;
         auto nextPlaceholder = getPlaceholder(outputName);
         if (!tryResolveInput(store, inputSrcs, inputRewrites,
-            &nextPlaceholder, actualPath, childNode,
+            &nextPlaceholder,
-            inputDrvOutputs))
+            make_ref<const SingleDerivedPath>(SingleDerivedPath::Built{drvPath, outputName}),
+            childNode,
+            queryResolutionChain))
             return false;
     }
     return true;
@@ -1120,7 +1106,7 @@ static bool tryResolveInput(
 
 std::optional<BasicDerivation> Derivation::tryResolve(
     Store & store,
-    const std::map<std::pair<StorePath, std::string>, StorePath> & inputDrvOutputs) const
+    std::function<std::optional<StorePath>(ref<const SingleDerivedPath> drvPath, const std::string & outputName)> queryResolutionChain) const
 {
     BasicDerivation resolved { *this };
 
@@ -1129,7 +1115,7 @@ std::optional<BasicDerivation> Derivation::tryResolve(
 
     for (auto & [inputDrv, inputNode] : inputDrvs.map)
         if (!tryResolveInput(store, resolved.inputSrcs, inputRewrites,
-            nullptr, inputDrv, inputNode, inputDrvOutputs))
+            nullptr, make_ref<const SingleDerivedPath>(SingleDerivedPath::Opaque{inputDrv}), inputNode, queryResolutionChain))
             return std::nullopt;
 
     rewriteDerivation(store, resolved, inputRewrites);

src/libstore/derivations.hh

@@ -369,7 +369,7 @@ struct Derivation : BasicDerivation
      */
     std::optional<BasicDerivation> tryResolve(
         Store & store,
-        const std::map<std::pair<StorePath, std::string>, StorePath> & inputDrvOutputs) const;
+        std::function<std::optional<StorePath>(ref<const SingleDerivedPath> drvPath, const std::string & outputName)> queryResolutionChain) const;
 
     /**
      * Check that the derivation is valid and does not present any

src/libstore/derived-path.cc

@@ -170,7 +170,7 @@ void drvRequireExperiment(
 }
 
 SingleDerivedPath::Built SingleDerivedPath::Built::parse(
-    const StoreDirConfig & store, ref<SingleDerivedPath> drv,
+    const StoreDirConfig & store, ref<const SingleDerivedPath> drv,
     OutputNameView output,
     const ExperimentalFeatureSettings & xpSettings)
 {
@@ -182,7 +182,7 @@ SingleDerivedPath::Built SingleDerivedPath::Built::parse(
 }
 
 DerivedPath::Built DerivedPath::Built::parse(
-    const StoreDirConfig & store, ref<SingleDerivedPath> drv,
+    const StoreDirConfig & store, ref<const SingleDerivedPath> drv,
     OutputNameView outputsS,
     const ExperimentalFeatureSettings & xpSettings)
 {
@@ -201,7 +201,7 @@ static SingleDerivedPath parseWithSingle(
     return n == s.npos
         ? (SingleDerivedPath) SingleDerivedPath::Opaque::parse(store, s)
         : (SingleDerivedPath) SingleDerivedPath::Built::parse(store,
-            make_ref<SingleDerivedPath>(parseWithSingle(
+            make_ref<const SingleDerivedPath>(parseWithSingle(
                 store,
                 s.substr(0, n),
                 separator,
@@ -234,7 +234,7 @@ static DerivedPath parseWith(
     return n == s.npos
         ? (DerivedPath) DerivedPath::Opaque::parse(store, s)
         : (DerivedPath) DerivedPath::Built::parse(store,
-            make_ref<SingleDerivedPath>(parseWithSingle(
+            make_ref<const SingleDerivedPath>(parseWithSingle(
                 store,
                 s.substr(0, n),
                 separator,

src/libstore/derived-path.hh

@@ -45,7 +45,7 @@ struct SingleDerivedPath;
  * path of the given output name.
  */
 struct SingleDerivedPathBuilt {
-    ref<SingleDerivedPath> drvPath;
+    ref<const SingleDerivedPath> drvPath;
     OutputName output;
 
     /**
@@ -74,7 +74,7 @@ struct SingleDerivedPathBuilt {
      * @param xpSettings Stop-gap to avoid globals during unit tests.
      */
     static SingleDerivedPathBuilt parse(
-        const StoreDirConfig & store, ref<SingleDerivedPath> drvPath,
+        const StoreDirConfig & store, ref<const SingleDerivedPath> drvPath,
         OutputNameView outputs,
         const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings);
     nlohmann::json toJSON(Store & store) const;
@@ -172,7 +172,7 @@ static inline ref<SingleDerivedPath> makeConstantStorePathRef(StorePath drvPath)
  * output name.
  */
 struct DerivedPathBuilt {
-    ref<SingleDerivedPath> drvPath;
+    ref<const SingleDerivedPath> drvPath;
     OutputsSpec outputs;
 
     /**
@@ -201,7 +201,7 @@ struct DerivedPathBuilt {
      * @param xpSettings Stop-gap to avoid globals during unit tests.
      */
     static DerivedPathBuilt parse(
-        const StoreDirConfig & store, ref<SingleDerivedPath>,
+        const StoreDirConfig & store, ref<const SingleDerivedPath>,
         std::string_view,
         const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings);
     nlohmann::json toJSON(Store & store) const;

src/nix/log.cc

@@ -35,7 +35,7 @@ struct CmdLog : InstallableCommand
         // For compat with CLI today, TODO revisit
         auto oneUp = std::visit(overloaded {
             [&](const DerivedPath::Opaque & bo) {
-                return make_ref<SingleDerivedPath>(bo);
+                return make_ref<const SingleDerivedPath>(bo);
             },
             [&](const DerivedPath::Built & bfd) {
                 return bfd.drvPath;