diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 072bbfa93..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) { @@ -716,11 +718,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); @@ -786,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, @@ -795,24 +802,16 @@ 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(); - try { - - /* Okay, we have to build. */ - 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(); 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 45fbba3f5..7fad2837a 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; @@ -117,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 */ @@ -157,15 +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. + * @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. + * + * @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 bool prepareBuild() = 0; - - /** - * Start building a derivation. - */ - virtual void 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 de0b46295..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; - - void 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; } -void 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. */ @@ -841,9 +834,10 @@ void DerivationBuilderImpl::startBuilder() startChild(); pid.setSeparatePG(true); - miscMethods->childStarted(builderOut.get()); processSandboxSetupMessages(); + + return builderOut.get(); } PathsInChroot DerivationBuilderImpl::getPathsInSandbox()