From dfcab1c3f09971cba6a198ba81158d1190975165 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 26 Jul 2024 19:24:38 +0200 Subject: libstore: return finishedness from Goal methods this is the first step towards removing all result-related mutation of Goal state from goal implementations themselves, and into Worker state instead. once that is done we can treat all non-const Goal fields like private state of the goal itself, and make threading of goals possible Change-Id: I69ff7d02a6fd91a65887c6640bfc4f5fb785b45c --- src/libstore/build/derivation-goal.cc | 126 ++++++++++----------- src/libstore/build/derivation-goal.hh | 36 +++--- src/libstore/build/drv-output-substitution-goal.cc | 50 ++++---- src/libstore/build/drv-output-substitution-goal.hh | 16 +-- src/libstore/build/goal.cc | 3 +- src/libstore/build/goal.hh | 17 ++- src/libstore/build/local-derivation-goal.cc | 11 +- src/libstore/build/local-derivation-goal.hh | 2 +- src/libstore/build/substitution-goal.cc | 74 ++++++------ src/libstore/build/substitution-goal.hh | 22 ++-- 10 files changed, 177 insertions(+), 180 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 975a0658d..a916da191 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -130,16 +130,16 @@ void DerivationGoal::killChild() } -void DerivationGoal::timedOut(Error && ex) +Goal::Finished DerivationGoal::timedOut(Error && ex) { killChild(); - done(BuildResult::TimedOut, {}, std::move(ex)); + return done(BuildResult::TimedOut, {}, std::move(ex)); } -void DerivationGoal::work() +Goal::WorkResult DerivationGoal::work() { - (this->*state)(); + return (this->*state)(); } void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) @@ -163,7 +163,7 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) } -void DerivationGoal::getDerivation() +Goal::WorkResult DerivationGoal::getDerivation() { trace("init"); @@ -171,23 +171,22 @@ void DerivationGoal::getDerivation() exists. If it doesn't, it may be created through a substitute. */ if (buildMode == bmNormal && worker.evalStore.isValidPath(drvPath)) { - loadDerivation(); - return; + return loadDerivation(); } addWaitee(worker.makePathSubstitutionGoal(drvPath)); state = &DerivationGoal::loadDerivation; + return StillAlive{}; } -void DerivationGoal::loadDerivation() +Goal::WorkResult DerivationGoal::loadDerivation() { trace("loading derivation"); if (nrFailed != 0) { - done(BuildResult::MiscFailure, {}, Error("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath))); - return; + return done(BuildResult::MiscFailure, {}, Error("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath))); } /* `drvPath' should already be a root, but let's be on the safe @@ -209,11 +208,11 @@ void DerivationGoal::loadDerivation() } assert(drv); - haveDerivation(); + return haveDerivation(); } -void DerivationGoal::haveDerivation() +Goal::WorkResult DerivationGoal::haveDerivation() { trace("have derivation"); @@ -241,8 +240,7 @@ void DerivationGoal::haveDerivation() }); } - gaveUpOnSubstitution(); - return; + return gaveUpOnSubstitution(); } for (auto & i : drv->outputsAndOptPaths(worker.store)) @@ -264,8 +262,7 @@ void DerivationGoal::haveDerivation() /* If they are all valid, then we're done. */ if (allValid && buildMode == bmNormal) { - done(BuildResult::AlreadyValid, std::move(validOutputs)); - return; + return done(BuildResult::AlreadyValid, std::move(validOutputs)); } /* We are first going to try to create the invalid output paths @@ -290,24 +287,24 @@ void DerivationGoal::haveDerivation() } } - if (waitees.empty()) /* to prevent hang (no wake-up event) */ - outputsSubstitutionTried(); - else + if (waitees.empty()) { /* to prevent hang (no wake-up event) */ + return outputsSubstitutionTried(); + } else { state = &DerivationGoal::outputsSubstitutionTried; + return StillAlive{}; + } } - -void DerivationGoal::outputsSubstitutionTried() +Goal::WorkResult DerivationGoal::outputsSubstitutionTried() { trace("all outputs substituted (maybe)"); assert(drv->type().isPure()); if (nrFailed > 0 && nrFailed > nrNoSubstituters + nrIncompleteClosure && !settings.tryFallback) { - done(BuildResult::TransientFailure, {}, + return done(BuildResult::TransientFailure, {}, Error("some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ", worker.store.printStorePath(drvPath))); - return; } /* If the substitutes form an incomplete closure, then we should @@ -341,32 +338,29 @@ void DerivationGoal::outputsSubstitutionTried() if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) { needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed; - haveDerivation(); - return; + return haveDerivation(); } auto [allValid, validOutputs] = checkPathValidity(); if (buildMode == bmNormal && allValid) { - done(BuildResult::Substituted, std::move(validOutputs)); - return; + return done(BuildResult::Substituted, std::move(validOutputs)); } if (buildMode == bmRepair && allValid) { - repairClosure(); - return; + return repairClosure(); } if (buildMode == bmCheck && !allValid) throw Error("some outputs of '%s' are not valid, so checking is not possible", worker.store.printStorePath(drvPath)); /* Nothing to wait for; tail call */ - gaveUpOnSubstitution(); + return gaveUpOnSubstitution(); } /* At least one of the output paths could not be produced using a substitute. So we have to build instead. */ -void DerivationGoal::gaveUpOnSubstitution() +Goal::WorkResult DerivationGoal::gaveUpOnSubstitution() { /* At this point we are building all outputs, so if more are wanted there is no need to restart. */ @@ -427,14 +421,16 @@ void DerivationGoal::gaveUpOnSubstitution() addWaitee(worker.makePathSubstitutionGoal(i)); } - if (waitees.empty()) /* to prevent hang (no wake-up event) */ - inputsRealised(); - else + if (waitees.empty()) {/* to prevent hang (no wake-up event) */ + return inputsRealised(); + } else { state = &DerivationGoal::inputsRealised; + return StillAlive{}; + } } -void DerivationGoal::repairClosure() +Goal::WorkResult DerivationGoal::repairClosure() { assert(drv->type().isPure()); @@ -488,41 +484,39 @@ void DerivationGoal::repairClosure() } if (waitees.empty()) { - done(BuildResult::AlreadyValid, assertPathValidity()); - return; + return done(BuildResult::AlreadyValid, assertPathValidity()); } state = &DerivationGoal::closureRepaired; + return StillAlive{}; } -void DerivationGoal::closureRepaired() +Goal::WorkResult DerivationGoal::closureRepaired() { trace("closure repaired"); if (nrFailed > 0) throw Error("some paths in the output closure of derivation '%s' could not be repaired", worker.store.printStorePath(drvPath)); - done(BuildResult::AlreadyValid, assertPathValidity()); + return done(BuildResult::AlreadyValid, assertPathValidity()); } -void DerivationGoal::inputsRealised() +Goal::WorkResult DerivationGoal::inputsRealised() { trace("all inputs realised"); if (nrFailed != 0) { if (!useDerivation) throw Error("some dependencies of '%s' are missing", worker.store.printStorePath(drvPath)); - done(BuildResult::DependencyFailed, {}, Error( + return done(BuildResult::DependencyFailed, {}, Error( "%s dependencies of derivation '%s' failed to build", nrFailed, worker.store.printStorePath(drvPath))); - return; } if (retrySubstitution == RetrySubstitution::YesNeed) { retrySubstitution = RetrySubstitution::AlreadyRetried; - haveDerivation(); - return; + return haveDerivation(); } /* Gather information necessary for computing the closure and/or @@ -589,7 +583,7 @@ void DerivationGoal::inputsRealised() addWaitee(resolvedDrvGoal); state = &DerivationGoal::resolvedFinished; - return; + return StillAlive{}; } std::function::ChildNode &)> accumInputPaths; @@ -655,9 +649,10 @@ void DerivationGoal::inputsRealised() build hook. */ state = &DerivationGoal::tryToBuild; worker.wakeUp(shared_from_this()); + return StillAlive{}; } -void DerivationGoal::started() +Goal::WorkResult DerivationGoal::started() { auto msg = fmt( buildMode == bmRepair ? "repairing outputs of '%s'" : @@ -668,9 +663,10 @@ void 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{}; } -void DerivationGoal::tryToBuild() +Goal::WorkResult DerivationGoal::tryToBuild() { trace("trying to build"); @@ -706,7 +702,7 @@ void DerivationGoal::tryToBuild() actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, fmt("waiting for lock on %s", Magenta(showPaths(lockFiles)))); worker.waitForAWhile(shared_from_this()); - return; + return StillAlive{}; } actLock.reset(); @@ -723,8 +719,7 @@ void DerivationGoal::tryToBuild() if (buildMode != bmCheck && allValid) { debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath)); outputLocks.setDeletion(true); - done(BuildResult::AlreadyValid, std::move(validOutputs)); - return; + return done(BuildResult::AlreadyValid, std::move(validOutputs)); } /* If any of the outputs already exist but are not valid, delete @@ -751,8 +746,7 @@ void DerivationGoal::tryToBuild() actLock.reset(); buildResult.startTime = time(0); // inexact state = &DerivationGoal::buildDone; - started(); - return; + return started(); case rpPostpone: /* Not now; wait until at least one child finishes or the wake-up timeout expires. */ @@ -761,7 +755,7 @@ void DerivationGoal::tryToBuild() fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath)))); worker.waitForAWhile(shared_from_this()); outputLocks.unlock(); - return; + return StillAlive{}; case rpDecline: /* We should do it ourselves. */ break; @@ -772,9 +766,10 @@ void DerivationGoal::tryToBuild() state = &DerivationGoal::tryLocalBuild; worker.wakeUp(shared_from_this()); + return StillAlive{}; } -void DerivationGoal::tryLocalBuild() { +Goal::WorkResult DerivationGoal::tryLocalBuild() { throw Error( "unable to build with a primary store that isn't a local store; " "either pass a different '--store' or enable remote builds." @@ -932,7 +927,7 @@ void runPostBuildHook( proc.getStdout()->drainInto(sink); } -void DerivationGoal::buildDone() +Goal::WorkResult DerivationGoal::buildDone() { trace("build done"); @@ -1027,8 +1022,7 @@ void DerivationGoal::buildDone() outputLocks.setDeletion(true); outputLocks.unlock(); - done(BuildResult::Built, std::move(builtOutputs)); - + return done(BuildResult::Built, std::move(builtOutputs)); } catch (BuildError & e) { outputLocks.unlock(); @@ -1049,12 +1043,11 @@ void DerivationGoal::buildDone() BuildResult::PermanentFailure; } - done(st, {}, std::move(e)); - return; + return done(st, {}, std::move(e)); } } -void DerivationGoal::resolvedFinished() +Goal::WorkResult DerivationGoal::resolvedFinished() { trace("resolved derivation finished"); @@ -1122,7 +1115,7 @@ void DerivationGoal::resolvedFinished() if (status == BuildResult::AlreadyValid) status = BuildResult::ResolvesToAlreadyValid; - done(status, std::move(builtOutputs)); + return done(status, std::move(builtOutputs)); } HookReply DerivationGoal::tryBuildHook() @@ -1293,7 +1286,7 @@ bool DerivationGoal::isReadDesc(int fd) return fd == hook->builderOut.readSide.get(); } -void DerivationGoal::handleChildOutput(int fd, std::string_view data) +Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data) { // local & `ssh://`-builds are dealt with here. auto isWrittenToLog = isReadDesc(fd); @@ -1302,11 +1295,10 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data) logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { killChild(); - done( + return done( BuildResult::LogLimitExceeded, {}, Error("%s killed after writing more than %d bytes of log output", getName(), settings.maxLogSize)); - return; } for (auto c : data) @@ -1357,6 +1349,8 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data) } else currentHookLine += c; } + + return StillAlive{}; } @@ -1505,7 +1499,7 @@ SingleDrvOutputs DerivationGoal::assertPathValidity() } -void DerivationGoal::done( +Goal::Finished DerivationGoal::done( BuildResult::Status status, SingleDrvOutputs builtOutputs, std::optional ex) @@ -1540,7 +1534,7 @@ void DerivationGoal::done( fs << worker.store.printStorePath(drvPath) << "\t" << buildResult.toString() << std::endl; } - amDone(buildResult.success() ? ecSuccess : ecFailed, std::move(ex)); + return amDone(buildResult.success() ? ecSuccess : ecFailed, std::move(ex)); } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 28aa6afbe..268b717dd 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -188,7 +188,7 @@ struct DerivationGoal : public Goal */ std::optional derivationType; - typedef void (DerivationGoal::*GoalState)(); + typedef WorkResult (DerivationGoal::*GoalState)(); GoalState state; BuildMode buildMode; @@ -217,11 +217,11 @@ struct DerivationGoal : public Goal BuildMode buildMode = bmNormal); virtual ~DerivationGoal() noexcept(false); - void timedOut(Error && ex) override; + Finished timedOut(Error && ex) override; std::string key() override; - void work() override; + WorkResult work() override; /** * Add wanted outputs to an already existing derivation goal. @@ -231,18 +231,18 @@ struct DerivationGoal : public Goal /** * The states. */ - void getDerivation(); - void loadDerivation(); - void haveDerivation(); - void outputsSubstitutionTried(); - void gaveUpOnSubstitution(); - void closureRepaired(); - void inputsRealised(); - void tryToBuild(); - virtual void tryLocalBuild(); - void buildDone(); + WorkResult getDerivation(); + WorkResult loadDerivation(); + WorkResult haveDerivation(); + WorkResult outputsSubstitutionTried(); + WorkResult gaveUpOnSubstitution(); + WorkResult closureRepaired(); + WorkResult inputsRealised(); + WorkResult tryToBuild(); + virtual WorkResult tryLocalBuild(); + WorkResult buildDone(); - void resolvedFinished(); + WorkResult resolvedFinished(); /** * Is the build hook willing to perform the build? @@ -292,7 +292,7 @@ struct DerivationGoal : public Goal /** * Callback used by the worker to write to the log. */ - void handleChildOutput(int fd, std::string_view data) override; + WorkResult handleChildOutput(int fd, std::string_view data) override; void handleEOF(int fd) override; void flushLine(); @@ -323,11 +323,11 @@ struct DerivationGoal : public Goal */ virtual void killChild(); - void repairClosure(); + WorkResult repairClosure(); - void started(); + WorkResult started(); - void done( + Finished done( BuildResult::Status status, SingleDrvOutputs builtOutputs = {}, std::optional ex = {}); diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 94c9206a3..4d10543cd 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -20,21 +20,20 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal( } -void DrvOutputSubstitutionGoal::init() +Goal::WorkResult DrvOutputSubstitutionGoal::init() { trace("init"); /* If the derivation already exists, we’re done */ if (worker.store.queryRealisation(id)) { - amDone(ecSuccess); - return; + return amDone(ecSuccess); } subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list>(); - tryNext(); + return tryNext(); } -void DrvOutputSubstitutionGoal::tryNext() +Goal::WorkResult DrvOutputSubstitutionGoal::tryNext() { trace("trying next substituter"); @@ -43,7 +42,7 @@ void DrvOutputSubstitutionGoal::tryNext() prevents infinite waiting. */ if (worker.runningSubstitutions >= std::max(1U, settings.maxSubstitutionJobs.get())) { worker.waitForBuildSlot(shared_from_this()); - return; + return StillAlive{}; } maintainRunningSubstitutions = @@ -54,16 +53,14 @@ void DrvOutputSubstitutionGoal::tryNext() with it. */ debug("derivation output '%s' is required, but there is no substituter that can provide it", id.to_string()); - /* Hack: don't indicate failure if there were no substituters. - In that case the calling derivation should just do a - build. */ - amDone(substituterFailed ? ecFailed : ecNoSubstituters); - if (substituterFailed) { worker.failedSubstitutions++; } - return; + /* 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); } sub = subs.front(); @@ -85,9 +82,10 @@ void DrvOutputSubstitutionGoal::tryNext() worker.childStarted(shared_from_this(), {downloadState->outPipe.readSide.get()}, true, false); state = &DrvOutputSubstitutionGoal::realisationFetched; + return StillAlive{}; } -void DrvOutputSubstitutionGoal::realisationFetched() +Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched() { worker.childTerminated(this); maintainRunningSubstitutions.reset(); @@ -116,8 +114,7 @@ void DrvOutputSubstitutionGoal::realisationFetched() worker.store.printStorePath(localOutputInfo->outPath), worker.store.printStorePath(depPath) ); - tryNext(); - return; + return tryNext(); } addWaitee(worker.makeDrvOutputSubstitutionGoal(depId)); } @@ -125,29 +122,32 @@ void DrvOutputSubstitutionGoal::realisationFetched() addWaitee(worker.makePathSubstitutionGoal(outputInfo->outPath)); - if (waitees.empty()) outPathValid(); - else state = &DrvOutputSubstitutionGoal::outPathValid; + if (waitees.empty()) { + return outPathValid(); + } else { + state = &DrvOutputSubstitutionGoal::outPathValid; + return StillAlive{}; + } } -void DrvOutputSubstitutionGoal::outPathValid() +Goal::WorkResult DrvOutputSubstitutionGoal::outPathValid() { assert(outputInfo); trace("output path substituted"); if (nrFailed > 0) { debug("The output path of the derivation output '%s' could not be substituted", id.to_string()); - amDone(nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed); - return; + return amDone(nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed); } worker.store.registerDrvOutput(*outputInfo); - finished(); + return finished(); } -void DrvOutputSubstitutionGoal::finished() +Goal::WorkResult DrvOutputSubstitutionGoal::finished() { trace("finished"); - amDone(ecSuccess); + return amDone(ecSuccess); } std::string DrvOutputSubstitutionGoal::key() @@ -157,9 +157,9 @@ std::string DrvOutputSubstitutionGoal::key() return "a$" + std::string(id.to_string()); } -void DrvOutputSubstitutionGoal::work() +Goal::WorkResult DrvOutputSubstitutionGoal::work() { - (this->*state)(); + return (this->*state)(); } diff --git a/src/libstore/build/drv-output-substitution-goal.hh b/src/libstore/build/drv-output-substitution-goal.hh index 598b119dc..a28347f15 100644 --- a/src/libstore/build/drv-output-substitution-goal.hh +++ b/src/libstore/build/drv-output-substitution-goal.hh @@ -58,20 +58,20 @@ class DrvOutputSubstitutionGoal : public Goal { public: DrvOutputSubstitutionGoal(const DrvOutput& id, Worker & worker, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); - typedef void (DrvOutputSubstitutionGoal::*GoalState)(); + typedef WorkResult (DrvOutputSubstitutionGoal::*GoalState)(); GoalState state; - void init(); - void tryNext(); - void realisationFetched(); - void outPathValid(); - void finished(); + WorkResult init(); + WorkResult tryNext(); + WorkResult realisationFetched(); + WorkResult outPathValid(); + WorkResult finished(); - void timedOut(Error && ex) override { abort(); }; + Finished timedOut(Error && ex) override { abort(); }; std::string key() override; - void work() override; + WorkResult work() override; JobCategory jobCategory() const override { return JobCategory::Substitution; diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index 4db6af6e6..70ef5706e 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -45,7 +45,7 @@ void Goal::waiteeDone(GoalPtr waitee, ExitCode result) } -void Goal::amDone(ExitCode result, std::optional ex) +Goal::Finished Goal::amDone(ExitCode result, std::optional ex) { trace("done"); assert(!exitCode.has_value()); @@ -66,6 +66,7 @@ void Goal::amDone(ExitCode result, std::optional ex) worker.removeGoal(shared_from_this()); cleanup(); + return Finished{}; } diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 575621037..db20b5cdb 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -105,6 +105,15 @@ struct Goal : public std::enable_shared_from_this public: + struct StillAlive {}; + struct Finished {}; + + struct WorkResult : std::variant + { + WorkResult() = delete; + using variant::variant; + }; + /** * Exception containing an error message, if any. */ @@ -119,13 +128,13 @@ public: trace("goal destroyed"); } - virtual void work() = 0; + virtual WorkResult work() = 0; void addWaitee(GoalPtr waitee); virtual void waiteeDone(GoalPtr waitee, ExitCode result); - virtual void handleChildOutput(int fd, std::string_view data) + virtual WorkResult handleChildOutput(int fd, std::string_view data) { abort(); } @@ -146,11 +155,11 @@ public: * get rid of any running child processes that are being monitored * by the worker (important!), etc. */ - virtual void timedOut(Error && ex) = 0; + virtual Finished timedOut(Error && ex) = 0; virtual std::string key() = 0; - void amDone(ExitCode result, std::optional ex = {}); + Finished amDone(ExitCode result, std::optional ex = {}); virtual void cleanup() { } diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 2f1f338c1..fb5ccc6f1 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -161,7 +161,7 @@ void LocalDerivationGoal::killSandbox(bool getStats) } -void LocalDerivationGoal::tryLocalBuild() +Goal::WorkResult LocalDerivationGoal::tryLocalBuild() { #if __APPLE__ additionalSandboxProfile = parsedDrv->getStringAttr("__sandboxProfile").value_or(""); @@ -172,7 +172,7 @@ void LocalDerivationGoal::tryLocalBuild() state = &DerivationGoal::tryToBuild; worker.waitForBuildSlot(shared_from_this()); outputLocks.unlock(); - return; + return StillAlive{}; } assert(derivationType); @@ -215,7 +215,7 @@ void LocalDerivationGoal::tryLocalBuild() actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath)))); worker.waitForAWhile(shared_from_this()); - return; + return StillAlive{}; } } @@ -250,15 +250,14 @@ void LocalDerivationGoal::tryLocalBuild() outputLocks.unlock(); buildUser.reset(); worker.permanentFailure = true; - done(BuildResult::InputRejected, {}, std::move(e)); - return; + return done(BuildResult::InputRejected, {}, std::move(e)); } /* This state will be reached when we get EOF on the child's log pipe. */ state = &DerivationGoal::buildDone; - started(); + return started(); } diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 4ac18b974..237417b42 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -211,7 +211,7 @@ struct LocalDerivationGoal : public DerivationGoal /** * The additional states. */ - void tryLocalBuild() override; + WorkResult tryLocalBuild() override; /** * Start building a derivation. diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 027a7e161..2c5dfea84 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -25,7 +25,7 @@ PathSubstitutionGoal::~PathSubstitutionGoal() } -void PathSubstitutionGoal::done( +Goal::Finished PathSubstitutionGoal::done( ExitCode result, BuildResult::Status status, std::optional errorMsg) @@ -35,17 +35,17 @@ void PathSubstitutionGoal::done( debug(*errorMsg); buildResult.errorMsg = *errorMsg; } - amDone(result); + return amDone(result); } -void PathSubstitutionGoal::work() +Goal::WorkResult PathSubstitutionGoal::work() { - (this->*state)(); + return (this->*state)(); } -void PathSubstitutionGoal::init() +Goal::WorkResult PathSubstitutionGoal::init() { trace("init"); @@ -53,8 +53,7 @@ void PathSubstitutionGoal::init() /* If the path already exists we're done. */ if (!repair && worker.store.isValidPath(storePath)) { - done(ecSuccess, BuildResult::AlreadyValid); - return; + return done(ecSuccess, BuildResult::AlreadyValid); } if (settings.readOnlyMode) @@ -62,11 +61,11 @@ void PathSubstitutionGoal::init() subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list>(); - tryNext(); + return tryNext(); } -void PathSubstitutionGoal::tryNext() +Goal::WorkResult PathSubstitutionGoal::tryNext() { trace("trying next substituter"); @@ -75,20 +74,17 @@ void PathSubstitutionGoal::tryNext() if (subs.size() == 0) { /* None left. Terminate this goal and let someone else deal with it. */ + if (substituterFailed) { + worker.failedSubstitutions++; + } /* Hack: don't indicate failure if there were no substituters. In that case the calling derivation should just do a build. */ - done( + return done( substituterFailed ? ecFailed : ecNoSubstituters, BuildResult::NoSubstituters, fmt("path '%s' is required, but there is no substituter that can build it", worker.store.printStorePath(storePath))); - - if (substituterFailed) { - worker.failedSubstitutions++; - } - - return; } sub = subs.front(); @@ -101,27 +97,23 @@ void PathSubstitutionGoal::tryNext() if (sub->storeDir == worker.store.storeDir) assert(subPath == storePath); } else if (sub->storeDir != worker.store.storeDir) { - tryNext(); - return; + return tryNext(); } try { // FIXME: make async info = sub->queryPathInfo(subPath ? *subPath : storePath); } catch (InvalidPath &) { - tryNext(); - return; + return tryNext(); } catch (SubstituterDisabled &) { if (settings.tryFallback) { - tryNext(); - return; + return tryNext(); } throw; } catch (Error & e) { if (settings.tryFallback) { logError(e.info()); - tryNext(); - return; + return tryNext(); } throw; } @@ -134,8 +126,7 @@ void PathSubstitutionGoal::tryNext() } else { printError("asked '%s' for '%s' but got '%s'", sub->getUri(), worker.store.printStorePath(storePath), sub->printStorePath(info->path)); - tryNext(); - return; + return tryNext(); } } @@ -156,8 +147,7 @@ void PathSubstitutionGoal::tryNext() { warn("ignoring substitute for '%s' from '%s', as it's not signed by any of the keys in 'trusted-public-keys'", worker.store.printStorePath(storePath), sub->getUri()); - tryNext(); - return; + return tryNext(); } /* To maintain the closure invariant, we first have to realise the @@ -166,23 +156,24 @@ void PathSubstitutionGoal::tryNext() if (i != storePath) /* ignore self-references */ addWaitee(worker.makePathSubstitutionGoal(i)); - if (waitees.empty()) /* to prevent hang (no wake-up event) */ - referencesValid(); - else + if (waitees.empty()) {/* to prevent hang (no wake-up event) */ + return referencesValid(); + } else { state = &PathSubstitutionGoal::referencesValid; + return StillAlive{}; + } } -void PathSubstitutionGoal::referencesValid() +Goal::WorkResult PathSubstitutionGoal::referencesValid() { trace("all references realised"); if (nrFailed > 0) { - done( + return done( nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed, BuildResult::DependencyFailed, fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath))); - return; } for (auto & i : info->references) @@ -191,10 +182,11 @@ void PathSubstitutionGoal::referencesValid() state = &PathSubstitutionGoal::tryToRun; worker.wakeUp(shared_from_this()); + return StillAlive{}; } -void PathSubstitutionGoal::tryToRun() +Goal::WorkResult PathSubstitutionGoal::tryToRun() { trace("trying to run"); @@ -203,7 +195,7 @@ void PathSubstitutionGoal::tryToRun() prevents infinite waiting. */ if (worker.getNrSubstitutions() >= std::max(1U, (unsigned int) settings.maxSubstitutionJobs)) { worker.waitForBuildSlot(shared_from_this()); - return; + return StillAlive{}; } maintainRunningSubstitutions = std::make_unique>(worker.runningSubstitutions); @@ -236,10 +228,11 @@ void PathSubstitutionGoal::tryToRun() worker.childStarted(shared_from_this(), {outPipe.readSide.get()}, true, false); state = &PathSubstitutionGoal::finished; + return StillAlive{}; } -void PathSubstitutionGoal::finished() +Goal::WorkResult PathSubstitutionGoal::finished() { trace("substitute finished"); @@ -264,7 +257,7 @@ void PathSubstitutionGoal::finished() /* Try the next substitute. */ state = &PathSubstitutionGoal::tryNext; worker.wakeUp(shared_from_this()); - return; + return StillAlive{}; } worker.markContentsGood(storePath); @@ -285,12 +278,13 @@ void PathSubstitutionGoal::finished() worker.doneNarSize += maintainExpectedNar->delta; maintainExpectedNar.reset(); - done(ecSuccess, BuildResult::Substituted); + return done(ecSuccess, BuildResult::Substituted); } -void PathSubstitutionGoal::handleChildOutput(int fd, std::string_view data) +Goal::WorkResult PathSubstitutionGoal::handleChildOutput(int fd, std::string_view data) { + return StillAlive{}; } diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index d85b3beb3..89cf3c759 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -66,7 +66,7 @@ struct PathSubstitutionGoal : public Goal std::unique_ptr> maintainExpectedSubstitutions, maintainRunningSubstitutions, maintainExpectedNar, maintainExpectedDownload; - typedef void (PathSubstitutionGoal::*GoalState)(); + typedef WorkResult (PathSubstitutionGoal::*GoalState)(); GoalState state; /** @@ -74,7 +74,7 @@ struct PathSubstitutionGoal : public Goal */ std::optional ca; - void done( + Finished done( ExitCode result, BuildResult::Status status, std::optional errorMsg = {}); @@ -83,7 +83,7 @@ public: PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); ~PathSubstitutionGoal(); - void timedOut(Error && ex) override { abort(); }; + Finished timedOut(Error && ex) override { abort(); }; /** * We prepend "a$" to the key name to ensure substitution goals @@ -94,22 +94,22 @@ public: return "a$" + std::string(storePath.name()) + "$" + worker.store.printStorePath(storePath); } - void work() override; + WorkResult work() override; /** * The states. */ - void init(); - void tryNext(); - void gotInfo(); - void referencesValid(); - void tryToRun(); - void finished(); + WorkResult init(); + WorkResult tryNext(); + WorkResult gotInfo(); + WorkResult referencesValid(); + WorkResult tryToRun(); + WorkResult finished(); /** * Callback used by the worker to write to the log. */ - void handleChildOutput(int fd, std::string_view data) override; + WorkResult handleChildOutput(int fd, std::string_view data) override; /* Called by destructor, can't be overridden */ void cleanup() override final; -- cgit v1.2.3