From 7f3314a68cf250163b2a61691100739536a6bb99 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 3 Sep 2025 17:27:07 -0400 Subject: [PATCH 1/3] `DerivationBuilder::initialOutputs` make `const` At one point I remember it did mutatate `initialOutputs`, but not anymore! --- src/libstore/include/nix/store/build/derivation-builder.hh | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index 45fbba3f5..deb4612b4 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -76,10 +76,7 @@ struct DerivationBuilderParams */ const StorePathSet & inputPaths; - /** - * @note we do in fact mutate this - */ - std::map & initialOutputs; + const std::map & initialOutputs; const BuildMode & buildMode; From 14c206f05a3f1b080cce457a67e54aa587867a5f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 3 Sep 2025 17:33:48 -0400 Subject: [PATCH 2/3] `DerivationBuilder` no more callback soup for logging `startBuilder` just returns the descriptor for the pipe now. --- src/libstore/build/derivation-building-goal.cc | 10 ++++------ .../include/nix/store/build/derivation-builder.hh | 11 +++-------- src/libstore/unix/build/derivation-builder.cc | 7 ++++--- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 072bbfa93..4760c039b 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -716,11 +716,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild() ~DerivationBuildingGoalCallbacks() override = default; - void childStarted(Descriptor builderOut) override - { - goal.worker.childStarted(goal.shared_from_this(), {builderOut}, true, true); - } - void childTerminated() override { goal.worker.childTerminated(&goal); @@ -802,10 +797,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() actLock.reset(); + Descriptor builderOut; try { /* Okay, we have to build. */ - builder->startBuilder(); + builderOut = builder->startBuilder(); } catch (BuildError & e) { builder.reset(); @@ -814,6 +810,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild() co_return doneFailure(std::move(e)); // InputRejected } + worker.childStarted(shared_from_this(), {builderOut}, true, true); + started(); co_await Suspend{}; diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index deb4612b4..e8aefa377 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -114,13 +114,6 @@ struct DerivationBuilderCallbacks */ virtual void closeLogFile() = 0; - /** - * Hook up `builderOut` to some mechanism to ingest the log - * - * @todo this should be reworked - */ - virtual void childStarted(Descriptor builderOut) = 0; - /** * @todo this should be reworked */ @@ -161,8 +154,10 @@ struct DerivationBuilder : RestrictionContext /** * Start building a derivation. + * + * @return logging pipe */ - virtual void startBuilder() = 0; + virtual Descriptor startBuilder() = 0; /** * Tear down build environment after the builder exits (either on diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index de0b46295..577385093 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -216,7 +216,7 @@ public: bool prepareBuild() override; - void startBuilder() override; + Descriptor startBuilder() override; SingleDrvOutputs unprepareBuild() override; @@ -679,7 +679,7 @@ static bool checkNotWorldWritable(std::filesystem::path path) return true; } -void DerivationBuilderImpl::startBuilder() +Descriptor DerivationBuilderImpl::startBuilder() { /* Make sure that no other processes are executing under the sandbox uids. This must be done before any chownToBuilder() @@ -841,9 +841,10 @@ void DerivationBuilderImpl::startBuilder() startChild(); pid.setSeparatePG(true); - miscMethods->childStarted(builderOut.get()); processSandboxSetupMessages(); + + return builderOut.get(); } PathsInChroot DerivationBuilderImpl::getPathsInSandbox() From 2acb9559d531a952d779970fc5f2ccd536d8d272 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 3 Sep 2025 17:58:50 -0400 Subject: [PATCH 3/3] Combine `DerivationBuilder::{prepareBuild,startBuilder}` After many other cleanups, it turns out there is no reason for these to be separate methods. We can combine them to simplify things. --- .../build/derivation-building-goal.cc | 31 ++++++++++--------- .../nix/store/build/derivation-builder.hh | 16 +++++----- src/libstore/unix/build/derivation-builder.cc | 27 ++++++---------- 3 files changed, 33 insertions(+), 41 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 4760c039b..ebef2a375 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -689,6 +689,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild() #else assert(!hook); + Descriptor builderOut; + // Will continue here while waiting for a build user below while (true) { @@ -781,7 +783,17 @@ Goal::Co DerivationBuildingGoal::tryToBuild() }); } - if (!builder->prepareBuild()) { + std::optional builderOutOpt; + try { + /* Okay, we have to build. */ + builderOutOpt = builder->startBuild(); + } catch (BuildError & e) { + builder.reset(); + outputLocks.unlock(); + worker.permanentFailure = true; + co_return doneFailure(std::move(e)); // InputRejected + } + if (!builderOutOpt) { if (!actLock) actLock = std::make_unique( *logger, @@ -790,26 +802,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild() fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath)))); co_await waitForAWhile(); continue; - } + } else { + builderOut = *std::move(builderOutOpt); + }; break; } actLock.reset(); - Descriptor builderOut; - try { - - /* Okay, we have to build. */ - builderOut = builder->startBuilder(); - - } catch (BuildError & e) { - builder.reset(); - outputLocks.unlock(); - worker.permanentFailure = true; - co_return doneFailure(std::move(e)); // InputRejected - } - worker.childStarted(shared_from_this(), {builderOut}, true, true); started(); diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index e8aefa377..7fad2837a 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -147,17 +147,15 @@ struct DerivationBuilder : RestrictionContext * 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. - */ - virtual bool prepareBuild() = 0; - - /** - * Start building a derivation. + * @returns logging pipe if successful, `std::nullopt` if we could + * not acquire a build user. In that case, the caller must wait and + * then try again. * - * @return logging pipe + * @note "success" just means that we were able to set up the environment + * and start the build. The builder could have immediately exited with + * failure, and that would still be considered a successful start. */ - virtual Descriptor startBuilder() = 0; + virtual std::optional startBuild() = 0; /** * Tear down build environment after the builder exits (either on diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 577385093..d6979ab5f 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -214,9 +214,7 @@ protected: public: - bool prepareBuild() override; - - Descriptor startBuilder() override; + std::optional startBuild() override; SingleDrvOutputs unprepareBuild() override; @@ -470,19 +468,6 @@ bool DerivationBuilderImpl::killChild() return ret; } -bool DerivationBuilderImpl::prepareBuild() -{ - if (useBuildUsers()) { - if (!buildUser) - buildUser = getBuildUser(); - - if (!buildUser) - return false; - } - - return true; -} - SingleDrvOutputs DerivationBuilderImpl::unprepareBuild() { /* Since we got an EOF on the logger pipe, the builder is presumed @@ -679,8 +664,16 @@ static bool checkNotWorldWritable(std::filesystem::path path) return true; } -Descriptor DerivationBuilderImpl::startBuilder() +std::optional DerivationBuilderImpl::startBuild() { + if (useBuildUsers()) { + if (!buildUser) + buildUser = getBuildUser(); + + if (!buildUser) + return std::nullopt; + } + /* Make sure that no other processes are executing under the sandbox uids. This must be done before any chownToBuilder() calls. */