From e731c43eae9c08b8649708dcc5a76e8a99eda929 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 23 Sep 2025 18:09:56 -0400 Subject: [PATCH] Use `std::variant` to enforce `BuildResult` invariants There is now a clean separation between successful and failing build results. --- src/libcmd/installables.cc | 18 +- src/libstore-c/nix_api_store.cc | 8 +- src/libstore-tests/serve-protocol.cc | 108 +++++----- src/libstore-tests/worker-protocol.cc | 194 +++++++++--------- src/libstore/build-result.cc | 6 + .../build/derivation-building-goal.cc | 87 ++++---- src/libstore/build/derivation-check.cc | 12 +- src/libstore/build/derivation-goal.cc | 72 ++++--- .../build/derivation-trampoline-goal.cc | 9 +- src/libstore/build/entry-points.cc | 6 +- src/libstore/build/substitution-goal.cc | 32 +-- src/libstore/derivation-options.cc | 4 +- .../include/nix/store/build-result.hh | 174 ++++++++++------ .../nix/store/build/derivation-builder.hh | 2 +- .../store/build/derivation-building-goal.hh | 2 +- .../nix/store/build/derivation-goal.hh | 2 +- .../nix/store/build/substitution-goal.hh | 4 +- src/libstore/legacy-ssh-store.cc | 13 +- src/libstore/local-store.cc | 2 +- src/libstore/misc.cc | 2 +- src/libstore/posix-fs-canonicalise.cc | 2 +- src/libstore/remote-store.cc | 21 +- src/libstore/restricted-store.cc | 12 +- src/libstore/serve-protocol.cc | 60 ++++-- src/libstore/store-api.cc | 2 +- src/libstore/unix/build/derivation-builder.cc | 20 +- src/libstore/worker-protocol.cc | 68 ++++-- src/nix/build-remote/build-remote.cc | 17 +- .../functional/test-libstoreconsumer/main.cc | 6 +- 29 files changed, 568 insertions(+), 397 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 96ff06ad3..91ad74308 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -604,28 +604,28 @@ std::vector Installable::build( static void throwBuildErrors(std::vector & buildResults, const Store & store) { - std::vector failed; + std::vector> failed; for (auto & buildResult : buildResults) { - if (!buildResult.success()) { - failed.push_back(buildResult); + if (auto * failure = buildResult.tryGetFailure()) { + failed.push_back({&buildResult, failure}); } } auto failedResult = failed.begin(); if (failedResult != failed.end()) { if (failed.size() == 1) { - failedResult->rethrow(); + failedResult->second->rethrow(); } else { StringSet failedPaths; for (; failedResult != failed.end(); failedResult++) { - if (!failedResult->errorMsg.empty()) { + if (!failedResult->second->errorMsg.empty()) { logError( ErrorInfo{ .level = lvlError, - .msg = failedResult->errorMsg, + .msg = failedResult->second->errorMsg, }); } - failedPaths.insert(failedResult->path.to_string(store)); + failedPaths.insert(failedResult->first->path.to_string(store)); } throw Error("build of %s failed", concatStringsSep(", ", quoteStrings(failedPaths))); } @@ -695,12 +695,14 @@ std::vector, BuiltPathWithResult>> Installable::build auto buildResults = store->buildPathsWithResults(pathsToBuild, bMode, evalStore); throwBuildErrors(buildResults, *store); for (auto & buildResult : buildResults) { + // If we didn't throw, they must all be sucesses + auto & success = std::get(buildResult.inner); for (auto & aux : backmap[buildResult.path]) { std::visit( overloaded{ [&](const DerivedPath::Built & bfd) { std::map outputs; - for (auto & [outputName, realisation] : buildResult.builtOutputs) + for (auto & [outputName, realisation] : success.builtOutputs) outputs.emplace(outputName, realisation.outPath); res.push_back( {aux.installable, diff --git a/src/libstore-c/nix_api_store.cc b/src/libstore-c/nix_api_store.cc index c4c17f127..68b642d86 100644 --- a/src/libstore-c/nix_api_store.cc +++ b/src/libstore-c/nix_api_store.cc @@ -145,9 +145,11 @@ nix_err nix_store_realise( if (callback) { for (const auto & result : results) { - for (const auto & [outputName, realisation] : result.builtOutputs) { - StorePath p{realisation.outPath}; - callback(userdata, outputName.c_str(), &p); + if (auto * success = result.tryGetSuccess()) { + for (const auto & [outputName, realisation] : success->builtOutputs) { + StorePath p{realisation.outPath}; + callback(userdata, outputName.c_str(), &p); + } } } } diff --git a/src/libstore-tests/serve-protocol.cc b/src/libstore-tests/serve-protocol.cc index b513e1365..a63201164 100644 --- a/src/libstore-tests/serve-protocol.cc +++ b/src/libstore-tests/serve-protocol.cc @@ -127,17 +127,17 @@ VERSIONED_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{ - .status = BuildResult::OutputRejected, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, - BuildResult{ - .status = BuildResult::NotDeterministic, + }}}, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::NotDeterministic, .errorMsg = "no idea why", - }, - BuildResult{ - .status = BuildResult::Built, - }, + }}}, + BuildResult{.inner{BuildResult::Success{ + .status = BuildResult::Success::Built, + }}}, }; t; })) @@ -145,20 +145,24 @@ 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{ - .status = BuildResult::OutputRejected, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, + }}}, BuildResult{ - .status = BuildResult::NotDeterministic, - .errorMsg = "no idea why", + .inner{BuildResult::Failure{ + .status = BuildResult::Failure::NotDeterministic, + .errorMsg = "no idea why", + .isNonDeterministic = true, + }}, .timesBuilt = 3, - .isNonDeterministic = true, .startTime = 30, .stopTime = 50, }, BuildResult{ - .status = BuildResult::Built, + .inner{BuildResult::Success{ + .status = BuildResult::Success::Built, + }}, .startTime = 30, .stopTime = 50, }, @@ -170,48 +174,52 @@ VERSIONED_CHARACTERIZATION_TEST( ServeProtoTest, buildResult_2_6, "build-result-2.6", 2 << 8 | 6, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{ - .status = BuildResult::OutputRejected, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, + }}}, BuildResult{ - .status = BuildResult::NotDeterministic, - .errorMsg = "no idea why", + .inner{BuildResult::Failure{ + .status = BuildResult::Failure::NotDeterministic, + .errorMsg = "no idea why", + .isNonDeterministic = true, + }}, .timesBuilt = 3, - .isNonDeterministic = true, .startTime = 30, .stopTime = 50, }, BuildResult{ - .status = BuildResult::Built, + .inner{BuildResult::Success{ + .status = BuildResult::Success::Built, + .builtOutputs = + { + { + "foo", + { + .id = + DrvOutput{ + .drvHash = + Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), + .outputName = "foo", + }, + .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo"}, + }, + }, + { + "bar", + { + .id = + DrvOutput{ + .drvHash = + Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), + .outputName = "bar", + }, + .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar"}, + }, + }, + }, + }}, .timesBuilt = 1, - .builtOutputs = - { - { - "foo", - { - .id = - DrvOutput{ - .drvHash = - Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), - .outputName = "foo", - }, - .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo"}, - }, - }, - { - "bar", - { - .id = - DrvOutput{ - .drvHash = - Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), - .outputName = "bar", - }, - .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar"}, - }, - }, - }, .startTime = 30, .stopTime = 50, #if 0 diff --git a/src/libstore-tests/worker-protocol.cc b/src/libstore-tests/worker-protocol.cc index 823d8d85a..489151c8c 100644 --- a/src/libstore-tests/worker-protocol.cc +++ b/src/libstore-tests/worker-protocol.cc @@ -180,17 +180,17 @@ VERSIONED_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{ - .status = BuildResult::OutputRejected, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, - BuildResult{ - .status = BuildResult::NotDeterministic, + }}}, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::NotDeterministic, .errorMsg = "no idea why", - }, - BuildResult{ - .status = BuildResult::Built, - }, + }}}, + BuildResult{.inner{BuildResult::Success{ + .status = BuildResult::Success::Built, + }}}, }; t; })) @@ -199,16 +199,16 @@ VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, buildResult_1_28, "build-result-1.28", 1 << 8 | 28, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{ - .status = BuildResult::OutputRejected, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, - BuildResult{ - .status = BuildResult::NotDeterministic, + }}}, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::NotDeterministic, .errorMsg = "no idea why", - }, - BuildResult{ - .status = BuildResult::Built, + }}}, + BuildResult{.inner{BuildResult::Success{ + .status = BuildResult::Success::Built, .builtOutputs = { { @@ -236,7 +236,7 @@ VERSIONED_CHARACTERIZATION_TEST( }, }, }, - }, + }}}, }; t; })) @@ -245,48 +245,52 @@ VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, buildResult_1_29, "build-result-1.29", 1 << 8 | 29, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{ - .status = BuildResult::OutputRejected, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, + }}}, BuildResult{ - .status = BuildResult::NotDeterministic, - .errorMsg = "no idea why", + .inner{BuildResult::Failure{ + .status = BuildResult::Failure::NotDeterministic, + .errorMsg = "no idea why", + .isNonDeterministic = true, + }}, .timesBuilt = 3, - .isNonDeterministic = true, .startTime = 30, .stopTime = 50, }, BuildResult{ - .status = BuildResult::Built, + .inner{BuildResult::Success{ + .status = BuildResult::Success::Built, + .builtOutputs = + { + { + "foo", + { + .id = + DrvOutput{ + .drvHash = + Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), + .outputName = "foo", + }, + .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo"}, + }, + }, + { + "bar", + { + .id = + DrvOutput{ + .drvHash = + Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), + .outputName = "bar", + }, + .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar"}, + }, + }, + }, + }}, .timesBuilt = 1, - .builtOutputs = - { - { - "foo", - { - .id = - DrvOutput{ - .drvHash = - Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), - .outputName = "foo", - }, - .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo"}, - }, - }, - { - "bar", - { - .id = - DrvOutput{ - .drvHash = - Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), - .outputName = "bar", - }, - .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar"}, - }, - }, - }, .startTime = 30, .stopTime = 50, }, @@ -298,48 +302,52 @@ VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, buildResult_1_37, "build-result-1.37", 1 << 8 | 37, ({ using namespace std::literals::chrono_literals; std::tuple t{ - BuildResult{ - .status = BuildResult::OutputRejected, + BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, + }}}, BuildResult{ - .status = BuildResult::NotDeterministic, - .errorMsg = "no idea why", + .inner{BuildResult::Failure{ + .status = BuildResult::Failure::NotDeterministic, + .errorMsg = "no idea why", + .isNonDeterministic = true, + }}, .timesBuilt = 3, - .isNonDeterministic = true, .startTime = 30, .stopTime = 50, }, BuildResult{ - .status = BuildResult::Built, + .inner{BuildResult::Success{ + .status = BuildResult::Success::Built, + .builtOutputs = + { + { + "foo", + { + .id = + DrvOutput{ + .drvHash = + Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), + .outputName = "foo", + }, + .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo"}, + }, + }, + { + "bar", + { + .id = + DrvOutput{ + .drvHash = + Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), + .outputName = "bar", + }, + .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar"}, + }, + }, + }, + }}, .timesBuilt = 1, - .builtOutputs = - { - { - "foo", - { - .id = - DrvOutput{ - .drvHash = - Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), - .outputName = "foo", - }, - .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo"}, - }, - }, - { - "bar", - { - .id = - DrvOutput{ - .drvHash = - Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="), - .outputName = "bar", - }, - .outPath = StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar"}, - }, - }, - }, .startTime = 30, .stopTime = 50, .cpuUser = std::chrono::microseconds(500s), @@ -353,10 +361,10 @@ VERSIONED_CHARACTERIZATION_TEST(WorkerProtoTest, keyedBuildResult_1_29, "keyed-b using namespace std::literals::chrono_literals; std::tuple t{ KeyedBuildResult{ - { - .status = KeyedBuildResult::OutputRejected, + {.inner{BuildResult::Failure{ + .status = KeyedBuildResult::Failure::OutputRejected, .errorMsg = "no idea why", - }, + }}}, /* .path = */ DerivedPath::Opaque{ StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-xxx"}, @@ -364,10 +372,12 @@ VERSIONED_CHARACTERIZATION_TEST(WorkerProtoTest, keyedBuildResult_1_29, "keyed-b }, KeyedBuildResult{ { - .status = KeyedBuildResult::NotDeterministic, - .errorMsg = "no idea why", + .inner{BuildResult::Failure{ + .status = KeyedBuildResult::Failure::NotDeterministic, + .errorMsg = "no idea why", + .isNonDeterministic = true, + }}, .timesBuilt = 3, - .isNonDeterministic = true, .startTime = 30, .stopTime = 50, }, diff --git a/src/libstore/build-result.cc b/src/libstore/build-result.cc index 43c7adb11..ecbd27b49 100644 --- a/src/libstore/build-result.cc +++ b/src/libstore/build-result.cc @@ -5,4 +5,10 @@ namespace nix { bool BuildResult::operator==(const BuildResult &) const noexcept = default; std::strong_ordering BuildResult::operator<=>(const BuildResult &) const noexcept = default; +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; + } // namespace nix diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index ebef2a375..001816ca0 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -90,7 +90,7 @@ void DerivationBuildingGoal::timedOut(Error && ex) killChild(); // We're not inside a coroutine, hence we can't use co_return here. // Thus we ignore the return value. - [[maybe_unused]] Done _ = doneFailure({BuildResult::TimedOut, std::move(ex)}); + [[maybe_unused]] Done _ = doneFailure({BuildResult::Failure::TimedOut, std::move(ex)}); } /** @@ -205,7 +205,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() nrFailed, nrFailed == 1 ? "dependency" : "dependencies"); msg += showKnownOutputs(worker.store, *drv); - co_return doneFailure(BuildError(BuildResult::DependencyFailed, msg)); + co_return doneFailure(BuildError(BuildResult::Failure::DependencyFailed, msg)); } /* Gather information necessary for computing the closure and/or @@ -256,14 +256,18 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() return std::nullopt; auto & buildResult = (*mEntry)->buildResult; - if (!buildResult.success()) - return std::nullopt; + return std::visit( + overloaded{ + [](const BuildResult::Failure &) -> std::optional { return std::nullopt; }, + [&](const BuildResult::Success & success) -> std::optional { + auto i = get(success.builtOutputs, outputName); + if (!i) + return std::nullopt; - auto i = get(buildResult.builtOutputs, outputName); - if (!i) - return std::nullopt; - - return i->outPath; + return i->outPath; + }, + }, + buildResult.inner); }); if (!attempt) { /* TODO (impure derivations-induced tech debt) (see below): @@ -306,7 +310,9 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() auto resolvedResult = resolvedDrvGoal->buildResult; - if (resolvedResult.success()) { + // No `std::visit` for coroutines yet + if (auto * successP = resolvedResult.tryGetSuccess()) { + auto & success = *successP; SingleDrvOutputs builtOutputs; auto outputHashes = staticOutputHashes(worker.evalStore, *drv); @@ -324,7 +330,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() outputName); auto realisation = [&] { - auto take1 = get(resolvedResult.builtOutputs, outputName); + auto take1 = get(success.builtOutputs, outputName); if (take1) return *take1; @@ -360,18 +366,19 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() runPostBuildHook(worker.store, *logger, drvPath, outputPaths); - auto status = resolvedResult.status; - if (status == BuildResult::AlreadyValid) - status = BuildResult::ResolvesToAlreadyValid; + auto status = success.status; + if (status == BuildResult::Success::AlreadyValid) + status = BuildResult::Success::ResolvesToAlreadyValid; - co_return doneSuccess(status, std::move(builtOutputs)); - } else { + co_return doneSuccess(success.status, std::move(builtOutputs)); + } else if (resolvedResult.tryGetFailure()) { co_return doneFailure({ - BuildResult::DependencyFailed, + BuildResult::Failure::DependencyFailed, "build of resolved derivation '%s' failed", worker.store.printStorePath(pathResolved), }); - } + } else + assert(false); } /* If we get this far, we know no dynamic drvs inputs */ @@ -536,7 +543,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath)); outputLocks.setDeletion(true); outputLocks.unlock(); - co_return doneSuccess(BuildResult::AlreadyValid, std::move(validOutputs)); + co_return doneSuccess(BuildResult::Success::AlreadyValid, std::move(validOutputs)); } /* If any of the outputs already exist but are not valid, delete @@ -628,7 +635,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() /* Check the exit status. */ if (!statusOk(status)) { - auto e = fixupBuilderFailureErrorMessage({BuildResult::MiscFailure, status, ""}); + auto e = fixupBuilderFailureErrorMessage({BuildResult::Failure::MiscFailure, status, ""}); outputLocks.unlock(); @@ -669,7 +676,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() outputLocks.setDeletion(true); outputLocks.unlock(); - co_return doneSuccess(BuildResult::Built, std::move(builtOutputs)); + co_return doneSuccess(BuildResult::Success::Built, std::move(builtOutputs)); } co_await yield(); @@ -832,15 +839,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild() # pragma GCC diagnostic push # pragma GCC diagnostic ignored "-Wswitch-enum" switch (e.status) { - case BuildResult::HashMismatch: + case BuildResult::Failure::HashMismatch: worker.hashMismatch = true; /* See header, the protocols don't know about `HashMismatch` yet, so change it to `OutputRejected`, which they expect for this case (hash mismatch is a type of output rejection). */ - e.status = BuildResult::OutputRejected; + e.status = BuildResult::Failure::OutputRejected; break; - case BuildResult::NotDeterministic: + case BuildResult::Failure::NotDeterministic: worker.checkMismatch = true; break; default: @@ -866,7 +873,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() (unlinked) lock files. */ outputLocks.setDeletion(true); outputLocks.unlock(); - co_return doneSuccess(BuildResult::Built, std::move(builtOutputs)); + co_return doneSuccess(BuildResult::Success::Built, std::move(builtOutputs)); } #endif } @@ -1149,7 +1156,7 @@ void DerivationBuildingGoal::handleChildOutput(Descriptor fd, std::string_view d // We're not inside a coroutine, hence we can't use co_return here. // Thus we ignore the return value. [[maybe_unused]] Done _ = doneFailure(BuildError( - BuildResult::LogLimitExceeded, + BuildResult::Failure::LogLimitExceeded, "%s killed after writing more than %d bytes of log output", getName(), settings.maxLogSize)); @@ -1306,16 +1313,16 @@ DerivationBuildingGoal::checkPathValidity(std::map & return {allValid, validOutputs}; } -Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs) +Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Success::Status status, SingleDrvOutputs builtOutputs) { - buildResult.status = status; - - assert(buildResult.success()); + buildResult.inner = BuildResult::Success{ + .status = status, + .builtOutputs = std::move(builtOutputs), + }; mcRunningBuilds.reset(); - buildResult.builtOutputs = std::move(builtOutputs); - if (status == BuildResult::Built) + if (status == BuildResult::Success::Built) worker.doneBuilds++; worker.updateProgress(); @@ -1325,16 +1332,18 @@ Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, Singl Goal::Done DerivationBuildingGoal::doneFailure(BuildError ex) { - buildResult.status = ex.status; - buildResult.errorMsg = fmt("%s", Uncolored(ex.info().msg)); - if (buildResult.status == BuildResult::TimedOut) - worker.timedOut = true; - if (buildResult.status == BuildResult::PermanentFailure) - worker.permanentFailure = true; + buildResult.inner = BuildResult::Failure{ + .status = ex.status, + .errorMsg = fmt("%s", Uncolored(ex.info().msg)), + }; mcRunningBuilds.reset(); - if (ex.status != BuildResult::DependencyFailed) + if (ex.status == BuildResult::Failure::TimedOut) + worker.timedOut = true; + if (ex.status == BuildResult::Failure::PermanentFailure) + worker.permanentFailure = true; + if (ex.status != BuildResult::Failure::DependencyFailed) worker.failedBuilds++; worker.updateProgress(); diff --git a/src/libstore/build/derivation-check.cc b/src/libstore/build/derivation-check.cc index 82e92e1f3..db3ec7c3d 100644 --- a/src/libstore/build/derivation-check.cc +++ b/src/libstore/build/derivation-check.cc @@ -33,7 +33,7 @@ void checkOutputs( /* Throw an error after registering the path as valid. */ throw BuildError( - BuildResult::HashMismatch, + BuildResult::Failure::HashMismatch, "hash mismatch in fixed-output derivation '%s':\n specified: %s\n got: %s", store.printStorePath(drvPath), wanted.to_string(HashFormat::SRI, true), @@ -42,7 +42,7 @@ void checkOutputs( if (!info.references.empty()) { auto numViolations = info.references.size(); throw BuildError( - BuildResult::HashMismatch, + BuildResult::Failure::HashMismatch, "fixed-output derivations must not reference store paths: '%s' references %d distinct paths, e.g. '%s'", store.printStorePath(drvPath), numViolations, @@ -84,7 +84,7 @@ void checkOutputs( auto applyChecks = [&](const DerivationOptions::OutputChecks & checks) { if (checks.maxSize && info.narSize > *checks.maxSize) throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "path '%s' is too large at %d bytes; limit is %d bytes", store.printStorePath(info.path), info.narSize, @@ -94,7 +94,7 @@ void checkOutputs( uint64_t closureSize = getClosure(info.path).second; if (closureSize > *checks.maxClosureSize) throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "closure of path '%s' is too large at %d bytes; limit is %d bytes", store.printStorePath(info.path), closureSize, @@ -115,7 +115,7 @@ void checkOutputs( std::string outputsListing = concatMapStringsSep(", ", outputs, [](auto & o) { return o.first; }); throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "derivation '%s' output check for '%s' contains an illegal reference specifier '%s'," " expected store path or output name (one of [%s])", store.printStorePath(drvPath), @@ -148,7 +148,7 @@ void checkOutputs( badPathsStr += store.printStorePath(i); } throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "output '%s' is not allowed to refer to the following paths:%s", store.printStorePath(info.path), badPathsStr); diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index b9046744a..5dfc334a8 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -94,7 +94,7 @@ Goal::Co DerivationGoal::haveDerivation() /* If they are all valid, then we're done. */ if (checkResult && checkResult->second == PathStatus::Valid && buildMode == bmNormal) { - co_return doneSuccess(BuildResult::AlreadyValid, checkResult->first); + co_return doneSuccess(BuildResult::Success::AlreadyValid, checkResult->first); } Goals waitees; @@ -123,7 +123,7 @@ Goal::Co DerivationGoal::haveDerivation() if (nrFailed > 0 && nrFailed > nrNoSubstituters && !settings.tryFallback) { co_return doneFailure(BuildError( - BuildResult::TransientFailure, + BuildResult::Failure::TransientFailure, "some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ", worker.store.printStorePath(drvPath))); } @@ -135,7 +135,7 @@ Goal::Co DerivationGoal::haveDerivation() bool allValid = checkResult && checkResult->second == PathStatus::Valid; if (buildMode == bmNormal && allValid) { - co_return doneSuccess(BuildResult::Substituted, checkResult->first); + co_return doneSuccess(BuildResult::Success::Substituted, checkResult->first); } if (buildMode == bmRepair && allValid) { co_return repairClosure(); @@ -163,25 +163,27 @@ Goal::Co DerivationGoal::haveDerivation() buildResult = g->buildResult; - if (buildMode == bmCheck) { - /* In checking mode, the builder will not register any outputs. - So we want to make sure the ones that we wanted to check are - properly there. */ - buildResult.builtOutputs = {{wantedOutput, assertPathValidity()}}; - } else { - /* Otherwise the builder will give us info for out output, but - also for other outputs. Filter down to just our output so as - not to leak info on unrelated things. */ - for (auto it = buildResult.builtOutputs.begin(); it != buildResult.builtOutputs.end();) { - if (it->first != wantedOutput) { - it = buildResult.builtOutputs.erase(it); - } else { - ++it; + if (auto * successP = buildResult.tryGetSuccess()) { + auto & success = *successP; + if (buildMode == bmCheck) { + /* In checking mode, the builder will not register any outputs. + So we want to make sure the ones that we wanted to check are + properly there. */ + success.builtOutputs = {{wantedOutput, assertPathValidity()}}; + } else { + /* Otherwise the builder will give us info for out output, but + also for other outputs. Filter down to just our output so as + not to leak info on unrelated things. */ + for (auto it = success.builtOutputs.begin(); it != success.builtOutputs.end();) { + if (it->first != wantedOutput) { + it = success.builtOutputs.erase(it); + } else { + ++it; + } } - } - if (buildResult.success()) - assert(buildResult.builtOutputs.count(wantedOutput) > 0); + assert(success.builtOutputs.count(wantedOutput) > 0); + } } co_return amDone(g->exitCode, g->ex); @@ -279,7 +281,7 @@ Goal::Co DerivationGoal::repairClosure() "some paths in the output closure of derivation '%s' could not be repaired", worker.store.printStorePath(drvPath)); } - co_return doneSuccess(BuildResult::AlreadyValid, assertPathValidity()); + co_return doneSuccess(BuildResult::Success::AlreadyValid, assertPathValidity()); } std::optional> DerivationGoal::checkPathValidity() @@ -337,16 +339,16 @@ Realisation DerivationGoal::assertPathValidity() return checkResult->first; } -Goal::Done DerivationGoal::doneSuccess(BuildResult::Status status, Realisation builtOutput) +Goal::Done DerivationGoal::doneSuccess(BuildResult::Success::Status status, Realisation builtOutput) { - buildResult.status = status; - - assert(buildResult.success()); + buildResult.inner = BuildResult::Success{ + .status = status, + .builtOutputs = {{wantedOutput, std::move(builtOutput)}}, + }; mcExpectedBuilds.reset(); - buildResult.builtOutputs = {{wantedOutput, std::move(builtOutput)}}; - if (status == BuildResult::Built) + if (status == BuildResult::Success::Built) worker.doneBuilds++; worker.updateProgress(); @@ -356,16 +358,18 @@ Goal::Done DerivationGoal::doneSuccess(BuildResult::Status status, Realisation b Goal::Done DerivationGoal::doneFailure(BuildError ex) { - buildResult.status = ex.status; - buildResult.errorMsg = fmt("%s", Uncolored(ex.info().msg)); - if (buildResult.status == BuildResult::TimedOut) - worker.timedOut = true; - if (buildResult.status == BuildResult::PermanentFailure) - worker.permanentFailure = true; + buildResult.inner = BuildResult::Failure{ + .status = ex.status, + .errorMsg = fmt("%s", Uncolored(ex.info().msg)), + }; mcExpectedBuilds.reset(); - if (ex.status != BuildResult::DependencyFailed) + if (ex.status == BuildResult::Failure::TimedOut) + worker.timedOut = true; + if (ex.status == BuildResult::Failure::PermanentFailure) + worker.permanentFailure = true; + if (ex.status != BuildResult::Failure::DependencyFailed) worker.failedBuilds++; worker.updateProgress(); diff --git a/src/libstore/build/derivation-trampoline-goal.cc b/src/libstore/build/derivation-trampoline-goal.cc index 5038a4ea0..205f5c427 100644 --- a/src/libstore/build/derivation-trampoline-goal.cc +++ b/src/libstore/build/derivation-trampoline-goal.cc @@ -164,10 +164,11 @@ Goal::Co DerivationTrampolineGoal::haveDerivation(StorePath drvPath, Derivation auto & g = *concreteDrvGoals.begin(); buildResult = g->buildResult; - for (auto & g2 : concreteDrvGoals) { - for (auto && [x, y] : g2->buildResult.builtOutputs) - buildResult.builtOutputs.insert_or_assign(x, y); - } + if (auto * successP = buildResult.tryGetSuccess()) + for (auto & g2 : concreteDrvGoals) + if (auto * successP2 = g2->buildResult.tryGetSuccess()) + for (auto && [x, y] : successP2->builtOutputs) + successP->builtOutputs.insert_or_assign(x, y); co_return amDone(g->exitCode, g->ex); } diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 1dd540265..4bbd4c8f0 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -82,10 +82,10 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat worker.run(Goals{goal}); return goal->buildResult; } catch (Error & e) { - return BuildResult{ - .status = BuildResult::MiscFailure, + return BuildResult{.inner{BuildResult::Failure{ + .status = BuildResult::Failure::MiscFailure, .errorMsg = e.msg(), - }; + }}}; }; } diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index d219834f2..d16e530a4 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -27,13 +27,21 @@ PathSubstitutionGoal::~PathSubstitutionGoal() cleanup(); } -Goal::Done PathSubstitutionGoal::done(ExitCode result, BuildResult::Status status, std::optional errorMsg) +Goal::Done PathSubstitutionGoal::doneSuccess(BuildResult::Success::Status status) { - buildResult.status = status; - if (errorMsg) { - debug(*errorMsg); - buildResult.errorMsg = *errorMsg; - } + buildResult.inner = BuildResult::Success{ + .status = status, + }; + return amDone(ecSuccess); +} + +Goal::Done PathSubstitutionGoal::doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg) +{ + debug(errorMsg); + buildResult.inner = BuildResult::Failure{ + .status = status, + .errorMsg = std::move(errorMsg), + }; return amDone(result); } @@ -45,7 +53,7 @@ Goal::Co PathSubstitutionGoal::init() /* If the path already exists we're done. */ if (!repair && worker.store.isValidPath(storePath)) { - co_return done(ecSuccess, BuildResult::AlreadyValid); + co_return doneSuccess(BuildResult::Success::AlreadyValid); } if (settings.readOnlyMode) @@ -165,9 +173,9 @@ Goal::Co PathSubstitutionGoal::init() /* Hack: don't indicate failure if there were no substituters. In that case the calling derivation should just do a build. */ - co_return done( + co_return doneFailure( substituterFailed ? ecFailed : ecNoSubstituters, - BuildResult::NoSubstituters, + BuildResult::Failure::NoSubstituters, fmt("path '%s' is required, but there is no substituter that can build it", worker.store.printStorePath(storePath))); } @@ -178,9 +186,9 @@ Goal::Co PathSubstitutionGoal::tryToRun( trace("all references realised"); if (nrFailed > 0) { - co_return done( + co_return doneFailure( nrNoSubstituters > 0 ? ecNoSubstituters : ecFailed, - BuildResult::DependencyFailed, + BuildResult::Failure::DependencyFailed, fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath))); } @@ -297,7 +305,7 @@ Goal::Co PathSubstitutionGoal::tryToRun( worker.updateProgress(); - co_return done(ecSuccess, BuildResult::Substituted); + co_return doneSuccess(BuildResult::Success::Substituted); } void PathSubstitutionGoal::handleEOF(Descriptor fd) diff --git a/src/libstore/derivation-options.cc b/src/libstore/derivation-options.cc index 4cb9bf726..844bce840 100644 --- a/src/libstore/derivation-options.cc +++ b/src/libstore/derivation-options.cc @@ -266,7 +266,9 @@ DerivationOptions::getParsedExportReferencesGraph(const StoreDirConfig & store) for (auto & storePathS : ss) { if (!store.isInStore(storePathS)) throw BuildError( - BuildResult::InputRejected, "'exportReferencesGraph' contains a non-store path '%1%'", storePathS); + BuildResult::Failure::InputRejected, + "'exportReferencesGraph' contains a non-store path '%1%'", + storePathS); storePaths.insert(store.toStorePath(storePathS).first); } res.insert_or_assign(fileName, storePaths); diff --git a/src/libstore/include/nix/store/build-result.hh b/src/libstore/include/nix/store/build-result.hh index 1911fef39..0446c4038 100644 --- a/src/libstore/include/nix/store/build-result.hh +++ b/src/libstore/include/nix/store/build-result.hh @@ -12,63 +12,121 @@ namespace nix { struct BuildResult { - /** - * @note This is directly used in the nix-store --serve protocol. - * That means we need to worry about compatibility across versions. - * Therefore, don't remove status codes, and only add new status - * codes at the end of the list. - */ - enum Status { - Built = 0, - Substituted = 1, - AlreadyValid = 2, - PermanentFailure = 3, - InputRejected = 4, - OutputRejected = 5, - /// possibly transient - TransientFailure = 6, - /// no longer used - CachedFailure = 7, - TimedOut = 8, - MiscFailure = 9, - DependencyFailed = 10, - LogLimitExceeded = 11, - NotDeterministic = 12, - ResolvesToAlreadyValid = 13, - NoSubstituters = 14, - /// A certain type of `OutputRejected`. The protocols do not yet - /// know about this one, so change it back to `OutputRejected` - /// before serialization. - HashMismatch = 15, - } status = MiscFailure; + struct Success + { + /** + * @note This is directly used in the nix-store --serve protocol. + * That means we need to worry about compatibility across versions. + * Therefore, don't remove status codes, and only add new status + * codes at the end of the list. + * + * Must be disjoint with `Failure::Status`. + */ + enum Status : uint8_t { + Built = 0, + Substituted = 1, + AlreadyValid = 2, + ResolvesToAlreadyValid = 13, + } status; + + /** + * For derivations, a mapping from the names of the wanted outputs + * to actual paths. + */ + SingleDrvOutputs builtOutputs; + + bool operator==(const BuildResult::Success &) const noexcept; + std::strong_ordering operator<=>(const BuildResult::Success &) const noexcept; + + static bool statusIs(uint8_t status) + { + return status == Built || status == Substituted || status == AlreadyValid + || status == ResolvesToAlreadyValid; + } + }; + + struct Failure + { + /** + * @note This is directly used in the nix-store --serve protocol. + * That means we need to worry about compatibility across versions. + * Therefore, don't remove status codes, and only add new status + * codes at the end of the list. + * + * Must be disjoint with `Success::Status`. + */ + enum Status : uint8_t { + PermanentFailure = 3, + InputRejected = 4, + OutputRejected = 5, + /// possibly transient + TransientFailure = 6, + /// no longer used + CachedFailure = 7, + TimedOut = 8, + MiscFailure = 9, + DependencyFailed = 10, + LogLimitExceeded = 11, + NotDeterministic = 12, + NoSubstituters = 14, + /// A certain type of `OutputRejected`. The protocols do not yet + /// know about this one, so change it back to `OutputRejected` + /// before serialization. + HashMismatch = 15, + } 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 + { + throw Error("%s", errorMsg); + } + }; + + std::variant inner = Failure{}; /** - * Information about the error if the build failed. - * - * @todo This should be an entire ErrorInfo object, not just a - * string, for richer information. + * Convenience wrapper to avoid a longer `std::get_if` usage by the + * caller (which will have to add more `BuildResult::` than we do + * below also, do note.) */ - std::string errorMsg; + auto * tryGetSuccess(this auto & self) + { + return std::get_if(&self.inner); + } + + /** + * Convenience wrapper to avoid a longer `std::get_if` usage by the + * caller (which will have to add more `BuildResult::` than we do + * below also, do note.) + */ + auto * tryGetFailure(this auto & self) + { + return std::get_if(&self.inner); + } /** * How many times this build was performed. */ unsigned int timesBuilt = 0; - /** - * 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; - - /** - * For derivations, a mapping from the names of the wanted outputs - * to actual paths. - */ - SingleDrvOutputs builtOutputs; - /** * The start/stop times of the build (or one of the rounds, if it * was repeated). @@ -82,16 +140,6 @@ struct BuildResult bool operator==(const BuildResult &) const noexcept; std::strong_ordering operator<=>(const BuildResult &) const noexcept; - - bool success() - { - return status == Built || status == Substituted || status == AlreadyValid || status == ResolvesToAlreadyValid; - } - - void rethrow() - { - throw Error("%s", errorMsg); - } }; /** @@ -99,15 +147,9 @@ struct BuildResult */ struct BuildError : public Error { - BuildResult::Status status; + BuildResult::Failure::Status status; - BuildError(BuildResult::Status status, BuildError && error) - : Error{std::move(error)} - , status{status} - { - } - - BuildError(BuildResult::Status status, auto &&... args) + BuildError(BuildResult::Failure::Status status, auto &&... args) : Error{args...} , status{status} { diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index 7fad2837a..63ef2b665 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -22,7 +22,7 @@ struct BuilderFailureError : BuildError std::string extraMsgAfter; - BuilderFailureError(BuildResult::Status status, int builderStatus, std::string extraMsgAfter) + BuilderFailureError(BuildResult::Failure::Status status, int builderStatus, std::string extraMsgAfter) : BuildError{ status, /* No message for now, because the caller will make for 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 d394eb3c9..edb496024 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -147,7 +147,7 @@ private: */ void killChild(); - Done doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs); + Done doneSuccess(BuildResult::Success::Status status, SingleDrvOutputs builtOutputs); Done doneFailure(BuildError ex); diff --git a/src/libstore/include/nix/store/build/derivation-goal.hh b/src/libstore/include/nix/store/build/derivation-goal.hh index 85b471e28..e05bf1c0b 100644 --- a/src/libstore/include/nix/store/build/derivation-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-goal.hh @@ -99,7 +99,7 @@ private: Co repairClosure(); - Done doneSuccess(BuildResult::Status status, Realisation builtOutput); + Done doneSuccess(BuildResult::Success::Status status, Realisation builtOutput); Done doneFailure(BuildError ex); }; diff --git a/src/libstore/include/nix/store/build/substitution-goal.hh b/src/libstore/include/nix/store/build/substitution-goal.hh index 9fc6450b1..5f6cb6a18 100644 --- a/src/libstore/include/nix/store/build/substitution-goal.hh +++ b/src/libstore/include/nix/store/build/substitution-goal.hh @@ -41,7 +41,9 @@ struct PathSubstitutionGoal : public Goal */ std::optional ca; - Done done(ExitCode result, BuildResult::Status status, std::optional errorMsg = {}); + Done doneSuccess(BuildResult::Success::Status status); + + Done doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg); public: PathSubstitutionGoal( diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index f935de206..3b466c9bb 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -241,12 +241,13 @@ void LegacySSHStore::buildPaths( conn->to.flush(); - BuildResult result; - result.status = (BuildResult::Status) readInt(conn->from); - - if (!result.success()) { - conn->from >> result.errorMsg; - throw Error(result.status, result.errorMsg); + auto status = readInt(conn->from); + if (!BuildResult::Success::statusIs(status)) { + BuildResult::Failure failure{ + .status = (BuildResult::Failure::Status) status, + }; + conn->from >> failure.errorMsg; + throw Error(failure.status, std::move(failure.errorMsg)); } } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 4cadf5282..ebc987ee0 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -997,7 +997,7 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos) }}, {[&](const StorePath & path, const StorePath & parent) { return BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "cycle detected in the references of '%s' from '%s'", printStorePath(path), printStorePath(parent)); diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index c5e1747c1..7efaa4f86 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -322,7 +322,7 @@ StorePaths Store::topoSortPaths(const StorePathSet & paths) }}, {[&](const StorePath & path, const StorePath & parent) { return BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "cycle detected in the references of '%s' from '%s'", printStorePath(path), printStorePath(parent)); diff --git a/src/libstore/posix-fs-canonicalise.cc b/src/libstore/posix-fs-canonicalise.cc index b6a64e65b..a274468c3 100644 --- a/src/libstore/posix-fs-canonicalise.cc +++ b/src/libstore/posix-fs-canonicalise.cc @@ -98,7 +98,7 @@ static void canonicalisePathMetaData_( (i.e. "touch $out/foo; ln $out/foo $out/bar"). */ if (uidRange && (st.st_uid < uidRange->first || st.st_uid > uidRange->second)) { if (S_ISDIR(st.st_mode) || !inodesSeen.count(Inode(st.st_dev, st.st_ino))) - throw BuildError(BuildResult::OutputRejected, "invalid ownership on file '%1%'", path); + throw BuildError(BuildResult::Failure::OutputRejected, "invalid ownership on file '%1%'", path); mode_t mode = st.st_mode & ~S_IFMT; assert( S_ISLNK(st.st_mode) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index bb7425081..a6994f844 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -598,16 +598,15 @@ std::vector RemoteStore::buildPathsWithResults( [&](const DerivedPath::Opaque & bo) { results.push_back( KeyedBuildResult{ - { - .status = BuildResult::Substituted, - }, + {.inner{BuildResult::Success{ + .status = BuildResult::Success::Substituted, + }}}, /* .path = */ bo, }); }, [&](const DerivedPath::Built & bfd) { - KeyedBuildResult res{ - {.status = BuildResult::Built}, - /* .path = */ bfd, + BuildResult::Success success{ + .status = BuildResult::Success::Built, }; OutputPathMap outputs; @@ -627,9 +626,9 @@ std::vector RemoteStore::buildPathsWithResults( auto realisation = queryRealisation(outputId); if (!realisation) throw MissingRealisation(outputId); - res.builtOutputs.emplace(output, *realisation); + success.builtOutputs.emplace(output, *realisation); } else { - res.builtOutputs.emplace( + success.builtOutputs.emplace( output, Realisation{ .id = outputId, @@ -638,7 +637,11 @@ std::vector RemoteStore::buildPathsWithResults( } } - results.push_back(res); + results.push_back( + KeyedBuildResult{ + {.inner = std::move(success)}, + /* .path = */ bfd, + }); }}, path.raw()); } diff --git a/src/libstore/restricted-store.cc b/src/libstore/restricted-store.cc index e0f43ab6c..a1cb41606 100644 --- a/src/libstore/restricted-store.cc +++ b/src/libstore/restricted-store.cc @@ -257,8 +257,8 @@ void RestrictedStore::buildPaths( const std::vector & paths, BuildMode buildMode, std::shared_ptr evalStore) { for (auto & result : buildPathsWithResults(paths, buildMode, evalStore)) - if (!result.success()) - result.rethrow(); + if (auto * failureP = result.tryGetFailure()) + failureP->rethrow(); } std::vector RestrictedStore::buildPathsWithResults( @@ -280,9 +280,11 @@ std::vector RestrictedStore::buildPathsWithResults( auto results = next->buildPathsWithResults(paths, buildMode); for (auto & result : results) { - for (auto & [outputName, output] : result.builtOutputs) { - newPaths.insert(output.outPath); - newRealisations.insert(output); + if (auto * successP = result.tryGetSuccess()) { + for (auto & [outputName, output] : successP->builtOutputs) { + newPaths.insert(output.outPath); + newRealisations.insert(output); + } } } diff --git a/src/libstore/serve-protocol.cc b/src/libstore/serve-protocol.cc index 7cf5e6997..51b575fcd 100644 --- a/src/libstore/serve-protocol.cc +++ b/src/libstore/serve-protocol.cc @@ -16,32 +16,62 @@ namespace nix { BuildResult ServeProto::Serialise::read(const StoreDirConfig & store, ServeProto::ReadConn conn) { BuildResult status; - status.status = (BuildResult::Status) readInt(conn.from); - conn.from >> status.errorMsg; + BuildResult::Success success; + BuildResult::Failure failure; + + auto rawStatus = readInt(conn.from); + conn.from >> failure.errorMsg; if (GET_PROTOCOL_MINOR(conn.version) >= 3) - conn.from >> status.timesBuilt >> status.isNonDeterministic >> status.startTime >> status.stopTime; + conn.from >> status.timesBuilt >> failure.isNonDeterministic >> status.startTime >> status.stopTime; if (GET_PROTOCOL_MINOR(conn.version) >= 6) { auto builtOutputs = ServeProto::Serialise::read(store, conn); for (auto && [output, realisation] : builtOutputs) - status.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation)); + success.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation)); } + + if (BuildResult::Success::statusIs(rawStatus)) { + success.status = static_cast(rawStatus); + status.inner = std::move(success); + } else { + failure.status = static_cast(rawStatus); + status.inner = std::move(failure); + } + return status; } void ServeProto::Serialise::write( - const StoreDirConfig & store, ServeProto::WriteConn conn, const BuildResult & status) + const StoreDirConfig & store, ServeProto::WriteConn conn, const BuildResult & res) { - conn.to << status.status << status.errorMsg; - - if (GET_PROTOCOL_MINOR(conn.version) >= 3) - conn.to << status.timesBuilt << status.isNonDeterministic << status.startTime << status.stopTime; - if (GET_PROTOCOL_MINOR(conn.version) >= 6) { - DrvOutputs builtOutputs; - for (auto & [output, realisation] : status.builtOutputs) - builtOutputs.insert_or_assign(realisation.id, realisation); - ServeProto::write(store, conn, builtOutputs); - } + /* The protocol predates the use of sum types (std::variant) to + separate the success or failure cases. As such, it transits some + success- or failure-only fields in both cases. This helper + function helps support this: in each case, we just pass the old + default value for the fields that don't exist in that case. */ + auto common = [&](std::string_view errorMsg, bool isNonDeterministic, const auto & builtOutputs) { + conn.to << errorMsg; + if (GET_PROTOCOL_MINOR(conn.version) >= 3) + conn.to << res.timesBuilt << isNonDeterministic << res.startTime << res.stopTime; + if (GET_PROTOCOL_MINOR(conn.version) >= 6) { + DrvOutputs builtOutputsFullKey; + for (auto & [output, realisation] : builtOutputs) + builtOutputsFullKey.insert_or_assign(realisation.id, realisation); + ServeProto::write(store, conn, builtOutputsFullKey); + } + }; + std::visit( + overloaded{ + [&](const BuildResult::Failure & failure) { + conn.to << failure.status; + common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){}); + }, + [&](const BuildResult::Success & success) { + conn.to << success.status; + common(/*errorMsg=*/"", /*isNonDeterministic=*/false, success.builtOutputs); + }, + }, + res.inner); } UnkeyedValidPathInfo ServeProto::Serialise::read(const StoreDirConfig & store, ReadConn conn) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index a0b06db54..56dffe19d 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -764,7 +764,7 @@ StorePathSet Store::exportReferences(const StorePathSet & storePaths, const Stor for (auto & storePath : storePaths) { if (!inputPaths.count(storePath)) throw BuildError( - BuildResult::InputRejected, + BuildResult::Failure::InputRejected, "cannot export references of path '%s' because it is not in the input closure of the derivation", printStorePath(storePath)); diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 770bdad4d..d765de562 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -50,7 +50,7 @@ namespace nix { struct NotDeterministic : BuildError { NotDeterministic(auto &&... args) - : BuildError(BuildResult::NotDeterministic, args...) + : BuildError(BuildResult::Failure::NotDeterministic, args...) { } }; @@ -518,7 +518,8 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild() cleanupBuild(false); throw BuilderFailureError{ - !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure : BuildResult::PermanentFailure, + !derivationType.isSandboxed() || diskFull ? BuildResult::Failure::TransientFailure + : BuildResult::Failure::PermanentFailure, status, diskFull ? "\nnote: build failure may have been caused by lack of free disk space" : "", }; @@ -700,7 +701,7 @@ std::optional DerivationBuilderImpl::startBuild() fmt("\nNote: run `%s` to run programs for x86_64-darwin", Magenta("/usr/sbin/softwareupdate --install-rosetta && launchctl stop org.nixos.nix-daemon")); - throw BuildError(BuildResult::InputRejected, msg); + throw BuildError(BuildResult::Failure::InputRejected, msg); } auto buildDir = store.config->getBuildDir(); @@ -1389,7 +1390,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() auto optSt = maybeLstat(actualPath.c_str()); if (!optSt) throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "builder for '%s' failed to produce output path for output '%s' at '%s'", store.printStorePath(drvPath), outputName, @@ -1404,7 +1405,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() if ((!S_ISLNK(st.st_mode) && (st.st_mode & (S_IWGRP | S_IWOTH))) || (buildUser && st.st_uid != buildUser->getUID())) throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "suspicious ownership or permission on '%s' for output '%s'; rejecting this build output", actualPath, outputName); @@ -1442,7 +1443,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() auto orifu = get(outputReferencesIfUnregistered, name); if (!orifu) throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "no output reference for '%s' in build of '%s'", name, store.printStorePath(drvPath)); @@ -1467,7 +1468,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() {[&](const std::string & path, const std::string & parent) { // TODO with more -vvvv also show the temporary paths for manual inspection. return BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "cycle detected in build of '%s' in the references of output '%s' from output '%s'", store.printStorePath(drvPath), path, @@ -1561,12 +1562,13 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() auto newInfoFromCA = [&](const DerivationOutput::CAFloating outputHash) -> ValidPathInfo { auto st = get(outputStats, outputName); if (!st) - throw BuildError(BuildResult::OutputRejected, "output path %1% without valid stats info", actualPath); + throw BuildError( + BuildResult::Failure::OutputRejected, "output path %1% without valid stats info", actualPath); if (outputHash.method.getFileIngestionMethod() == FileIngestionMethod::Flat) { /* The output path should be a regular file without execute permission. */ if (!S_ISREG(st->st_mode) || (st->st_mode & S_IXUSR) != 0) throw BuildError( - BuildResult::OutputRejected, + BuildResult::Failure::OutputRejected, "output path '%1%' should be a non-executable regular file " "since recursive hashing is not enabled (one of outputHashMode={flat,text} is true)", actualPath); diff --git a/src/libstore/worker-protocol.cc b/src/libstore/worker-protocol.cc index 1bbff64a2..4f7c28409 100644 --- a/src/libstore/worker-protocol.cc +++ b/src/libstore/worker-protocol.cc @@ -165,10 +165,14 @@ void WorkerProto::Serialise::write( BuildResult WorkerProto::Serialise::read(const StoreDirConfig & store, WorkerProto::ReadConn conn) { BuildResult res; - res.status = static_cast(readInt(conn.from)); - conn.from >> res.errorMsg; + BuildResult::Success success; + BuildResult::Failure failure; + + auto rawStatus = readInt(conn.from); + conn.from >> failure.errorMsg; + if (GET_PROTOCOL_MINOR(conn.version) >= 29) { - conn.from >> res.timesBuilt >> res.isNonDeterministic >> res.startTime >> res.stopTime; + conn.from >> res.timesBuilt >> failure.isNonDeterministic >> res.startTime >> res.stopTime; } if (GET_PROTOCOL_MINOR(conn.version) >= 37) { res.cpuUser = WorkerProto::Serialise>::read(store, conn); @@ -177,28 +181,56 @@ BuildResult WorkerProto::Serialise::read(const StoreDirConfig & sto if (GET_PROTOCOL_MINOR(conn.version) >= 28) { auto builtOutputs = WorkerProto::Serialise::read(store, conn); for (auto && [output, realisation] : builtOutputs) - res.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation)); + success.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation)); } + + if (BuildResult::Success::statusIs(rawStatus)) { + success.status = static_cast(rawStatus); + res.inner = std::move(success); + } else { + failure.status = static_cast(rawStatus); + res.inner = std::move(failure); + } + return res; } void WorkerProto::Serialise::write( const StoreDirConfig & store, WorkerProto::WriteConn conn, const BuildResult & res) { - conn.to << res.status << res.errorMsg; - if (GET_PROTOCOL_MINOR(conn.version) >= 29) { - conn.to << res.timesBuilt << res.isNonDeterministic << res.startTime << res.stopTime; - } - if (GET_PROTOCOL_MINOR(conn.version) >= 37) { - WorkerProto::write(store, conn, res.cpuUser); - WorkerProto::write(store, conn, res.cpuSystem); - } - if (GET_PROTOCOL_MINOR(conn.version) >= 28) { - DrvOutputs builtOutputs; - for (auto & [output, realisation] : res.builtOutputs) - builtOutputs.insert_or_assign(realisation.id, realisation); - WorkerProto::write(store, conn, builtOutputs); - } + /* The protocol predates the use of sum types (std::variant) to + separate the success or failure cases. As such, it transits some + success- or failure-only fields in both cases. This helper + function helps support this: in each case, we just pass the old + default value for the fields that don't exist in that case. */ + auto common = [&](std::string_view errorMsg, bool isNonDeterministic, const auto & builtOutputs) { + conn.to << errorMsg; + if (GET_PROTOCOL_MINOR(conn.version) >= 29) { + conn.to << res.timesBuilt << isNonDeterministic << res.startTime << res.stopTime; + } + if (GET_PROTOCOL_MINOR(conn.version) >= 37) { + WorkerProto::write(store, conn, res.cpuUser); + WorkerProto::write(store, conn, res.cpuSystem); + } + if (GET_PROTOCOL_MINOR(conn.version) >= 28) { + DrvOutputs builtOutputsFullKey; + for (auto & [output, realisation] : builtOutputs) + builtOutputsFullKey.insert_or_assign(realisation.id, realisation); + WorkerProto::write(store, conn, builtOutputsFullKey); + } + }; + std::visit( + overloaded{ + [&](const BuildResult::Failure & failure) { + conn.to << failure.status; + common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){}); + }, + [&](const BuildResult::Success & success) { + conn.to << success.status; + common(/*errorMsg=*/"", /*isNonDeterministic=*/false, success.builtOutputs); + }, + }, + res.inner); } ValidPathInfo WorkerProto::Serialise::read(const StoreDirConfig & store, ReadConn conn) diff --git a/src/nix/build-remote/build-remote.cc b/src/nix/build-remote/build-remote.cc index 11df8cc5e..ffb77ddf1 100644 --- a/src/nix/build-remote/build-remote.cc +++ b/src/nix/build-remote/build-remote.cc @@ -324,7 +324,7 @@ static int main_build_remote(int argc, char ** argv) drv.inputSrcs = store->parseStorePathSet(inputs); optResult = sshStore->buildDerivation(*drvPath, (const BasicDerivation &) drv); auto & result = *optResult; - if (!result.success()) { + if (auto * failureP = result.tryGetFailure()) { if (settings.keepFailed) { warn( "The failed build directory was kept on the remote builder due to `--keep-failed`.%s", @@ -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, result.errorMsg); + "build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, failureP->errorMsg); } } else { copyClosure(*store, *sshStore, StorePathSet{*drvPath}, NoRepair, NoCheckSigs, substitute); @@ -357,11 +357,14 @@ static int main_build_remote(int argc, char ** argv) debug("missing output %s", outputName); assert(optResult); auto & result = *optResult; - auto i = result.builtOutputs.find(outputName); - assert(i != result.builtOutputs.end()); - auto & newRealisation = i->second; - missingRealisations.insert(newRealisation); - missingPaths.insert(newRealisation.outPath); + if (auto * successP = result.tryGetSuccess()) { + auto & success = *successP; + auto i = success.builtOutputs.find(outputName); + assert(i != success.builtOutputs.end()); + auto & newRealisation = i->second; + missingRealisations.insert(newRealisation); + missingPaths.insert(newRealisation.outPath); + } } } } else { diff --git a/tests/functional/test-libstoreconsumer/main.cc b/tests/functional/test-libstoreconsumer/main.cc index d8db67a4d..5b0132934 100644 --- a/tests/functional/test-libstoreconsumer/main.cc +++ b/tests/functional/test-libstoreconsumer/main.cc @@ -34,8 +34,10 @@ int main(int argc, char ** argv) const auto results = store->buildPathsWithResults(paths, bmNormal, store); for (const auto & result : results) { - for (const auto & [outputName, realisation] : result.builtOutputs) { - std::cout << store->printStorePath(realisation.outPath) << "\n"; + if (auto * successP = result.tryGetSuccess()) { + for (const auto & [outputName, realisation] : successP->builtOutputs) { + std::cout << store->printStorePath(realisation.outPath) << "\n"; + } } }