From 7f8d348f3ddee2161331c0ecc89c7780272c48b1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 16 Mar 2025 18:32:20 -0400 Subject: [PATCH 1/2] Move `derivationType` from `DerivationGoal` to `LocalDerivationGoal` The super class doesn't actually care. --- src/libstore/build/derivation-goal.cc | 3 --- src/libstore/build/derivation-goal.hh | 5 ----- src/libstore/unix/build/local-derivation-goal.cc | 10 +++++++++- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 907d7f8eb..c49b52e8d 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -533,9 +533,6 @@ Goal::Co DerivationGoal::gaveUpOnSubstitution() debug("added input paths %s", worker.store.showPaths(inputPaths)); - /* What type of derivation are we building? */ - derivationType = drv->type(); - /* Okay, try to build. Note that here we don't wait for a build slot to become available, since we don't need one if there is a build hook. */ diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 25ab90183..f95f10b39 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -198,11 +198,6 @@ struct DerivationGoal : public Goal std::unique_ptr hook; #endif - /** - * The sort of derivation we are building. - */ - std::optional derivationType; - BuildMode buildMode; std::unique_ptr> mcExpectedBuilds, mcRunningBuilds; diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 371d94b95..a3015480d 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -150,6 +150,13 @@ struct LocalDerivationGoal : DerivationGoal, RestrictionContext */ std::shared_ptr autoDelChroot; + /** + * The sort of derivation we are building. + * + * Just a cached value, can be recomputed from `drv`. + */ + std::optional derivationType; + /** * Stuff we need to pass to initChild(). */ @@ -478,7 +485,8 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() co_return tryToBuild(); } - assert(derivationType); + /* Cache this */ + derivationType = drv->type(); /* Are we doing a chroot build? */ { From 1c022077ead393b8624742202b002f9a8c041a1d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 16 Mar 2025 19:21:21 -0400 Subject: [PATCH 2/2] Get rid of on usage pair of `actLock` Now that we have coroutines, we can go back to loops and regular RAII, which is must less error-proone! I look forward to removing the other instances! --- src/libstore/build/derivation-goal.cc | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index c49b52e8d..4978eecd2 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -594,16 +594,18 @@ Goal::Co DerivationGoal::tryToBuild() } } - if (!outputLocks.lockPaths(lockFiles, "", false)) { - if (!actLock) - actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, + if (!outputLocks.lockPaths(lockFiles, "", false)) + { + Activity act(*logger, lvlWarn, actBuildWaiting, fmt("waiting for lock on %s", Magenta(showPaths(lockFiles)))); - worker.waitForAWhile(shared_from_this()); - co_await Suspend{}; - co_return tryToBuild(); - } - actLock.reset(); + /* Wait then try locking again, repeat until success (returned + boolean is true). */ + do { + worker.waitForAWhile(shared_from_this()); + co_await Suspend{}; + } while (!outputLocks.lockPaths(lockFiles, "", false)); + } /* Now check again whether the outputs are valid. This is because another process may have started building in parallel. After