From f81c06accfd58ff1649877d25b74ed89c993e5d4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 14 Mar 2025 15:37:59 -0400 Subject: [PATCH] Gut `LocalDerivationGoal::tryLocalBuild` Now, most of it is in two new functions: `LocalDerivationGoal::{,un}repareBuild`. This might seems like a step backwards from coroutines --- now we have more functions, and are stuck with class vars --- but I don't think it needs to be. There's a few options here: - (Re)introduce coroutines for the isolated building logic. We could use the same coroutines types, or simpler ones specialized to this use-case. The `tryLocalBuild` caller can still use `Goal::Co`, and just will manually "pump" this inner coroutine. - Return closures from each step. This is sort of like coroutines by hand, but it still allows us to stop writing down the local variables in each type. Being able to fully-use RAII again would be very nice! - Keep top-level first-order functions like now, but make more functional. Instead of having one state object (`DerivationBuilder`) for all steps (setup, run, teardown), we can have separate structs for the live variables at each point we consume and return. This at least avoids "are these variables active at this time?" questions, but doesn't give us the full benefit of RAII as we must manually ensure FIFO create/destroy orders still. One thing to note is that by keeping the `outputLock` unlocking in `tryLocalBuild`, we are arguably uncovering a rebuild scheduling vs building distinction, as the output locks are pretty squarely a scheduling concern. It's nice that the builder doesn't need to know about them at all. --- .../unix/build/local-derivation-goal.cc | 109 ++++++++++++------ 1 file changed, 76 insertions(+), 33 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 859056f10..8c74ed4e1 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -252,11 +252,32 @@ struct LocalDerivationGoal : DerivationGoal, RestrictionContext */ Goal::Co tryLocalBuild() override; + /** + * Set up build environment / sandbox, acquiring resources (e.g. + * locks as needed). After this is run, the builder should be + * started. + * + * @returns true if successful, false if we could not acquire a build + * user. In that case, the caller must wait and then try again. + */ + bool prepareBuild(); + /** * Start building a derivation. */ void startBuilder(); + /** + * Tear down build environment after the builder exits (either on + * its own or if it is killed). + * + * @returns The first case indicates failure during output + * processing. A status code and exception are returned, providing + * more information. The second case indicates success, and + * realisations for each output of the derivation are returned. + */ + std::variant, SingleDrvOutputs> unprepareBuild(); + /** * Fill in the environment for the builder. */ @@ -488,6 +509,53 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() co_return tryToBuild(); } + if (!prepareBuild()) { + if (!actLock) + actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, + fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath)))); + co_await waitForAWhile(); + co_return tryLocalBuild(); + } + + actLock.reset(); + + try { + + /* Okay, we have to build. */ + startBuilder(); + + } catch (BuildError & e) { + outputLocks.unlock(); + buildUser.reset(); + worker.permanentFailure = true; + co_return done(BuildResult::InputRejected, {}, std::move(e)); + } + + started(); + co_await Suspend{}; + + trace("build done"); + + auto res = unprepareBuild(); + // N.B. cannot use `std::visit` with co-routine return + if (auto * ste = std::get_if<0>(&res)) { + outputLocks.unlock(); + co_return done(std::move(ste->first), {}, std::move(ste->second)); + } else if (auto * builtOutputs = std::get_if<1>(&res)) { + /* 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)); + } else { + unreachable(); + } +} + +bool LocalDerivationGoal::prepareBuild() +{ /* Cache this */ derivationType = drv->type(); @@ -535,33 +603,16 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() buildUser = acquireUserLock(drvOptions->useUidRange(*drv) ? 65536 : 1, useChroot); if (!buildUser) { - if (!actLock) - actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, - fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath)))); - co_await waitForAWhile(); - co_return tryLocalBuild(); + return false; } } - actLock.reset(); + return true; +} - try { - - /* Okay, we have to build. */ - startBuilder(); - - } catch (BuildError & e) { - outputLocks.unlock(); - buildUser.reset(); - worker.permanentFailure = true; - co_return done(BuildResult::InputRejected, {}, std::move(e)); - } - - started(); - co_await Suspend{}; - - trace("build done"); +std::variant, SingleDrvOutputs> LocalDerivationGoal::unprepareBuild() +{ Finally releaseBuildUser([&](){ /* Release the build user at the end of this function. We don't do it right away because we don't want another build grabbing this @@ -654,18 +705,9 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() deleteTmpDir(true); - /* 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)); + return std::move(builtOutputs); } catch (BuildError & e) { - outputLocks.unlock(); - assert(derivationType); BuildResult::Status st = dynamic_cast(&e) ? BuildResult::NotDeterministic : @@ -673,10 +715,11 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : BuildResult::PermanentFailure; - co_return done(st, {}, std::move(e)); + return std::pair{std::move(st), std::move(e)}; } } + static void chmod_(const Path & path, mode_t mode) { if (chmod(path.c_str(), mode) == -1)