From 76125f8eb1705ac3230acf134961d6e87da144f3 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 13:15:35 -0400 Subject: [PATCH 1/2] Get rid of `Finally` in `DerivationBuilderImpl::unprepareBuild` Calling `reset` on this `std::optional` field of `DerivationBuilderImpl` is also what the (automatically created) destructor of `DerivationBuilderImpl` will do. We should be making sure that the derivation builder is cleaned up by the goal anyways, and if we do that, then this `Finally` is no longer needed. --- src/libstore/build/derivation-building-goal.cc | 3 +++ src/libstore/unix/build/derivation-builder.cc | 8 -------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 6aab48a80..4497a6070 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -769,9 +769,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() try { builtOutputs = builder->unprepareBuild(); } catch (BuilderFailureError & e) { + builder.reset(); outputLocks.unlock(); co_return doneFailure(fixupBuilderFailureErrorMessage(std::move(e))); } catch (BuildError & e) { + builder.reset(); outputLocks.unlock(); // Allow selecting a subset of enum values # pragma GCC diagnostic push @@ -796,6 +798,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() co_return doneFailure(std::move(e)); } { + builder.reset(); StorePathSet outputPaths; for (auto & [_, output] : builtOutputs) { // for sake of `bmRepair` diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 60509560d..f837efe5a 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -484,14 +484,6 @@ bool DerivationBuilderImpl::prepareBuild() SingleDrvOutputs DerivationBuilderImpl::unprepareBuild() { - // FIXME: get rid of this, rely on RAII. - Finally releaseBuildUser([&]() { - /* Release the build user at the end of this function. We don't do - it right away because we don't want another build grabbing this - uid and then messing around with our output. */ - buildUser.reset(); - }); - /* Since we got an EOF on the logger pipe, the builder is presumed to have terminated. In fact, the builder could also have simply have closed its end of the pipe, so just to be sure, From d7ed86ceb1af865592435c3672a39677be438d47 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 16:10:25 -0400 Subject: [PATCH 2/2] Move deleting redirected outputs in to `cleanupBuild` It is only done in the `force = true` case, and the only `cleanupBuild(true)` call is right after where it used to be, so this has the exact same behavior as before. --- src/libstore/unix/build/derivation-builder.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index f837efe5a..b81deaddc 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -542,10 +542,6 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild() being valid. */ auto builtOutputs = registerOutputs(); - /* Delete unused redirected outputs (when doing hash rewriting). */ - for (auto & i : redirectedOutputs) - deletePath(store.Store::toRealPath(i.second)); - cleanupBuild(true); return builtOutputs; @@ -1855,6 +1851,12 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() void DerivationBuilderImpl::cleanupBuild(bool force) { + if (force) { + /* Delete unused redirected outputs (when doing hash rewriting). */ + for (auto & i : redirectedOutputs) + deletePath(store.Store::toRealPath(i.second)); + } + if (topTmpDir != "") { /* As an extra precaution, even in the event of `deletePath` failing to * clean up, the `tmpDir` will be chowned as if we were to move