From 869666cb651f97cdce3a6aabf62073bfe1130cbb Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Thu, 29 Aug 2024 21:06:30 +0200 Subject: [PATCH] libstore: hide Worker goal factory methods this doesn't serve a great purpose yet except to confine construction of goals to the stack frame of Worker::run() and its child frames. we don't need this yet (and the goal constructors remain fully visible), but in a future change that fully removes the current worker loop we'll need some way of knowing which goals are top-level goals without passing the goals themselves around. once that's possible we can remove visible goals as a concept and rely on build result futures and a scheduler built upon them Change-Id: Ia73cdeffcfb9ba1ce9d69b702dc0bc637a4c4ce6 --- src/libstore/build/derivation-goal.cc | 16 ++--- .../build/drv-output-substitution-goal.cc | 4 +- src/libstore/build/entry-points.cc | 65 ++++++++++--------- src/libstore/build/substitution-goal.cc | 2 +- src/libstore/build/worker.cc | 5 +- src/libstore/build/worker.hh | 63 +++++++++++++++--- 6 files changed, 106 insertions(+), 49 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 462c381b8..3dca7a3c0 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -170,7 +170,7 @@ Goal::WorkResult DerivationGoal::getDerivation(bool inBuildSlot) state = &DerivationGoal::loadDerivation; - return WaitForGoals{{worker.makePathSubstitutionGoal(drvPath)}}; + return WaitForGoals{{worker.goalFactory().makePathSubstitutionGoal(drvPath)}}; } @@ -268,14 +268,14 @@ Goal::WorkResult DerivationGoal::haveDerivation(bool inBuildSlot) if (!status.wanted) continue; if (!status.known) result.goals.insert( - worker.makeDrvOutputSubstitutionGoal( + worker.goalFactory().makeDrvOutputSubstitutionGoal( DrvOutput{status.outputHash, outputName}, buildMode == bmRepair ? Repair : NoRepair ) ); else { auto * cap = getDerivationCA(*drv); - result.goals.insert(worker.makePathSubstitutionGoal( + result.goals.insert(worker.goalFactory().makePathSubstitutionGoal( status.known->path, buildMode == bmRepair ? Repair : NoRepair, cap ? std::optional { *cap } : std::nullopt)); @@ -374,7 +374,7 @@ Goal::WorkResult DerivationGoal::gaveUpOnSubstitution(bool inBuildSlot) addWaiteeDerivedPath = [&](ref inputDrv, const DerivedPathMap::ChildNode & inputNode) { if (!inputNode.value.empty()) - result.goals.insert(worker.makeGoal( + result.goals.insert(worker.goalFactory().makeGoal( DerivedPath::Built { .drvPath = inputDrv, .outputs = inputNode.value, @@ -419,7 +419,7 @@ Goal::WorkResult DerivationGoal::gaveUpOnSubstitution(bool inBuildSlot) if (!settings.useSubstitutes) throw Error("dependency '%s' of '%s' does not exist, and substitution is disabled", worker.store.printStorePath(i), worker.store.printStorePath(drvPath)); - result.goals.insert(worker.makePathSubstitutionGoal(i)); + result.goals.insert(worker.goalFactory().makePathSubstitutionGoal(i)); } if (result.goals.empty()) {/* to prevent hang (no wake-up event) */ @@ -475,9 +475,9 @@ Goal::WorkResult DerivationGoal::repairClosure() worker.store.printStorePath(i), worker.store.printStorePath(drvPath)); auto drvPath2 = outputsToDrv.find(i); if (drvPath2 == outputsToDrv.end()) - result.goals.insert(worker.makePathSubstitutionGoal(i, Repair)); + result.goals.insert(worker.goalFactory().makePathSubstitutionGoal(i, Repair)); else - result.goals.insert(worker.makeGoal( + result.goals.insert(worker.goalFactory().makeGoal( DerivedPath::Built { .drvPath = makeConstantStorePathRef(drvPath2->second), .outputs = OutputsSpec::All { }, @@ -580,7 +580,7 @@ Goal::WorkResult DerivationGoal::inputsRealised(bool inBuildSlot) worker.store.printStorePath(pathResolved), }); - resolvedDrvGoal = worker.makeDerivationGoal( + resolvedDrvGoal = worker.goalFactory().makeDerivationGoal( pathResolved, wantedOutputs, buildMode); state = &DerivationGoal::resolvedFinished; diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 54d81d7a0..9acff14b0 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -112,11 +112,11 @@ Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched(bool inBuildSlot) ); return tryNext(inBuildSlot); } - result.goals.insert(worker.makeDrvOutputSubstitutionGoal(depId)); + result.goals.insert(worker.goalFactory().makeDrvOutputSubstitutionGoal(depId)); } } - result.goals.insert(worker.makePathSubstitutionGoal(outputInfo->outPath)); + result.goals.insert(worker.goalFactory().makePathSubstitutionGoal(outputInfo->outPath)); if (result.goals.empty()) { return outPathValid(inBuildSlot); diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 1f85881fa..a5bb05b24 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -10,11 +10,12 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod { Worker worker(*this, evalStore ? *evalStore : *this); - Goals goals; - for (auto & br : reqs) - goals.insert(worker.makeGoal(br, buildMode)); - - worker.run(goals); + auto goals = worker.run([&](GoalFactory & gf) { + Goals goals; + for (auto & br : reqs) + goals.insert(gf.makeGoal(br, buildMode)); + return goals; + }); StringSet failed; std::shared_ptr ex; @@ -48,17 +49,17 @@ std::vector Store::buildPathsWithResults( std::shared_ptr evalStore) { Worker worker(*this, evalStore ? *evalStore : *this); - - Goals goals; std::vector> state; - for (const auto & req : reqs) { - auto goal = worker.makeGoal(req, buildMode); - goals.insert(goal); - state.push_back({req, goal}); - } - - worker.run(goals); + auto goals = worker.run([&](GoalFactory & gf) { + Goals goals; + for (const auto & req : reqs) { + auto goal = gf.makeGoal(req, buildMode); + goals.insert(goal); + state.push_back({req, goal}); + } + return goals; + }); std::vector results; @@ -72,10 +73,12 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat BuildMode buildMode) { Worker worker(*this, *this); - auto goal = worker.makeBasicDerivationGoal(drvPath, drv, OutputsSpec::All {}, buildMode); try { - worker.run(Goals{goal}); + auto goals = worker.run([&](GoalFactory & gf) -> Goals { + return Goals{gf.makeBasicDerivationGoal(drvPath, drv, OutputsSpec::All{}, buildMode)}; + }); + auto goal = *goals.begin(); return goal->buildResult.restrictTo(DerivedPath::Built { .drvPath = makeConstantStorePathRef(drvPath), .outputs = OutputsSpec::All {}, @@ -95,10 +98,10 @@ void Store::ensurePath(const StorePath & path) if (isValidPath(path)) return; Worker worker(*this, *this); - GoalPtr goal = worker.makePathSubstitutionGoal(path); - Goals goals = {goal}; - worker.run(goals); + auto goals = + worker.run([&](GoalFactory & gf) { return Goals{gf.makePathSubstitutionGoal(path)}; }); + auto goal = *goals.begin(); if (goal->exitCode != Goal::ecSuccess) { if (goal->ex) { @@ -113,23 +116,27 @@ void Store::ensurePath(const StorePath & path) void Store::repairPath(const StorePath & path) { Worker worker(*this, *this); - GoalPtr goal = worker.makePathSubstitutionGoal(path, Repair); - Goals goals = {goal}; - worker.run(goals); + auto goals = worker.run([&](GoalFactory & gf) { + return Goals{gf.makePathSubstitutionGoal(path, Repair)}; + }); + auto goal = *goals.begin(); if (goal->exitCode != Goal::ecSuccess) { /* Since substituting the path didn't work, if we have a valid deriver, then rebuild the deriver. */ auto info = queryPathInfo(path); if (info->deriver && isValidPath(*info->deriver)) { - goals.clear(); - goals.insert(worker.makeGoal(DerivedPath::Built { - .drvPath = makeConstantStorePathRef(*info->deriver), - // FIXME: Should just build the specific output we need. - .outputs = OutputsSpec::All { }, - }, bmRepair)); - worker.run(goals); + worker.run([&](GoalFactory & gf) { + return Goals{gf.makeGoal( + DerivedPath::Built{ + .drvPath = makeConstantStorePathRef(*info->deriver), + // FIXME: Should just build the specific output we need. + .outputs = OutputsSpec::All{}, + }, + bmRepair + )}; + }); } else throw Error(worker.failingExitStatus(), "cannot repair path '%s'", printStorePath(path)); } diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 390aabb45..673b3c503 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -161,7 +161,7 @@ Goal::WorkResult PathSubstitutionGoal::tryNext(bool inBuildSlot) WaitForGoals result; for (auto & i : info->references) if (i != storePath) /* ignore self-references */ - result.goals.insert(worker.makePathSubstitutionGoal(i)); + result.goals.insert(worker.goalFactory().makePathSubstitutionGoal(i)); if (result.goals.empty()) {/* to prevent hang (no wake-up event) */ return referencesValid(inBuildSlot); diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 9e548cc16..5d0cc920a 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -326,8 +326,9 @@ void Worker::waitForAWhile(GoalPtr goal) } -void Worker::run(const Goals & _topGoals) +Goals Worker::run(std::function req) { + auto _topGoals = req(goalFactory()); std::vector topPaths; assert(!running); @@ -411,6 +412,8 @@ void Worker::run(const Goals & _topGoals) assert(!settings.keepGoing || awake.empty()); assert(!settings.keepGoing || wantingToBuild.empty()); assert(!settings.keepGoing || children.empty()); + + return _topGoals; } void Worker::waitForInput() diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 360366f8d..3fbf457fe 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -40,10 +40,57 @@ struct Child /* Forward definition. */ struct HookInstance; +class GoalFactory +{ +public: + virtual std::shared_ptr makeDerivationGoal( + const StorePath & drvPath, const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal + ) = 0; + virtual std::shared_ptr makeBasicDerivationGoal( + const StorePath & drvPath, + const BasicDerivation & drv, + const OutputsSpec & wantedOutputs, + BuildMode buildMode = bmNormal + ) = 0; + + /** + * @ref SubstitutionGoal "substitution goal" + */ + virtual std::shared_ptr makePathSubstitutionGoal( + const StorePath & storePath, + RepairFlag repair = NoRepair, + std::optional ca = std::nullopt + ) = 0; + virtual std::shared_ptr makeDrvOutputSubstitutionGoal( + const DrvOutput & id, + RepairFlag repair = NoRepair, + std::optional ca = std::nullopt + ) = 0; + + /** + * Make a goal corresponding to the `DerivedPath`. + * + * It will be a `DerivationGoal` for a `DerivedPath::Built` or + * a `SubstitutionGoal` for a `DerivedPath::Opaque`. + */ + virtual GoalPtr makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal) = 0; +}; + +// elaborate hoax to let goals access factory methods while hiding them from the public +class WorkerBase : protected GoalFactory +{ + friend struct DerivationGoal; + friend struct PathSubstitutionGoal; + friend class DrvOutputSubstitutionGoal; + +protected: + GoalFactory & goalFactory() { return *this; } +}; + /** * The worker class. */ -class Worker +class Worker : public WorkerBase { private: @@ -215,19 +262,18 @@ private: std::shared_ptr makeDerivationGoalCommon( const StorePath & drvPath, const OutputsSpec & wantedOutputs, std::function()> mkDrvGoal); -public: std::shared_ptr makeDerivationGoal( const StorePath & drvPath, - const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal); + const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal) override; std::shared_ptr makeBasicDerivationGoal( const StorePath & drvPath, const BasicDerivation & drv, - const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal); + const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal) override; /** * @ref SubstitutionGoal "substitution goal" */ - std::shared_ptr makePathSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); - std::shared_ptr makeDrvOutputSubstitutionGoal(const DrvOutput & id, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); + std::shared_ptr makePathSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional ca = std::nullopt) override; + std::shared_ptr makeDrvOutputSubstitutionGoal(const DrvOutput & id, RepairFlag repair = NoRepair, std::optional ca = std::nullopt) override; /** * Make a goal corresponding to the `DerivedPath`. @@ -235,8 +281,9 @@ public: * It will be a `DerivationGoal` for a `DerivedPath::Built` or * a `SubstitutionGoal` for a `DerivedPath::Opaque`. */ - GoalPtr makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal); + GoalPtr makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal) override; +public: /** * Unregisters a running child process. */ @@ -245,7 +292,7 @@ public: /** * Loop until the specified top-level goals have finished. */ - void run(const Goals & topGoals); + Goals run(std::function req); /*** * The exit status in case of failure.