From 374f8e79a195bbcf606b0c3452f7e7de67b68150 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 28 Aug 2025 11:58:28 -0400 Subject: [PATCH] `DerivationBuilderImpl::unprepareBuild` Just throw error Aftet the previous simplifications, there is no reason to catch the error and immediately return it with a `std::variant` --- just let the caller catch it instead. --- .../build/derivation-building-goal.cc | 22 +++--- .../nix/store/build/derivation-builder.hh | 4 +- src/libstore/unix/build/derivation-builder.cc | 67 +++++++++---------- .../unix/build/linux-derivation-builder.cc | 2 +- 4 files changed, 45 insertions(+), 50 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index c9b562817..d2752dfb5 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -804,21 +804,22 @@ Goal::Co DerivationBuildingGoal::tryToBuild() trace("build done"); - auto res = builder->unprepareBuild(); - // N.B. cannot use `std::visit` with co-routine return - if (auto * ste = std::get_if<0>(&res)) { + SingleDrvOutputs builtOutputs; + try { + builtOutputs = builder->unprepareBuild(); + } catch (BuildError & e) { outputLocks.unlock(); // Allow selecting a subset of enum values # pragma GCC diagnostic push # pragma GCC diagnostic ignored "-Wswitch-enum" - switch (ste->status) { + switch (e.status) { case BuildResult::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). */ - ste->status = BuildResult::OutputRejected; + e.status = BuildResult::OutputRejected; break; case BuildResult::NotDeterministic: worker.checkMismatch = true; @@ -828,10 +829,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() break; } # pragma GCC diagnostic pop - co_return doneFailure(std::move(*ste)); - } else if (auto * builtOutputs = std::get_if<1>(&res)) { + co_return doneFailure(std::move(e)); + } + { StorePathSet outputPaths; - for (auto & [_, output] : *builtOutputs) + for (auto & [_, output] : builtOutputs) outputPaths.insert(output.outPath); runPostBuildHook(worker.store, *logger, drvPath, outputPaths); @@ -841,9 +843,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() (unlinked) lock files. */ outputLocks.setDeletion(true); outputLocks.unlock(); - co_return doneSuccess(BuildResult::Built, std::move(*builtOutputs)); - } else { - unreachable(); + co_return doneSuccess(BuildResult::Built, std::move(builtOutputs)); } #endif } diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index fd487c5fe..08708ec05 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -187,8 +187,10 @@ struct DerivationBuilder : RestrictionContext * processing. A status code and exception are returned, providing * more information. The second case indicates success, and * realisations for each output of the derivation are returned. + * + * @throws BuildError */ - virtual std::variant unprepareBuild() = 0; + virtual SingleDrvOutputs unprepareBuild() = 0; /** * Stop the in-process nix daemon thread. diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index c9b603db9..6a5b6934e 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -191,7 +191,7 @@ public: void startBuilder() override; - std::variant unprepareBuild() override; + SingleDrvOutputs unprepareBuild() override; protected: @@ -426,7 +426,7 @@ bool DerivationBuilderImpl::prepareBuild() return true; } -std::variant DerivationBuilderImpl::unprepareBuild() +SingleDrvOutputs DerivationBuilderImpl::unprepareBuild() { // FIXME: get rid of this, rely on RAII. Finally releaseBuildUser([&]() { @@ -477,49 +477,42 @@ std::variant DerivationBuilderImpl::unprepareBuild bool diskFull = false; - try { + /* Check the exit status. */ + if (!statusOk(status)) { - /* Check the exit status. */ - if (!statusOk(status)) { + diskFull |= decideWhetherDiskFull(); - diskFull |= decideWhetherDiskFull(); + cleanupBuild(); - cleanupBuild(); + auto msg = + fmt("Cannot build '%s'.\n" + "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".", + Magenta(store.printStorePath(drvPath)), + statusToString(status)); - auto msg = - fmt("Cannot build '%s'.\n" - "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".", - Magenta(store.printStorePath(drvPath)), - statusToString(status)); + msg += showKnownOutputs(store, drv); - msg += showKnownOutputs(store, drv); + miscMethods->appendLogTailErrorMsg(msg); - miscMethods->appendLogTailErrorMsg(msg); + if (diskFull) + msg += "\nnote: build failure may have been caused by lack of free disk space"; - if (diskFull) - msg += "\nnote: build failure may have been caused by lack of free disk space"; - - throw BuildError( - !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure - : BuildResult::PermanentFailure, - msg); - } - - /* Compute the FS closure of the outputs and register them as - being valid. */ - auto builtOutputs = registerOutputs(); - - /* Delete unused redirected outputs (when doing hash rewriting). */ - for (auto & i : redirectedOutputs) - deletePath(store.Store::toRealPath(i.second)); - - deleteTmpDir(true); - - return std::move(builtOutputs); - - } catch (BuildError & e) { - return std::move(e); + throw BuildError( + !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure : BuildResult::PermanentFailure, + msg); } + + /* Compute the FS closure of the outputs and register them as + being valid. */ + auto builtOutputs = registerOutputs(); + + /* Delete unused redirected outputs (when doing hash rewriting). */ + for (auto & i : redirectedOutputs) + deletePath(store.Store::toRealPath(i.second)); + + deleteTmpDir(true); + + return builtOutputs; } void DerivationBuilderImpl::cleanupBuild() diff --git a/src/libstore/unix/build/linux-derivation-builder.cc b/src/libstore/unix/build/linux-derivation-builder.cc index 39b8f09ae..d474c001e 100644 --- a/src/libstore/unix/build/linux-derivation-builder.cc +++ b/src/libstore/unix/build/linux-derivation-builder.cc @@ -659,7 +659,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu throw SysError("setuid failed"); } - std::variant unprepareBuild() override + SingleDrvOutputs unprepareBuild() override { sandboxMountNamespace = -1; sandboxUserNamespace = -1;