From a58e0584f5e250ace8e31f53b63e323ebf35dee0 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 24 Feb 2025 10:55:02 -0500 Subject: [PATCH] Rework derivation input resolution I refactored the way that input resolution works in `DerivationGoal`. To be honest, it is probably unclear to the reader whether this new way is better or worse. I suppose *intrinsic* motivation, I can say that - the more structured use of `inputGoal` (a local variable) is better than the shotgrun approach with `inputDrvOutputs` - A virtual `waiteeDone` was a hack, and now it's gone. However, the *real* motivation of this is not the above things, but that it is needed for my mammoth refactor fixing #11897 and #11928. It is nice that this step could come first, rather than making that refactor even bigger. --- src/libstore/build/derivation-goal.cc | 143 +++++++++++--------------- src/libstore/build/derivation-goal.hh | 9 -- src/libstore/build/goal.hh | 2 +- src/libstore/build/worker.cc | 5 + src/libstore/derivations.cc | 60 +++++------ src/libstore/derivations.hh | 2 +- 6 files changed, 91 insertions(+), 130 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index b2bb9da75..a32a714e3 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -322,6 +322,18 @@ Goal::Co DerivationGoal::haveDerivation() } +/** + * Used for `inputGoals` local variable below + */ +struct value_comparison +{ + template + bool operator()(const ref & lhs, const ref & 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. */ - inputDrvOutputs.clear(); + std::map, GoalPtr, value_comparison> inputGoals; + if (useDerivation) { std::function, const DerivedPathMap::ChildNode &)> addWaiteeDerivedPath; addWaiteeDerivedPath = [&](ref inputDrv, const DerivedPathMap::ChildNode & inputNode) { - if (!inputNode.value.empty()) - addWaitee(worker.makeGoal( + 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::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 drvPath, const std::string & outputName) -> std::optional { + 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::ChildNode &)> accumInputPaths; + /* If we get this far, we know no dynamic drvs inputs */ - accumInputPaths = [&](const StorePath & depDrvPath, const DerivedPathMap::ChildNode & inputNode) { - /* Add the relevant output closures of the input derivation - `i' as input paths. Only add the closures of output paths - that are specified as inputs. */ - auto getOutput = [&](const std::string & outputName) { - /* TODO (impure derivations-induced tech debt): - Tracking input derivation outputs statefully through the - goals is error prone and has led to bugs. - For a robust nix, we need to move towards the `else` branch, - which does not rely on goal state to match up with the - reality of the store, which is our real source of truth. - 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 - in itself. When implementing this logic entirely with lookups - make sure that they're cached. */ - if (auto outPath = get(inputDrvOutputs, { depDrvPath, outputName })) { - return *outPath; + for (auto & [depDrvPath, depNode] : fullDrv.inputDrvs.map) { + for (auto & outputName : depNode.value) { + /* Don't need to worry about `inputGoals`, because + impure derivations are always resolved above. Can + just use DB. This case only happens in the (older) + input addressed and fixed output derivation cases. */ + 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); + 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)); } - 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); - 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); + worker.store.computeFSClosure(outMapPath->second, inputPaths); + } + } } /* 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(drv.get()); - - auto * dg = dynamic_cast(&*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); - } - } -} - } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 4622cb2b1..7f594b92c 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/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, StorePath> inputDrvOutputs; - /** * See `needRestart`; just for that field. */ @@ -331,8 +324,6 @@ struct DerivationGoal : public Goal SingleDrvOutputs builtOutputs = {}, std::optional ex = {}); - void waiteeDone(GoalPtr waitee, ExitCode result) override; - StorePathSet exportReferences(const StorePathSet & storePaths); JobCategory jobCategory() const override { diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 1dd7ed525..95572ef02 100644 --- a/src/libstore/build/goal.hh +++ b/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) { diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index dbe86f43f..dad460a1e 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -552,4 +552,9 @@ GoalPtr upcast_goal(std::shared_ptr subGoal) return subGoal; } +GoalPtr upcast_goal(std::shared_ptr subGoal) +{ + return subGoal; +} + } diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index b54838a0a..c31c801c2 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -1052,49 +1052,36 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String std::optional Derivation::tryResolve(Store & store, Store * evalStore) const { - std::map, StorePath> inputDrvOutputs; - - std::function::ChildNode &)> accum; - accum = [&](auto & inputDrv, auto & node) { - for (auto & [outputName, outputPath] : store.queryPartialDerivationOutputMap(inputDrv, evalStore)) { - if (outputPath) { - inputDrvOutputs.insert_or_assign({inputDrv, outputName}, *outputPath); - if (auto p = get(node.childMap, outputName)) - accum(*outputPath, *p); + return tryResolve( + store, + [&](ref drvPath, const std::string & outputName) -> std::optional { + try { + return resolveDerivedPath(store, SingleDerivedPath::Built{drvPath, outputName}, evalStore); + } catch (Error &) { + return std::nullopt; } - } - }; - - 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::ChildNode & inputNode, - const std::map, StorePath> & inputDrvOutputs) + ref drvPath, const DerivedPathMap::ChildNode & inputNode, + std::function(ref 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(&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, - inputDrvOutputs)) + &nextPlaceholder, + make_ref(SingleDerivedPath::Built{drvPath, outputName}), + childNode, + queryResolutionChain)) return false; } return true; @@ -1120,7 +1106,7 @@ static bool tryResolveInput( std::optional Derivation::tryResolve( Store & store, - const std::map, StorePath> & inputDrvOutputs) const + std::function(ref drvPath, const std::string & outputName)> queryResolutionChain) const { BasicDerivation resolved { *this }; @@ -1129,7 +1115,7 @@ std::optional Derivation::tryResolve( for (auto & [inputDrv, inputNode] : inputDrvs.map) if (!tryResolveInput(store, resolved.inputSrcs, inputRewrites, - nullptr, inputDrv, inputNode, inputDrvOutputs)) + nullptr, make_ref(SingleDerivedPath::Opaque{inputDrv}), inputNode, queryResolutionChain)) return std::nullopt; rewriteDerivation(store, resolved, inputRewrites); diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 5b2101ed5..7b62b81ba 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -369,7 +369,7 @@ struct Derivation : BasicDerivation */ std::optional tryResolve( Store & store, - const std::map, StorePath> & inputDrvOutputs) const; + std::function(ref drvPath, const std::string & outputName)> queryResolutionChain) const; /** * Check that the derivation is valid and does not present any