From 06af9cb532a3ae3d95cc6bf90484e0bb462cda22 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Wed, 12 Mar 2025 16:26:16 -0400
Subject: [PATCH] Inline the try-catch `BuildError` in the hook case

In the local building case, there is many things which can through
`BuildError`, but in the hook case there is just this one. We can
therefore simplify the code by "cinching" down the logic just to the
spot the error is thrown.

There is other code outside `libstore/build` which also uses
`BuildError`, but I believe those cases are mistakes. The point of
`BuildError` is the narrow technical use-cases of "errors which should
not be fatal with `--keep-going`". Using it outside the
building/scheduling code doesn't really make sense in that regard. It
seems likely that those usages were instead merely because "oh, this
error has something to do with building, so I guess `BuildError` is
better than `Error`".

It is quite likely that I myself used `BuildError` incorrectly as
described above :).
---
 src/libstore/build/derivation-goal.cc | 80 +++++++++++++--------------
 1 file changed, 38 insertions(+), 42 deletions(-)

diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
index 77ab559a6..4c91c65fb 100644
--- a/src/libstore/build/derivation-goal.cc
+++ b/src/libstore/build/derivation-goal.cc
@@ -922,51 +922,16 @@ Goal::Co DerivationGoal::hookDone()
     /* Close the log file. */
     closeLogFile();
 
-    try {
+    /* Check the exit status. */
+    if (!statusOk(status)) {
+        auto msg = fmt("builder for '%s' %s",
+            Magenta(worker.store.printStorePath(drvPath)),
+            statusToString(status));
 
-        /* Check the exit status. */
-        if (!statusOk(status)) {
-            auto msg = fmt("builder for '%s' %s",
-                Magenta(worker.store.printStorePath(drvPath)),
-                statusToString(status));
+        appendLogTailErrorMsg(worker, drvPath, logTail, msg);
 
-            appendLogTailErrorMsg(worker, drvPath, logTail, msg);
+        auto e = BuildError(msg);
 
-            throw BuildError(msg);
-        }
-
-        /* Compute the FS closure of the outputs and register them as
-           being valid. */
-        auto builtOutputs =
-            /* When using a build hook, the build hook can register the output
-               as valid (by doing `nix-store --import').  If so we don't have
-               to do anything here.
-
-               We can only early return when the outputs are known a priori. For
-               floating content-addressing derivations this isn't the case.
-             */
-            assertPathValidity();
-
-        StorePathSet outputPaths;
-        for (auto & [_, output] : builtOutputs)
-            outputPaths.insert(output.outPath);
-        runPostBuildHook(
-            worker.store,
-            *logger,
-            drvPath,
-            outputPaths
-        );
-
-        /* 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));
-
-    } catch (BuildError & e) {
         outputLocks.unlock();
 
         BuildResult::Status st = BuildResult::MiscFailure;
@@ -988,6 +953,37 @@ Goal::Co DerivationGoal::hookDone()
 
         co_return done(st, {}, std::move(e));
     }
+
+    /* Compute the FS closure of the outputs and register them as
+       being valid. */
+    auto builtOutputs =
+        /* When using a build hook, the build hook can register the output
+           as valid (by doing `nix-store --import').  If so we don't have
+           to do anything here.
+
+           We can only early return when the outputs are known a priori. For
+           floating content-addressing derivations this isn't the case.
+         */
+        assertPathValidity();
+
+    StorePathSet outputPaths;
+    for (auto & [_, output] : builtOutputs)
+        outputPaths.insert(output.outPath);
+    runPostBuildHook(
+        worker.store,
+        *logger,
+        drvPath,
+        outputPaths
+    );
+
+    /* 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));
 }
 
 Goal::Co DerivationGoal::resolvedFinished()