From c97b050a6c212d0b748303080b5604309b7abdce Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 26 Sep 2025 01:40:00 -0400 Subject: [PATCH] Fix `ca/eval-store.sh` test The refactor in the last commit fixed the bug it was supposed to fix, but introduced a new bug in that sometimes we tried to write a resolved derivation to a store before all its `inputSrcs` were in that store. The solution is to defer writing the derivation until inside `DerivationBuildingGoal`, just before we do an actual build. At this point, we are sure that all inputs in are the store. This does have the side effect of meaning we don't write down the resolved derivation in the substituting case, only the building case, but I think that is actually fine. The store that actually does the building should make a record of what it built by storing the resolved derivation. Other stores that just substitute from that store don't necessary want that derivation however. They can trust the substituter to keep the record around, or baring that, they can attempt to re resolve everything, if they need to be audited. --- src/libstore/build/derivation-building-goal.cc | 13 ++++++++++--- src/libstore/build/derivation-goal.cc | 16 +++++++--------- src/libstore/build/worker.cc | 15 ++++++++++----- .../nix/store/build/derivation-building-goal.hh | 17 +++++++++++++++-- .../include/nix/store/build/derivation-goal.hh | 8 ++++++-- src/libstore/include/nix/store/build/worker.hh | 10 +++++++--- 6 files changed, 55 insertions(+), 24 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 98b80862d..fa819c96b 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -26,8 +26,8 @@ namespace nix { DerivationBuildingGoal::DerivationBuildingGoal( - const StorePath & drvPath, const Derivation & drv_, Worker & worker, BuildMode buildMode) - : Goal(worker, gaveUpOnSubstitution()) + const StorePath & drvPath, const Derivation & drv_, Worker & worker, BuildMode buildMode, bool storeDerivation) + : Goal(worker, gaveUpOnSubstitution(storeDerivation)) , drvPath(drvPath) , buildMode(buildMode) { @@ -124,7 +124,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; @@ -172,6 +172,13 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() /* Determine the full set of input paths. */ + if (storeDerivation) { + assert(drv->inputDrvs.map.empty()); + /* Store the resolved derivation, as part of the record of + what we're actually building */ + writeDerivation(worker.store, *drv); + } + { /* If we get this far, we know no dynamic drvs inputs */ diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 8e924fd4a..cc3ba2b7b 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -30,8 +30,9 @@ DerivationGoal::DerivationGoal( const Derivation & drv, const OutputName & wantedOutput, Worker & worker, - BuildMode buildMode) - : Goal(worker, haveDerivation()) + BuildMode buildMode, + bool storeDerivation) + : Goal(worker, haveDerivation(storeDerivation)) , drvPath(drvPath) , wantedOutput(wantedOutput) , outputHash{[&] { @@ -65,7 +66,7 @@ std::string DerivationGoal::key() }.to_string(worker.store); } -Goal::Co DerivationGoal::haveDerivation() +Goal::Co DerivationGoal::haveDerivation(bool storeDerivation) { trace("have derivation"); @@ -159,11 +160,8 @@ Goal::Co DerivationGoal::haveDerivation() 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); - - auto resolvedDrvGoal = worker.makeDerivationGoal(pathResolved, drvResolved, wantedOutput, buildMode); + auto resolvedDrvGoal = + worker.makeDerivationGoal(pathResolved, drvResolved, wantedOutput, buildMode, /*storeDerivation=*/true); { Goals waitees{resolvedDrvGoal}; co_await await(std::move(waitees)); @@ -239,7 +237,7 @@ Goal::Co DerivationGoal::haveDerivation() /* 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; diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index f597abb63..53175a8c4 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -76,9 +76,14 @@ std::shared_ptr Worker::makeDerivationTrampolineGoal( } std::shared_ptr 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 @@ -87,10 +92,10 @@ Worker::makeDerivationResolutionGoal(const StorePath & drvPath, const Derivation return initGoalIfNeeded(derivationResolutionGoals[drvPath], drvPath, drv, *this, buildMode); } -std::shared_ptr -Worker::makeDerivationBuildingGoal(const StorePath & drvPath, const Derivation & drv, BuildMode buildMode) +std::shared_ptr Worker::makeDerivationBuildingGoal( + 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 diff --git a/src/libstore/include/nix/store/build/derivation-building-goal.hh b/src/libstore/include/nix/store/build/derivation-building-goal.hh index 8192dc778..ab063ff3f 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -29,8 +29,21 @@ 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 = bmNormal, + bool storeDerivation = false); ~DerivationBuildingGoal(); private: @@ -100,7 +113,7 @@ private: /** * The states. */ - Co gaveUpOnSubstitution(); + Co gaveUpOnSubstitution(bool storeDerivation); Co tryToBuild(); /** diff --git a/src/libstore/include/nix/store/build/derivation-goal.hh b/src/libstore/include/nix/store/build/derivation-goal.hh index e05bf1c0b..353e7c489 100644 --- a/src/libstore/include/nix/store/build/derivation-goal.hh +++ b/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 = bmNormal, + bool storeDerivation = false); ~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 diff --git a/src/libstore/include/nix/store/build/worker.hh b/src/libstore/include/nix/store/build/worker.hh index 9660d66b2..9767590ac 100644 --- a/src/libstore/include/nix/store/build/worker.hh +++ b/src/libstore/include/nix/store/build/worker.hh @@ -223,7 +223,8 @@ public: const StorePath & drvPath, const Derivation & drv, const OutputName & wantedOutput, - BuildMode buildMode = bmNormal); + BuildMode buildMode = bmNormal, + bool storeDerivation = false); /** * @ref DerivationResolutionGoal "derivation resolution goal" @@ -234,8 +235,11 @@ public: /** * @ref DerivationBuildingGoal "derivation building goal" */ - std::shared_ptr - makeDerivationBuildingGoal(const StorePath & drvPath, const Derivation & drv, BuildMode buildMode = bmNormal); + std::shared_ptr makeDerivationBuildingGoal( + const StorePath & drvPath, + const Derivation & drv, + BuildMode buildMode = bmNormal, + bool storeDerivation = false); /** * @ref PathSubstitutionGoal "substitution goal"