Merge pull request #13862 from obsidiansystems/build-failure-content-vs-presentation

Properly separater builder failure content and presentation

src/libstore/build/derivation-building-goal.cc

@@ -633,11 +633,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                 {
                     goal.closeLogFile();
                 }
-
-                void appendLogTailErrorMsg(std::string & msg) override
-                {
-                    goal.appendLogTailErrorMsg(msg);
-                }
             };
 
             auto * localStoreP = dynamic_cast<LocalStore *>(&worker.store);
@@ -725,6 +720,9 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
     SingleDrvOutputs builtOutputs;
     try {
         builtOutputs = builder->unprepareBuild();
+    } catch (BuilderFailureError & e) {
+        outputLocks.unlock();
+        co_return doneFailure(fixupBuilderFailureErrorMessage(std::move(e)));
     } catch (BuildError & e) {
         outputLocks.unlock();
 // Allow selecting a subset of enum values
@@ -835,8 +833,16 @@ static void runPostBuildHook(
     });
 }
 
-void DerivationBuildingGoal::appendLogTailErrorMsg(std::string & msg)
+BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailureError e)
 {
+    auto msg =
+        fmt("Cannot build '%s'.\n"
+            "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".",
+            Magenta(worker.store.printStorePath(drvPath)),
+            statusToString(e.builderStatus));
+
+    msg += showKnownOutputs(worker.store, *drv);
+
     if (!logger->isVerbose() && !logTail.empty()) {
         msg += fmt("\nLast %d log lines:\n", logTail.size());
         for (auto & line : logTail) {
@@ -853,6 +859,10 @@ void DerivationBuildingGoal::appendLogTailErrorMsg(std::string & msg)
                 nixLogCommand,
                 worker.store.printStorePath(drvPath));
     }
+
+    msg += e.extraMsgAfter;
+
+    return BuildError{e.status, msg};
 }
 
 Goal::Co DerivationBuildingGoal::hookDone()
@@ -893,21 +903,13 @@ Goal::Co DerivationBuildingGoal::hookDone()
 
     /* Check the exit status. */
     if (!statusOk(status)) {
-        auto msg =
-            fmt("Cannot build '%s'.\n"
-                "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".",
-                Magenta(worker.store.printStorePath(drvPath)),
-                statusToString(status));
-
-        msg += showKnownOutputs(worker.store, *drv);
-
-        appendLogTailErrorMsg(msg);
+        auto e = fixupBuilderFailureErrorMessage({BuildResult::MiscFailure, status, ""});
 
         outputLocks.unlock();
 
         /* TODO (once again) support fine-grained error codes, see issue #12641. */
 
-        co_return doneFailure(BuildError{BuildResult::MiscFailure, msg});
+        co_return doneFailure(std::move(e));
     }
 
     /* Compute the FS closure of the outputs and register them as

src/libstore/include/nix/store/build/derivation-builder.hh

@@ -12,6 +12,29 @@
 
 namespace nix {
 
+/**
+ * Denotes a build failure that stemmed from the builder exiting with a
+ * failing exist status.
+ */
+struct BuilderFailureError : BuildError
+{
+    int builderStatus;
+
+    std::string extraMsgAfter;
+
+    BuilderFailureError(BuildResult::Status status, int builderStatus, std::string extraMsgAfter)
+        : BuildError{
+            status,
+              /* No message for now, because the caller will make for
+                 us, with extra context */
+              "",
+          }
+        , builderStatus{std::move(builderStatus)}
+        , extraMsgAfter{std::move(extraMsgAfter)}
+    {
+    }
+};
+
 /**
  * Stuff we need to pass to initChild().
  */
@@ -94,8 +117,6 @@ struct DerivationBuilderCallbacks
      */
     virtual void closeLogFile() = 0;
 
-    virtual void appendLogTailErrorMsg(std::string & msg) = 0;
-
     /**
      * Hook up `builderOut` to some mechanism to ingest the log
      *

src/libstore/include/nix/store/build/derivation-building-goal.hh

@@ -14,6 +14,7 @@ namespace nix {
 
 using std::map;
 
+struct BuilderFailureError;
 #ifndef _WIN32 // TODO enable build hook on Windows
 struct HookInstance;
 struct DerivationBuilder;
@@ -174,7 +175,7 @@ struct DerivationBuildingGoal : public Goal
 
     Done doneFailure(BuildError ex);
 
-    void appendLogTailErrorMsg(std::string & msg);
+    BuildError fixupBuilderFailureErrorMessage(BuilderFailureError msg);
 
     JobCategory jobCategory() const override
     {

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

@@ -535,26 +535,16 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
     /* Check the exit status. */
     if (!statusOk(status)) {
 
+        /* Check *before* cleaning up. */
         bool diskFull = decideWhetherDiskFull();
 
         cleanupBuild(false);
 
-        auto msg =
-            fmt("Cannot build '%s'.\n"
-                "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".",
-                Magenta(store.printStorePath(drvPath)),
-                statusToString(status));
-
-        msg += showKnownOutputs(store, drv);
-
-        miscMethods->appendLogTailErrorMsg(msg);
-
-        if (diskFull)
-            msg += "\nnote: build failure may have been caused by lack of free disk space";
-
-        throw BuildError(
+        throw BuilderFailureError{
             !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure : BuildResult::PermanentFailure,
-            msg);
+            status,
+            diskFull ? "\nnote: build failure may have been caused by lack of free disk space" : "",
+        };
     }
 
     /* Compute the FS closure of the outputs and register them as