From 2ee41976c22f1252a439f1940d6190b82830283b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 26 Sep 2025 01:13:22 -0400 Subject: [PATCH] Fix #13247 Resolve the derivation before creating a building goal, in a context where we know what output(s) we want. That way we have a chance just to download the outputs we want. Fix #13247 (cherry picked from commit 39f6fd9b464298f37a08cfe7485271b9294fd278) --- .../build/derivation-building-goal.cc | 103 ------------------ src/libstore/build/derivation-goal.cc | 91 ++++++++++++++++ tests/functional/ca/issue-13247.sh | 5 +- 3 files changed, 92 insertions(+), 107 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 037401ccb..c00123634 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/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" @@ -158,107 +156,6 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() /* Determine the full set of input paths. */ { - 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; - - /* Store the resolved derivation, as part of the record of - what we're actually building */ - writeDerivation(worker.store, drvResolved); - - /* 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) { diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index dc12ab55a..1f8eb1262 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/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" @@ -140,6 +141,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; + + /* 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); + { + 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); diff --git a/tests/functional/ca/issue-13247.sh b/tests/functional/ca/issue-13247.sh index 686d90ced..705919513 100755 --- a/tests/functional/ca/issue-13247.sh +++ b/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')" ]] - -# 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" +[[ ! -e "$(jq -r <"$TEST_ROOT"/a.json '.[2].outputs.second')" ]]