From 8825bfa7fe9acdf549faafda6242c3cee6f281de Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 28 Aug 2025 17:34:09 -0400 Subject: [PATCH] Properly separater builer failure content and presentation Before, had a very ugly `appendLogTailErrorMsg` callback. Now, we instead have a `fixupBuilderFailureErrorMessage` that is just used by `DerivationBuildingGoal`, and `DerivationBuilder` just returns the raw data needed by this. --- .../build/derivation-building-goal.cc | 34 ++++++++++--------- .../nix/store/build/derivation-builder.hh | 25 ++++++++++++-- .../store/build/derivation-building-goal.hh | 3 +- src/libstore/unix/build/derivation-builder.cc | 20 +++-------- 4 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index b1920cadb..6aab48a80 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -632,11 +632,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild() { goal.closeLogFile(); } - - void appendLogTailErrorMsg(std::string & msg) override - { - goal.appendLogTailErrorMsg(msg); - } }; auto * localStoreP = dynamic_cast(&worker.store); @@ -773,6 +768,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 @@ -883,8 +881,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) { @@ -901,6 +907,10 @@ void DerivationBuildingGoal::appendLogTailErrorMsg(std::string & msg) nixLogCommand, worker.store.printStorePath(drvPath)); } + + msg += e.extraMsgAfter; + + return BuildError{e.status, msg}; } Goal::Co DerivationBuildingGoal::hookDone() @@ -941,21 +951,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 diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index a373c4729..4a3993b83 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -11,6 +11,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(). */ @@ -120,8 +143,6 @@ struct DerivationBuilderCallbacks */ virtual void closeLogFile() = 0; - virtual void appendLogTailErrorMsg(std::string & msg) = 0; - /** * Hook up `builderOut` to some mechanism to ingest the log * diff --git a/src/libstore/include/nix/store/build/derivation-building-goal.hh b/src/libstore/include/nix/store/build/derivation-building-goal.hh index 38f0fc7bf..162cf14ad 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/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 { diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index bf99c4c1a..60509560d 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -534,26 +534,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