From 1de97bbe2ebbdc7ae0f4f23aba1dacc622b14ab7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Mar 2025 17:07:34 -0400 Subject: [PATCH 01/13] Factor out "last 10 log lines" error message code This will help avoid duplication later. In particular, the next commit will not need to duplicate as much. --- src/libstore/build/derivation-goal.cc | 45 +++++++++++++++++---------- src/libstore/build/derivation-goal.hh | 7 +++++ 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 01da37df6..4a91e9811 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -908,6 +908,33 @@ void runPostBuildHook( }); } + +void appendLogTailErrorMsg( + Worker & worker, + const StorePath & drvPath, + const std::list & logTail, + std::string & msg) +{ + if (!logger->isVerbose() && !logTail.empty()) { + msg += fmt(";\nlast %d log lines:\n", logTail.size()); + for (auto & line : logTail) { + msg += "> "; + msg += line; + msg += "\n"; + } + auto nixLogCommand = experimentalFeatureSettings.isEnabled(Xp::NixCommand) + ? "nix log" + : "nix-store -l"; + // The command is on a separate line for easy copying, such as with triple click. + // This message will be indented elsewhere, so removing the indentation before the + // command will not put it at the start of the line unfortunately. + msg += fmt("For full logs, run:\n " ANSI_BOLD "%s %s" ANSI_NORMAL, + nixLogCommand, + worker.store.printStorePath(drvPath)); + } +} + + Goal::Co DerivationGoal::buildDone() { trace("build done"); @@ -959,23 +986,7 @@ Goal::Co DerivationGoal::buildDone() Magenta(worker.store.printStorePath(drvPath)), statusToString(status)); - if (!logger->isVerbose() && !logTail.empty()) { - msg += fmt(";\nlast %d log lines:\n", logTail.size()); - for (auto & line : logTail) { - msg += "> "; - msg += line; - msg += "\n"; - } - auto nixLogCommand = experimentalFeatureSettings.isEnabled(Xp::NixCommand) - ? "nix log" - : "nix-store -l"; - // The command is on a separate line for easy copying, such as with triple click. - // This message will be indented elsewhere, so removing the indentation before the - // command will not put it at the start of the line unfortunately. - msg += fmt("For full logs, run:\n " ANSI_BOLD "%s %s" ANSI_NORMAL, - nixLogCommand, - worker.store.printStorePath(drvPath)); - } + appendLogTailErrorMsg(worker, drvPath, logTail, msg); if (diskFull) msg += "\nnote: build failure may have been caused by lack of free disk space"; diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 4622cb2b1..fc4de9bdd 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -55,6 +55,13 @@ struct InitialOutput { std::optional known; }; +/** Used internally */ +void appendLogTailErrorMsg( + Worker & worker, + const StorePath & drvPath, + const std::list & logTail, + std::string & msg); + /** * A goal for building some or all of the outputs of a derivation. */ From e87ba8570546fc8b22f73cb4dfd1a60ebb80f63c Mon Sep 17 00:00:00 2001 From: Las Date: Mon, 10 Mar 2025 21:08:35 +0000 Subject: [PATCH 02/13] Inline `buildDone` from `DerivationGoal` into use sites The basic idea is that while we have duplicated this function, we now have one call-site in the local build case, and one call site in the build hook case. This unlocks big opportunities to specialize each copy, since they really shouldn't be doing the same things. By the time we are are done, there should not be much duplication left. See #12628 for further info. --- src/libstore/build/derivation-goal.cc | 4 +- src/libstore/build/derivation-goal.hh | 9 +- .../unix/build/local-derivation-goal.cc | 110 +++++++++++++++++- 3 files changed, 118 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 4a91e9811..6f69321b7 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -640,7 +640,7 @@ Goal::Co DerivationGoal::tryToBuild() buildResult.startTime = time(0); // inexact started(); co_await Suspend{}; - co_return buildDone(); + co_return hookDone(); case rpPostpone: /* Not now; wait until at least one child finishes or the wake-up timeout expires. */ @@ -935,7 +935,7 @@ void appendLogTailErrorMsg( } -Goal::Co DerivationGoal::buildDone() +Goal::Co DerivationGoal::hookDone() { trace("build done"); diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index fc4de9bdd..52830bbfc 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -55,6 +55,13 @@ struct InitialOutput { std::optional known; }; +/** Used internally */ +void runPostBuildHook( + Store & store, + Logger & logger, + const StorePath & drvPath, + const StorePathSet & outputPaths); + /** Used internally */ void appendLogTailErrorMsg( Worker & worker, @@ -246,7 +253,7 @@ struct DerivationGoal : public Goal Co gaveUpOnSubstitution(); Co tryToBuild(); virtual Co tryLocalBuild(); - Co buildDone(); + Co hookDone(); Co resolvedFinished(); diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 61a36dd51..80e339a53 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -263,8 +263,114 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() started(); co_await Suspend{}; - // after EOF on child - co_return buildDone(); + + trace("build done"); + + Finally releaseBuildUser([&](){ this->cleanupHookFinally(); }); + + cleanupPreChildKill(); + + /* 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 = getChildStatus(); + + debug("builder process 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. */ + closeReadPipes(); + + /* Close the log file. */ + closeLogFile(); + + cleanupPostChildKill(); + + if (buildResult.cpuUser && buildResult.cpuSystem) { + debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", + worker.store.printStorePath(drvPath), + status, + ((double) buildResult.cpuUser->count()) / 1000000, + ((double) buildResult.cpuSystem->count()) / 1000000); + } + + bool diskFull = false; + + try { + + /* Check the exit status. */ + if (!statusOk(status)) { + + diskFull |= cleanupDecideWhetherDiskFull(); + + auto msg = fmt("builder for '%s' %s", + Magenta(worker.store.printStorePath(drvPath)), + statusToString(status)); + + appendLogTailErrorMsg(worker, drvPath, logTail, msg); + + if (diskFull) + msg += "\nnote: build failure may have been caused by lack of free disk space"; + + throw BuildError(msg); + } + + /* Compute the FS closure of the outputs and register them as + being valid. */ + auto builtOutputs = registerOutputs(); + + StorePathSet outputPaths; + for (auto & [_, output] : builtOutputs) + outputPaths.insert(output.outPath); + runPostBuildHook( + worker.store, + *logger, + drvPath, + outputPaths + ); + + cleanupPostOutputsRegisteredModeNonCheck(); + + /* 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 done(BuildResult::Built, std::move(builtOutputs)); + + } catch (BuildError & e) { + outputLocks.unlock(); + + BuildResult::Status st = BuildResult::MiscFailure; + +#ifndef _WIN32 // TODO abstract over proc exit status + if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101) + st = BuildResult::TimedOut; + + else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) { + } + + else +#endif + { + assert(derivationType); + st = + dynamic_cast(&e) ? BuildResult::NotDeterministic : + statusOk(status) ? BuildResult::OutputRejected : + !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : + BuildResult::PermanentFailure; + } + + co_return done(st, {}, std::move(e)); + } } static void chmod_(const Path & path, mode_t mode) From 75feeecd5d616d663672909176ffe81952914cf1 Mon Sep 17 00:00:00 2001 From: Las Date: Mon, 10 Mar 2025 21:57:54 +0000 Subject: [PATCH 03/13] Start simplifying `{Local,}DerivationGoal` cleanup code Thanks to the previous commit, we can inline all these small callbacks. In the build-hook case, they were empty, and now they disappear entirely. While `LocalDerivationGoal` can be used in the hook case (we use it based on whether we have a local store, not based on whether we are using the build hook, a decision which comes later), the previous commit's inline moved the code into a spot where we know we are cleaning up after local building, *not* after running the build hook. This allows for much more simplification. --- src/libstore/build/derivation-goal.cc | 77 +++------------- src/libstore/build/derivation-goal.hh | 15 ---- .../unix/build/local-derivation-goal.cc | 89 ++++++------------- .../unix/build/local-derivation-goal.hh | 17 +--- 4 files changed, 43 insertions(+), 155 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 6f69321b7..c5931f00e 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -811,45 +811,6 @@ int DerivationGoal::getChildStatus() } -void DerivationGoal::closeReadPipes() -{ -#ifndef _WIN32 // TODO enable build hook on Windows - hook->builderOut.readSide.close(); - hook->fromHook.readSide.close(); -#endif -} - - -void DerivationGoal::cleanupHookFinally() -{ -} - - -void DerivationGoal::cleanupPreChildKill() -{ -} - - -void DerivationGoal::cleanupPostChildKill() -{ -} - - -bool DerivationGoal::cleanupDecideWhetherDiskFull() -{ - return false; -} - - -void DerivationGoal::cleanupPostOutputsRegisteredModeCheck() -{ -} - - -void DerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() -{ -} - void runPostBuildHook( Store & store, Logger & logger, @@ -937,11 +898,11 @@ void appendLogTailErrorMsg( Goal::Co DerivationGoal::hookDone() { - trace("build done"); +#ifndef _WIN32 + assert(hook); +#endif - Finally releaseBuildUser([&](){ this->cleanupHookFinally(); }); - - cleanupPreChildKill(); + 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 @@ -958,13 +919,14 @@ Goal::Co DerivationGoal::hookDone() worker.childTerminated(this); /* Close the read side of the logger pipe. */ - closeReadPipes(); +#ifndef _WIN32 // TODO enable build hook on Windows + hook->builderOut.readSide.close(); + hook->fromHook.readSide.close(); +#endif /* Close the log file. */ closeLogFile(); - cleanupPostChildKill(); - if (buildResult.cpuUser && buildResult.cpuSystem) { debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", worker.store.printStorePath(drvPath), @@ -973,24 +935,16 @@ Goal::Co DerivationGoal::hookDone() ((double) buildResult.cpuSystem->count()) / 1000000); } - bool diskFull = false; - try { /* Check the exit status. */ if (!statusOk(status)) { - - diskFull |= cleanupDecideWhetherDiskFull(); - auto msg = fmt("builder for '%s' %s", Magenta(worker.store.printStorePath(drvPath)), statusToString(status)); appendLogTailErrorMsg(worker, drvPath, logTail, msg); - if (diskFull) - msg += "\nnote: build failure may have been caused by lack of free disk space"; - throw BuildError(msg); } @@ -1008,8 +962,6 @@ Goal::Co DerivationGoal::hookDone() outputPaths ); - cleanupPostOutputsRegisteredModeNonCheck(); - /* 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 @@ -1024,23 +976,20 @@ Goal::Co DerivationGoal::hookDone() BuildResult::Status st = BuildResult::MiscFailure; -#ifndef _WIN32 // TODO abstract over proc exit status - if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101) +#ifndef _WIN32 + if (WIFEXITED(status) && WEXITSTATUS(status) == 101) st = BuildResult::TimedOut; - else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) { - } - - else -#endif + else if (WIFEXITED(status) && WEXITSTATUS(status) == 100) { assert(derivationType); st = dynamic_cast(&e) ? BuildResult::NotDeterministic : statusOk(status) ? BuildResult::OutputRejected : - !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : + !derivationType->isSandboxed() ? BuildResult::TransientFailure : BuildResult::PermanentFailure; } +#endif co_return done(st, {}, std::move(e)); } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 52830bbfc..0fd9afa71 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -285,21 +285,6 @@ struct DerivationGoal : public Goal */ void closeLogFile(); - /** - * Close the read side of the logger pipe. - */ - virtual void closeReadPipes(); - - /** - * Cleanup hooks for buildDone() - */ - virtual void cleanupHookFinally(); - virtual void cleanupPreChildKill(); - virtual void cleanupPostChildKill(); - virtual bool cleanupDecideWhetherDiskFull(); - virtual void cleanupPostOutputsRegisteredModeCheck(); - virtual void cleanupPostOutputsRegisteredModeNonCheck(); - virtual bool isReadDesc(Descriptor fd); /** diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 80e339a53..18f6518e3 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -184,6 +184,8 @@ void LocalDerivationGoal::killSandbox(bool getStats) Goal::Co LocalDerivationGoal::tryLocalBuild() { + assert(!hook); + unsigned int curBuilds = worker.getNrLocalBuilds(); if (curBuilds >= settings.maxBuildJobs) { worker.waitForBuildSlot(shared_from_this()); @@ -266,9 +268,15 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() trace("build done"); - Finally releaseBuildUser([&](){ this->cleanupHookFinally(); }); + 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(); + }); - cleanupPreChildKill(); + sandboxMountNamespace = -1; + sandboxUserNamespace = -1; /* Since we got an EOF on the logger pipe, the builder is presumed to have terminated. In fact, the builder could also have @@ -285,12 +293,20 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() worker.childTerminated(this); /* Close the read side of the logger pipe. */ - closeReadPipes(); + builderOut.close(); /* Close the log file. */ closeLogFile(); - cleanupPostChildKill(); + /* When running under a build user, make sure that all processes + running under that uid are gone. This is to prevent a + malicious user from leaving behind a process that keeps files + open and modifies them after they have been chown'ed to + root. */ + killSandbox(true); + + /* Terminate the recursive Nix daemon. */ + stopDaemon(); if (buildResult.cpuUser && buildResult.cpuSystem) { debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", @@ -335,7 +351,14 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() outputPaths ); - cleanupPostOutputsRegisteredModeNonCheck(); + /* Delete unused redirected outputs (when doing hash rewriting). */ + for (auto & i : redirectedOutputs) + deletePath(worker.store.Store::toRealPath(i.second)); + + /* Delete the chroot (if we were using one). */ + autoDelChroot.reset(); /* this runs the destructor */ + + deleteTmpDir(true); /* It is now safe to delete the lock files, since all future lockers will see that the output paths are valid; they will @@ -407,44 +430,6 @@ int LocalDerivationGoal::getChildStatus() return hook ? DerivationGoal::getChildStatus() : pid.kill(); } -void LocalDerivationGoal::closeReadPipes() -{ - if (hook) { - DerivationGoal::closeReadPipes(); - } else - builderOut.close(); -} - - -void LocalDerivationGoal::cleanupHookFinally() -{ - /* 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(); -} - - -void LocalDerivationGoal::cleanupPreChildKill() -{ - sandboxMountNamespace = -1; - sandboxUserNamespace = -1; -} - - -void LocalDerivationGoal::cleanupPostChildKill() -{ - /* When running under a build user, make sure that all processes - running under that uid are gone. This is to prevent a - malicious user from leaving behind a process that keeps files - open and modifies them after they have been chown'ed to - root. */ - killSandbox(true); - - /* Terminate the recursive Nix daemon. */ - stopDaemon(); -} - bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() { @@ -486,24 +471,6 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() } -void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck() -{ - deleteTmpDir(true); -} - - -void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() -{ - /* Delete unused redirected outputs (when doing hash rewriting). */ - for (auto & i : redirectedOutputs) - deletePath(worker.store.Store::toRealPath(i.second)); - - /* Delete the chroot (if we were using one). */ - autoDelChroot.reset(); /* this runs the destructor */ - - cleanupPostOutputsRegisteredModeCheck(); -} - #if __linux__ static void doBind(const Path & source, const Path & target, bool optional = false) { debug("bind mounting '%1%' to '%2%'", source, target); diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index c7a129f90..59b33d72a 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -264,21 +264,6 @@ struct LocalDerivationGoal : public DerivationGoal */ void checkOutputs(const std::map & outputs); - /** - * Close the read side of the logger pipe. - */ - void closeReadPipes() override; - - /** - * Cleanup hooks for buildDone() - */ - void cleanupHookFinally() override; - void cleanupPreChildKill() override; - void cleanupPostChildKill() override; - bool cleanupDecideWhetherDiskFull() override; - void cleanupPostOutputsRegisteredModeCheck() override; - void cleanupPostOutputsRegisteredModeNonCheck() override; - bool isReadDesc(int fd) override; /** @@ -299,6 +284,8 @@ struct LocalDerivationGoal : public DerivationGoal */ void killSandbox(bool getStats); + bool cleanupDecideWhetherDiskFull(); + /** * Create alternative path calculated from but distinct from the * input, so we can avoid overwriting outputs (or other store paths) From 145aa2f118c4403455854c951dfe83f92b96d538 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Mar 2025 18:08:10 -0400 Subject: [PATCH 04/13] Remove dead hook code in `LocalDerivationGoal::tryLocalBuild` The `assert` above proves that `hook` is not set. --- .../unix/build/local-derivation-goal.cc | 23 +++++-------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 18f6518e3..f17b0bced 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -374,23 +374,12 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() BuildResult::Status st = BuildResult::MiscFailure; -#ifndef _WIN32 // TODO abstract over proc exit status - if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101) - st = BuildResult::TimedOut; - - else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) { - } - - else -#endif - { - assert(derivationType); - st = - dynamic_cast(&e) ? BuildResult::NotDeterministic : - statusOk(status) ? BuildResult::OutputRejected : - !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : - BuildResult::PermanentFailure; - } + assert(derivationType); + st = + dynamic_cast(&e) ? BuildResult::NotDeterministic : + statusOk(status) ? BuildResult::OutputRejected : + !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : + BuildResult::PermanentFailure; co_return done(st, {}, std::move(e)); } From 87824bca6ba579498e98a4ccd447597fc01e0abf Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Mar 2025 18:08:58 -0400 Subject: [PATCH 05/13] Avoid pointless mutation The code that was in between is now gone. We can just set `st` correctly the first time. --- src/libstore/unix/build/local-derivation-goal.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index f17b0bced..b2e4c1186 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -372,10 +372,8 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() } catch (BuildError & e) { outputLocks.unlock(); - BuildResult::Status st = BuildResult::MiscFailure; - assert(derivationType); - st = + BuildResult::Status st = dynamic_cast(&e) ? BuildResult::NotDeterministic : statusOk(status) ? BuildResult::OutputRejected : !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : From 4b521f14ac14cb0767ba3bb512478396f8c90beb Mon Sep 17 00:00:00 2001 From: Las Date: Mon, 10 Mar 2025 22:03:22 +0000 Subject: [PATCH 06/13] Remove `privateNetwork` variable from local drv goal Can just inline its definition, it was immutable. --- src/libstore/unix/build/local-derivation-goal.cc | 7 ++----- src/libstore/unix/build/local-derivation-goal.hh | 5 ----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index b2e4c1186..bfc5e4c34 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -1032,9 +1032,6 @@ void LocalDerivationGoal::startBuilder() us. */ - if (derivationType->isSandboxed()) - privateNetwork = true; - userNamespaceSync.create(); usingUserNamespace = userNamespacesSupported(); @@ -1062,7 +1059,7 @@ void LocalDerivationGoal::startBuilder() ProcessOptions options; options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; - if (privateNetwork) + if (derivationType->isSandboxed()) options.cloneFlags |= CLONE_NEWNET; if (usingUserNamespace) options.cloneFlags |= CLONE_NEWUSER; @@ -1879,7 +1876,7 @@ void LocalDerivationGoal::runChild() userNamespaceSync.readSide = -1; - if (privateNetwork) { + if (derivationType->isSandboxed()) { /* Initialise the loopback interface. */ AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP)); diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index 59b33d72a..d52008eef 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -88,11 +88,6 @@ struct LocalDerivationGoal : public DerivationGoal */ std::shared_ptr autoDelChroot; - /** - * Whether to run the build in a private network namespace. - */ - bool privateNetwork = false; - /** * Stuff we need to pass to initChild(). */ From a87589a0359ac01fe03086cbd46d396e12256f65 Mon Sep 17 00:00:00 2001 From: Las Date: Mon, 10 Mar 2025 22:31:13 +0000 Subject: [PATCH 07/13] Simplify local drv goal a bit more - `chrootParentDir` can be a local variable instead of a class variable. - `getChildStatus` can be inlined. Again, we have the `assert(!hook);` in the local building case, which makes for a simpler thing inlined. --- src/libstore/build/derivation-goal.cc | 17 ++++++----------- src/libstore/build/derivation-goal.hh | 2 -- .../unix/build/local-derivation-goal.cc | 10 ++-------- .../unix/build/local-derivation-goal.hh | 9 --------- 4 files changed, 8 insertions(+), 30 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index c5931f00e..476f29238 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -801,16 +801,6 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath) } -int DerivationGoal::getChildStatus() -{ -#ifndef _WIN32 // TODO enable build hook on Windows - return hook->pid.kill(); -#else - return 0; -#endif -} - - void runPostBuildHook( Store & store, Logger & logger, @@ -908,7 +898,12 @@ Goal::Co DerivationGoal::hookDone() 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 = getChildStatus(); + int status = +#ifndef _WIN32 // TODO enable build hook on Windows + hook->pid.kill(); +#else + 0; +#endif debug("builder process for '%s' finished", worker.store.printStorePath(drvPath)); diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 0fd9afa71..177892996 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -262,8 +262,6 @@ struct DerivationGoal : public Goal */ HookReply tryBuildHook(); - virtual int getChildStatus(); - /** * Check that the derivation outputs all exist and register them * as valid. diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index bfc5e4c34..91dc85fdc 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -282,7 +282,7 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() 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 = getChildStatus(); + int status = pid.kill(); debug("builder process for '%s' finished", worker.store.printStorePath(drvPath)); @@ -412,12 +412,6 @@ static void movePath(const Path & src, const Path & dst) extern void replaceValidPath(const Path & storePath, const Path & tmpPath); -int LocalDerivationGoal::getChildStatus() -{ - return hook ? DerivationGoal::getChildStatus() : pid.kill(); -} - - bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() { bool diskFull = false; @@ -787,7 +781,7 @@ void LocalDerivationGoal::startBuilder() environment using bind-mounts. We put it in the Nix store so that the build outputs can be moved efficiently from the chroot to their final location. */ - chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot"; + auto chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot"; deletePath(chrootParentDir); /* Clean up the chroot directory automatically. */ diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index d52008eef..67d544b65 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -71,13 +71,6 @@ struct LocalDerivationGoal : public DerivationGoal */ bool useChroot = false; - /** - * The parent directory of `chrootRootDir`. It has permission 700 - * and is owned by root to ensure other users cannot mess with - * `chrootRootDir`. - */ - Path chrootParentDir; - /** * The root of the chroot environment. */ @@ -237,8 +230,6 @@ struct LocalDerivationGoal : public DerivationGoal */ void chownToBuilder(const Path & path); - int getChildStatus() override; - /** * Run the builder's process. */ From 0e7e1f5b57bf10166dbef70f23123033462477ad Mon Sep 17 00:00:00 2001 From: Las Date: Mon, 10 Mar 2025 23:15:59 +0000 Subject: [PATCH 08/13] Remove `registerOutputs` from drv goal Easy to inline in one spot, and assert in the other. --- src/libstore/build/derivation-goal.cc | 22 ++++++++----------- src/libstore/build/derivation-goal.hh | 6 ----- .../unix/build/local-derivation-goal.cc | 10 +-------- .../unix/build/local-derivation-goal.hh | 2 +- 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 476f29238..b673153a3 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -945,7 +945,15 @@ Goal::Co DerivationGoal::hookDone() /* Compute the FS closure of the outputs and register them as being valid. */ - auto builtOutputs = registerOutputs(); + 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) @@ -1174,18 +1182,6 @@ HookReply DerivationGoal::tryBuildHook() } -SingleDrvOutputs DerivationGoal::registerOutputs() -{ - /* 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. - */ - return assertPathValidity(); -} - Path DerivationGoal::openLogFile() { logSize = 0; diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 177892996..84d425b95 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -262,12 +262,6 @@ struct DerivationGoal : public Goal */ HookReply tryBuildHook(); - /** - * Check that the derivation outputs all exist and register them - * as valid. - */ - virtual SingleDrvOutputs registerOutputs(); - /** * Open a log file and a pipe to it. */ diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 91dc85fdc..659ac1b10 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -2330,15 +2330,7 @@ void LocalDerivationGoal::runChild() SingleDrvOutputs LocalDerivationGoal::registerOutputs() { - /* 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. - */ - if (hook) - return DerivationGoal::registerOutputs(); + assert(!hook); std::map infos; diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index 67d544b65..6bd3ce45f 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -239,7 +239,7 @@ struct LocalDerivationGoal : public DerivationGoal * Check that the derivation outputs all exist and register them * as valid. */ - SingleDrvOutputs registerOutputs() override; + SingleDrvOutputs registerOutputs(); void signRealisation(Realisation &) override; From db8439c32842139cca013078cf3230e6aec2d1be Mon Sep 17 00:00:00 2001 From: Las Date: Mon, 10 Mar 2025 23:21:03 +0000 Subject: [PATCH 09/13] Remove `signRealisation` from drv goal We can move this method from `LocalStore` to `Store` --- even if we only want the actual builder to sign things in many cases, there is no reason to try to enforce this policy by spurious moving the method to a subclass. Now, we might technically sign class, but CA derivations is experimental, and @Ericson2314 is going to revisit all this stuff with issue #11896 anyways. --- src/libstore/build/derivation-goal.cc | 2 +- src/libstore/build/derivation-goal.hh | 5 ----- src/libstore/local-store.cc | 13 ------------- src/libstore/local-store.hh | 1 - src/libstore/store-api.cc | 13 +++++++++++++ src/libstore/store-api.hh | 2 ++ src/libstore/unix/build/local-derivation-goal.cc | 7 +------ src/libstore/unix/build/local-derivation-goal.hh | 2 -- 8 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index b673153a3..3e7778656 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -1047,7 +1047,7 @@ Goal::Co DerivationGoal::resolvedFinished() : worker.store; newRealisation.dependentRealisations = drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore); } - signRealisation(newRealisation); + worker.store.signRealisation(newRealisation); worker.store.registerDrvOutput(newRealisation); } outputPaths.insert(realisation.outPath); diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 84d425b95..2eb36e98a 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -267,11 +267,6 @@ struct DerivationGoal : public Goal */ Path openLogFile(); - /** - * Sign the newly built realisation if the store allows it - */ - virtual void signRealisation(Realisation&) {} - /** * Close the log file. */ diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index b54903432..2fd160dd4 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1585,19 +1585,6 @@ void LocalStore::addSignatures(const StorePath & storePath, const StringSet & si } -void LocalStore::signRealisation(Realisation & realisation) -{ - // FIXME: keep secret keys in memory. - - auto secretKeyFiles = settings.secretKeyFiles; - - for (auto & secretKeyFile : secretKeyFiles.get()) { - SecretKey secretKey(readFile(secretKeyFile)); - LocalSigner signer(std::move(secretKey)); - realisation.sign(signer); - } -} - void LocalStore::signPathInfo(ValidPathInfo & info) { // FIXME: keep secret keys in memory. diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 83154d651..07d9cec0a 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -401,7 +401,6 @@ private: * specified by the ‘secret-key-files’ option. */ void signPathInfo(ValidPathInfo & info); - void signRealisation(Realisation &); void addBuildLog(const StorePath & drvPath, std::string_view log) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index fc3fbcc0f..e99e96aac 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1274,6 +1274,19 @@ Derivation Store::readDerivation(const StorePath & drvPath) Derivation Store::readInvalidDerivation(const StorePath & drvPath) { return readDerivationCommon(*this, drvPath, false); } +void Store::signRealisation(Realisation & realisation) +{ + // FIXME: keep secret keys in memory. + + auto secretKeyFiles = settings.secretKeyFiles; + + for (auto & secretKeyFile : secretKeyFiles.get()) { + SecretKey secretKey(readFile(secretKeyFile)); + LocalSigner signer(std::move(secretKey)); + realisation.sign(signer); + } +} + } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 2eba88ea0..83ca8344b 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -622,6 +622,8 @@ public: virtual void addSignatures(const StorePath & storePath, const StringSet & sigs) { unsupported("addSignatures"); } + void signRealisation(Realisation &); + /* Utility functions. */ /** diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 659ac1b10..7f6bace86 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -2872,7 +2872,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations) && !drv->type().isImpure()) { - signRealisation(thisRealisation); + worker.store.signRealisation(thisRealisation); worker.store.registerDrvOutput(thisRealisation); } builtOutputs.emplace(outputName, thisRealisation); @@ -2881,11 +2881,6 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() return builtOutputs; } -void LocalDerivationGoal::signRealisation(Realisation & realisation) -{ - getLocalStore().signRealisation(realisation); -} - void LocalDerivationGoal::checkOutputs(const std::map & outputs) { diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index 6bd3ce45f..ec8cf2c0b 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -241,8 +241,6 @@ struct LocalDerivationGoal : public DerivationGoal */ SingleDrvOutputs registerOutputs(); - void signRealisation(Realisation &) override; - /** * Check that an output meets the requirements specified by the * 'outputChecks' attribute (or the legacy From a39ed67180fa93d30de2d85cd8f486f34f4e0591 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Mar 2025 16:18:18 -0400 Subject: [PATCH 10/13] Do no store timestamps in the build result in the build hook case The variables are only set by CGroup mechanisms in `killSandbox` in the local build. In the build hook case, these variables will not be set, so there is nothing to do. --- src/libstore/build/derivation-goal.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 3e7778656..77ab559a6 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -922,14 +922,6 @@ Goal::Co DerivationGoal::hookDone() /* Close the log file. */ closeLogFile(); - if (buildResult.cpuUser && buildResult.cpuSystem) { - debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", - worker.store.printStorePath(drvPath), - status, - ((double) buildResult.cpuUser->count()) / 1000000, - ((double) buildResult.cpuSystem->count()) / 1000000); - } - try { /* Check the exit status. */ From 06af9cb532a3ae3d95cc6bf90484e0bb462cda22 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Mar 2025 16:26:16 -0400 Subject: [PATCH 11/13] Inline the try-catch `BuildError` in the hook case In the local building case, there is many things which can through `BuildError`, but in the hook case there is just this one. We can therefore simplify the code by "cinching" down the logic just to the spot the error is thrown. There is other code outside `libstore/build` which also uses `BuildError`, but I believe those cases are mistakes. The point of `BuildError` is the narrow technical use-cases of "errors which should not be fatal with `--keep-going`". Using it outside the building/scheduling code doesn't really make sense in that regard. It seems likely that those usages were instead merely because "oh, this error has something to do with building, so I guess `BuildError` is better than `Error`". It is quite likely that I myself used `BuildError` incorrectly as described above :). --- src/libstore/build/derivation-goal.cc | 80 +++++++++++++-------------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 77ab559a6..4c91c65fb 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -922,51 +922,16 @@ Goal::Co DerivationGoal::hookDone() /* Close the log file. */ closeLogFile(); - try { + /* Check the exit status. */ + if (!statusOk(status)) { + auto msg = fmt("builder for '%s' %s", + Magenta(worker.store.printStorePath(drvPath)), + statusToString(status)); - /* Check the exit status. */ - if (!statusOk(status)) { - auto msg = fmt("builder for '%s' %s", - Magenta(worker.store.printStorePath(drvPath)), - statusToString(status)); + appendLogTailErrorMsg(worker, drvPath, logTail, msg); - appendLogTailErrorMsg(worker, drvPath, logTail, msg); + auto e = BuildError(msg); - throw BuildError(msg); - } - - /* 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 done(BuildResult::Built, std::move(builtOutputs)); - - } catch (BuildError & e) { outputLocks.unlock(); BuildResult::Status st = BuildResult::MiscFailure; @@ -988,6 +953,37 @@ Goal::Co DerivationGoal::hookDone() co_return done(st, {}, 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 done(BuildResult::Built, std::move(builtOutputs)); } Goal::Co DerivationGoal::resolvedFinished() From 99d0dd3a432e5099e9990f7d44b523346d26e13b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Mar 2025 16:54:39 -0400 Subject: [PATCH 12/13] Simplify hook error status logic The simplification here is due to a long-standing bug, but it is not worth fixing the bug at this time. Instead we've finally written up an issue for the bug, and referenced the issue number in the code. --- src/libstore/build/derivation-goal.cc | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 4c91c65fb..7f2735489 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -930,28 +930,11 @@ Goal::Co DerivationGoal::hookDone() appendLogTailErrorMsg(worker, drvPath, logTail, msg); - auto e = BuildError(msg); - outputLocks.unlock(); - BuildResult::Status st = BuildResult::MiscFailure; + /* TODO (once again) support fine-grained error codes, see issue #12641. */ -#ifndef _WIN32 - if (WIFEXITED(status) && WEXITSTATUS(status) == 101) - st = BuildResult::TimedOut; - - else if (WIFEXITED(status) && WEXITSTATUS(status) == 100) - { - assert(derivationType); - st = - dynamic_cast(&e) ? BuildResult::NotDeterministic : - statusOk(status) ? BuildResult::OutputRejected : - !derivationType->isSandboxed() ? BuildResult::TransientFailure : - BuildResult::PermanentFailure; - } -#endif - - co_return done(st, {}, std::move(e)); + co_return done(BuildResult::MiscFailure, {}, BuildError(msg)); } /* Compute the FS closure of the outputs and register them as From dc0bc7f0a30837edca7551e03968a0b76139f4ea Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Mar 2025 17:07:18 -0400 Subject: [PATCH 13/13] Make debug message more precise --- src/libstore/build/derivation-goal.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 7f2735489..244846374 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -905,7 +905,7 @@ Goal::Co DerivationGoal::hookDone() 0; #endif - debug("builder process for '%s' finished", worker.store.printStorePath(drvPath)); + debug("build hook for '%s' finished", worker.store.printStorePath(drvPath)); buildResult.timesBuilt++; buildResult.stopTime = time(0);