From e87ba8570546fc8b22f73cb4dfd1a60ebb80f63c Mon Sep 17 00:00:00 2001 From: Las Date: Mon, 10 Mar 2025 21:08:35 +0000 Subject: [PATCH] Inline `buildDone` from `DerivationGoal` into use sites The basic idea is that while we have duplicated this function, we now have one call-site in the local build case, and one call site in the build hook case. This unlocks big opportunities to specialize each copy, since they really shouldn't be doing the same things. By the time we are are done, there should not be much duplication left. See #12628 for further info. --- src/libstore/build/derivation-goal.cc | 4 +- src/libstore/build/derivation-goal.hh | 9 +- .../unix/build/local-derivation-goal.cc | 110 +++++++++++++++++- 3 files changed, 118 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 4a91e9811..6f69321b7 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -640,7 +640,7 @@ Goal::Co DerivationGoal::tryToBuild() buildResult.startTime = time(0); // inexact started(); co_await Suspend{}; - co_return buildDone(); + co_return hookDone(); case rpPostpone: /* Not now; wait until at least one child finishes or the wake-up timeout expires. */ @@ -935,7 +935,7 @@ void appendLogTailErrorMsg( } -Goal::Co DerivationGoal::buildDone() +Goal::Co DerivationGoal::hookDone() { trace("build done"); diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index fc4de9bdd..52830bbfc 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -55,6 +55,13 @@ struct InitialOutput { std::optional known; }; +/** Used internally */ +void runPostBuildHook( + Store & store, + Logger & logger, + const StorePath & drvPath, + const StorePathSet & outputPaths); + /** Used internally */ void appendLogTailErrorMsg( Worker & worker, @@ -246,7 +253,7 @@ struct DerivationGoal : public Goal Co gaveUpOnSubstitution(); Co tryToBuild(); virtual Co tryLocalBuild(); - Co buildDone(); + Co hookDone(); Co resolvedFinished(); diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 61a36dd51..80e339a53 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -263,8 +263,114 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() started(); co_await Suspend{}; - // after EOF on child - co_return buildDone(); + + trace("build done"); + + Finally releaseBuildUser([&](){ this->cleanupHookFinally(); }); + + cleanupPreChildKill(); + + /* Since we got an EOF on the logger pipe, the builder is presumed + to have terminated. In fact, the builder could also have + simply have closed its end of the pipe, so just to be sure, + kill it. */ + int status = getChildStatus(); + + debug("builder process for '%s' finished", worker.store.printStorePath(drvPath)); + + buildResult.timesBuilt++; + buildResult.stopTime = time(0); + + /* So the child is gone now. */ + worker.childTerminated(this); + + /* Close the read side of the logger pipe. */ + closeReadPipes(); + + /* Close the log file. */ + closeLogFile(); + + cleanupPostChildKill(); + + if (buildResult.cpuUser && buildResult.cpuSystem) { + debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", + worker.store.printStorePath(drvPath), + status, + ((double) buildResult.cpuUser->count()) / 1000000, + ((double) buildResult.cpuSystem->count()) / 1000000); + } + + bool diskFull = false; + + try { + + /* Check the exit status. */ + if (!statusOk(status)) { + + diskFull |= cleanupDecideWhetherDiskFull(); + + auto msg = fmt("builder for '%s' %s", + Magenta(worker.store.printStorePath(drvPath)), + statusToString(status)); + + appendLogTailErrorMsg(worker, drvPath, logTail, msg); + + if (diskFull) + msg += "\nnote: build failure may have been caused by lack of free disk space"; + + throw BuildError(msg); + } + + /* Compute the FS closure of the outputs and register them as + being valid. */ + auto builtOutputs = registerOutputs(); + + StorePathSet outputPaths; + for (auto & [_, output] : builtOutputs) + outputPaths.insert(output.outPath); + runPostBuildHook( + worker.store, + *logger, + drvPath, + outputPaths + ); + + cleanupPostOutputsRegisteredModeNonCheck(); + + /* It is now safe to delete the lock files, since all future + lockers will see that the output paths are valid; they will + not create new lock files with the same names as the old + (unlinked) lock files. */ + outputLocks.setDeletion(true); + outputLocks.unlock(); + + co_return done(BuildResult::Built, std::move(builtOutputs)); + + } catch (BuildError & e) { + outputLocks.unlock(); + + BuildResult::Status st = BuildResult::MiscFailure; + +#ifndef _WIN32 // TODO abstract over proc exit status + if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101) + st = BuildResult::TimedOut; + + else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) { + } + + else +#endif + { + assert(derivationType); + st = + dynamic_cast(&e) ? BuildResult::NotDeterministic : + statusOk(status) ? BuildResult::OutputRejected : + !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : + BuildResult::PermanentFailure; + } + + co_return done(st, {}, std::move(e)); + } } static void chmod_(const Path & path, mode_t mode)