Merge pull request #14225 from obsidiansystems/derivation-resolution-goal-2

Reapply the rest of #14022

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

@@ -1,7 +1,5 @@
 #include "nix/store/build/derivation-building-goal.hh"
-#include "nix/store/build/derivation-resolution-goal.hh"
 #include "nix/store/build/derivation-env-desugar.hh"
-#include "nix/store/build/derivation-trampoline-goal.hh"
 #ifndef _WIN32 // TODO enable build hook on Windows
 #  include "nix/store/build/hook-instance.hh"
 #  include "nix/store/build/derivation-builder.hh"
@@ -28,8 +26,8 @@
 namespace nix {
 
 DerivationBuildingGoal::DerivationBuildingGoal(
-    const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode)
+    const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode, bool storeDerivation)
-    : Goal(worker, gaveUpOnSubstitution())
+    : Goal(worker, gaveUpOnSubstitution(storeDerivation))
     , drvPath(drvPath)
     , drv{std::make_unique<Derivation>(drv)}
     , buildMode(buildMode)
@@ -109,7 +107,7 @@ static void runPostBuildHook(
 
 /* At least one of the output paths could not be
    produced using a substitute.  So we have to build instead. */
-Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
+Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution(bool storeDerivation)
 {
     Goals waitees;
 
@@ -157,108 +155,14 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
 
     /* Determine the full set of input paths. */
 
-    {
+    if (storeDerivation) {
-        auto resolutionGoal = worker.makeDerivationResolutionGoal(drvPath, *drv, buildMode);
+        assert(drv->inputDrvs.map.empty());
-        {
-            Goals waitees{resolutionGoal};
-            co_await await(std::move(waitees));
-        }
-        if (nrFailed != 0) {
-            co_return doneFailure({BuildResult::Failure::DependencyFailed, "resolution failed"});
-        }
-
-        if (resolutionGoal->resolvedDrv) {
-            auto & [pathResolved, drvResolved] = *resolutionGoal->resolvedDrv;
-
         /* Store the resolved derivation, as part of the record of
            what we're actually building */
-            writeDerivation(worker.store, drvResolved);
+        writeDerivation(worker.store, *drv);
+    }
 
-            /* TODO https://github.com/NixOS/nix/issues/13247 we should
-               let the calling goal do this, so it has a change to pass
-               just the output(s) it cares about. */
-            auto resolvedDrvGoal =
-                worker.makeDerivationTrampolineGoal(pathResolved, OutputsSpec::All{}, drvResolved, buildMode);
     {
-                Goals waitees{resolvedDrvGoal};
-                co_await await(std::move(waitees));
-            }
-
-            trace("resolved derivation finished");
-
-            auto resolvedResult = resolvedDrvGoal->buildResult;
-
-            // No `std::visit` for coroutines yet
-            if (auto * successP = resolvedResult.tryGetSuccess()) {
-                auto & success = *successP;
-                SingleDrvOutputs builtOutputs;
-
-                auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
-                auto resolvedHashes = staticOutputHashes(worker.store, drvResolved);
-
-                StorePathSet outputPaths;
-
-                for (auto & outputName : drvResolved.outputNames()) {
-                    auto outputHash = get(outputHashes, outputName);
-                    auto resolvedHash = get(resolvedHashes, outputName);
-                    if ((!outputHash) || (!resolvedHash))
-                        throw Error(
-                            "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/resolve)",
-                            worker.store.printStorePath(drvPath),
-                            outputName);
-
-                    auto realisation = [&] {
-                        auto take1 = get(success.builtOutputs, outputName);
-                        if (take1)
-                            return *take1;
-
-                        /* The above `get` should work. But stateful tracking of
-                           outputs in resolvedResult, this can get out of sync with the
-                           store, which is our actual source of truth. For now we just
-                           check the store directly if it fails. */
-                        auto take2 = worker.evalStore.queryRealisation(DrvOutput{*resolvedHash, outputName});
-                        if (take2)
-                            return *take2;
-
-                        throw Error(
-                            "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/realisation)",
-                            worker.store.printStorePath(pathResolved),
-                            outputName);
-                    }();
-
-                    if (!drv->type().isImpure()) {
-                        auto newRealisation = realisation;
-                        newRealisation.id = DrvOutput{*outputHash, outputName};
-                        newRealisation.signatures.clear();
-                        if (!drv->type().isFixed()) {
-                            auto & drvStore = worker.evalStore.isValidPath(drvPath) ? worker.evalStore : worker.store;
-                            newRealisation.dependentRealisations =
-                                drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore);
-                        }
-                        worker.store.signRealisation(newRealisation);
-                        worker.store.registerDrvOutput(newRealisation);
-                    }
-                    outputPaths.insert(realisation.outPath);
-                    builtOutputs.emplace(outputName, realisation);
-                }
-
-                runPostBuildHook(worker.store, *logger, drvPath, outputPaths);
-
-                auto status = success.status;
-                if (status == BuildResult::Success::AlreadyValid)
-                    status = BuildResult::Success::ResolvesToAlreadyValid;
-
-                co_return doneSuccess(success.status, std::move(builtOutputs));
-            } else if (resolvedResult.tryGetFailure()) {
-                co_return doneFailure({
-                    BuildResult::Failure::DependencyFailed,
-                    "build of resolved derivation '%s' failed",
-                    worker.store.printStorePath(pathResolved),
-                });
-            } else
-                assert(false);
-        }
-
         /* If we get this far, we know no dynamic drvs inputs */
 
         for (auto & [depDrvPath, depNode] : drv->inputDrvs.map) {

src/libstore/build/derivation-goal.cc

@@ -1,5 +1,6 @@
 #include "nix/store/build/derivation-goal.hh"
 #include "nix/store/build/derivation-building-goal.hh"
+#include "nix/store/build/derivation-resolution-goal.hh"
 #ifndef _WIN32 // TODO enable build hook on Windows
 #  include "nix/store/build/hook-instance.hh"
 #  include "nix/store/build/derivation-builder.hh"
@@ -29,8 +30,9 @@ DerivationGoal::DerivationGoal(
     const Derivation & drv,
     const OutputName & wantedOutput,
     Worker & worker,
-    BuildMode buildMode)
+    BuildMode buildMode,
-    : Goal(worker, haveDerivation())
+    bool storeDerivation)
+    : Goal(worker, haveDerivation(storeDerivation))
     , drvPath(drvPath)
     , wantedOutput(wantedOutput)
     , drv{std::make_unique<Derivation>(drv)}
@@ -58,7 +60,7 @@ std::string DerivationGoal::key()
     }.to_string(worker.store);
 }
 
-Goal::Co DerivationGoal::haveDerivation()
+Goal::Co DerivationGoal::haveDerivation(bool storeDerivation)
 {
     trace("have derivation");
 
@@ -140,9 +142,96 @@ Goal::Co DerivationGoal::haveDerivation()
                 worker.store.printStorePath(drvPath));
     }
 
+    auto resolutionGoal = worker.makeDerivationResolutionGoal(drvPath, *drv, buildMode);
+    {
+        Goals waitees{resolutionGoal};
+        co_await await(std::move(waitees));
+    }
+    if (nrFailed != 0) {
+        co_return doneFailure({BuildResult::Failure::DependencyFailed, "resolution failed"});
+    }
+
+    if (resolutionGoal->resolvedDrv) {
+        auto & [pathResolved, drvResolved] = *resolutionGoal->resolvedDrv;
+
+        auto resolvedDrvGoal =
+            worker.makeDerivationGoal(pathResolved, drvResolved, wantedOutput, buildMode, /*storeDerivation=*/true);
+        {
+            Goals waitees{resolvedDrvGoal};
+            co_await await(std::move(waitees));
+        }
+
+        trace("resolved derivation finished");
+
+        auto resolvedResult = resolvedDrvGoal->buildResult;
+
+        // No `std::visit` for coroutines yet
+        if (auto * successP = resolvedResult.tryGetSuccess()) {
+            auto & success = *successP;
+            auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
+            auto resolvedHashes = staticOutputHashes(worker.store, drvResolved);
+
+            StorePathSet outputPaths;
+
+            auto outputHash = get(outputHashes, wantedOutput);
+            auto resolvedHash = get(resolvedHashes, wantedOutput);
+            if ((!outputHash) || (!resolvedHash))
+                throw Error(
+                    "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/resolve)",
+                    worker.store.printStorePath(drvPath),
+                    wantedOutput);
+
+            auto realisation = [&] {
+                auto take1 = get(success.builtOutputs, wantedOutput);
+                if (take1)
+                    return *take1;
+
+                /* The above `get` should work. But stateful tracking of
+                   outputs in resolvedResult, this can get out of sync with the
+                   store, which is our actual source of truth. For now we just
+                   check the store directly if it fails. */
+                auto take2 = worker.evalStore.queryRealisation(DrvOutput{*resolvedHash, wantedOutput});
+                if (take2)
+                    return *take2;
+
+                throw Error(
+                    "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/realisation)",
+                    worker.store.printStorePath(pathResolved),
+                    wantedOutput);
+            }();
+
+            if (!drv->type().isImpure()) {
+                auto newRealisation = realisation;
+                newRealisation.id = DrvOutput{*outputHash, wantedOutput};
+                newRealisation.signatures.clear();
+                if (!drv->type().isFixed()) {
+                    auto & drvStore = worker.evalStore.isValidPath(drvPath) ? worker.evalStore : worker.store;
+                    newRealisation.dependentRealisations =
+                        drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore);
+                }
+                worker.store.signRealisation(newRealisation);
+                worker.store.registerDrvOutput(newRealisation);
+            }
+            outputPaths.insert(realisation.outPath);
+
+            auto status = success.status;
+            if (status == BuildResult::Success::AlreadyValid)
+                status = BuildResult::Success::ResolvesToAlreadyValid;
+
+            co_return doneSuccess(status, std::move(realisation));
+        } else if (resolvedResult.tryGetFailure()) {
+            co_return doneFailure({
+                BuildResult::Failure::DependencyFailed,
+                "build of resolved derivation '%s' failed",
+                worker.store.printStorePath(pathResolved),
+            });
+        } else
+            assert(false);
+    }
+
     /* Give up on substitution for the output we want, actually build this derivation */
 
-    auto g = worker.makeDerivationBuildingGoal(drvPath, *drv, buildMode);
+    auto g = worker.makeDerivationBuildingGoal(drvPath, *drv, buildMode, storeDerivation);
 
     /* We will finish with it ourselves, as if we were the derivational goal. */
     g->preserveException = true;

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

@@ -145,7 +145,7 @@ Goal::Co DerivationTrampolineGoal::haveDerivation(StorePath drvPath, Derivation
     /* Build this step! */
 
     for (auto & output : resolvedWantedOutputs) {
-        auto g = upcast_goal(worker.makeDerivationGoal(drvPath, drv, output, buildMode));
+        auto g = upcast_goal(worker.makeDerivationGoal(drvPath, drv, output, buildMode, false));
         g->preserveException = true;
         /* We will finish with it ourselves, as if we were the derivational goal. */
         concreteDrvGoals.insert(std::move(g));

src/libstore/build/worker.cc

@@ -76,9 +76,14 @@ std::shared_ptr<DerivationTrampolineGoal> Worker::makeDerivationTrampolineGoal(
 }
 
 std::shared_ptr<DerivationGoal> Worker::makeDerivationGoal(
-    const StorePath & drvPath, const Derivation & drv, const OutputName & wantedOutput, BuildMode buildMode)
+    const StorePath & drvPath,
+    const Derivation & drv,
+    const OutputName & wantedOutput,
+    BuildMode buildMode,
+    bool storeDerivation)
 {
-    return initGoalIfNeeded(derivationGoals[drvPath][wantedOutput], drvPath, drv, wantedOutput, *this, buildMode);
+    return initGoalIfNeeded(
+        derivationGoals[drvPath][wantedOutput], drvPath, drv, wantedOutput, *this, buildMode, storeDerivation);
 }
 
 std::shared_ptr<DerivationResolutionGoal>
@@ -87,10 +92,10 @@ Worker::makeDerivationResolutionGoal(const StorePath & drvPath, const Derivation
     return initGoalIfNeeded(derivationResolutionGoals[drvPath], drvPath, drv, *this, buildMode);
 }
 
-std::shared_ptr<DerivationBuildingGoal>
+std::shared_ptr<DerivationBuildingGoal> Worker::makeDerivationBuildingGoal(
-Worker::makeDerivationBuildingGoal(const StorePath & drvPath, const Derivation & drv, BuildMode buildMode)
+    const StorePath & drvPath, const Derivation & drv, BuildMode buildMode, bool storeDerivation)
 {
-    return initGoalIfNeeded(derivationBuildingGoals[drvPath], drvPath, drv, *this, buildMode);
+    return initGoalIfNeeded(derivationBuildingGoals[drvPath], drvPath, drv, *this, buildMode, storeDerivation);
 }
 
 std::shared_ptr<PathSubstitutionGoal>

src/libstore/include/nix/store/build/derivation-building-goal.hh

@@ -29,8 +29,17 @@ typedef enum { rpAccept, rpDecline, rpPostpone } HookReply;
  */
 struct DerivationBuildingGoal : public Goal
 {
+    /**
+     * @param storeDerivation Whether to store the derivation in
+     * `worker.store`. This is useful for newly-resolved derivations. In this
+     * case, the derivation was not created a priori, e.g. purely (or close
+     * enough) from evaluation of the Nix language, but also depends on the
+     * exact content produced by upstream builds. It is strongly advised to
+     * have a permanent record of such a resolved derivation in order to
+     * faithfully reconstruct the build history.
+     */
     DerivationBuildingGoal(
-        const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode = bmNormal);
+        const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode, bool storeDerivation);
     ~DerivationBuildingGoal();
 
 private:
@@ -100,7 +109,7 @@ private:
     /**
      * The states.
      */
-    Co gaveUpOnSubstitution();
+    Co gaveUpOnSubstitution(bool storeDerivation);
     Co tryToBuild();
 
     /**

src/libstore/include/nix/store/build/derivation-goal.hh

@@ -40,12 +40,16 @@ struct DerivationGoal : public Goal
      */
     OutputName wantedOutput;
 
+    /**
+     * @param storeDerivation See `DerivationBuildingGoal`. This is just passed along.
+     */
     DerivationGoal(
         const StorePath & drvPath,
         const Derivation & drv,
         const OutputName & wantedOutput,
         Worker & worker,
-        BuildMode buildMode = bmNormal);
+        BuildMode buildMode,
+        bool storeDerivation);
     ~DerivationGoal() = default;
 
     void timedOut(Error && ex) override
@@ -80,7 +84,7 @@ private:
     /**
      * The states.
      */
-    Co haveDerivation();
+    Co haveDerivation(bool storeDerivation);
 
     /**
      * Return `std::nullopt` if the output is unknown, e.g. un unbuilt

src/libstore/include/nix/store/build/derivation-resolution-goal.hh

@@ -35,8 +35,7 @@ struct BuilderFailureError;
  */
 struct DerivationResolutionGoal : public Goal
 {
-    DerivationResolutionGoal(
+    DerivationResolutionGoal(const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode);
-        const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode = bmNormal);
 
     /**
      * If the derivation needed to be resolved, this is resulting

src/libstore/include/nix/store/build/worker.hh

@@ -210,32 +210,30 @@ private:
     std::shared_ptr<G> initGoalIfNeeded(std::weak_ptr<G> & goal_weak, Args &&... args);
 
     std::shared_ptr<DerivationTrampolineGoal> makeDerivationTrampolineGoal(
-        ref<const SingleDerivedPath> drvReq, const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal);
+        ref<const SingleDerivedPath> drvReq, const OutputsSpec & wantedOutputs, BuildMode buildMode);
 
 public:
     std::shared_ptr<DerivationTrampolineGoal> makeDerivationTrampolineGoal(
-        const StorePath & drvPath,
+        const StorePath & drvPath, const OutputsSpec & wantedOutputs, const Derivation & drv, BuildMode buildMode);
-        const OutputsSpec & wantedOutputs,
-        const Derivation & drv,
-        BuildMode buildMode = bmNormal);
 
     std::shared_ptr<DerivationGoal> makeDerivationGoal(
         const StorePath & drvPath,
         const Derivation & drv,
         const OutputName & wantedOutput,
-        BuildMode buildMode = bmNormal);
+        BuildMode buildMode,
+        bool storeDerivation);
 
     /**
      * @ref DerivationResolutionGoal "derivation resolution goal"
      */
     std::shared_ptr<DerivationResolutionGoal>
-    makeDerivationResolutionGoal(const StorePath & drvPath, const Derivation & drv, BuildMode buildMode = bmNormal);
+    makeDerivationResolutionGoal(const StorePath & drvPath, const Derivation & drv, BuildMode buildMode);
 
     /**
      * @ref DerivationBuildingGoal "derivation building goal"
      */
-    std::shared_ptr<DerivationBuildingGoal>
+    std::shared_ptr<DerivationBuildingGoal> makeDerivationBuildingGoal(
-    makeDerivationBuildingGoal(const StorePath & drvPath, const Derivation & drv, BuildMode buildMode = bmNormal);
+        const StorePath & drvPath, const Derivation & drv, BuildMode buildMode, bool storeDerivation);
 
     /**
      * @ref PathSubstitutionGoal "substitution goal"

tests/functional/ca/issue-13247.sh

@@ -65,7 +65,4 @@ buildViaSubstitute use-a-prime-more-outputs^first
 # Should only fetch the output we asked for
 [[ -d "$(jq -r <"$TEST_ROOT"/a.json '.[0].outputs.out')" ]]
 [[ -f "$(jq -r <"$TEST_ROOT"/a.json '.[2].outputs.first')" ]]
-
+[[ ! -e "$(jq -r <"$TEST_ROOT"/a.json '.[2].outputs.second')" ]]
-# Output should *not* be here, this is the bug
-[[ -e "$(jq -r <"$TEST_ROOT"/a.json '.[2].outputs.second')" ]]
-skipTest "bug is not yet fixed"