From 2acb9559d531a952d779970fc5f2ccd536d8d272 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Wed, 3 Sep 2025 17:58:50 -0400
Subject: [PATCH] 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<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,
@@ -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<Descriptor> 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<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;
 }
 
-Descriptor 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. */