From 95c577988023d69f5596a8763fae445b21396abb Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 13:52:17 -0400 Subject: [PATCH 01/11] `DerivationBuildingGoal::tryToBuild` pull hook waiting out of switch Do this with a new `useHook` boolean we carefully make sure is set in all cases. This change isn't really worthwhile by itself, but it allows us to make further refactors (see later commits) which are well-motivated. --- .../build/derivation-building-goal.cc | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 53343ce84c8..327955714ee 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -470,6 +470,8 @@ void DerivationBuildingGoal::started() Goal::Co DerivationBuildingGoal::tryToBuild() { + bool useHook; + trace("trying to build"); /* Obtain locks on all output paths, if the paths are known a priori. @@ -539,16 +541,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild() bool buildLocally = (buildMode != bmNormal || drvOptions->willBuildLocally(worker.store, *drv)) && settings.maxBuildJobs.get() != 0; - if (!buildLocally) { + if (buildLocally) { + useHook = false; + } else { switch (tryBuildHook()) { case rpAccept: /* Yes, it has started doing so. Wait until we get EOF from the hook. */ - actLock.reset(); - buildResult.startTime = time(0); // inexact - started(); - co_await Suspend{}; - co_return hookDone(); + useHook = true; + break; case rpPostpone: /* Not now; wait until at least one child finishes or the wake-up timeout expires. */ @@ -563,12 +564,20 @@ Goal::Co DerivationBuildingGoal::tryToBuild() co_return tryToBuild(); case rpDecline: /* We should do it ourselves. */ + useHook = false; break; } } actLock.reset(); + if (useHook) { + buildResult.startTime = time(0); // inexact + started(); + co_await Suspend{}; + co_return hookDone(); + } + co_await yield(); if (!dynamic_cast(&worker.store)) { From 4c44a213a330daf315c2464db95d29495945a206 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 13:59:47 -0400 Subject: [PATCH 02/11] Get rid of a `tryToBuild` tail recursive call with loop This will make it easier to convert somethings to RAII. --- .../build/derivation-building-goal.cc | 176 +++++++++--------- 1 file changed, 90 insertions(+), 86 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 327955714ee..77ab23b4c56 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -472,101 +472,105 @@ Goal::Co DerivationBuildingGoal::tryToBuild() { bool useHook; - trace("trying to build"); - - /* Obtain locks on all output paths, if the paths are known a priori. - - The locks are automatically released when we exit this function or Nix - crashes. If we can't acquire the lock, then continue; hopefully some - other goal can start a build, and if not, the main loop will sleep a few - seconds and then retry this goal. */ - PathSet lockFiles; - /* FIXME: Should lock something like the drv itself so we don't build same - CA drv concurrently */ - if (dynamic_cast(&worker.store)) { - /* If we aren't a local store, we might need to use the local store as - a build remote, but that would cause a deadlock. */ - /* FIXME: Make it so we can use ourselves as a build remote even if we - are the local store (separate locking for building vs scheduling? */ - /* FIXME: find some way to lock for scheduling for the other stores so - a forking daemon with --store still won't farm out redundant builds. - */ - for (auto & i : drv->outputsAndOptPaths(worker.store)) { - if (i.second.second) - lockFiles.insert(worker.store.Store::toRealPath(*i.second.second)); - else - lockFiles.insert(worker.store.Store::toRealPath(drvPath) + "." + i.first); + while (true) { + trace("trying to build"); + + /* Obtain locks on all output paths, if the paths are known a priori. + + The locks are automatically released when we exit this function or Nix + crashes. If we can't acquire the lock, then continue; hopefully some + other goal can start a build, and if not, the main loop will sleep a few + seconds and then retry this goal. */ + PathSet lockFiles; + /* FIXME: Should lock something like the drv itself so we don't build same + CA drv concurrently */ + if (dynamic_cast(&worker.store)) { + /* If we aren't a local store, we might need to use the local store as + a build remote, but that would cause a deadlock. */ + /* FIXME: Make it so we can use ourselves as a build remote even if we + are the local store (separate locking for building vs scheduling? */ + /* FIXME: find some way to lock for scheduling for the other stores so + a forking daemon with --store still won't farm out redundant builds. + */ + for (auto & i : drv->outputsAndOptPaths(worker.store)) { + if (i.second.second) + lockFiles.insert(worker.store.Store::toRealPath(*i.second.second)); + else + lockFiles.insert(worker.store.Store::toRealPath(drvPath) + "." + i.first); + } } - } - if (!outputLocks.lockPaths(lockFiles, "", false)) { - Activity act(*logger, lvlWarn, actBuildWaiting, fmt("waiting for lock on %s", Magenta(showPaths(lockFiles)))); + if (!outputLocks.lockPaths(lockFiles, "", false)) { + Activity act( + *logger, lvlWarn, actBuildWaiting, fmt("waiting for lock on %s", Magenta(showPaths(lockFiles)))); - /* Wait then try locking again, repeat until success (returned - boolean is true). */ - do { - co_await waitForAWhile(); - } while (!outputLocks.lockPaths(lockFiles, "", false)); - } + /* Wait then try locking again, repeat until success (returned + boolean is true). */ + do { + co_await waitForAWhile(); + } 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 - it has finished and released the locks, we can (and should) - reuse its results. (Strictly speaking the first check can be - omitted, but that would be less efficient.) Note that since we - now hold the locks on the output paths, no other process can - build this derivation, so no further checks are necessary. */ - auto [allValid, validOutputs] = checkPathValidity(); + /* Now check again whether the outputs are valid. This is because + another process may have started building in parallel. After + it has finished and released the locks, we can (and should) + reuse its results. (Strictly speaking the first check can be + omitted, but that would be less efficient.) Note that since we + now hold the locks on the output paths, no other process can + build this derivation, so no further checks are necessary. */ + auto [allValid, validOutputs] = checkPathValidity(); + + if (buildMode != bmCheck && allValid) { + 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)); + } - if (buildMode != bmCheck && allValid) { - 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)); - } + /* If any of the outputs already exist but are not valid, delete + them. */ + for (auto & [_, status] : initialOutputs) { + if (!status.known || status.known->isValid()) + continue; + auto storePath = status.known->path; + debug("removing invalid path '%s'", worker.store.printStorePath(status.known->path)); + deletePath(worker.store.Store::toRealPath(storePath)); + } - /* If any of the outputs already exist but are not valid, delete - them. */ - for (auto & [_, status] : initialOutputs) { - if (!status.known || status.known->isValid()) - continue; - auto storePath = status.known->path; - debug("removing invalid path '%s'", worker.store.printStorePath(status.known->path)); - deletePath(worker.store.Store::toRealPath(storePath)); - } + /* Don't do a remote build if the derivation has the attribute + `preferLocalBuild' set. Also, check and repair modes are only + supported for local builds. */ + bool buildLocally = (buildMode != bmNormal || drvOptions->willBuildLocally(worker.store, *drv)) + && settings.maxBuildJobs.get() != 0; - /* Don't do a remote build if the derivation has the attribute - `preferLocalBuild' set. Also, check and repair modes are only - supported for local builds. */ - bool buildLocally = - (buildMode != bmNormal || drvOptions->willBuildLocally(worker.store, *drv)) && settings.maxBuildJobs.get() != 0; - - if (buildLocally) { - useHook = false; - } else { - switch (tryBuildHook()) { - case rpAccept: - /* Yes, it has started doing so. Wait until we get - EOF from the hook. */ - useHook = true; - break; - case rpPostpone: - /* Not now; wait until at least one child finishes or - the wake-up timeout expires. */ - if (!actLock) - actLock = std::make_unique( - *logger, - lvlWarn, - actBuildWaiting, - fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath)))); - outputLocks.unlock(); - co_await waitForAWhile(); - co_return tryToBuild(); - case rpDecline: - /* We should do it ourselves. */ + if (buildLocally) { useHook = false; - break; + } else { + switch (tryBuildHook()) { + case rpAccept: + /* Yes, it has started doing so. Wait until we get + EOF from the hook. */ + useHook = true; + break; + case rpPostpone: + /* Not now; wait until at least one child finishes or + the wake-up timeout expires. */ + if (!actLock) + actLock = std::make_unique( + *logger, + lvlWarn, + actBuildWaiting, + fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath)))); + outputLocks.unlock(); + co_await waitForAWhile(); + continue; + case rpDecline: + /* We should do it ourselves. */ + useHook = false; + break; + } } + break; } actLock.reset(); From 7c1e5b3345b2e0a95b1a04b65ddcb6350be2e86a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 14:02:01 -0400 Subject: [PATCH 03/11] In `DerivationBuildingGoal` Demote `actLock` to local variable It doesn't need to be a field any more, because we just use it with two loops. --- src/libstore/build/derivation-building-goal.cc | 5 +++++ .../include/nix/store/build/derivation-building-goal.hh | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 77ab23b4c56..fd85a066d40 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -470,6 +470,11 @@ void DerivationBuildingGoal::started() Goal::Co DerivationBuildingGoal::tryToBuild() { + /** + * Activity that denotes waiting for a lock. + */ + std::unique_ptr actLock; + bool useHook; while (true) { 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 162cf14ad86..dd8b27dc2ce 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -92,11 +92,6 @@ struct DerivationBuildingGoal : public Goal std::unique_ptr act; - /** - * Activity that denotes waiting for a lock. - */ - std::unique_ptr actLock; - std::map builderActivities; /** From 51dadaded444907ecb97e19a34483f06d10d1ab5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 14:40:41 -0400 Subject: [PATCH 04/11] Move up `assert(!hook);` We don't need to keep doing this every loop iteration, hook stuff it is only set above. --- src/libstore/build/derivation-building-goal.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index fd85a066d40..510304653c8 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -602,12 +602,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() #ifdef _WIN32 // TODO enable `DerivationBuilder` on Windows throw UnimplementedError("building derivations is not yet implemented on Windows"); #else + assert(!hook); // Will continue here while waiting for a build user below while (true) { - assert(!hook); - unsigned int curBuilds = worker.getNrLocalBuilds(); if (curBuilds >= settings.maxBuildJobs) { outputLocks.unlock(); From a63ac8d98b005937e0b65389c9a40dd953b90888 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 15:07:01 -0400 Subject: [PATCH 05/11] Inline `DerivationBuildingGoal::hookDone` --- .../build/derivation-building-goal.cc | 146 +++++++++--------- .../store/build/derivation-building-goal.hh | 1 - 2 files changed, 71 insertions(+), 76 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 510304653c8..5493845a5b0 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -584,7 +584,77 @@ Goal::Co DerivationBuildingGoal::tryToBuild() buildResult.startTime = time(0); // inexact started(); co_await Suspend{}; - co_return hookDone(); + +#ifndef _WIN32 + assert(hook); +#endif + + trace("hook build done"); + + /* 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, + kill it. */ + int status = +#ifndef _WIN32 // TODO enable build hook on Windows + hook->pid.kill(); +#else + 0; +#endif + + debug("build hook for '%s' finished", worker.store.printStorePath(drvPath)); + + buildResult.timesBuilt++; + buildResult.stopTime = time(0); + + /* So the child is gone now. */ + worker.childTerminated(this); + + /* Close the read side of the logger pipe. */ +#ifndef _WIN32 // TODO enable build hook on Windows + hook->builderOut.readSide.close(); + hook->fromHook.readSide.close(); +#endif + + /* Close the log file. */ + closeLogFile(); + + /* Check the exit status. */ + if (!statusOk(status)) { + auto e = fixupBuilderFailureErrorMessage({BuildResult::MiscFailure, status, ""}); + + outputLocks.unlock(); + + /* TODO (once again) support fine-grained error codes, see issue #12641. */ + + co_return doneFailure(std::move(e)); + } + + /* Compute the FS closure of the outputs and register them as + being valid. */ + auto builtOutputs = + /* When using a build hook, the build hook can register the output + as valid (by doing `nix-store --import'). If so we don't have + to do anything here. + + We can only early return when the outputs are known a priori. For + floating content-addressing derivations this isn't the case. + */ + assertPathValidity(); + + StorePathSet outputPaths; + for (auto & [_, output] : builtOutputs) + outputPaths.insert(output.outPath); + runPostBuildHook(worker.store, *logger, drvPath, outputPaths); + + /* It is now safe to delete the lock files, since all future + lockers will see that the output paths are valid; they will + not create new lock files with the same names as the old + (unlinked) lock files. */ + outputLocks.setDeletion(true); + outputLocks.unlock(); + + co_return doneSuccess(BuildResult::Built, std::move(builtOutputs)); } co_await yield(); @@ -885,80 +955,6 @@ BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailur return BuildError{e.status, msg}; } -Goal::Co DerivationBuildingGoal::hookDone() -{ -#ifndef _WIN32 - assert(hook); -#endif - - trace("hook build done"); - - /* 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, - kill it. */ - int status = -#ifndef _WIN32 // TODO enable build hook on Windows - hook->pid.kill(); -#else - 0; -#endif - - debug("build hook for '%s' finished", worker.store.printStorePath(drvPath)); - - buildResult.timesBuilt++; - buildResult.stopTime = time(0); - - /* So the child is gone now. */ - worker.childTerminated(this); - - /* Close the read side of the logger pipe. */ -#ifndef _WIN32 // TODO enable build hook on Windows - hook->builderOut.readSide.close(); - hook->fromHook.readSide.close(); -#endif - - /* Close the log file. */ - closeLogFile(); - - /* Check the exit status. */ - if (!statusOk(status)) { - auto e = fixupBuilderFailureErrorMessage({BuildResult::MiscFailure, status, ""}); - - outputLocks.unlock(); - - /* TODO (once again) support fine-grained error codes, see issue #12641. */ - - co_return doneFailure(std::move(e)); - } - - /* Compute the FS closure of the outputs and register them as - being valid. */ - auto builtOutputs = - /* When using a build hook, the build hook can register the output - as valid (by doing `nix-store --import'). If so we don't have - to do anything here. - - We can only early return when the outputs are known a priori. For - floating content-addressing derivations this isn't the case. - */ - assertPathValidity(); - - StorePathSet outputPaths; - for (auto & [_, output] : builtOutputs) - outputPaths.insert(output.outPath); - runPostBuildHook(worker.store, *logger, drvPath, outputPaths); - - /* It is now safe to delete the lock files, since all future - lockers will see that the output paths are valid; they will - not create new lock files with the same names as the old - (unlinked) lock files. */ - outputLocks.setDeletion(true); - outputLocks.unlock(); - - co_return doneSuccess(BuildResult::Built, std::move(builtOutputs)); -} - HookReply DerivationBuildingGoal::tryBuildHook() { #ifdef _WIN32 // TODO enable build hook on Windows 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 dd8b27dc2ce..041abfad26b 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -112,7 +112,6 @@ struct DerivationBuildingGoal : public Goal */ Co gaveUpOnSubstitution(); Co tryToBuild(); - Co hookDone(); /** * Is the build hook willing to perform the build? From 3b9c510ab1b9eb7ebf8e48c4c8a3ebe0d3c6f570 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 15:08:35 -0400 Subject: [PATCH 06/11] `DerivationBuildingGoal::outputLocks` make local variable --- src/libstore/build/derivation-building-goal.cc | 7 +++++-- .../include/nix/store/build/derivation-building-goal.hh | 5 ----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 5493845a5b0..3cb9c8135be 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -475,6 +475,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() */ std::unique_ptr actLock; + /** + * Locks on (fixed) output paths. + */ + PathLocks outputLocks; + bool useHook; while (true) { @@ -1301,7 +1306,6 @@ SingleDrvOutputs DerivationBuildingGoal::assertPathValidity() Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs) { - outputLocks.unlock(); buildResult.status = status; assert(buildResult.success()); @@ -1319,7 +1323,6 @@ Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, Singl Goal::Done DerivationBuildingGoal::doneFailure(BuildError ex) { - outputLocks.unlock(); buildResult.status = ex.status; buildResult.errorMsg = fmt("%s", Uncolored(ex.info().msg)); if (buildResult.status == BuildResult::TimedOut) 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 041abfad26b..2ec573293c7 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -43,11 +43,6 @@ struct DerivationBuildingGoal : public Goal * The remainder is state held during the build. */ - /** - * Locks on (fixed) output paths. - */ - PathLocks outputLocks; - /** * All input paths (that is, the union of FS closures of the * immediate input paths). From c6ba120000ae7ca489ad476a8a3d961d36d64459 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 15:10:56 -0400 Subject: [PATCH 07/11] `DerivationBuildingGoal::started` make local (lambda) variable --- .../build/derivation-building-goal.cc | 49 +++++++++---------- .../store/build/derivation-building-goal.hh | 2 - 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 3cb9c8135be..008549acb84 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -439,37 +439,36 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() co_return tryToBuild(); } -void DerivationBuildingGoal::started() +Goal::Co DerivationBuildingGoal::tryToBuild() { - auto msg = - fmt(buildMode == bmRepair ? "repairing outputs of '%s'" - : buildMode == bmCheck ? "checking outputs of '%s'" - : "building '%s'", - worker.store.printStorePath(drvPath)); - fmt("building '%s'", worker.store.printStorePath(drvPath)); + auto started = [&]() { + auto msg = + fmt(buildMode == bmRepair ? "repairing outputs of '%s'" + : buildMode == bmCheck ? "checking outputs of '%s'" + : "building '%s'", + worker.store.printStorePath(drvPath)); + fmt("building '%s'", worker.store.printStorePath(drvPath)); #ifndef _WIN32 // TODO enable build hook on Windows - if (hook) - msg += fmt(" on '%s'", machineName); + if (hook) + msg += fmt(" on '%s'", machineName); #endif - act = std::make_unique( - *logger, - lvlInfo, - actBuild, - msg, - Logger::Fields{ - worker.store.printStorePath(drvPath), + act = std::make_unique( + *logger, + lvlInfo, + actBuild, + msg, + Logger::Fields{ + worker.store.printStorePath(drvPath), #ifndef _WIN32 // TODO enable build hook on Windows - hook ? machineName : + hook ? machineName : #endif - "", - 1, - 1}); - mcRunningBuilds = std::make_unique>(worker.runningBuilds); - worker.updateProgress(); -} + "", + 1, + 1}); + mcRunningBuilds = std::make_unique>(worker.runningBuilds); + worker.updateProgress(); + }; -Goal::Co DerivationBuildingGoal::tryToBuild() -{ /** * Activity that denotes waiting for a lock. */ 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 2ec573293c7..f6dcad83db1 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -158,8 +158,6 @@ struct DerivationBuildingGoal : public Goal */ void killChild(); - void started(); - Done doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs); Done doneFailure(BuildError ex); From eb56b181aeaeef40de996202267e12a73f245adb Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 15:40:10 -0400 Subject: [PATCH 08/11] DerivationBuildingGoal: Make almost everything private --- .../nix/store/build/derivation-building-goal.hh | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 f6dcad83db1..2cb111760a2 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -29,6 +29,12 @@ typedef enum { rpAccept, rpDecline, rpPostpone } HookReply; */ struct DerivationBuildingGoal : public Goal { + DerivationBuildingGoal( + const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode = bmNormal); + ~DerivationBuildingGoal(); + +private: + /** The path of the derivation. */ StorePath drvPath; @@ -94,10 +100,6 @@ struct DerivationBuildingGoal : public Goal */ std::string machineName; - DerivationBuildingGoal( - const StorePath & drvPath, const Derivation & drv, Worker & worker, BuildMode buildMode = bmNormal); - ~DerivationBuildingGoal(); - void timedOut(Error && ex) override; std::string key() override; From 450633aa8cd0c0871164a24ac34eac2386218bc7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 15:49:58 -0400 Subject: [PATCH 09/11] Move `machineName` from `DerivationBuildingGoal` to `HookInstance` Exactly why is is correct is a little subtle, because sometimes the worker is owned by the worker. But the commit message in e437b0825018b1935f9a849382c12b1df0aeae06 explained the situation well enough: I made that commit message part of the ABI docs, and now it should be understandable to the next person. --- src/libstore/build/derivation-building-goal.cc | 6 +++--- .../nix/store/build/derivation-building-goal.hh | 5 ----- .../include/nix/store/build/hook-instance.hh | 17 +++++++++++++++++ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 008549acb84..75295eab760 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -450,7 +450,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() fmt("building '%s'", worker.store.printStorePath(drvPath)); #ifndef _WIN32 // TODO enable build hook on Windows if (hook) - msg += fmt(" on '%s'", machineName); + msg += fmt(" on '%s'", hook->machineName); #endif act = std::make_unique( *logger, @@ -460,7 +460,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() Logger::Fields{ worker.store.printStorePath(drvPath), #ifndef _WIN32 // TODO enable build hook on Windows - hook ? machineName : + hook ? hook->machineName : #endif "", 1, @@ -1027,7 +1027,7 @@ HookReply DerivationBuildingGoal::tryBuildHook() hook = std::move(worker.hook); try { - machineName = readLine(hook->fromHook.readSide.get()); + hook->machineName = readLine(hook->fromHook.readSide.get()); } catch (Error & e) { e.addTrace({}, "while reading the machine name from the build hook"); throw; 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 2cb111760a2..07f9b21ae1b 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -95,11 +95,6 @@ private: std::map builderActivities; - /** - * The remote machine on which we're building. - */ - std::string machineName; - void timedOut(Error && ex) override; std::string key() override; diff --git a/src/libstore/unix/include/nix/store/build/hook-instance.hh b/src/libstore/unix/include/nix/store/build/hook-instance.hh index 87e03665c72..7657d5dbd08 100644 --- a/src/libstore/unix/include/nix/store/build/hook-instance.hh +++ b/src/libstore/unix/include/nix/store/build/hook-instance.hh @@ -7,6 +7,14 @@ namespace nix { +/** + * @note Sometimes this is owned by the `Worker`, and sometimes it is + * owned by a `Goal`. This is for efficiency: rather than starting the + * hook every time we want to ask whether we can run a remote build + * (which can be very often), we reuse a hook process for answering + * those queries until it accepts a build. So if there are N + * derivations to be built, at most N hooks will be started. + */ struct HookInstance { /** @@ -29,6 +37,15 @@ struct HookInstance */ Pid pid; + /** + * The remote machine on which we're building. + * + * @Invariant When the hook instance is owned by the `Worker`, this + * is the empty string. When it is owned by a `Goal`, this should be + * set. + */ + std::string machineName; + FdSink sink; std::map activities; From c0c2a89f05a14b70363870408eee29f5a15cdff0 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 3 Sep 2025 16:51:53 -0400 Subject: [PATCH 10/11] `DerivationBuildingGoal::initialOutputs` move initialization down to `tryToBuild` Will help us make this a local variable. --- .../build/derivation-building-goal.cc | 57 ++++++++++--------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 75295eab760..95f0ee9d50d 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -127,31 +127,6 @@ static void runPostBuildHook( produced using a substitute. So we have to build instead. */ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() { - /* Recheck at goal start. In particular, whereas before we were - given this information by the downstream goal, that cannot happen - anymore if the downstream goal only cares about one output, but - we care about all outputs. */ - auto outputHashes = staticOutputHashes(worker.evalStore, *drv); - for (auto & [outputName, outputHash] : outputHashes) { - InitialOutput v{.outputHash = outputHash}; - - /* TODO we might want to also allow randomizing the paths - for regular CA derivations, e.g. for sake of checking - determinism. */ - if (drv->type().isImpure()) { - v.known = InitialOutputStatus{ - .path = StorePath::random(outputPathName(drv->name, outputName)), - .status = PathStatus::Absent, - }; - } - - initialOutputs.insert({ - outputName, - std::move(v), - }); - } - checkPathValidity(); - Goals waitees; std::map, GoalPtr, value_comparison> inputGoals; @@ -334,14 +309,15 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() if (resolvedResult.success()) { SingleDrvOutputs builtOutputs; + auto outputHashes = staticOutputHashes(worker.evalStore, *drv); auto resolvedHashes = staticOutputHashes(worker.store, drvResolved); StorePathSet outputPaths; for (auto & outputName : drvResolved.outputNames()) { - auto initialOutput = get(initialOutputs, outputName); + auto outputHash = get(outputHashes, outputName); auto resolvedHash = get(resolvedHashes, outputName); - if ((!initialOutput) || (!resolvedHash)) + if ((!outputHash) || (!resolvedHash)) throw Error( "derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/resolve)", worker.store.printStorePath(drvPath), @@ -368,7 +344,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() if (!drv->type().isImpure()) { auto newRealisation = realisation; - newRealisation.id = DrvOutput{initialOutput->outputHash, outputName}; + newRealisation.id = DrvOutput{*outputHash, outputName}; newRealisation.signatures.clear(); if (!drv->type().isFixed()) { auto & drvStore = worker.evalStore.isValidPath(drvPath) ? worker.evalStore : worker.store; @@ -441,6 +417,31 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() Goal::Co DerivationBuildingGoal::tryToBuild() { + /* Recheck at goal start. In particular, whereas before we were + given this information by the downstream goal, that cannot happen + anymore if the downstream goal only cares about one output, but + we care about all outputs. */ + auto outputHashes = staticOutputHashes(worker.evalStore, *drv); + for (auto & [outputName, outputHash] : outputHashes) { + InitialOutput v{.outputHash = outputHash}; + + /* TODO we might want to also allow randomizing the paths + for regular CA derivations, e.g. for sake of checking + determinism. */ + if (drv->type().isImpure()) { + v.known = InitialOutputStatus{ + .path = StorePath::random(outputPathName(drv->name, outputName)), + .status = PathStatus::Absent, + }; + } + + initialOutputs.insert({ + outputName, + std::move(v), + }); + } + checkPathValidity(); + auto started = [&]() { auto msg = fmt(buildMode == bmRepair ? "repairing outputs of '%s'" From a30bf96349604442265561ba305cb24793a09c79 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 Aug 2025 15:26:15 -0400 Subject: [PATCH 11/11] `DerivationBuildingGoal::initialOutputs` make local variable Also inline `assertPathValidity` in the process. --- .../build/derivation-building-goal.cc | 33 ++++++++++--------- .../store/build/derivation-building-goal.hh | 12 ++----- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 95f0ee9d50d..072bbfa93c3 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -417,7 +417,9 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution() Goal::Co DerivationBuildingGoal::tryToBuild() { - /* Recheck at goal start. In particular, whereas before we were + std::map initialOutputs; + + /* Recheck at this point. In particular, whereas before we were given this information by the downstream goal, that cannot happen anymore if the downstream goal only cares about one output, but we care about all outputs. */ @@ -440,7 +442,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() std::move(v), }); } - checkPathValidity(); + checkPathValidity(initialOutputs); auto started = [&]() { auto msg = @@ -528,7 +530,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() omitted, but that would be less efficient.) Note that since we now hold the locks on the output paths, no other process can build this derivation, so no further checks are necessary. */ - auto [allValid, validOutputs] = checkPathValidity(); + auto [allValid, validOutputs] = checkPathValidity(initialOutputs); if (buildMode != bmCheck && allValid) { debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath)); @@ -556,7 +558,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (buildLocally) { useHook = false; } else { - switch (tryBuildHook()) { + switch (tryBuildHook(initialOutputs)) { case rpAccept: /* Yes, it has started doing so. Wait until we get EOF from the hook. */ @@ -644,8 +646,16 @@ Goal::Co DerivationBuildingGoal::tryToBuild() We can only early return when the outputs are known a priori. For floating content-addressing derivations this isn't the case. + + Aborts if any output is not valid or corrupt, and otherwise + returns a 'SingleDrvOutputs' structure containing all outputs. */ - assertPathValidity(); + [&] { + auto [allValid, validOutputs] = checkPathValidity(initialOutputs); + if (!allValid) + throw Error("some outputs are unexpectedly invalid"); + return validOutputs; + }(); StorePathSet outputPaths; for (auto & [_, output] : builtOutputs) @@ -960,7 +970,7 @@ BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailur return BuildError{e.status, msg}; } -HookReply DerivationBuildingGoal::tryBuildHook() +HookReply DerivationBuildingGoal::tryBuildHook(const std::map & initialOutputs) { #ifdef _WIN32 // TODO enable build hook on Windows return rpDecline; @@ -1239,7 +1249,8 @@ std::map> DerivationBuildingGoal::queryPar return res; } -std::pair DerivationBuildingGoal::checkPathValidity() +std::pair +DerivationBuildingGoal::checkPathValidity(std::map & initialOutputs) { if (drv->type().isImpure()) return {false, {}}; @@ -1296,14 +1307,6 @@ std::pair DerivationBuildingGoal::checkPathValidity() return {allValid, validOutputs}; } -SingleDrvOutputs DerivationBuildingGoal::assertPathValidity() -{ - auto [allValid, validOutputs] = checkPathValidity(); - if (!allValid) - throw Error("some outputs are unexpectedly invalid"); - return validOutputs; -} - Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs) { buildResult.status = status; 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 07f9b21ae1b..d394eb3c9c3 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -55,8 +55,6 @@ private: */ StorePathSet inputPaths; - std::map initialOutputs; - /** * File descriptor for the log file. */ @@ -108,7 +106,7 @@ private: /** * Is the build hook willing to perform the build? */ - HookReply tryBuildHook(); + HookReply tryBuildHook(const std::map & initialOutputs); /** * Open a log file and a pipe to it. @@ -142,13 +140,7 @@ private: * whether all outputs are valid and non-corrupt, and a * 'SingleDrvOutputs' structure containing the valid outputs. */ - std::pair checkPathValidity(); - - /** - * Aborts if any output is not valid or corrupt, and otherwise - * returns a 'SingleDrvOutputs' structure containing all outputs. - */ - SingleDrvOutputs assertPathValidity(); + std::pair checkPathValidity(std::map & initialOutputs); /** * Forcibly kill the child process, if any.