Merge pull request #13906 from obsidiansystems/derivation-builder-simpler

More `DerivationBuilder` simplifications

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<Descriptor> 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<Activity>(
                     *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{};

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<std::string, InitialOutput> & initialOutputs;
+    const std::map<std::string, InitialOutput> & 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<Descriptor> startBuild() = 0;
 
     /**
      * Tear down build environment after the builder exits (either on

src/libstore/unix/build/derivation-builder.cc

@@ -214,9 +214,7 @@ protected:
 
 public:
 
-    bool prepareBuild() override;
-
-    void startBuilder() override;
+    std::optional<Descriptor> 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<Descriptor> 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()