diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 415d63d876c..9ae24b44c45 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -583,16 +583,12 @@ static void throwBuildErrors(std::vector & buildResults, const auto failedResult = failed.begin(); if (failedResult != failed.end()) { if (failed.size() == 1) { - failedResult->second->rethrow(); + throw *failedResult->second; } else { StringSet failedPaths; for (; failedResult != failed.end(); failedResult++) { - if (!failedResult->second->errorMsg.empty()) { - logError( - ErrorInfo{ - .level = lvlError, - .msg = failedResult->second->errorMsg, - }); + if (!failedResult->second->message().empty()) { + logError(failedResult->second->info()); } failedPaths.insert(failedResult->first->path.to_string(store)); } diff --git a/src/libstore-c/nix_api_store.cc b/src/libstore-c/nix_api_store.cc index d164efd596d..158c142836c 100644 --- a/src/libstore-c/nix_api_store.cc +++ b/src/libstore-c/nix_api_store.cc @@ -182,10 +182,8 @@ nix_err nix_store_realise( assert(results.size() == 1); // Check if any builds failed - for (auto & result : results) { - if (auto * failureP = result.tryGetFailure()) - failureP->rethrow(); - } + for (auto & result : results) + result.tryThrowBuildError(); if (callback) { for (const auto & result : results) { diff --git a/src/libstore-tests/build-result.cc b/src/libstore-tests/build-result.cc index 85e799c2a73..b7e8f83f9e7 100644 --- a/src/libstore-tests/build-result.cc +++ b/src/libstore-tests/build-result.cc @@ -1,6 +1,7 @@ #include #include "nix/store/build-result.hh" +#include "nix/util/tests/characterization.hh" #include "nix/util/tests/json-characterization.hh" namespace nix { @@ -44,22 +45,22 @@ INSTANTIATE_TEST_SUITE_P( std::pair{ "not-deterministic", BuildResult{ - .inner{BuildResult::Failure{ + .inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", + .msg = HintFmt("no idea why"), .isNonDeterministic = false, // Note: This field is separate from the status - }}, + }}}, .timesBuilt = 1, }, }, std::pair{ "output-rejected", BuildResult{ - .inner{BuildResult::Failure{ + .inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", + .msg = HintFmt("no idea why"), .isNonDeterministic = false, - }}, + }}}, .timesBuilt = 3, .startTime = 30, .stopTime = 50, diff --git a/src/libstore-tests/serve-protocol.cc b/src/libstore-tests/serve-protocol.cc index 4be4b006015..b7be7b56d65 100644 --- a/src/libstore-tests/serve-protocol.cc +++ b/src/libstore-tests/serve-protocol.cc @@ -147,14 +147,14 @@ VERSIONED_READ_CHARACTERIZATION_TEST( VERSIONED_CHARACTERIZATION_TEST(ServeProtoTest, buildResult_2_2, "build-result-2.2", 2 << 8 | 2, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{.inner{BuildResult::Failure{ + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, - BuildResult{.inner{BuildResult::Failure{ + .msg = HintFmt("no idea why"), + }}}}, + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, BuildResult{.inner{BuildResult::Success{ .status = BuildResult::Success::Built, }}}, @@ -165,16 +165,16 @@ VERSIONED_CHARACTERIZATION_TEST(ServeProtoTest, buildResult_2_2, "build-result-2 VERSIONED_CHARACTERIZATION_TEST(ServeProtoTest, buildResult_2_3, "build-result-2.3", 2 << 8 | 3, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{.inner{BuildResult::Failure{ + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, BuildResult{ - .inner{BuildResult::Failure{ + .inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", + .msg = HintFmt("no idea why"), .isNonDeterministic = true, - }}, + }}}, .timesBuilt = 3, .startTime = 30, .stopTime = 50, @@ -194,16 +194,16 @@ VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, buildResult_2_6, "build-result-2.6", 2 << 8 | 6, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{.inner{BuildResult::Failure{ + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, BuildResult{ - .inner{BuildResult::Failure{ + .inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", + .msg = HintFmt("no idea why"), .isNonDeterministic = true, - }}, + }}}, .timesBuilt = 3, .startTime = 30, .stopTime = 50, diff --git a/src/libstore-tests/worker-protocol.cc b/src/libstore-tests/worker-protocol.cc index ac4aafe7bf3..ea3be80efc0 100644 --- a/src/libstore-tests/worker-protocol.cc +++ b/src/libstore-tests/worker-protocol.cc @@ -200,14 +200,14 @@ VERSIONED_READ_CHARACTERIZATION_TEST( VERSIONED_CHARACTERIZATION_TEST(WorkerProtoTest, buildResult_1_27, "build-result-1.27", 1 << 8 | 27, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{.inner{BuildResult::Failure{ + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, - BuildResult{.inner{BuildResult::Failure{ + .msg = HintFmt("no idea why"), + }}}}, + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, BuildResult{.inner{BuildResult::Success{ .status = BuildResult::Success::Built, }}}, @@ -219,14 +219,14 @@ VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, buildResult_1_28, "build-result-1.28", 1 << 8 | 28, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{.inner{BuildResult::Failure{ + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, - BuildResult{.inner{BuildResult::Failure{ + .msg = HintFmt("no idea why"), + }}}}, + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, BuildResult{.inner{BuildResult::Success{ .status = BuildResult::Success::Built, .builtOutputs = @@ -265,16 +265,16 @@ VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, buildResult_1_29, "build-result-1.29", 1 << 8 | 29, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{.inner{BuildResult::Failure{ + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, BuildResult{ - .inner{BuildResult::Failure{ + .inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", + .msg = HintFmt("no idea why"), .isNonDeterministic = true, - }}, + }}}, .timesBuilt = 3, .startTime = 30, .stopTime = 50, @@ -324,16 +324,16 @@ VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, buildResult_1_37, "build-result-1.37", 1 << 8 | 37, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{.inner{BuildResult::Failure{ + BuildResult{.inner{BuildResult::Failure{{ .status = BuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, BuildResult{ - .inner{BuildResult::Failure{ + .inner{BuildResult::Failure{{ .status = BuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", + .msg = HintFmt("no idea why"), .isNonDeterministic = true, - }}, + }}}, .timesBuilt = 3, .startTime = 30, .stopTime = 50, @@ -385,22 +385,22 @@ VERSIONED_CHARACTERIZATION_TEST(WorkerProtoTest, keyedBuildResult_1_29, "keyed-b using namespace std::literals::chrono_literals; std::tuple t{ KeyedBuildResult{ - {.inner{BuildResult::Failure{ + BuildResult{.inner{KeyedBuildResult::Failure{{ .status = KeyedBuildResult::Failure::OutputRejected, - .errorMsg = "no idea why", - }}}, + .msg = HintFmt("no idea why"), + }}}}, /* .path = */ DerivedPath::Opaque{ StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-xxx"}, }, }, KeyedBuildResult{ - { - .inner{BuildResult::Failure{ + BuildResult{ + .inner{KeyedBuildResult::Failure{{ .status = KeyedBuildResult::Failure::NotDeterministic, - .errorMsg = "no idea why", + .msg = HintFmt("no idea why"), .isNonDeterministic = true, - }}, + }}}, .timesBuilt = 3, .startTime = 30, .stopTime = 50, diff --git a/src/libstore/build-result.cc b/src/libstore/build-result.cc index 19fb0024949..5abb451ac83 100644 --- a/src/libstore/build-result.cc +++ b/src/libstore/build-result.cc @@ -10,9 +10,6 @@ std::strong_ordering BuildResult::operator<=>(const BuildResult &) const noexcep bool BuildResult::Success::operator==(const BuildResult::Success &) const noexcept = default; std::strong_ordering BuildResult::Success::operator<=>(const BuildResult::Success &) const noexcept = default; -bool BuildResult::Failure::operator==(const BuildResult::Failure &) const noexcept = default; -std::strong_ordering BuildResult::Failure::operator<=>(const BuildResult::Failure &) const noexcept = default; - static constexpr std::array, 4> successStatusStrings{{ #define ENUM_ENTRY(e) {BuildResult::Success::e, #e} ENUM_ENTRY(Built), @@ -75,9 +72,18 @@ static BuildResult::Failure::Status failureStatusFromString(std::string_view str throw Error("unknown built result failure status '%s'", str); } -[[noreturn]] void BuildResult::Failure::rethrow() const +bool BuildError::operator==(const BuildError & other) const noexcept +{ + return status == other.status && isNonDeterministic == other.isNonDeterministic && message() == other.message(); +} + +std::strong_ordering BuildError::operator<=>(const BuildError & other) const noexcept { - throw BuildError(status, "%s", errorMsg); + if (auto cmp = status <=> other.status; cmp != 0) + return cmp; + if (auto cmp = isNonDeterministic <=> other.isNonDeterministic; cmp != 0) + return cmp; + return message() <=> other.message(); } } // namespace nix @@ -113,7 +119,7 @@ void adl_serializer::to_json(json & res, const BuildResult & br) [&](const BuildResult::Failure & failure) { res["success"] = false; res["status"] = failureStatusToString(failure.status); - res["errorMsg"] = failure.errorMsg; + res["errorMsg"] = failure.message(); res["isNonDeterministic"] = failure.isNonDeterministic; }, }, @@ -148,11 +154,11 @@ BuildResult adl_serializer::from_json(const json & _json) s.builtOutputs = valueAt(json, "builtOutputs"); br.inner = std::move(s); } else { - BuildResult::Failure f; - f.status = failureStatusFromString(statusStr); - f.errorMsg = getString(valueAt(json, "errorMsg")); - f.isNonDeterministic = getBoolean(valueAt(json, "isNonDeterministic")); - br.inner = std::move(f); + br.inner = BuildResult::Failure{{ + .status = failureStatusFromString(statusStr), + .msg = HintFmt(getString(valueAt(json, "errorMsg"))), + .isNonDeterministic = getBoolean(valueAt(json, "isNonDeterministic")), + }}; } return br; diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index b161bfb9900..cca80feb49a 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -1210,13 +1210,7 @@ Goal::Done DerivationBuildingGoal::doneFailure(BuildError ex) worker.updateProgress(); - return Goal::doneFailure( - ecFailed, - BuildResult::Failure{ - .status = ex.status, - .errorMsg = fmt("%s", Uncolored(ex.info().msg)), - }, - std::move(ex)); + return Goal::doneFailure(ecFailed, std::move(ex)); } } // namespace nix diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 4c536bb1d90..2142c5e5b03 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -252,7 +252,7 @@ Goal::Co DerivationGoal::haveDerivation(bool storeDerivation) auto g = worker.makeDerivationBuildingGoal(drvPath, *drv, buildMode, storeDerivation); /* We will finish with it ourselves, as if we were the derivational goal. */ - g->preserveException = true; + g->preserveFailure = true; { Goals waitees; @@ -312,7 +312,7 @@ Goal::Co DerivationGoal::haveDerivation(bool storeDerivation) } } - co_return amDone(g->exitCode, g->ex); + co_return amDone(g->exitCode); } Goal::Co DerivationGoal::repairClosure() @@ -501,13 +501,7 @@ Goal::Done DerivationGoal::doneFailure(BuildError ex) worker.updateProgress(); - return Goal::doneFailure( - ecFailed, - BuildResult::Failure{ - .status = ex.status, - .errorMsg = fmt("%s", Uncolored(ex.info().msg)), - }, - std::move(ex)); + return Goal::doneFailure(ecFailed, std::move(ex)); } } // namespace nix diff --git a/src/libstore/build/derivation-resolution-goal.cc b/src/libstore/build/derivation-resolution-goal.cc index 6cb9702f4f6..e205fc774bb 100644 --- a/src/libstore/build/derivation-resolution-goal.cc +++ b/src/libstore/build/derivation-resolution-goal.cc @@ -90,7 +90,12 @@ Goal::Co DerivationResolutionGoal::resolveDerivation() nrFailed, nrFailed == 1 ? "dependency" : "dependencies"); msg += showKnownOutputs(worker.store, *drv); - co_return amDone(ecFailed, {BuildError(BuildResult::Failure::DependencyFailed, msg)}); + co_return doneFailure( + ecFailed, + BuildResult::Failure{{ + .status = BuildResult::Failure::DependencyFailed, + .msg = HintFmt(msg), + }}); } /* Gather information necessary for computing the closure and/or @@ -185,7 +190,7 @@ Goal::Co DerivationResolutionGoal::resolveDerivation() } } - co_return amDone(ecSuccess, std::nullopt); + co_return amDone(ecSuccess); } } // namespace nix diff --git a/src/libstore/build/derivation-trampoline-goal.cc b/src/libstore/build/derivation-trampoline-goal.cc index cfa0c538f95..58a43c043a2 100644 --- a/src/libstore/build/derivation-trampoline-goal.cc +++ b/src/libstore/build/derivation-trampoline-goal.cc @@ -100,11 +100,10 @@ Goal::Co DerivationTrampolineGoal::init() if (nrFailed != 0) { co_return doneFailure( ecFailed, - BuildResult::Failure{ + BuildResult::Failure{{ .status = BuildResult::Failure::DependencyFailed, - .errorMsg = fmt("failed to obtain derivation of '%s'", drvReq->to_string(worker.store)), - }, - Error("failed to obtain derivation of '%s'", drvReq->to_string(worker.store))); + .msg = HintFmt("failed to obtain derivation of '%s'", drvReq->to_string(worker.store)), + }}); } StorePath drvPath = resolveDerivedPath(worker.store, *drvReq); @@ -152,7 +151,7 @@ Goal::Co DerivationTrampolineGoal::haveDerivation(StorePath drvPath, Derivation for (auto & output : resolvedWantedOutputs) { auto g = upcast_goal(worker.makeDerivationGoal(drvPath, drv, output, buildMode, false)); - g->preserveException = true; + g->preserveFailure = true; /* We will finish with it ourselves, as if we were the derivational goal. */ concreteDrvGoals.insert(std::move(g)); } @@ -170,7 +169,7 @@ Goal::Co DerivationTrampolineGoal::haveDerivation(StorePath drvPath, Derivation for (auto && [x, y] : successP2->builtOutputs) successP->builtOutputs.insert_or_assign(x, y); - co_return amDone(g->exitCode, g->ex); + co_return amDone(g->exitCode); } } // namespace nix diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 04b16f5a8b1..774f00c0e83 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -18,13 +18,13 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod worker.run(goals); StringSet failed; - std::optional ex; + BuildResult::Failure * failure = nullptr; for (auto & i : goals) { - if (i->ex) { - if (ex) - logError(i->ex->info()); + if (auto * f = i->buildResult.tryGetFailure()) { + if (failure) + logError(f->info()); else - ex = std::move(i->ex); + failure = f; } if (i->exitCode != Goal::ecSuccess) { if (auto i2 = dynamic_cast(i.get())) @@ -34,12 +34,12 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod } } - if (failed.size() == 1 && ex) { - ex->withExitStatus(worker.failingExitStatus()); - throw std::move(*ex); + if (failed.size() == 1 && failure) { + failure->withExitStatus(worker.failingExitStatus()); + throw *failure; } else if (!failed.empty()) { - if (ex) - logError(ex->info()); + if (failure) + logError(failure->info()); throw Error(worker.failingExitStatus(), "build of %s failed", concatStringsSep(", ", quoteStrings(failed))); } } @@ -88,10 +88,11 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat worker.run(Goals{goal}); return goal->buildResult; } catch (Error & e) { - return BuildResult{.inner{BuildResult::Failure{ - .status = BuildResult::Failure::MiscFailure, - .errorMsg = e.msg(), - }}}; + return BuildResult{ + .inner = BuildResult::Failure{{ + .status = BuildResult::Failure::MiscFailure, + .msg = e.msg(), + }}}; }; } @@ -108,12 +109,8 @@ void Store::ensurePath(const StorePath & path) worker.run(goals); if (goal->exitCode != Goal::ecSuccess) { - if (goal->ex) { - goal->ex->withExitStatus(worker.failingExitStatus()); - throw std::move(*goal->ex); - } else - throw Error( - worker.failingExitStatus(), "path '%s' does not exist and cannot be created", printStorePath(path)); + goal->buildResult.tryThrowBuildError(worker.failingExitStatus()); + throw Error(worker.failingExitStatus(), "path '%s' does not exist and cannot be created", printStorePath(path)); } } diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index d119384da3f..618a7294cab 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -189,14 +189,14 @@ Goal::Done Goal::doneSuccess(BuildResult::Success success) return amDone(ecSuccess); } -Goal::Done Goal::doneFailure(ExitCode result, BuildResult::Failure failure, std::optional ex) +Goal::Done Goal::doneFailure(ExitCode result, BuildResult::Failure failure) { assert(result == ecFailed || result == ecNoSubstituters); buildResult.inner = std::move(failure); - return amDone(result, std::move(ex)); + return amDone(result); } -Goal::Done Goal::amDone(ExitCode result, std::optional ex) +Goal::Done Goal::amDone(ExitCode result) { trace("done"); assert(top_co); @@ -204,11 +204,15 @@ Goal::Done Goal::amDone(ExitCode result, std::optional ex) assert(result == ecSuccess || result == ecFailed || result == ecNoSubstituters); exitCode = result; - if (ex) { - if (!preserveException && !waiters.empty()) - logError(ex->info()); - else - this->ex = std::move(*ex); + // Log the failure if we have one and shouldn't preserve it. + // Only log for actual failures (ecFailed), not for ecNoSubstituters + // which indicates "couldn't substitute, will try building" - that's + // expected behavior, not an error. + if (result == ecFailed) { + if (auto * failure = buildResult.tryGetFailure()) { + if (!preserveFailure && !waiters.empty()) + logError(failure->info()); + } } for (auto & i : waiters) { diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index e833f75c2ef..3ee6b898732 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -26,25 +26,6 @@ PathSubstitutionGoal::~PathSubstitutionGoal() cleanup(); } -Goal::Done PathSubstitutionGoal::doneSuccess(BuildResult::Success::Status status) -{ - return Goal::doneSuccess( - BuildResult::Success{ - .status = status, - }); -} - -Goal::Done PathSubstitutionGoal::doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg) -{ - debug(errorMsg); - return Goal::doneFailure( - result, - BuildResult::Failure{ - .status = status, - .errorMsg = std::move(errorMsg), - }); -} - Goal::Co PathSubstitutionGoal::init() { trace("init"); @@ -53,7 +34,7 @@ Goal::Co PathSubstitutionGoal::init() /* If the path already exists we're done. */ if (!repair && worker.store.isValidPath(storePath)) { - co_return doneSuccess(BuildResult::Success::AlreadyValid); + co_return doneSuccess(BuildResult::Success{.status = BuildResult::Success::AlreadyValid}); } if (settings.readOnlyMode) @@ -175,9 +156,12 @@ Goal::Co PathSubstitutionGoal::init() build. */ co_return doneFailure( substituterFailed ? ecFailed : ecNoSubstituters, - BuildResult::Failure::NoSubstituters, - fmt("path '%s' is required, but there is no substituter that can build it", - worker.store.printStorePath(storePath))); + BuildResult::Failure{{ + .status = BuildResult::Failure::NoSubstituters, + .msg = HintFmt( + "path '%s' is required, but there is no substituter that can build it", + worker.store.printStorePath(storePath)), + }}); } Goal::Co PathSubstitutionGoal::tryToRun( @@ -188,8 +172,11 @@ Goal::Co PathSubstitutionGoal::tryToRun( if (nrFailed > 0) { co_return doneFailure( nrNoSubstituters > 0 ? ecNoSubstituters : ecFailed, - BuildResult::Failure::DependencyFailed, - fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath))); + BuildResult::Failure{{ + .status = BuildResult::Failure::DependencyFailed, + .msg = HintFmt( + "some references of path '%s' could not be realised", worker.store.printStorePath(storePath)), + }}); } for (auto & i : info->references) @@ -316,7 +303,7 @@ Goal::Co PathSubstitutionGoal::tryToRun( worker.updateProgress(); - co_return doneSuccess(BuildResult::Success::Substituted); + co_return doneSuccess(BuildResult::Success{.status = BuildResult::Success::Substituted}); } void PathSubstitutionGoal::cleanup() diff --git a/src/libstore/include/nix/store/build-result.hh b/src/libstore/include/nix/store/build-result.hh index 134a90abbc5..d3be9cc5065 100644 --- a/src/libstore/include/nix/store/build-result.hh +++ b/src/libstore/include/nix/store/build-result.hh @@ -7,6 +7,8 @@ #include "nix/store/derived-path.hh" #include "nix/store/realisation.hh" +#include "nix/util/error.hh" +#include "nix/util/fmt.hh" #include "nix/util/json-impls.hh" namespace nix { @@ -50,6 +52,70 @@ enum struct BuildResultFailureStatus : uint8_t { HashMismatch, }; +/** + * Denotes a permanent build failure. + * + * This is both an exception type (inherits from Error) and serves as + * the failure variant in BuildResult::inner. + */ +struct BuildError : public Error +{ + using Status = BuildResultFailureStatus; + using enum Status; + + Status status = MiscFailure; + + /** + * If timesBuilt > 1, whether some builds did not produce the same + * result. (Note that 'isNonDeterministic = false' does not mean + * the build is deterministic, just that we don't have evidence of + * non-determinism.) + */ + bool isNonDeterministic = false; + +public: + /** + * Variadic constructor for throwing with format strings. + * Delegates to the string constructor after formatting. + */ + template + BuildError(Status status, const Args &... args) + : Error(args...) + , status{status} + { + } + + struct Args + { + Status status; + HintFmt msg; + bool isNonDeterministic = false; + }; + + /** + * Constructor taking a pre-formatted error message. + * Also used for deserialization. + */ + BuildError(Args args) + : Error(std::move(args.msg)) + , status{args.status} + , isNonDeterministic{args.isNonDeterministic} + + { + } + + /** + * Default constructor for deserialization. + */ + BuildError() + : Error("") + { + } + + bool operator==(const BuildError &) const noexcept; + std::strong_ordering operator<=>(const BuildError &) const noexcept; +}; + struct BuildResult { struct Success @@ -68,33 +134,10 @@ struct BuildResult std::strong_ordering operator<=>(const BuildResult::Success &) const noexcept; }; - struct Failure - { - using Status = enum BuildResultFailureStatus; - using enum Status; - Status status = MiscFailure; - - /** - * Information about the error if the build failed. - * - * @todo This should be an entire ErrorInfo object, not just a - * string, for richer information. - */ - std::string errorMsg; - - /** - * If timesBuilt > 1, whether some builds did not produce the same - * result. (Note that 'isNonDeterministic = false' does not mean - * the build is deterministic, just that we don't have evidence of - * non-determinism.) - */ - bool isNonDeterministic = false; - - bool operator==(const BuildResult::Failure &) const noexcept; - std::strong_ordering operator<=>(const BuildResult::Failure &) const noexcept; - - [[noreturn]] void rethrow() const; - }; + /** + * Failure is now an alias for BuildError. + */ + using Failure = BuildError; std::variant inner = Failure{}; @@ -118,6 +161,19 @@ struct BuildResult return std::get_if(&self.inner); } + /** + * Throw the build error if this result represents a failure. + * Optionally set the exit status on the error before throwing. + */ + void tryThrowBuildError(std::optional exitStatus = std::nullopt) + { + if (auto * failure = tryGetFailure()) { + if (exitStatus) + failure->withExitStatus(*exitStatus); + throw *failure; + } + } + /** * How many times this build was performed. */ @@ -138,20 +194,6 @@ struct BuildResult std::strong_ordering operator<=>(const BuildResult &) const noexcept; }; -/** - * denotes a permanent build failure - */ -struct BuildError : public Error -{ - BuildResult::Failure::Status status; - - BuildError(BuildResult::Failure::Status status, auto &&... args) - : Error{args...} - , status{status} - { - } -}; - /** * A `BuildResult` together with its "primary key". */ diff --git a/src/libstore/include/nix/store/build/goal.hh b/src/libstore/include/nix/store/build/goal.hh index f1df869ec8f..6a92d282898 100644 --- a/src/libstore/include/nix/store/build/goal.hh +++ b/src/libstore/include/nix/store/build/goal.hh @@ -504,7 +504,7 @@ protected: * Prefer using `doneSuccess` or `doneFailure` instead, which ensure * `buildResult` is set correctly. */ - Done amDone(ExitCode result, std::optional ex = {}); + Done amDone(ExitCode result); /** * Signals successful completion of the goal. @@ -518,15 +518,14 @@ protected: * * @param result The exit code (ecFailed or ecNoSubstituters) * @param failure The failure details including status and error message - * @param ex Optional exception to store/log */ - Done doneFailure(ExitCode result, BuildResult::Failure failure, std::optional ex = {}); + Done doneFailure(ExitCode result, BuildResult::Failure failure); public: virtual void cleanup() {} /** - * Hack to say that this goal should not log `ex`, but instead keep + * Hack to say that this goal should not log the failure, but instead keep * it around. Set by a waitee which sees itself as the designated * continuation of this goal, responsible for reporting its * successes or failures. @@ -534,12 +533,7 @@ public: * @todo this is yet another not-nice hack in the goal system that * we ought to get rid of. See #11927 */ - bool preserveException = false; - - /** - * Exception containing an error message, if any. - */ - std::optional ex; + bool preserveFailure = false; Goal(Worker & worker, Co init) : worker(worker) diff --git a/src/libstore/include/nix/store/build/substitution-goal.hh b/src/libstore/include/nix/store/build/substitution-goal.hh index 9f5315b462b..7ce28d1deff 100644 --- a/src/libstore/include/nix/store/build/substitution-goal.hh +++ b/src/libstore/include/nix/store/build/substitution-goal.hh @@ -41,10 +41,6 @@ struct PathSubstitutionGoal : public Goal */ std::optional ca; - Done doneSuccess(BuildResult::Success::Status status); - - Done doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg); - public: PathSubstitutionGoal( const StorePath & storePath, diff --git a/src/libstore/restricted-store.cc b/src/libstore/restricted-store.cc index c0bd273322f..15b2a3d049c 100644 --- a/src/libstore/restricted-store.cc +++ b/src/libstore/restricted-store.cc @@ -257,8 +257,7 @@ void RestrictedStore::buildPaths( const std::vector & paths, BuildMode buildMode, std::shared_ptr evalStore) { for (auto & result : buildPathsWithResults(paths, buildMode, evalStore)) - if (auto * failureP = result.tryGetFailure()) - failureP->rethrow(); + result.tryThrowBuildError(); } std::vector RestrictedStore::buildPathsWithResults( diff --git a/src/libstore/serve-protocol.cc b/src/libstore/serve-protocol.cc index 74404233d27..a1250fee450 100644 --- a/src/libstore/serve-protocol.cc +++ b/src/libstore/serve-protocol.cc @@ -18,13 +18,16 @@ BuildResult ServeProto::Serialise::read(const StoreDirConfig & stor { BuildResult res; BuildResult::Success success; - BuildResult::Failure failure; + + // Temp variables for failure fields since BuildError uses methods + std::string errorMsg; + bool isNonDeterministic = false; auto status = ServeProto::Serialise::read(store, {conn.from}); - conn.from >> failure.errorMsg; + conn.from >> errorMsg; if (GET_PROTOCOL_MINOR(conn.version) >= 3) - conn.from >> res.timesBuilt >> failure.isNonDeterministic >> res.startTime >> res.stopTime; + conn.from >> res.timesBuilt >> isNonDeterministic >> res.startTime >> res.stopTime; if (GET_PROTOCOL_MINOR(conn.version) >= 6) { auto builtOutputs = ServeProto::Serialise::read(store, conn); for (auto && [output, realisation] : builtOutputs) @@ -38,8 +41,11 @@ BuildResult ServeProto::Serialise::read(const StoreDirConfig & stor return std::move(success); }, [&](BuildResult::Failure::Status s) -> decltype(res.inner) { - failure.status = s; - return std::move(failure); + return BuildResult::Failure{{ + .status = s, + .msg = HintFmt(std::move(errorMsg)), + .isNonDeterministic = isNonDeterministic, + }}; }, }, status); @@ -70,7 +76,7 @@ void ServeProto::Serialise::write( overloaded{ [&](const BuildResult::Failure & failure) { ServeProto::write(store, {conn.to}, BuildResultStatus{failure.status}); - common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){}); + common(failure.message(), failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){}); }, [&](const BuildResult::Success & success) { ServeProto::write(store, {conn.to}, BuildResultStatus{success.status}); diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 83dfba7e097..ed8b41ca454 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -59,6 +59,7 @@ struct NotDeterministic : BuildError NotDeterministic(auto &&... args) : BuildError(BuildResult::Failure::NotDeterministic, args...) { + isNonDeterministic = true; } }; diff --git a/src/libstore/worker-protocol.cc b/src/libstore/worker-protocol.cc index 9dc4bc8c98a..a78bb87bebe 100644 --- a/src/libstore/worker-protocol.cc +++ b/src/libstore/worker-protocol.cc @@ -208,13 +208,16 @@ BuildResult WorkerProto::Serialise::read(const StoreDirConfig & sto { BuildResult res; BuildResult::Success success; - BuildResult::Failure failure; + + // Temp variables for failure fields since BuildError uses methods + std::string errorMsg; + bool isNonDeterministic = false; auto status = WorkerProto::Serialise::read(store, {conn.from}); - conn.from >> failure.errorMsg; + conn.from >> errorMsg; if (GET_PROTOCOL_MINOR(conn.version) >= 29) { - conn.from >> res.timesBuilt >> failure.isNonDeterministic >> res.startTime >> res.stopTime; + conn.from >> res.timesBuilt >> isNonDeterministic >> res.startTime >> res.stopTime; } if (GET_PROTOCOL_MINOR(conn.version) >= 37) { res.cpuUser = WorkerProto::Serialise>::read(store, conn); @@ -233,8 +236,11 @@ BuildResult WorkerProto::Serialise::read(const StoreDirConfig & sto return std::move(success); }, [&](BuildResult::Failure::Status s) -> decltype(res.inner) { - failure.status = s; - return std::move(failure); + return BuildResult::Failure{{ + .status = s, + .msg = HintFmt(std::move(errorMsg)), + .isNonDeterministic = isNonDeterministic, + }}; }, }, status); @@ -270,7 +276,7 @@ void WorkerProto::Serialise::write( overloaded{ [&](const BuildResult::Failure & failure) { WorkerProto::write(store, {conn.to}, BuildResultStatus{failure.status}); - common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){}); + common(failure.message(), failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){}); }, [&](const BuildResult::Success & success) { WorkerProto::write(store, {conn.to}, BuildResultStatus{success.status}); diff --git a/src/libutil/include/nix/util/error.hh b/src/libutil/include/nix/util/error.hh index 17de21ce5bc..74bb3e4f46a 100644 --- a/src/libutil/include/nix/util/error.hh +++ b/src/libutil/include/nix/util/error.hh @@ -159,7 +159,7 @@ public: } /** The error message without "error: " prefixed to it. */ - std::string message() + std::string message() const { return err.msg.str(); } diff --git a/src/nix/build-remote/build-remote.cc b/src/nix/build-remote/build-remote.cc index 40502d19313..cc0baee327a 100644 --- a/src/nix/build-remote/build-remote.cc +++ b/src/nix/build-remote/build-remote.cc @@ -333,7 +333,7 @@ static int main_build_remote(int argc, char ** argv) : ""); } throw Error( - "build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, failureP->errorMsg); + "build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, failureP->message()); } } else { copyClosure(*store, *sshStore, StorePathSet{*drvPath}, NoRepair, NoCheckSigs, substitute); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 32bd9124d12..07868c37933 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -824,12 +824,12 @@ struct CmdFlakeCheck : FlakeCommand "failed to build attribute '%s', build of '%s' failed: %s", attrPath.to_string(*state), result.path.to_string(*store), - failure->errorMsg)); + failure->message())); } } else { // Derivation has no attribute path (e.g., a build dependency) reportError( - Error("build of '%s' failed: %s", result.path.to_string(*store), failure->errorMsg)); + Error("build of '%s' failed: %s", result.path.to_string(*store), failure->message())); } } }