From e5177dddff13e7e5bb1bdecf28776822c6dba528 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 26 Jul 2024 20:19:21 +0200 Subject: [PATCH] libstore: move Goal::amDone to Worker we still mutate goal state to store the results of any given goal run, but now we also have that information in Worker and could in theory do something else with it. we could return a map of goal to goal results, which would also let us better diagnose failures of subgoals (at all). Change-Id: I1df956bbd9fa8cc9485fb6df32918d68dda3ff48 --- src/libstore/build/derivation-goal.cc | 5 +- .../build/drv-output-substitution-goal.cc | 10 ++-- src/libstore/build/goal.cc | 25 --------- src/libstore/build/goal.hh | 11 ++-- src/libstore/build/substitution-goal.cc | 2 +- src/libstore/build/worker.cc | 56 ++++++++++++++++--- src/libstore/build/worker.hh | 3 + 7 files changed, 69 insertions(+), 43 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index a916da191..ab7b2b88c 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1534,7 +1534,10 @@ Goal::Finished DerivationGoal::done( fs << worker.store.printStorePath(drvPath) << "\t" << buildResult.toString() << std::endl; } - return amDone(buildResult.success() ? ecSuccess : ecFailed, std::move(ex)); + return Finished{ + .result = buildResult.success() ? ecSuccess : ecFailed, + .ex = ex ? std::make_unique(std::move(*ex)) : nullptr, + }; } diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 4d10543cd..62e86e1cc 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -26,7 +26,7 @@ Goal::WorkResult DrvOutputSubstitutionGoal::init() /* If the derivation already exists, we’re done */ if (worker.store.queryRealisation(id)) { - return amDone(ecSuccess); + return Finished{ecSuccess}; } subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list>(); @@ -60,7 +60,7 @@ Goal::WorkResult DrvOutputSubstitutionGoal::tryNext() /* Hack: don't indicate failure if there were no substituters. In that case the calling derivation should just do a build. */ - return amDone(substituterFailed ? ecFailed : ecNoSubstituters); + return Finished{substituterFailed ? ecFailed : ecNoSubstituters}; } sub = subs.front(); @@ -137,7 +137,9 @@ Goal::WorkResult DrvOutputSubstitutionGoal::outPathValid() if (nrFailed > 0) { debug("The output path of the derivation output '%s' could not be substituted", id.to_string()); - return amDone(nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed); + return Finished{ + nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed + }; } worker.store.registerDrvOutput(*outputInfo); @@ -147,7 +149,7 @@ Goal::WorkResult DrvOutputSubstitutionGoal::outPathValid() Goal::WorkResult DrvOutputSubstitutionGoal::finished() { trace("finished"); - return amDone(ecSuccess); + return Finished{ecSuccess}; } std::string DrvOutputSubstitutionGoal::key() diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index 70ef5706e..40a3bae8d 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -45,31 +45,6 @@ void Goal::waiteeDone(GoalPtr waitee, ExitCode result) } -Goal::Finished Goal::amDone(ExitCode result, std::optional ex) -{ - trace("done"); - assert(!exitCode.has_value()); - exitCode = result; - - if (ex) { - if (!waiters.empty()) - logError(ex->info()); - else - this->ex = std::make_unique(std::move(*ex)); - } - - for (auto & i : waiters) { - GoalPtr goal = i.lock(); - if (goal) goal->waiteeDone(shared_from_this(), result); - } - waiters.clear(); - worker.removeGoal(shared_from_this()); - - cleanup(); - return Finished{}; -} - - void Goal::trace(std::string_view s) { debug("%1%: %2%", name, s); diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index db20b5cdb..adb3ab94b 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -105,10 +105,13 @@ struct Goal : public std::enable_shared_from_this public: - struct StillAlive {}; - struct Finished {}; + struct [[nodiscard]] StillAlive {}; + struct [[nodiscard]] Finished { + ExitCode result; + std::unique_ptr ex; + }; - struct WorkResult : std::variant + struct [[nodiscard]] WorkResult : std::variant { WorkResult() = delete; using variant::variant; @@ -159,8 +162,6 @@ public: virtual std::string key() = 0; - Finished amDone(ExitCode result, std::optional ex = {}); - virtual void cleanup() { } /** diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 2c5dfea84..fb2949fd0 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -35,7 +35,7 @@ Goal::Finished PathSubstitutionGoal::done( debug(*errorMsg); buildResult.errorMsg = *errorMsg; } - return amDone(result); + return Finished{result}; } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 5b2e36acb..f4c352b61 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -139,6 +139,40 @@ static void removeGoal(std::shared_ptr goal, std::map> & } +void Worker::goalFinished(GoalPtr goal, Goal::Finished & f) +{ + goal->trace("done"); + assert(!goal->exitCode.has_value()); + goal->exitCode = f.result; + + if (f.ex) { + if (!goal->waiters.empty()) + logError(f.ex->info()); + else + goal->ex = std::move(f.ex); + } + + for (auto & i : goal->waiters) { + if (GoalPtr waiting = i.lock()) { + waiting->waiteeDone(goal, f.result); + } + } + goal->waiters.clear(); + removeGoal(goal); + goal->cleanup(); +} + +void Worker::handleWorkResult(GoalPtr goal, Goal::WorkResult how) +{ + std::visit( + overloaded{ + [&](Goal::StillAlive) {}, + [&](Goal::Finished & f) { goalFinished(goal, f); }, + }, + how + ); +} + void Worker::removeGoal(GoalPtr goal) { if (auto drvGoal = std::dynamic_pointer_cast(goal)) @@ -299,7 +333,7 @@ void Worker::run(const Goals & _topGoals) awake.clear(); for (auto & goal : awake2) { checkInterrupt(); - goal->work(); + handleWorkResult(goal, goal->work()); actDerivations.progress( doneBuilds, expectedBuilds + doneBuilds, runningBuilds, failedBuilds @@ -429,9 +463,14 @@ void Worker::waitForInput() j->respectTimeouts && after - j->lastOutput >= std::chrono::seconds(settings.maxSilentTime)) { - goal->timedOut(Error( + handleWorkResult( + goal, + goal->timedOut(Error( "%1% timed out after %2% seconds of silence", - goal->getName(), settings.maxSilentTime)); + goal->getName(), + settings.maxSilentTime + )) + ); continue; } @@ -440,9 +479,12 @@ void Worker::waitForInput() j->respectTimeouts && after - j->timeStarted >= std::chrono::seconds(settings.buildTimeout)) { - goal->timedOut(Error( - "%1% timed out after %2% seconds", - goal->getName(), settings.buildTimeout)); + handleWorkResult( + goal, + goal->timedOut( + Error("%1% timed out after %2% seconds", goal->getName(), settings.buildTimeout) + ) + ); continue; } @@ -469,7 +511,7 @@ void Worker::waitForInput() goal->getName(), rd); std::string_view data((char *) buffer.data(), rd); j->lastOutput = after; - goal->handleChildOutput(k, data); + handleWorkResult(goal, goal->handleChildOutput(k, data)); } } } diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 903380c45..5af93b49e 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -105,6 +105,9 @@ private: */ std::map pathContentsGoodCache; + void goalFinished(GoalPtr goal, Goal::Finished & f); + void handleWorkResult(GoalPtr goal, Goal::WorkResult how); + public: const Activity act;