From 766a52ce87458644591199aeb9c2888447095d20 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 13 Aug 2025 23:46:16 -0400 Subject: [PATCH] `DerivationOutput`: Remove `outputKnown` state Now that `DerivationGoal::checkPathValidity` is legible, we can see that it only sets `outputKnown`, and doesn't read it. Likewise, with co-routines, we don't have tiny scopes that make local variables difficult. Between these two things, we can simply have `checkPathValidity` return what it finds, rather than mutate some state, and update everyting to use local variables. The same transformation could probably be done to the other derivation goal types (which currently, unfortunately, contain their own `checkPathValidity`s, though they are diverging, and we hope and believe that they continue to diverge). --- src/libstore/build/derivation-goal.cc | 84 +++++++------------ .../nix/store/build/derivation-goal.hh | 16 ++-- 2 files changed, 40 insertions(+), 60 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 30eb7ca74..9923b0dc1 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -34,7 +34,6 @@ DerivationGoal::DerivationGoal( , drvPath(drvPath) , wantedOutput(wantedOutput) , outputHash{Hash::dummy} // will be updated - , outputKnown{} , buildMode(buildMode) { this->drv = std::make_unique(drv); @@ -98,7 +97,7 @@ Goal::Co DerivationGoal::haveDerivation() /* In checking mode, the builder will not register any outputs. So we want to make sure the ones that we wanted to check are properly there. */ - buildResult.builtOutputs = assertPathValidity(); + buildResult.builtOutputs = {{wantedOutput, assertPathValidity()}}; } for (auto it = buildResult.builtOutputs.begin(); it != buildResult.builtOutputs.end();) { @@ -125,19 +124,8 @@ Goal::Co DerivationGoal::haveDerivation() if (impure) experimentalFeatureSettings.require(Xp::ImpureDerivations); - auto outputHashes = staticOutputHashes(worker.evalStore, *drv); - if (auto * mOutputHash = get(outputHashes, wantedOutput)) { + if (auto * mOutputHash = get(staticOutputHashes(worker.evalStore, *drv), wantedOutput)) { outputHash = *mOutputHash; - - /* TODO we might want to also allow randomizing the paths - for regular CA derivations, e.g. for sake of checking - determinism. */ - if (impure) { - outputKnown = InitialOutputStatus{ - .path = StorePath::random(outputPathName(drv->name, wantedOutput)), - .status = PathStatus::Absent, - }; - } } if (impure) { @@ -147,14 +135,12 @@ Goal::Co DerivationGoal::haveDerivation() } } - { - /* Check what outputs paths are not already valid. */ - auto [allValid, validOutputs] = checkPathValidity(); + /* Check what outputs paths are not already valid. */ + auto checkResult = checkPathValidity(); - /* If they are all valid, then we're done. */ - if (allValid && buildMode == bmNormal) { - co_return done(BuildResult::AlreadyValid, std::move(validOutputs)); - } + /* If they are all valid, then we're done. */ + if (checkResult && checkResult->second == PathStatus::Valid && buildMode == bmNormal) { + co_return done(BuildResult::AlreadyValid, {{wantedOutput, checkResult->first}}); } Goals waitees; @@ -163,13 +149,13 @@ Goal::Co DerivationGoal::haveDerivation() through substitutes. If that doesn't work, we'll build them. */ if (settings.useSubstitutes && drvOptions.substitutesAllowed()) { - if (!outputKnown) + if (!checkResult) waitees.insert(upcast_goal(worker.makeDrvOutputSubstitutionGoal( DrvOutput{outputHash, wantedOutput}, buildMode == bmRepair ? Repair : NoRepair))); else { auto * cap = getDerivationCA(*drv); waitees.insert(upcast_goal(worker.makePathSubstitutionGoal( - outputKnown->path, + checkResult->first.outPath, buildMode == bmRepair ? Repair : NoRepair, cap ? std::optional{*cap} : std::nullopt))); } @@ -192,10 +178,12 @@ Goal::Co DerivationGoal::haveDerivation() nrFailed = nrNoSubstituters = 0; - auto [allValid, validOutputs] = checkPathValidity(); + checkResult = checkPathValidity(); + + bool allValid = checkResult && checkResult->second == PathStatus::Valid; if (buildMode == bmNormal && allValid) { - co_return done(BuildResult::Substituted, std::move(validOutputs)); + co_return done(BuildResult::Substituted, {{wantedOutput, checkResult->first}}); } if (buildMode == bmRepair && allValid) { co_return repairClosure(); @@ -300,15 +288,13 @@ Goal::Co DerivationGoal::repairClosure() "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(BuildResult::AlreadyValid, {{wantedOutput, assertPathValidity()}}); } -std::pair DerivationGoal::checkPathValidity() +std::optional> DerivationGoal::checkPathValidity() { if (drv->type().isImpure()) - return {false, {}}; - - SingleDrvOutputs validOutputs; + return std::nullopt; auto drvOutput = DrvOutput{outputHash, wantedOutput}; @@ -335,35 +321,29 @@ std::pair DerivationGoal::checkPathValidity() if (mRealisation) { auto & outputPath = mRealisation->outPath; bool checkHash = buildMode == bmRepair; - outputKnown = { - .path = outputPath, - .status = !worker.store.isValidPath(outputPath) ? PathStatus::Absent - : !checkHash || worker.pathContentsGood(outputPath) ? PathStatus::Valid - : PathStatus::Corrupt, - }; + PathStatus status = !worker.store.isValidPath(outputPath) ? PathStatus::Absent + : !checkHash || worker.pathContentsGood(outputPath) ? PathStatus::Valid + : PathStatus::Corrupt; - if (outputKnown->isValid()) { - if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) { - // We know the output because it's a static output of the - // derivation, and the output path is valid, but we don't have - // its realisation stored (probably because it has been built - // without the `ca-derivations` experimental flag). - worker.store.registerDrvOutput(*mRealisation); - } - - validOutputs.emplace(wantedOutput, *mRealisation); + if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations) && status == PathStatus::Valid) { + // We know the output because it's a static output of the + // derivation, and the output path is valid, but we don't have + // its realisation stored (probably because it has been built + // without the `ca-derivations` experimental flag). + worker.store.registerDrvOutput(*mRealisation); } - } - return {outputKnown && outputKnown->isValid(), validOutputs}; + return {{*mRealisation, status}}; + } else + return std::nullopt; } -SingleDrvOutputs DerivationGoal::assertPathValidity() +Realisation DerivationGoal::assertPathValidity() { - auto [allValid, validOutputs] = checkPathValidity(); - if (!allValid) + auto checkResult = checkPathValidity(); + if (!(checkResult && checkResult->second == PathStatus::Valid)) throw Error("some outputs are unexpectedly invalid"); - return validOutputs; + return checkResult->first; } Goal::Done DerivationGoal::done(BuildResult::Status status, SingleDrvOutputs builtOutputs, std::optional ex) diff --git a/src/libstore/include/nix/store/build/derivation-goal.hh b/src/libstore/include/nix/store/build/derivation-goal.hh index ca41d71aa..d0c069446 100644 --- a/src/libstore/include/nix/store/build/derivation-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-goal.hh @@ -75,7 +75,6 @@ private: */ Hash outputHash; - std::optional outputKnown; BuildMode buildMode; @@ -87,18 +86,19 @@ private: Co haveDerivation(); /** - * Update 'initialOutput' to determine the current status of the - * outputs of the derivation. Also returns a Boolean denoting - * whether all outputs are valid and non-corrupt, and a - * 'SingleDrvOutputs' structure containing the valid outputs. + * Return `std::nullopt` if the output is unknown, e.g. un unbuilt + * floating content-addressing derivation. Otherwise, returns a pair + * of a `Realisation`, containing among other things the store path + * of the wanted output, and a `PathStatus` with the + * current status of that output. */ - std::pair checkPathValidity(); + std::optional> checkPathValidity(); /** * Aborts if any output is not valid or corrupt, and otherwise - * returns a 'SingleDrvOutputs' structure containing all outputs. + * returns a 'Realisation' for the wanted output. */ - SingleDrvOutputs assertPathValidity(); + Realisation assertPathValidity(); Co repairClosure();