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
This commit is contained in:
eldritch horrors 2024-08-11 21:53:29 +02:00
parent 007211e7a2
commit e2d330aeed
4 changed files with 28 additions and 28 deletions

View file

@ -119,6 +119,7 @@ std::string DerivationGoal::key()
void DerivationGoal::killChild() void DerivationGoal::killChild()
{ {
hook.reset(); hook.reset();
builderOutFD = nullptr;
} }
@ -814,6 +815,7 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath)
int DerivationGoal::getChildStatus() int DerivationGoal::getChildStatus()
{ {
builderOutFD = nullptr;
return hook->pid.kill(); return hook->pid.kill();
} }
@ -822,6 +824,7 @@ void DerivationGoal::closeReadPipes()
{ {
hook->builderOut.readSide.reset(); hook->builderOut.readSide.reset();
hook->fromHook.readSide.reset(); hook->fromHook.readSide.reset();
builderOutFD = nullptr;
} }
@ -1209,6 +1212,7 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot)
std::set<int> fds; std::set<int> fds;
fds.insert(hook->fromHook.readSide.get()); fds.insert(hook->fromHook.readSide.get());
fds.insert(hook->builderOut.readSide.get()); fds.insert(hook->builderOut.readSide.get());
builderOutFD = &hook->builderOut.readSide;
worker.childStarted(shared_from_this(), fds, false, false); worker.childStarted(shared_from_this(), fds, false, false);
return rpAccept; 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) Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data)
{ {
assert(builderOutFD);
auto tooMuchLogs = [&] { auto tooMuchLogs = [&] {
killChild(); killChild();
return done( return done(
@ -1287,7 +1288,7 @@ Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data
}; };
// local & `ssh://`-builds are dealt with here. // local & `ssh://`-builds are dealt with here.
auto isWrittenToLog = isReadDesc(fd); auto isWrittenToLog = fd == builderOutFD->get();
if (isWrittenToLog) if (isWrittenToLog)
{ {
logSize += data.size(); logSize += data.size();

View file

@ -186,6 +186,13 @@ struct DerivationGoal : public Goal
*/ */
std::unique_ptr<HookInstance> hook; std::unique_ptr<HookInstance> 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. * The sort of derivation we are building.
*/ */
@ -290,8 +297,6 @@ struct DerivationGoal : public Goal
virtual void cleanupPostOutputsRegisteredModeCheck(); virtual void cleanupPostOutputsRegisteredModeCheck();
virtual void cleanupPostOutputsRegisteredModeNonCheck(); virtual void cleanupPostOutputsRegisteredModeNonCheck();
virtual bool isReadDesc(int fd);
/** /**
* Callback used by the worker to write to the log. * Callback used by the worker to write to the log.
*/ */

View file

@ -279,8 +279,10 @@ void LocalDerivationGoal::closeReadPipes()
{ {
if (hook) { if (hook) {
DerivationGoal::closeReadPipes(); DerivationGoal::closeReadPipes();
} else } else {
builderOut.close(); builderOutPTY.close();
builderOutFD = nullptr;
}
} }
@ -671,12 +673,13 @@ void LocalDerivationGoal::startBuilder()
Path logFile = openLogFile(); Path logFile = openLogFile();
/* Create a pseudoterminal to get the output of the builder. */ /* Create a pseudoterminal to get the output of the builder. */
builderOut = AutoCloseFD{posix_openpt(O_RDWR | O_NOCTTY)}; builderOutPTY = AutoCloseFD{posix_openpt(O_RDWR | O_NOCTTY)};
if (!builderOut) if (!builderOutPTY)
throw SysError("opening pseudoterminal master"); throw SysError("opening pseudoterminal master");
builderOutFD = &builderOutPTY;
// FIXME: not thread-safe, use ptsname_r // FIXME: not thread-safe, use ptsname_r
std::string slaveName = ptsname(builderOut.get()); std::string slaveName = ptsname(builderOutPTY.get());
if (buildUser) { if (buildUser) {
if (chmod(slaveName.c_str(), 0600)) if (chmod(slaveName.c_str(), 0600))
@ -687,12 +690,12 @@ void LocalDerivationGoal::startBuilder()
} }
#if __APPLE__ #if __APPLE__
else { else {
if (grantpt(builderOut.get())) if (grantpt(builderOutPTY.get()))
throw SysError("granting access to pseudoterminal slave"); throw SysError("granting access to pseudoterminal slave");
} }
#endif #endif
if (unlockpt(builderOut.get())) if (unlockpt(builderOutPTY.get()))
throw SysError("unlocking pseudoterminal"); throw SysError("unlocking pseudoterminal");
/* Open the slave side of the pseudoterminal and use it as stderr. */ /* Open the slave side of the pseudoterminal and use it as stderr. */
@ -723,14 +726,14 @@ void LocalDerivationGoal::startBuilder()
/* parent */ /* parent */
pid.setSeparatePG(true); 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. */ /* Check if setting up the build environment failed. */
std::vector<std::string> msgs; std::vector<std::string> msgs;
while (true) { while (true) {
std::string msg = [&]() { std::string msg = [&]() {
try { try {
return readLine(builderOut.get()); return readLine(builderOutPTY.get());
} catch (Error & e) { } catch (Error & e) {
auto status = pid.wait(); auto status = pid.wait();
e.addTrace({}, "while waiting for the build environment for '%s' to initialize (%s, previous messages: %s)", 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) == "\2") break;
if (msg.substr(0, 1) == "\1") { if (msg.substr(0, 1) == "\1") {
FdSource source(builderOut.get()); FdSource source(builderOutPTY.get());
auto ex = readError(source); auto ex = readError(source);
ex.addTrace({}, "while setting up the build environment"); ex.addTrace({}, "while setting up the build environment");
throw ex; 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) StorePath LocalDerivationGoal::makeFallbackPath(OutputNameView outputName)
{ {
return worker.store.makeStorePath( return worker.store.makeStorePath(

View file

@ -40,7 +40,7 @@ struct LocalDerivationGoal : public DerivationGoal
* Master side of the pseudoterminal used for the builder's * Master side of the pseudoterminal used for the builder's
* standard output/error. * standard output/error.
*/ */
AutoCloseFD builderOut; AutoCloseFD builderOutPTY;
/** /**
* Pipe for synchronising updates to the builder namespaces. * Pipe for synchronising updates to the builder namespaces.
@ -285,8 +285,6 @@ struct LocalDerivationGoal : public DerivationGoal
void cleanupPostOutputsRegisteredModeCheck() override; void cleanupPostOutputsRegisteredModeCheck() override;
void cleanupPostOutputsRegisteredModeNonCheck() override; void cleanupPostOutputsRegisteredModeNonCheck() override;
bool isReadDesc(int fd) override;
/** /**
* Delete the temporary directory, if we have one. * Delete the temporary directory, if we have one.
*/ */