From b40369942cdb3e713c473515b9760f8a0d2ed3cc Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 14 Aug 2024 12:32:26 +0200 Subject: [PATCH] libstore: make Worker::childStarted private this can be a proper WorkResult now. childTerminated is unfortunately a lot more stubborn and won't be made private for quite a while yet. once we can get rid of the Worker poll loop that *should* be possible though Change-Id: I2218df202da5cb84e852f6a37e4c20367495b617 --- src/libstore/build/derivation-goal.cc | 11 +++++------ src/libstore/build/derivation-goal.hh | 6 ++++-- .../build/drv-output-substitution-goal.cc | 4 +--- src/libstore/build/goal.hh | 5 +++++ src/libstore/build/local-derivation-goal.cc | 19 ++++++++++--------- src/libstore/build/local-derivation-goal.hh | 2 +- src/libstore/build/substitution-goal.cc | 4 +--- src/libstore/build/worker.cc | 1 + src/libstore/build/worker.hh | 14 +++++++------- 9 files changed, 35 insertions(+), 31 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index a1f628afc..f31c3acd5 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -646,7 +646,7 @@ Goal::WorkResult DerivationGoal::inputsRealised(bool inBuildSlot) return ContinueImmediately{}; } -Goal::WorkResult DerivationGoal::started() +void DerivationGoal::started() { auto msg = fmt( buildMode == bmRepair ? "repairing outputs of '%s'" : @@ -657,7 +657,6 @@ Goal::WorkResult DerivationGoal::started() act = std::make_unique(*logger, lvlInfo, actBuild, msg, Logger::Fields{worker.store.printStorePath(drvPath), hook ? machineName : "", 1, 1}); mcRunningBuilds = std::make_unique>(worker.runningBuilds); - return StillAlive{}; } Goal::WorkResult DerivationGoal::tryToBuild(bool inBuildSlot) @@ -735,13 +734,14 @@ Goal::WorkResult DerivationGoal::tryToBuild(bool inBuildSlot) auto hookReply = tryBuildHook(inBuildSlot); auto result = std::visit( overloaded{ - [&](HookReply::Accept) -> std::optional { + [&](HookReply::Accept & a) -> std::optional { /* Yes, it has started doing so. Wait until we get EOF from the hook. */ actLock.reset(); buildResult.startTime = time(0); // inexact state = &DerivationGoal::buildDone; - return started(); + started(); + return WaitForWorld{std::move(a.fds), false}; }, [&](HookReply::Postpone) -> std::optional { /* Not now; wait until at least one child finishes or @@ -1222,9 +1222,8 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot) fds.insert(hook->fromHook.get()); fds.insert(hook->builderOut.get()); builderOutFD = &hook->builderOut; - worker.childStarted(shared_from_this(), fds, false); - return HookReply::Accept{}; + return HookReply::Accept{std::move(fds)}; } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index f7d86ed2e..c9638c2b6 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -15,7 +15,9 @@ using std::map; struct HookInstance; struct HookReplyBase { - struct [[nodiscard]] Accept {}; + struct [[nodiscard]] Accept { + std::set fds; + }; struct [[nodiscard]] Decline {}; struct [[nodiscard]] Postpone {}; }; @@ -345,7 +347,7 @@ struct DerivationGoal : public Goal WorkResult repairClosure(); - WorkResult started(); + void started(); Finished done( BuildResult::Status status, diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index aea055624..2032478d1 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -75,10 +75,8 @@ Goal::WorkResult DrvOutputSubstitutionGoal::tryNext(bool inBuildSlot) return sub->queryRealisation(id); }); - worker.childStarted(shared_from_this(), {downloadState->outPipe.readSide.get()}, true); - state = &DrvOutputSubstitutionGoal::realisationFetched; - return StillAlive{}; + return WaitForWorld{{downloadState->outPipe.readSide.get()}, true}; } Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched(bool inBuildSlot) diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 114abda41..2ac96ace1 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -112,6 +112,10 @@ public: struct [[nodiscard]] WaitForGoals { Goals goals; }; + struct [[nodiscard]] WaitForWorld { + std::set fds; + bool inBuildSlot; + }; struct [[nodiscard]] Finished { ExitCode result; std::unique_ptr ex; @@ -127,6 +131,7 @@ public: WaitForAWhile, ContinueImmediately, WaitForGoals, + WaitForWorld, Finished> { WorkResult() = delete; diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 25b520c05..1e3c4109d 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -230,7 +230,14 @@ Goal::WorkResult LocalDerivationGoal::tryLocalBuild(bool inBuildSlot) try { /* Okay, we have to build. */ - startBuilder(); + auto fds = startBuilder(); + + /* This state will be reached when we get EOF on the child's + log pipe. */ + state = &DerivationGoal::buildDone; + + started(); + return WaitForWorld{std::move(fds), true}; } catch (BuildError & e) { outputLocks.unlock(); @@ -239,12 +246,6 @@ Goal::WorkResult LocalDerivationGoal::tryLocalBuild(bool inBuildSlot) report.permanentFailure = true; return report; } - - /* This state will be reached when we get EOF on the child's - log pipe. */ - state = &DerivationGoal::buildDone; - - return started(); } @@ -374,7 +375,7 @@ void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() cleanupPostOutputsRegisteredModeCheck(); } -void LocalDerivationGoal::startBuilder() +std::set LocalDerivationGoal::startBuilder() { if ((buildUser && buildUser->getUIDCount() != 1) #if __linux__ @@ -753,7 +754,7 @@ void LocalDerivationGoal::startBuilder() msgs.push_back(std::move(msg)); } - worker.childStarted(shared_from_this(), {builderOutPTY.get()}, true); + return {builderOutPTY.get()}; } diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 9b2391256..f17685af8 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -216,7 +216,7 @@ struct LocalDerivationGoal : public DerivationGoal /** * Start building a derivation. */ - void startBuilder(); + std::set startBuilder(); /** * Fill in the environment for the builder. diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 281ac216c..d013d1c78 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -221,10 +221,8 @@ Goal::WorkResult PathSubstitutionGoal::tryToRun(bool inBuildSlot) } }); - worker.childStarted(shared_from_this(), {outPipe.readSide.get()}, true); - state = &PathSubstitutionGoal::finished; - return StillAlive{}; + return WaitForWorld{{outPipe.readSide.get()}, true}; } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 325de3073..9740cdd4d 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -198,6 +198,7 @@ void Worker::handleWorkResult(GoalPtr goal, Goal::WorkResult how) dep->waiters.insert(goal); } }, + [&](Goal::WaitForWorld & w) { childStarted(goal, w.fds, w.inBuildSlot); }, [&](Goal::Finished & f) { goalFinished(goal, f); }, }, how diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 406bf205d..7abb966f9 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -157,6 +157,13 @@ private: */ void removeGoal(GoalPtr goal); + /** + * Registers a running child process. `inBuildSlot` means that + * the process counts towards the jobs limit. + */ + void childStarted(GoalPtr goal, const std::set & fds, + bool inBuildSlot); + public: const Activity act; @@ -228,13 +235,6 @@ public: */ GoalPtr makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal); - /** - * Registers a running child process. `inBuildSlot` means that - * the process counts towards the jobs limit. - */ - void childStarted(GoalPtr goal, const std::set & fds, - bool inBuildSlot); - /** * Unregisters a running child process. */