From e2d330aeed19a1bfed56ea55277ac9400fbc7fed Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 11 Aug 2024 21:53:29 +0200 Subject: [PATCH] libstore: remove DerivationGoal::isReadDesc all derivation goals need a log fd of some description. let's save this single fd in a dedicated pointer field for all subclasses so that later we have just the one spot to change if we turn this into async promises Change-Id: If223adf90909247363fb823d751cae34d25d0c0b --- src/libstore/build/derivation-goal.cc | 13 ++++----- src/libstore/build/derivation-goal.hh | 9 +++++-- src/libstore/build/local-derivation-goal.cc | 30 +++++++++------------ src/libstore/build/local-derivation-goal.hh | 4 +-- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 17b6afbb2..f43a2a5b6 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -119,6 +119,7 @@ std::string DerivationGoal::key() void DerivationGoal::killChild() { hook.reset(); + builderOutFD = nullptr; } @@ -814,6 +815,7 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath) int DerivationGoal::getChildStatus() { + builderOutFD = nullptr; return hook->pid.kill(); } @@ -822,6 +824,7 @@ void DerivationGoal::closeReadPipes() { hook->builderOut.readSide.reset(); hook->fromHook.readSide.reset(); + builderOutFD = nullptr; } @@ -1209,6 +1212,7 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot) std::set fds; fds.insert(hook->fromHook.readSide.get()); fds.insert(hook->builderOut.readSide.get()); + builderOutFD = &hook->builderOut.readSide; worker.childStarted(shared_from_this(), fds, false, false); return rpAccept; @@ -1271,13 +1275,10 @@ void DerivationGoal::closeLogFile() } -bool DerivationGoal::isReadDesc(int fd) -{ - return fd == hook->builderOut.readSide.get(); -} - Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data) { + assert(builderOutFD); + auto tooMuchLogs = [&] { killChild(); return done( @@ -1287,7 +1288,7 @@ Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data }; // local & `ssh://`-builds are dealt with here. - auto isWrittenToLog = isReadDesc(fd); + auto isWrittenToLog = fd == builderOutFD->get(); if (isWrittenToLog) { logSize += data.size(); diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 250b437da..da935ceb5 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -186,6 +186,13 @@ struct DerivationGoal : public Goal */ std::unique_ptr hook; + /** + * Builder output is pulled from this file descriptor when not null. + * Owned by the derivation goal or subclass, must not be reset until + * the build has finished and no more output must be processed by us + */ + AutoCloseFD * builderOutFD = nullptr; + /** * The sort of derivation we are building. */ @@ -290,8 +297,6 @@ struct DerivationGoal : public Goal virtual void cleanupPostOutputsRegisteredModeCheck(); virtual void cleanupPostOutputsRegisteredModeNonCheck(); - virtual bool isReadDesc(int fd); - /** * Callback used by the worker to write to the log. */ diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 841d1f2f6..116fa0ae5 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -279,8 +279,10 @@ void LocalDerivationGoal::closeReadPipes() { if (hook) { DerivationGoal::closeReadPipes(); - } else - builderOut.close(); + } else { + builderOutPTY.close(); + builderOutFD = nullptr; + } } @@ -671,12 +673,13 @@ void LocalDerivationGoal::startBuilder() Path logFile = openLogFile(); /* Create a pseudoterminal to get the output of the builder. */ - builderOut = AutoCloseFD{posix_openpt(O_RDWR | O_NOCTTY)}; - if (!builderOut) + builderOutPTY = AutoCloseFD{posix_openpt(O_RDWR | O_NOCTTY)}; + if (!builderOutPTY) throw SysError("opening pseudoterminal master"); + builderOutFD = &builderOutPTY; // FIXME: not thread-safe, use ptsname_r - std::string slaveName = ptsname(builderOut.get()); + std::string slaveName = ptsname(builderOutPTY.get()); if (buildUser) { if (chmod(slaveName.c_str(), 0600)) @@ -687,12 +690,12 @@ void LocalDerivationGoal::startBuilder() } #if __APPLE__ else { - if (grantpt(builderOut.get())) + if (grantpt(builderOutPTY.get())) throw SysError("granting access to pseudoterminal slave"); } #endif - if (unlockpt(builderOut.get())) + if (unlockpt(builderOutPTY.get())) throw SysError("unlocking pseudoterminal"); /* Open the slave side of the pseudoterminal and use it as stderr. */ @@ -723,14 +726,14 @@ void LocalDerivationGoal::startBuilder() /* parent */ pid.setSeparatePG(true); - worker.childStarted(shared_from_this(), {builderOut.get()}, true, true); + worker.childStarted(shared_from_this(), {builderOutPTY.get()}, true, true); /* Check if setting up the build environment failed. */ std::vector msgs; while (true) { std::string msg = [&]() { try { - return readLine(builderOut.get()); + return readLine(builderOutPTY.get()); } catch (Error & e) { auto status = pid.wait(); e.addTrace({}, "while waiting for the build environment for '%s' to initialize (%s, previous messages: %s)", @@ -742,7 +745,7 @@ void LocalDerivationGoal::startBuilder() }(); if (msg.substr(0, 1) == "\2") break; if (msg.substr(0, 1) == "\1") { - FdSource source(builderOut.get()); + FdSource source(builderOutPTY.get()); auto ex = readError(source); ex.addTrace({}, "while setting up the build environment"); throw ex; @@ -2563,13 +2566,6 @@ void LocalDerivationGoal::deleteTmpDir(bool force) } -bool LocalDerivationGoal::isReadDesc(int fd) -{ - return (hook && DerivationGoal::isReadDesc(fd)) || - (!hook && fd == builderOut.get()); -} - - StorePath LocalDerivationGoal::makeFallbackPath(OutputNameView outputName) { return worker.store.makeStorePath( diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 7617760a5..128e6fd36 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -40,7 +40,7 @@ struct LocalDerivationGoal : public DerivationGoal * Master side of the pseudoterminal used for the builder's * standard output/error. */ - AutoCloseFD builderOut; + AutoCloseFD builderOutPTY; /** * Pipe for synchronising updates to the builder namespaces. @@ -285,8 +285,6 @@ struct LocalDerivationGoal : public DerivationGoal void cleanupPostOutputsRegisteredModeCheck() override; void cleanupPostOutputsRegisteredModeNonCheck() override; - bool isReadDesc(int fd) override; - /** * Delete the temporary directory, if we have one. */