diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 1c76fe842..89dfcc6d9 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -54,24 +54,9 @@ DerivationBuildingGoal::~DerivationBuildingGoal() { /* Careful: we should never ever throw an exception from a destructor. */ - try { - killChild(); - } catch (...) { - ignoreExceptionInDestructor(); - } #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows - if (builder) { - try { - builder->stopDaemon(); - } catch (...) { - ignoreExceptionInDestructor(); - } - try { - builder->deleteTmpDir(false); - } catch (...) { - ignoreExceptionInDestructor(); - } - } + if (builder) + builder.reset(); #endif try { closeLogFile(); @@ -95,22 +80,8 @@ void DerivationBuildingGoal::killChild() hook.reset(); #endif #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows - if (builder && builder->pid != -1) { + if (builder && builder->killChild()) worker.childTerminated(this); - - // FIXME: move this into DerivationBuilder. - - /* If we're using a build user, then there is a tricky race - condition: if we kill the build user before the child has - done its setuid() to the build user uid, then it won't be - killed, and we'll potentially lock up in pid.wait(). So - also send a conventional kill to the child. */ - ::kill(-builder->pid, SIGKILL); /* ignore the result */ - - builder->killSandbox(true); - - builder->pid.wait(); - } #endif } @@ -653,11 +624,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild() goal.worker.childTerminated(&goal); } - void markContentsGood(const StorePath & path) override - { - goal.worker.markContentsGood(path); - } - Path openLogFile() override { return goal.openLogFile(); @@ -756,21 +722,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; @@ -780,11 +747,15 @@ 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) { + // for sake of `bmRepair` + worker.markContentsGood(output.outPath); outputPaths.insert(output.outPath); + } runPostBuildHook(worker.store, *logger, drvPath, outputPaths); /* It is now safe to delete the lock files, since all future @@ -793,9 +764,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 334f83b83..0338911c4 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -107,8 +107,6 @@ struct DerivationBuilderCallbacks * @todo this should be reworked */ virtual void childTerminated() = 0; - - virtual void markContentsGood(const StorePath & path) = 0; }; /** @@ -124,11 +122,6 @@ struct DerivationBuilderCallbacks */ struct DerivationBuilder : RestrictionContext { - /** - * The process ID of the builder. - */ - Pid pid; - DerivationBuilder() = default; virtual ~DerivationBuilder() = default; @@ -161,25 +154,18 @@ 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. - * @see startDaemon + * Forcibly kill the child process, if any. + * + * @returns whether the child was still alive and needed to be + * killed. */ - virtual void stopDaemon() = 0; - - /** - * Delete the temporary directory, if we have one. - */ - virtual void deleteTmpDir(bool force) = 0; - - /** - * Kill any processes running under the build user UID or in the - * cgroup of the build. - */ - virtual void killSandbox(bool getStats) = 0; + virtual bool killChild() = 0; }; #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows diff --git a/src/libstore/unix/build/chroot-derivation-builder.cc b/src/libstore/unix/build/chroot-derivation-builder.cc index 887bb47f0..8c9359533 100644 --- a/src/libstore/unix/build/chroot-derivation-builder.cc +++ b/src/libstore/unix/build/chroot-derivation-builder.cc @@ -22,13 +22,6 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl PathsInChroot pathsInChroot; - void deleteTmpDir(bool force) override - { - autoDelChroot.reset(); /* this runs the destructor */ - - DerivationBuilderImpl::deleteTmpDir(force); - } - bool needsHashRewrite() override { return false; @@ -166,13 +159,13 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl return !needsHashRewrite() ? chrootRootDir + p : store.toRealPath(p); } - void cleanupBuild() override + void cleanupBuild(bool force) override { - DerivationBuilderImpl::cleanupBuild(); + DerivationBuilderImpl::cleanupBuild(force); /* Move paths out of the chroot for easier debugging of build failures. */ - if (buildMode == bmNormal) + if (!force && buildMode == bmNormal) for (auto & [_, status] : initialOutputs) { if (!status.known) continue; @@ -182,6 +175,8 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl if (pathExists(chrootRootDir + p)) std::filesystem::rename((chrootRootDir + p), p); } + + autoDelChroot.reset(); /* this runs the destructor */ } std::pair addDependencyPrep(const StorePath & path) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 0dac66327..62864557a 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -71,6 +71,11 @@ class DerivationBuilderImpl : public DerivationBuilder, public DerivationBuilder { protected: + /** + * The process ID of the builder. + */ + Pid pid; + LocalStore & store; std::unique_ptr miscMethods; @@ -86,6 +91,27 @@ public: { } + ~DerivationBuilderImpl() + { + /* Careful: we should never ever throw an exception from a + destructor. */ + try { + killChild(); + } catch (...) { + ignoreExceptionInDestructor(); + } + try { + stopDaemon(); + } catch (...) { + ignoreExceptionInDestructor(); + } + try { + cleanupBuild(false); + } catch (...) { + ignoreExceptionInDestructor(); + } + } + protected: /** @@ -192,7 +218,7 @@ public: void startBuilder() override; - std::variant unprepareBuild() override; + SingleDrvOutputs unprepareBuild() override; protected: @@ -286,9 +312,11 @@ private: */ void startDaemon(); -public: - - void stopDaemon() override; + /** + * Stop the in-process nix daemon thread. + * @see startDaemon + */ + void stopDaemon(); protected: @@ -343,15 +371,25 @@ private: */ SingleDrvOutputs registerOutputs(); -public: - - void deleteTmpDir(bool force) override; - - void killSandbox(bool getStats) override; - protected: - virtual void cleanupBuild(); + /** + * Delete the temporary directory, if we have one. + * + * @param force We know the build suceeded, so don't attempt to + * preseve anything for debugging. + */ + virtual void cleanupBuild(bool force); + + /** + * Kill any processes running under the build user UID or in the + * cgroup of the build. + */ + virtual void killSandbox(bool getStats); + +public: + + bool killChild() override; private: @@ -414,6 +452,24 @@ void DerivationBuilderImpl::killSandbox(bool getStats) } } +bool DerivationBuilderImpl::killChild() +{ + bool ret = pid != -1; + if (ret) { + /* If we're using a build user, then there is a tricky race + condition: if we kill the build user before the child has + done its setuid() to the build user uid, then it won't be + killed, and we'll potentially lock up in pid.wait(). So + also send a conventional kill to the child. */ + ::kill(-pid, SIGKILL); /* ignore the result */ + + killSandbox(true); + + pid.wait(); + } + return ret; +} + bool DerivationBuilderImpl::prepareBuild() { if (useBuildUsers()) { @@ -427,7 +483,7 @@ bool DerivationBuilderImpl::prepareBuild() return true; } -std::variant DerivationBuilderImpl::unprepareBuild() +SingleDrvOutputs DerivationBuilderImpl::unprepareBuild() { // FIXME: get rid of this, rely on RAII. Finally releaseBuildUser([&]() { @@ -476,56 +532,42 @@ std::variant DerivationBuilderImpl::unprepareBuild ((double) buildResult.cpuSystem->count()) / 1000000); } - bool diskFull = false; + /* Check the exit status. */ + if (!statusOk(status)) { - try { + bool diskFull = decideWhetherDiskFull(); - /* Check the exit status. */ - if (!statusOk(status)) { + cleanupBuild(false); - diskFull |= decideWhetherDiskFull(); + auto msg = + fmt("Cannot build '%s'.\n" + "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".", + Magenta(store.printStorePath(drvPath)), + statusToString(status)); - cleanupBuild(); + msg += showKnownOutputs(store, drv); - auto msg = - fmt("Cannot build '%s'.\n" - "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".", - Magenta(store.printStorePath(drvPath)), - statusToString(status)); + miscMethods->appendLogTailErrorMsg(msg); - msg += showKnownOutputs(store, drv); + if (diskFull) + msg += "\nnote: build failure may have been caused by lack of free disk space"; - miscMethods->appendLogTailErrorMsg(msg); - - 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); } -} -void DerivationBuilderImpl::cleanupBuild() -{ - deleteTmpDir(false); + /* 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)); + + cleanupBuild(true); + + return builtOutputs; } static void chmod_(const Path & path, mode_t mode) @@ -1757,7 +1799,6 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() } store.optimisePath(actualPath, NoRepair); // FIXME: combine with scanForReferences() - miscMethods->markContentsGood(newInfo.path); newInfo.deriver = drvPath; newInfo.ultimate = true; @@ -1825,7 +1866,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() return builtOutputs; } -void DerivationBuilderImpl::deleteTmpDir(bool force) +void DerivationBuilderImpl::cleanupBuild(bool force) { if (topTmpDir != "") { /* As an extra precaution, even in the event of `deletePath` failing to 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;