From 4a1a5624619adf8133908bbb17893b3c4a030c0c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 13 Dec 2025 23:46:24 -0500 Subject: [PATCH 01/14] Inline the hook-only part of `processChildOutput` to that caller --- .../build/derivation-building-goal.cc | 84 ++++++++++--------- 1 file changed, 44 insertions(+), 40 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index a86a01fd8bb..53dce24d2d5 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -436,10 +436,53 @@ Goal::Co DerivationBuildingGoal::tryToBuild() buildResult.startTime = time(0); // inexact started(); +#ifndef _WIN32 + assert(hook); +#endif + while (true) { auto event = co_await WaitForChildEvent{}; if (auto * output = std::get_if(&event)) { - co_await processChildOutput(output->fd, output->data); + auto & fd = output->fd; + auto & data = output->data; + co_await processChildOutput(fd, data); + auto isWrittenToLog = isReadDesc(fd); + if (fd == hook->fromHook.readSide.get()) { + for (auto c : data) + if (c == '\n') { + auto json = parseJSONMessage(currentHookLine, "the derivation builder"); + if (json) { + auto s = handleJSONLogMessage( + *json, worker.act, hook->activities, "the derivation builder", true); + // ensure that logs from a builder using `ssh-ng://` as protocol + // are also available to `nix log`. + if (s && !isWrittenToLog && logSink) { + const auto type = (*json)["type"]; + const auto fields = (*json)["fields"]; + if (type == resBuildLogLine) { + (*logSink)((fields.size() > 0 ? fields[0].get() : "") + "\n"); + } else if (type == resSetPhase && !fields.is_null()) { + const auto phase = fields[0]; + if (!phase.is_null()) { + // nixpkgs' stdenv produces lines in the log to signal + // phase changes. + // We want to get the same lines in case of remote builds. + // The format is: + // @nix { "action": "setPhase", "phase": "$curPhase" } + const auto logLine = + nlohmann::json::object({{"action", "setPhase"}, {"phase", phase}}); + (*logSink)( + "@nix " + + logLine.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace) + + "\n"); + } + } + } + } + currentHookLine.clear(); + } else + currentHookLine += c; + } } else if (std::get_if(&event)) { if (!currentLogLine.empty()) flushLine(); @@ -450,10 +493,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild() } } -#ifndef _WIN32 - assert(hook); -#endif - trace("hook build done"); /* Since we got an EOF on the logger pipe, the builder is presumed @@ -1046,41 +1085,6 @@ Goal::Co DerivationBuildingGoal::processChildOutput(Descriptor fd, std::string_v (*logSink)(data); } -#ifndef _WIN32 // TODO enable build hook on Windows - if (hook && fd == hook->fromHook.readSide.get()) { - for (auto c : data) - if (c == '\n') { - auto json = parseJSONMessage(currentHookLine, "the derivation builder"); - if (json) { - auto s = handleJSONLogMessage(*json, worker.act, hook->activities, "the derivation builder", true); - // ensure that logs from a builder using `ssh-ng://` as protocol - // are also available to `nix log`. - if (s && !isWrittenToLog && logSink) { - const auto type = (*json)["type"]; - const auto fields = (*json)["fields"]; - if (type == resBuildLogLine) { - (*logSink)((fields.size() > 0 ? fields[0].get() : "") + "\n"); - } else if (type == resSetPhase && !fields.is_null()) { - const auto phase = fields[0]; - if (!phase.is_null()) { - // nixpkgs' stdenv produces lines in the log to signal - // phase changes. - // We want to get the same lines in case of remote builds. - // The format is: - // @nix { "action": "setPhase", "phase": "$curPhase" } - const auto logLine = nlohmann::json::object({{"action", "setPhase"}, {"phase", phase}}); - (*logSink)( - "@nix " + logLine.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace) - + "\n"); - } - } - } - } - currentHookLine.clear(); - } else - currentHookLine += c; - } -#endif co_return Return{}; } From 469212bd38217c2c7dcbea3865e373d0d34aca56 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 00:20:17 -0500 Subject: [PATCH 02/14] Inline `DerivationBuildingGoal::currentHookLine` That's easy to do now. --- src/libstore/build/derivation-building-goal.cc | 2 ++ .../include/nix/store/build/derivation-building-goal.hh | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 53dce24d2d5..b84bf83ace0 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -440,6 +440,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild() assert(hook); #endif + std::string currentHookLine; + while (true) { auto event = co_await WaitForChildEvent{}; if (auto * output = std::get_if(&event)) { 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 eb94df88bcd..6e61cb34510 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -81,8 +81,6 @@ private: std::string currentLogLine; size_t currentLogLinePos = 0; // to handle carriage return - std::string currentHookLine; - #ifndef _WIN32 // TODO enable build hook on Windows /** * The build hook. From 5be07abf6d6299a07104f5dfe14de8e00db8c2af Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 00:29:15 -0500 Subject: [PATCH 03/14] Inline `DerivationBuildingGoal::killChild` --- .../build/derivation-building-goal.cc | 24 ++++++++----------- .../store/build/derivation-building-goal.hh | 5 ---- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index b84bf83ace0..f428e1911db 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -60,17 +60,6 @@ std::string DerivationBuildingGoal::key() return "dd$" + std::string(drvPath.name()) + "$" + worker.store.printStorePath(drvPath); } -void DerivationBuildingGoal::killChild() -{ -#ifndef _WIN32 // TODO enable build hook on Windows - hook.reset(); -#endif -#ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows - if (builder && builder->killChild()) - worker.childTerminated(this); -#endif -} - std::string showKnownOutputs(const StoreDirConfig & store, const Derivation & drv) { std::string msg; @@ -490,7 +479,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() flushLine(); break; } else if (auto * timeout = std::get_if(&event)) { - killChild(); + hook.reset(); co_return doneFailure(std::move(*timeout)); } } @@ -721,7 +710,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild() flushLine(); break; } else if (auto * timeout = std::get_if(&event)) { - killChild(); + if (builder && builder->killChild()) + worker.childTerminated(this); co_return doneFailure(std::move(*timeout)); } } @@ -1064,7 +1054,13 @@ Goal::Co DerivationBuildingGoal::processChildOutput(Descriptor fd, std::string_v if (isWrittenToLog) { logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { - killChild(); +#ifndef _WIN32 // TODO enable build hook on Windows + hook.reset(); +#endif +#ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows + if (builder && builder->killChild()) + worker.childTerminated(this); +#endif co_return doneFailure(BuildError( BuildResult::Failure::LogLimitExceeded, "%s killed after writing more than %d bytes of log output", 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 6e61cb34510..4e5c69814d4 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -146,11 +146,6 @@ private: */ std::pair checkPathValidity(std::map & initialOutputs); - /** - * Forcibly kill the child process, if any. - */ - void killChild(); - Done doneSuccess(BuildResult::Success::Status status, SingleDrvOutputs builtOutputs); Done doneFailure(BuildError ex); From ee1383f75fb2d977540f53fcaa359d81e4d95198 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 00:33:44 -0500 Subject: [PATCH 04/14] Get cleanup logic out of `DerivationBuildingGoal::processChildOutput` This will allow for making `hook` and `builder` local variables. --- .../build/derivation-building-goal.cc | 37 +++++++++++-------- .../store/build/derivation-building-goal.hh | 6 ++- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index f428e1911db..b082a154a87 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -436,7 +436,10 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (auto * output = std::get_if(&event)) { auto & fd = output->fd; auto & data = output->data; - co_await processChildOutput(fd, data); + if (processChildOutput(fd, data)) { + hook.reset(); + co_return doneFailureLogTooLong(); + } auto isWrittenToLog = isReadDesc(fd); if (fd == hook->fromHook.readSide.get()) { for (auto c : data) @@ -704,7 +707,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() while (true) { auto event = co_await WaitForChildEvent{}; if (auto * output = std::get_if(&event)) { - co_await processChildOutput(output->fd, output->data); + if (processChildOutput(output->fd, output->data)) { + if (builder && builder->killChild()) + worker.childTerminated(this); + co_return doneFailureLogTooLong(); + } } else if (std::get_if(&event)) { if (!currentLogLine.empty()) flushLine(); @@ -1047,25 +1054,14 @@ bool DerivationBuildingGoal::isReadDesc(Descriptor fd) #endif } -Goal::Co DerivationBuildingGoal::processChildOutput(Descriptor fd, std::string_view data) +bool DerivationBuildingGoal::processChildOutput(Descriptor fd, std::string_view data) { // local & `ssh://`-builds are dealt with here. auto isWrittenToLog = isReadDesc(fd); if (isWrittenToLog) { logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { -#ifndef _WIN32 // TODO enable build hook on Windows - hook.reset(); -#endif -#ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows - if (builder && builder->killChild()) - worker.childTerminated(this); -#endif - co_return doneFailure(BuildError( - BuildResult::Failure::LogLimitExceeded, - "%s killed after writing more than %d bytes of log output", - getName(), - settings.maxLogSize)); + return true; } for (auto c : data) @@ -1083,7 +1079,16 @@ Goal::Co DerivationBuildingGoal::processChildOutput(Descriptor fd, std::string_v (*logSink)(data); } - co_return Return{}; + return false; +} + +Goal::Done DerivationBuildingGoal::doneFailureLogTooLong() +{ + return doneFailure(BuildError( + BuildResult::Failure::LogLimitExceeded, + "%s killed after writing more than %d bytes of log output", + getName(), + settings.maxLogSize)); } void DerivationBuildingGoal::flushLine() 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 4e5c69814d4..1e097d43d87 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -126,8 +126,12 @@ private: /** * Process output from a child process. + * + * @return true if log limit was exceeded and child should be killed. */ - Co processChildOutput(Descriptor fd, std::string_view data); + bool processChildOutput(Descriptor fd, std::string_view data); + + Done doneFailureLogTooLong(); void flushLine(); From 6e6dda9f67c637cd12ad2c3c2c022d2b36c30224 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 00:26:36 -0500 Subject: [PATCH 05/14] Inline `DerivationBuildingGoal::hook` Also remove uneeded arguments from `processChildOutput`. --- .../build/derivation-building-goal.cc | 74 ++++++++++--------- .../store/build/derivation-building-goal.hh | 13 ++-- 2 files changed, 44 insertions(+), 43 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index b082a154a87..cb7d4d807bd 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -238,6 +238,10 @@ Goal::Co DerivationBuildingGoal::tryToBuild() } checkPathValidity(initialOutputs); +#ifndef _WIN32 // TODO enable build hook on Windows + std::unique_ptr hook; +#endif + auto started = [&]() { auto msg = fmt(buildMode == bmRepair ? "repairing outputs of '%s'" @@ -355,7 +359,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (buildLocally) { useHook = false; } else { - switch (tryBuildHook(initialOutputs, drvOptions)) { + switch (tryBuildHook(initialOutputs, drvOptions, hook)) { case rpAccept: /* Yes, it has started doing so. Wait until we get EOF from the hook. */ @@ -436,12 +440,12 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (auto * output = std::get_if(&event)) { auto & fd = output->fd; auto & data = output->data; - if (processChildOutput(fd, data)) { - hook.reset(); - co_return doneFailureLogTooLong(); - } - auto isWrittenToLog = isReadDesc(fd); - if (fd == hook->fromHook.readSide.get()) { + if (fd == hook->builderOut.readSide.get()) { + if (processChildOutput(data)) { + hook.reset(); + co_return doneFailureLogTooLong(); + } + } else if (fd == hook->fromHook.readSide.get()) { for (auto c : data) if (c == '\n') { auto json = parseJSONMessage(currentHookLine, "the derivation builder"); @@ -450,7 +454,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() *json, worker.act, hook->activities, "the derivation builder", true); // ensure that logs from a builder using `ssh-ng://` as protocol // are also available to `nix log`. - if (s && !isWrittenToLog && logSink) { + if (s && logSink) { const auto type = (*json)["type"]; const auto fields = (*json)["fields"]; if (type == resBuildLogLine) { @@ -707,10 +711,12 @@ Goal::Co DerivationBuildingGoal::tryToBuild() while (true) { auto event = co_await WaitForChildEvent{}; if (auto * output = std::get_if(&event)) { - if (processChildOutput(output->fd, output->data)) { - if (builder && builder->killChild()) - worker.childTerminated(this); - co_return doneFailureLogTooLong(); + if (isReadDesc(output->fd)) { + if (processChildOutput(output->data)) { + if (builder && builder->killChild()) + worker.childTerminated(this); + co_return doneFailureLogTooLong(); + } } } else if (std::get_if(&event)) { if (!currentLogLine.empty()) @@ -885,7 +891,9 @@ BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailur } HookReply DerivationBuildingGoal::tryBuildHook( - const std::map & initialOutputs, const DerivationOptions & drvOptions) + const std::map & initialOutputs, + const DerivationOptions & drvOptions, + std::unique_ptr & hook) { #ifdef _WIN32 // TODO enable build hook on Windows return rpDecline; @@ -1050,34 +1058,30 @@ bool DerivationBuildingGoal::isReadDesc(Descriptor fd) #ifdef _WIN32 // TODO enable build hook on Windows return false; #else - return (hook && fd == hook->builderOut.readSide.get()) || (builder && fd == builder->builderOut.get()); + return builder && fd == builder->builderOut.get(); #endif } -bool DerivationBuildingGoal::processChildOutput(Descriptor fd, std::string_view data) +bool DerivationBuildingGoal::processChildOutput(std::string_view data) { - // local & `ssh://`-builds are dealt with here. - auto isWrittenToLog = isReadDesc(fd); - if (isWrittenToLog) { - logSize += data.size(); - if (settings.maxLogSize && logSize > settings.maxLogSize) { - return true; - } + logSize += data.size(); + if (settings.maxLogSize && logSize > settings.maxLogSize) { + return true; + } - for (auto c : data) - if (c == '\r') - currentLogLinePos = 0; - else if (c == '\n') - flushLine(); - else { - if (currentLogLinePos >= currentLogLine.size()) - currentLogLine.resize(currentLogLinePos + 1); - currentLogLine[currentLogLinePos++] = c; - } + for (auto c : data) + if (c == '\r') + currentLogLinePos = 0; + else if (c == '\n') + flushLine(); + else { + if (currentLogLinePos >= currentLogLine.size()) + currentLogLine.resize(currentLogLinePos + 1); + currentLogLine[currentLogLinePos++] = c; + } - if (logSink) - (*logSink)(data); - } + if (logSink) + (*logSink)(data); return false; } 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 1e097d43d87..5fa727d2ce2 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -82,11 +82,6 @@ private: size_t currentLogLinePos = 0; // to handle carriage return #ifndef _WIN32 // TODO enable build hook on Windows - /** - * The build hook. - */ - std::unique_ptr hook; - std::unique_ptr builder; #endif @@ -110,7 +105,9 @@ private: * Is the build hook willing to perform the build? */ HookReply tryBuildHook( - const std::map & initialOutputs, const DerivationOptions & drvOptions); + const std::map & initialOutputs, + const DerivationOptions & drvOptions, + std::unique_ptr & hook); /** * Open a log file and a pipe to it. @@ -125,11 +122,11 @@ private: bool isReadDesc(Descriptor fd); /** - * Process output from a child process. + * Process log output from a child process. * * @return true if log limit was exceeded and child should be killed. */ - bool processChildOutput(Descriptor fd, std::string_view data); + bool processChildOutput(std::string_view data); Done doneFailureLogTooLong(); From eff403b5abc45851d683989f35dd60d40fcda267 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 00:49:20 -0500 Subject: [PATCH 06/14] Inline `DerivationBuildingGoal::builder` --- .../build/derivation-building-goal.cc | 23 ++++--------------- .../store/build/derivation-building-goal.hh | 6 ----- 2 files changed, 4 insertions(+), 25 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index cb7d4d807bd..cc467daf054 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -44,10 +44,6 @@ DerivationBuildingGoal::~DerivationBuildingGoal() { /* Careful: we should never ever throw an exception from a destructor. */ -#ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows - if (builder) - builder.reset(); -#endif try { closeLogFile(); } catch (...) { @@ -409,9 +405,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild() Magenta( "/usr/sbin/softwareupdate --install-rosetta && launchctl stop org.nixos.nix-daemon")); -#ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows - builder.reset(); -#endif outputLocks.unlock(); worker.permanentFailure = true; co_return doneFailure({BuildResult::Failure::InputRejected, std::move(msg)}); @@ -584,6 +577,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() #else assert(!hook); + std::unique_ptr builder; Descriptor builderOut; // Will continue here while waiting for a build user below @@ -711,9 +705,9 @@ Goal::Co DerivationBuildingGoal::tryToBuild() while (true) { auto event = co_await WaitForChildEvent{}; if (auto * output = std::get_if(&event)) { - if (isReadDesc(output->fd)) { + if (output->fd == builder->builderOut.get()) { if (processChildOutput(output->data)) { - if (builder && builder->killChild()) + if (builder->killChild()) worker.childTerminated(this); co_return doneFailureLogTooLong(); } @@ -723,7 +717,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() flushLine(); break; } else if (auto * timeout = std::get_if(&event)) { - if (builder && builder->killChild()) + if (builder->killChild()) worker.childTerminated(this); co_return doneFailure(std::move(*timeout)); } @@ -1053,15 +1047,6 @@ void DerivationBuildingGoal::closeLogFile() fdLogFile.close(); } -bool DerivationBuildingGoal::isReadDesc(Descriptor fd) -{ -#ifdef _WIN32 // TODO enable build hook on Windows - return false; -#else - return builder && fd == builder->builderOut.get(); -#endif -} - bool DerivationBuildingGoal::processChildOutput(std::string_view data) { logSize += data.size(); 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 5fa727d2ce2..3072aed0a71 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -81,10 +81,6 @@ private: std::string currentLogLine; size_t currentLogLinePos = 0; // to handle carriage return -#ifndef _WIN32 // TODO enable build hook on Windows - std::unique_ptr builder; -#endif - BuildMode buildMode; std::unique_ptr> mcRunningBuilds; @@ -119,8 +115,6 @@ private: */ void closeLogFile(); - bool isReadDesc(Descriptor fd); - /** * Process log output from a child process. * From 43a472631b6c80424c7dc253bfe4ec27e1809c97 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 01:14:43 -0500 Subject: [PATCH 07/14] Factor out `BuildLog` This gets some trickier pure logic out of `DerivationBuilderGoal`. --- src/libstore/build/build-log.cc | 47 +++++++++++ .../build/derivation-building-goal.cc | 60 +++++--------- .../include/nix/store/build/build-log.hh | 80 +++++++++++++++++++ .../store/build/derivation-building-goal.hh | 22 +++-- src/libstore/include/nix/store/meson.build | 1 + src/libstore/meson.build | 1 + 6 files changed, 157 insertions(+), 54 deletions(-) create mode 100644 src/libstore/build/build-log.cc create mode 100644 src/libstore/include/nix/store/build/build-log.hh diff --git a/src/libstore/build/build-log.cc b/src/libstore/build/build-log.cc new file mode 100644 index 00000000000..2fa016113b5 --- /dev/null +++ b/src/libstore/build/build-log.cc @@ -0,0 +1,47 @@ +#include "nix/store/build/build-log.hh" + +namespace nix { + +BuildLog::BuildLog(size_t maxTailLines, LineCallback onLine) + : maxTailLines(maxTailLines) + , onLine(std::move(onLine)) +{ +} + +void BuildLog::operator()(std::string_view data) +{ + for (auto c : data) + if (c == '\r') + currentLogLinePos = 0; + else if (c == '\n') + flushLine(); + else { + if (currentLogLinePos >= currentLogLine.size()) + currentLogLine.resize(currentLogLinePos + 1); + currentLogLine[currentLogLinePos++] = c; + } +} + +void BuildLog::flush() +{ + if (!currentLogLine.empty()) + flushLine(); +} + +void BuildLog::flushLine() +{ + // Truncate to actual content (currentLogLinePos may be less than size due to \r) + currentLogLine.resize(currentLogLinePos); + + if (!onLine(currentLogLine)) { + // Line was not handled as JSON, add to tail + logTail.push_back(currentLogLine); + if (logTail.size() > maxTailLines) + logTail.pop_front(); + } + + currentLogLine.clear(); + currentLogLinePos = 0; +} + +} // namespace nix diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index cc467daf054..9cd1e4dccb3 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -434,10 +434,14 @@ Goal::Co DerivationBuildingGoal::tryToBuild() auto & fd = output->fd; auto & data = output->data; if (fd == hook->builderOut.readSide.get()) { - if (processChildOutput(data)) { + logSize += data.size(); + if (settings.maxLogSize && logSize > settings.maxLogSize) { hook.reset(); co_return doneFailureLogTooLong(); } + (*buildLog)(data); + if (logSink) + (*logSink)(data); } else if (fd == hook->fromHook.readSide.get()) { for (auto c : data) if (c == '\n') { @@ -475,8 +479,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() currentHookLine += c; } } else if (std::get_if(&event)) { - if (!currentLogLine.empty()) - flushLine(); + buildLog->flush(); break; } else if (auto * timeout = std::get_if(&event)) { hook.reset(); @@ -706,15 +709,18 @@ Goal::Co DerivationBuildingGoal::tryToBuild() auto event = co_await WaitForChildEvent{}; if (auto * output = std::get_if(&event)) { if (output->fd == builder->builderOut.get()) { - if (processChildOutput(output->data)) { + logSize += output->data.size(); + if (settings.maxLogSize && logSize > settings.maxLogSize) { if (builder->killChild()) worker.childTerminated(this); co_return doneFailureLogTooLong(); } + (*buildLog)(output->data); + if (logSink) + (*logSink)(output->data); } } else if (std::get_if(&event)) { - if (!currentLogLine.empty()) - flushLine(); + buildLog->flush(); break; } else if (auto * timeout = std::get_if(&event)) { if (builder->killChild()) @@ -862,6 +868,7 @@ BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailur msg += showKnownOutputs(worker.store, *drv); + auto & logTail = buildLog->getTail(); if (!logger->isVerbose() && !logTail.empty()) { msg += fmt("\nLast %d log lines:\n", logTail.size()); for (auto & line : logTail) { @@ -997,7 +1004,8 @@ HookReply DerivationBuildingGoal::tryBuildHook( Path DerivationBuildingGoal::openLogFile() { - logSize = 0; + buildLog = std::make_unique( + settings.logLines, [this](std::string_view line) { return handleLogLine(line); }); if (!settings.keepLog) return ""; @@ -1047,27 +1055,14 @@ void DerivationBuildingGoal::closeLogFile() fdLogFile.close(); } -bool DerivationBuildingGoal::processChildOutput(std::string_view data) +bool DerivationBuildingGoal::handleLogLine(std::string_view line) { - logSize += data.size(); - if (settings.maxLogSize && logSize > settings.maxLogSize) { + std::string lineStr{line}; + if (handleJSONLogMessage(lineStr, *act, builderActivities, "the derivation builder", false)) { return true; } - for (auto c : data) - if (c == '\r') - currentLogLinePos = 0; - else if (c == '\n') - flushLine(); - else { - if (currentLogLinePos >= currentLogLine.size()) - currentLogLine.resize(currentLogLinePos + 1); - currentLogLine[currentLogLinePos++] = c; - } - - if (logSink) - (*logSink)(data); - + act->result(resBuildLogLine, lineStr); return false; } @@ -1080,23 +1075,6 @@ Goal::Done DerivationBuildingGoal::doneFailureLogTooLong() settings.maxLogSize)); } -void DerivationBuildingGoal::flushLine() -{ - if (handleJSONLogMessage(currentLogLine, *act, builderActivities, "the derivation builder", false)) - ; - - else { - logTail.push_back(currentLogLine); - if (logTail.size() > settings.logLines) - logTail.pop_front(); - - act->result(resBuildLogLine, currentLogLine); - } - - currentLogLine = ""; - currentLogLinePos = 0; -} - std::map> DerivationBuildingGoal::queryPartialDerivationOutputMap() { assert(!drv->type().isImpure()); diff --git a/src/libstore/include/nix/store/build/build-log.hh b/src/libstore/include/nix/store/build/build-log.hh new file mode 100644 index 00000000000..28e53d3add1 --- /dev/null +++ b/src/libstore/include/nix/store/build/build-log.hh @@ -0,0 +1,80 @@ +#pragma once +///@file + +#include "nix/util/serialise.hh" + +#include +#include +#include + +namespace nix { + +/** + * Pure line buffering and log tracking for build output. + * + * This class handles: + * - Tracking log size (for enforcing limits) + * - Buffering partial lines (handling \r and \n) + * - Maintaining a tail of recent log lines (for error messages) + * + * Implements Sink so it can be used as a data destination. + * I/O is handled separately by the caller. + */ +struct BuildLog : Sink +{ + /** + * Callback for complete log lines. + * @param line The complete log line (without newline) + * @return true if line was handled as structured JSON (don't add to tail) + */ + using LineCallback = std::function; + +private: + size_t maxTailLines; + LineCallback onLine; + + std::list logTail; + std::string currentLogLine; + size_t currentLogLinePos = 0; // to handle carriage return + + void flushLine(); + +public: + /** + * @param maxTailLines Maximum number of tail lines to keep + * @param onLine Callback for each complete line + */ + BuildLog(size_t maxTailLines, LineCallback onLine); + + /** + * Process output data from child process. + * Calls the stored callback for each complete line encountered. + * @param data Raw output data from child + */ + void operator()(std::string_view data) override; + + /** + * Flush any remaining partial line. + * Call this when the child process exits. + */ + void flush(); + + /** + * Get the most recent log lines. + * Used for including in error messages. + */ + const std::list & getTail() const + { + return logTail; + } + + /** + * Check if there's an incomplete line buffered. + */ + bool hasPartialLine() const + { + return !currentLogLine.empty(); + } +}; + +} // namespace nix 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 3072aed0a71..abdb1cfeafb 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -9,6 +9,7 @@ #include "nix/store/store-api.hh" #include "nix/store/pathlocks.hh" #include "nix/store/build/goal.hh" +#include "nix/store/build/build-log.hh" namespace nix { @@ -69,17 +70,14 @@ private: std::shared_ptr logFileSink, logSink; /** - * Number of bytes received from the builder's stdout/stderr. + * Number of bytes received from the builder. */ - unsigned long logSize; + uint64_t logSize = 0; /** - * The most recent log lines. + * Build log line processor (pure, no I/O). */ - std::list logTail; - - std::string currentLogLine; - size_t currentLogLinePos = 0; // to handle carriage return + std::unique_ptr buildLog; BuildMode buildMode; @@ -116,16 +114,14 @@ private: void closeLogFile(); /** - * Process log output from a child process. - * - * @return true if log limit was exceeded and child should be killed. + * Callback for BuildLog line processing. + * Handles JSON log messages and emits to activity. + * @return true if line was handled as JSON */ - bool processChildOutput(std::string_view data); + bool handleLogLine(std::string_view line); Done doneFailureLogTooLong(); - void flushLine(); - /** * Wrappers around the corresponding Store methods that first consult the * derivation. This is currently needed because when there is no drv file diff --git a/src/libstore/include/nix/store/meson.build b/src/libstore/include/nix/store/meson.build index c17d6a9cb5a..c7026818ea7 100644 --- a/src/libstore/include/nix/store/meson.build +++ b/src/libstore/include/nix/store/meson.build @@ -13,6 +13,7 @@ headers = [ config_pub_h ] + files( 'aws-creds.hh', 'binary-cache-store.hh', 'build-result.hh', + 'build/build-log.hh', 'build/derivation-builder.hh', 'build/derivation-building-goal.hh', 'build/derivation-building-misc.hh', diff --git a/src/libstore/meson.build b/src/libstore/meson.build index 6d570229966..0e5a9cb7bb4 100644 --- a/src/libstore/meson.build +++ b/src/libstore/meson.build @@ -275,6 +275,7 @@ subdir('nix-meson-build-support/common') sources = files( 'binary-cache-store.cc', 'build-result.cc', + 'build/build-log.cc', 'build/derivation-builder.cc', 'build/derivation-building-goal.cc', 'build/derivation-check.cc', From 6a32b754a9790f3922f72bd423a2702e1b66db7d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 02:07:50 -0500 Subject: [PATCH 08/14] Inline `DerivationBuildingGoal::logSize` Separate state for each loop. --- src/libstore/build/derivation-building-goal.cc | 3 +++ .../include/nix/store/build/derivation-building-goal.hh | 5 ----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 9cd1e4dccb3..f68c2d2d409 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -427,6 +427,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() #endif std::string currentHookLine; + uint64_t logSize = 0; while (true) { auto event = co_await WaitForChildEvent{}; @@ -705,6 +706,8 @@ Goal::Co DerivationBuildingGoal::tryToBuild() started(); + uint64_t logSize = 0; + while (true) { auto event = co_await WaitForChildEvent{}; if (auto * output = std::get_if(&event)) { 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 abdb1cfeafb..9ee91b15bc8 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -69,11 +69,6 @@ private: AutoCloseFD fdLogFile; std::shared_ptr logFileSink, logSink; - /** - * Number of bytes received from the builder. - */ - uint64_t logSize = 0; - /** * Build log line processor (pure, no I/O). */ From a8e8614fd35a0a1683b6cd3bc3b38164a2a4999e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 02:17:15 -0500 Subject: [PATCH 09/14] `BuildLog` owns activities Now it does more, using the logging.hh functionality as needed. --- src/libstore/build/build-log.cc | 9 +-- .../build/derivation-building-goal.cc | 59 ++++++++----------- .../include/nix/store/build/build-log.hh | 32 +++++----- .../store/build/derivation-building-goal.hh | 13 +--- 4 files changed, 47 insertions(+), 66 deletions(-) diff --git a/src/libstore/build/build-log.cc b/src/libstore/build/build-log.cc index 2fa016113b5..a8fb64fc68b 100644 --- a/src/libstore/build/build-log.cc +++ b/src/libstore/build/build-log.cc @@ -2,9 +2,9 @@ namespace nix { -BuildLog::BuildLog(size_t maxTailLines, LineCallback onLine) +BuildLog::BuildLog(size_t maxTailLines, std::unique_ptr act) : maxTailLines(maxTailLines) - , onLine(std::move(onLine)) + , act(std::move(act)) { } @@ -33,8 +33,9 @@ void BuildLog::flushLine() // Truncate to actual content (currentLogLinePos may be less than size due to \r) currentLogLine.resize(currentLogLinePos); - if (!onLine(currentLogLine)) { - // Line was not handled as JSON, add to tail + if (!handleJSONLogMessage(currentLogLine, *act, builderActivities, "the derivation builder", false)) { + // Line was not handled as JSON, emit and add to tail + act->result(resBuildLogLine, currentLogLine); logTail.push_back(currentLogLine); if (logTail.size() > maxTailLines) logTail.pop_front(); diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index f68c2d2d409..475fd933c01 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -248,19 +248,21 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (hook) msg += fmt(" on '%s'", hook->machineName); #endif - act = std::make_unique( - *logger, - lvlInfo, - actBuild, - msg, - Logger::Fields{ - worker.store.printStorePath(drvPath), + buildLog = std::make_unique( + settings.logLines, + std::make_unique( + *logger, + lvlInfo, + actBuild, + msg, + Logger::Fields{ + worker.store.printStorePath(drvPath), #ifndef _WIN32 // TODO enable build hook on Windows - hook ? hook->machineName : + hook ? hook->machineName : #endif - "", - 1, - 1}); + "", + 1, + 1})); mcRunningBuilds = std::make_unique>(worker.runningBuilds); worker.updateProgress(); }; @@ -603,8 +605,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() { DerivationBuildingGoal & goal; - DerivationBuildingGoalCallbacks( - DerivationBuildingGoal & goal, std::unique_ptr & builder) + DerivationBuildingGoalCallbacks(DerivationBuildingGoal & goal) : goal{goal} { } @@ -673,15 +674,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild() /* If we have to wait and retry (see below), then `builder` will already be created, so we don't need to create it again. */ - builder = externalBuilder ? makeExternalDerivationBuilder( - *localStoreP, - std::make_unique(*this, builder), - std::move(params), - *externalBuilder) - : makeDerivationBuilder( - *localStoreP, - std::make_unique(*this, builder), - std::move(params)); + builder = + externalBuilder + ? makeExternalDerivationBuilder( + *localStoreP, + std::make_unique(*this), + std::move(params), + *externalBuilder) + : makeDerivationBuilder( + *localStoreP, std::make_unique(*this), std::move(params)); } if (auto builderOutOpt = builder->startBuild()) { @@ -1007,9 +1008,6 @@ HookReply DerivationBuildingGoal::tryBuildHook( Path DerivationBuildingGoal::openLogFile() { - buildLog = std::make_unique( - settings.logLines, [this](std::string_view line) { return handleLogLine(line); }); - if (!settings.keepLog) return ""; @@ -1058,17 +1056,6 @@ void DerivationBuildingGoal::closeLogFile() fdLogFile.close(); } -bool DerivationBuildingGoal::handleLogLine(std::string_view line) -{ - std::string lineStr{line}; - if (handleJSONLogMessage(lineStr, *act, builderActivities, "the derivation builder", false)) { - return true; - } - - act->result(resBuildLogLine, lineStr); - return false; -} - Goal::Done DerivationBuildingGoal::doneFailureLogTooLong() { return doneFailure(BuildError( diff --git a/src/libstore/include/nix/store/build/build-log.hh b/src/libstore/include/nix/store/build/build-log.hh index 28e53d3add1..cdc9125734d 100644 --- a/src/libstore/include/nix/store/build/build-log.hh +++ b/src/libstore/include/nix/store/build/build-log.hh @@ -1,37 +1,31 @@ #pragma once ///@file +#include "nix/util/logging.hh" #include "nix/util/serialise.hh" -#include #include +#include #include namespace nix { /** - * Pure line buffering and log tracking for build output. + * Line buffering and log tracking for build output. * * This class handles: - * - Tracking log size (for enforcing limits) + * - Owning the build Activity for logging * - Buffering partial lines (handling \r and \n) * - Maintaining a tail of recent log lines (for error messages) + * - Processing JSON log messages via handleJSONLogMessage * * Implements Sink so it can be used as a data destination. * I/O is handled separately by the caller. */ struct BuildLog : Sink { - /** - * Callback for complete log lines. - * @param line The complete log line (without newline) - * @return true if line was handled as structured JSON (don't add to tail) - */ - using LineCallback = std::function; - private: size_t maxTailLines; - LineCallback onLine; std::list logTail; std::string currentLogLine; @@ -40,15 +34,25 @@ private: void flushLine(); public: + /** + * The build activity. Owned by BuildLog. + */ + std::unique_ptr act; + + /** + * Map for tracking nested activities from JSON messages. + */ + std::map builderActivities; + /** * @param maxTailLines Maximum number of tail lines to keep - * @param onLine Callback for each complete line + * @param act Activity for this build */ - BuildLog(size_t maxTailLines, LineCallback onLine); + BuildLog(size_t maxTailLines, std::unique_ptr act); /** * Process output data from child process. - * Calls the stored callback for each complete line encountered. + * Handles JSON log messages and emits regular lines to activity. * @param data Raw output data from child */ void operator()(std::string_view data) override; 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 9ee91b15bc8..7b5b035c627 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -70,7 +70,7 @@ private: std::shared_ptr logFileSink, logSink; /** - * Build log line processor (pure, no I/O). + * Build log line processor. Also owns the build Activity. */ std::unique_ptr buildLog; @@ -78,10 +78,6 @@ private: std::unique_ptr> mcRunningBuilds; - std::unique_ptr act; - - std::map builderActivities; - std::string key() override; /** @@ -108,13 +104,6 @@ private: */ void closeLogFile(); - /** - * Callback for BuildLog line processing. - * Handles JSON log messages and emits to activity. - * @return true if line was handled as JSON - */ - bool handleLogLine(std::string_view line); - Done doneFailureLogTooLong(); /** From bde0b22dc8e50b7289b23c7ed469ce83502c8a6c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 01:34:58 -0500 Subject: [PATCH 10/14] Factor out `LogFile`, do more RAII It is weird that `LogFile` and `BuildLog` are basically unrelated, but the does currently reflect the logic that exists. --- .../build/derivation-building-goal.cc | 140 ++++++++++-------- .../nix/store/build/derivation-builder.hh | 2 +- .../store/build/derivation-building-goal.hh | 28 +--- 3 files changed, 85 insertions(+), 85 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 475fd933c01..91fc1221d11 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -40,16 +40,7 @@ DerivationBuildingGoal::DerivationBuildingGoal( worker.store.addTempRoot(this->drvPath); } -DerivationBuildingGoal::~DerivationBuildingGoal() -{ - /* Careful: we should never ever throw an exception from a - destructor. */ - try { - closeLogFile(); - } catch (...) { - ignoreExceptionInDestructor(); - } -} +DerivationBuildingGoal::~DerivationBuildingGoal() = default; std::string DerivationBuildingGoal::key() { @@ -173,6 +164,19 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution(bool storeDerivation) co_return tryToBuild(); } +/** + * RAII wrapper for build log file. + * Constructor opens the log file, destructor closes it. + */ +struct LogFile +{ + AutoCloseFD fd; + std::shared_ptr fileSink, sink; + + LogFile(Store & store, const StorePath & drvPath); + ~LogFile(); +}; + Goal::Co DerivationBuildingGoal::tryToBuild() { auto drvOptions = [&] { @@ -238,6 +242,13 @@ Goal::Co DerivationBuildingGoal::tryToBuild() std::unique_ptr hook; #endif + std::unique_ptr buildLog; + std::unique_ptr logFile; + + auto openLogFile = [&]() { logFile = std::make_unique(worker.store, drvPath); }; + + auto closeLogFile = [&]() { logFile.reset(); }; + auto started = [&]() { auto msg = fmt(buildMode == bmRepair ? "repairing outputs of '%s'" @@ -357,7 +368,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (buildLocally) { useHook = false; } else { - switch (tryBuildHook(initialOutputs, drvOptions, hook)) { + switch (tryBuildHook(initialOutputs, drvOptions, hook, openLogFile)) { case rpAccept: /* Yes, it has started doing so. Wait until we get EOF from the hook. */ @@ -440,11 +451,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { hook.reset(); - co_return doneFailureLogTooLong(); + co_return doneFailureLogTooLong(*buildLog); } (*buildLog)(data); - if (logSink) - (*logSink)(data); + if (logFile->sink) + (*logFile->sink)(data); } else if (fd == hook->fromHook.readSide.get()) { for (auto c : data) if (c == '\n') { @@ -454,11 +465,12 @@ Goal::Co DerivationBuildingGoal::tryToBuild() *json, worker.act, hook->activities, "the derivation builder", true); // ensure that logs from a builder using `ssh-ng://` as protocol // are also available to `nix log`. - if (s && logSink) { + if (s && logFile->sink) { const auto type = (*json)["type"]; const auto fields = (*json)["fields"]; if (type == resBuildLogLine) { - (*logSink)((fields.size() > 0 ? fields[0].get() : "") + "\n"); + (*logFile->sink)( + (fields.size() > 0 ? fields[0].get() : "") + "\n"); } else if (type == resSetPhase && !fields.is_null()) { const auto phase = fields[0]; if (!phase.is_null()) { @@ -469,7 +481,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() // @nix { "action": "setPhase", "phase": "$curPhase" } const auto logLine = nlohmann::json::object({{"action", "setPhase"}, {"phase", phase}}); - (*logSink)( + (*logFile->sink)( "@nix " + logLine.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace) + "\n"); @@ -522,7 +534,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() /* Check the exit status. */ if (!statusOk(status)) { - auto e = fixupBuilderFailureErrorMessage({BuildResult::Failure::MiscFailure, status, ""}); + auto e = fixupBuilderFailureErrorMessage({BuildResult::Failure::MiscFailure, status, ""}, *buildLog); outputLocks.unlock(); @@ -604,9 +616,16 @@ Goal::Co DerivationBuildingGoal::tryToBuild() struct DerivationBuildingGoalCallbacks : DerivationBuilderCallbacks { DerivationBuildingGoal & goal; + std::function openLogFileFn; + std::function closeLogFileFn; - DerivationBuildingGoalCallbacks(DerivationBuildingGoal & goal) + DerivationBuildingGoalCallbacks( + DerivationBuildingGoal & goal, + std::function openLogFileFn, + std::function closeLogFileFn) : goal{goal} + , openLogFileFn{std::move(openLogFileFn)} + , closeLogFileFn{std::move(closeLogFileFn)} { } @@ -617,14 +636,14 @@ Goal::Co DerivationBuildingGoal::tryToBuild() goal.worker.childTerminated(&goal); } - std::filesystem::path openLogFile() override + void openLogFile() override { - return goal.openLogFile(); + openLogFileFn(); } void closeLogFile() override { - goal.closeLogFile(); + closeLogFileFn(); } }; @@ -674,15 +693,16 @@ Goal::Co DerivationBuildingGoal::tryToBuild() /* If we have to wait and retry (see below), then `builder` will already be created, so we don't need to create it again. */ - builder = - externalBuilder - ? makeExternalDerivationBuilder( - *localStoreP, - std::make_unique(*this), - std::move(params), - *externalBuilder) - : makeDerivationBuilder( - *localStoreP, std::make_unique(*this), std::move(params)); + builder = externalBuilder + ? makeExternalDerivationBuilder( + *localStoreP, + std::make_unique(*this, openLogFile, closeLogFile), + std::move(params), + *externalBuilder) + : makeDerivationBuilder( + *localStoreP, + std::make_unique(*this, openLogFile, closeLogFile), + std::move(params)); } if (auto builderOutOpt = builder->startBuild()) { @@ -717,11 +737,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (settings.maxLogSize && logSize > settings.maxLogSize) { if (builder->killChild()) worker.childTerminated(this); - co_return doneFailureLogTooLong(); + co_return doneFailureLogTooLong(*buildLog); } (*buildLog)(output->data); - if (logSink) - (*logSink)(output->data); + if (logFile->sink) + (*logFile->sink)(output->data); } } else if (std::get_if(&event)) { buildLog->flush(); @@ -741,7 +761,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() } catch (BuilderFailureError & e) { builder.reset(); outputLocks.unlock(); - co_return doneFailure(fixupBuilderFailureErrorMessage(std::move(e))); + co_return doneFailure(fixupBuilderFailureErrorMessage(std::move(e), *buildLog)); } catch (BuildError & e) { builder.reset(); outputLocks.unlock(); @@ -862,7 +882,7 @@ static void runPostBuildHook( }); } -BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailureError e) +BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailureError e, BuildLog & buildLog) { auto msg = fmt("Cannot build '%s'.\n" @@ -872,7 +892,7 @@ BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailur msg += showKnownOutputs(worker.store, *drv); - auto & logTail = buildLog->getTail(); + auto & logTail = buildLog.getTail(); if (!logger->isVerbose() && !logTail.empty()) { msg += fmt("\nLast %d log lines:\n", logTail.size()); for (auto & line : logTail) { @@ -898,7 +918,8 @@ BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailur HookReply DerivationBuildingGoal::tryBuildHook( const std::map & initialOutputs, const DerivationOptions & drvOptions, - std::unique_ptr & hook) + std::unique_ptr & hook, + std::function openLogFile) { #ifdef _WIN32 // TODO enable build hook on Windows return rpDecline; @@ -995,7 +1016,7 @@ HookReply DerivationBuildingGoal::tryBuildHook( hook->toHook.writeSide.close(); /* Create the log file and pipe. */ - [[maybe_unused]] Path logFile = openLogFile(); + openLogFile(); std::set fds; fds.insert(hook->fromHook.readSide.get()); @@ -1006,16 +1027,15 @@ HookReply DerivationBuildingGoal::tryBuildHook( #endif } -Path DerivationBuildingGoal::openLogFile() +LogFile::LogFile(Store & store, const StorePath & drvPath) { if (!settings.keepLog) - return ""; + return; - auto baseName = std::string(baseNameOf(worker.store.printStorePath(drvPath))); + auto baseName = std::string(baseNameOf(store.printStorePath(drvPath))); - /* Create a log file. */ Path logDir; - if (auto localStore = dynamic_cast(&worker.store)) + if (auto localStore = dynamic_cast(&store)) logDir = localStore->config->logDir; else logDir = settings.nixLogDir; @@ -1024,7 +1044,7 @@ Path DerivationBuildingGoal::openLogFile() Path logFileName = fmt("%s/%s%s", dir, baseName.substr(2), settings.compressLog ? ".bz2" : ""); - fdLogFile = toDescriptor(open( + fd = toDescriptor(open( logFileName.c_str(), O_CREAT | O_WRONLY | O_TRUNC #ifndef _WIN32 @@ -1032,31 +1052,31 @@ Path DerivationBuildingGoal::openLogFile() #endif , 0666)); - if (!fdLogFile) + if (!fd) throw SysError("creating log file '%1%'", logFileName); - logFileSink = std::make_shared(fdLogFile.get()); + fileSink = std::make_shared(fd.get()); if (settings.compressLog) - logSink = std::shared_ptr(makeCompressionSink(CompressionAlgo::bzip2, *logFileSink)); + sink = std::shared_ptr(makeCompressionSink(CompressionAlgo::bzip2, *fileSink)); else - logSink = logFileSink; - - return logFileName; + sink = fileSink; } -void DerivationBuildingGoal::closeLogFile() +LogFile::~LogFile() { - auto logSink2 = std::dynamic_pointer_cast(logSink); - if (logSink2) - logSink2->finish(); - if (logFileSink) - logFileSink->flush(); - logSink = logFileSink = 0; - fdLogFile.close(); + try { + auto sink2 = std::dynamic_pointer_cast(sink); + if (sink2) + sink2->finish(); + if (fileSink) + fileSink->flush(); + } catch (...) { + ignoreExceptionInDestructor(); + } } -Goal::Done DerivationBuildingGoal::doneFailureLogTooLong() +Goal::Done DerivationBuildingGoal::doneFailureLogTooLong(BuildLog & buildLog) { return doneFailure(BuildError( BuildResult::Failure::LogLimitExceeded, diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index 29011ee5b55..f19b7002f15 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -111,7 +111,7 @@ struct DerivationBuilderCallbacks /** * Open a log file and a pipe to it. */ - virtual std::filesystem::path openLogFile() = 0; + virtual void openLogFile() = 0; /** * Close the log file. 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 7b5b035c627..835a2754ead 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -63,17 +63,6 @@ private: */ StorePathSet inputPaths; - /** - * File descriptor for the log file. - */ - AutoCloseFD fdLogFile; - std::shared_ptr logFileSink, logSink; - - /** - * Build log line processor. Also owns the build Activity. - */ - std::unique_ptr buildLog; - BuildMode buildMode; std::unique_ptr> mcRunningBuilds; @@ -92,19 +81,10 @@ private: HookReply tryBuildHook( const std::map & initialOutputs, const DerivationOptions & drvOptions, - std::unique_ptr & hook); - - /** - * Open a log file and a pipe to it. - */ - Path openLogFile(); - - /** - * Close the log file. - */ - void closeLogFile(); + std::unique_ptr & hook, + std::function openLogFile); - Done doneFailureLogTooLong(); + Done doneFailureLogTooLong(BuildLog & buildLog); /** * Wrappers around the corresponding Store methods that first consult the @@ -125,7 +105,7 @@ private: Done doneFailure(BuildError ex); - BuildError fixupBuilderFailureErrorMessage(BuilderFailureError msg); + BuildError fixupBuilderFailureErrorMessage(BuilderFailureError msg, BuildLog & buildLog); JobCategory jobCategory() const override { From ec65132ab91112ad7af8ad48200522299259c7ff Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 02:48:06 -0500 Subject: [PATCH 11/14] Shrink `DerivationBuildingGoal::tryBuildHook` There was a bunch of logic in there which was, effectively, using the build hook, rather than deciding *whether* to use the build hook. We want it to only be the latter. --- .../build/derivation-building-goal.cc | 86 +++++++++---------- .../store/build/derivation-building-goal.hh | 6 +- 2 files changed, 42 insertions(+), 50 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 91fc1221d11..8a53cb247a5 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -368,7 +368,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (buildLocally) { useHook = false; } else { - switch (tryBuildHook(initialOutputs, drvOptions, hook, openLogFile)) { + switch (tryBuildHook(drvOptions)) { case rpAccept: /* Yes, it has started doing so. Wait until we get EOF from the hook. */ @@ -432,6 +432,45 @@ Goal::Co DerivationBuildingGoal::tryToBuild() actLock.reset(); if (useHook) { + hook = std::move(worker.hook); + + try { + hook->machineName = readLine(hook->fromHook.readSide.get()); + } catch (Error & e) { + e.addTrace({}, "while reading the machine name from the build hook"); + throw; + } + + CommonProto::WriteConn conn{hook->sink}; + + /* Tell the hook all the inputs that have to be copied to the + remote system. */ + CommonProto::write(worker.store, conn, inputPaths); + + /* Tell the hooks the missing outputs that have to be copied back + from the remote system. */ + { + StringSet missingOutputs; + for (auto & [outputName, status] : initialOutputs) { + // XXX: Does this include known CA outputs? + if (buildMode != bmCheck && status.known && status.known->isValid()) + continue; + missingOutputs.insert(outputName); + } + CommonProto::write(worker.store, conn, missingOutputs); + } + + hook->sink = FdSink(); + hook->toHook.writeSide.close(); + + /* Create the log file and pipe. */ + openLogFile(); + + std::set fds; + fds.insert(hook->fromHook.readSide.get()); + fds.insert(hook->builderOut.readSide.get()); + worker.childStarted(shared_from_this(), fds, false, false); + buildResult.startTime = time(0); // inexact started(); @@ -915,11 +954,7 @@ BuildError DerivationBuildingGoal::fixupBuilderFailureErrorMessage(BuilderFailur return BuildError{e.status, msg}; } -HookReply DerivationBuildingGoal::tryBuildHook( - const std::map & initialOutputs, - const DerivationOptions & drvOptions, - std::unique_ptr & hook, - std::function openLogFile) +HookReply DerivationBuildingGoal::tryBuildHook(const DerivationOptions & drvOptions) { #ifdef _WIN32 // TODO enable build hook on Windows return rpDecline; @@ -984,45 +1019,6 @@ HookReply DerivationBuildingGoal::tryBuildHook( throw; } - hook = std::move(worker.hook); - - try { - hook->machineName = readLine(hook->fromHook.readSide.get()); - } catch (Error & e) { - e.addTrace({}, "while reading the machine name from the build hook"); - throw; - } - - CommonProto::WriteConn conn{hook->sink}; - - /* Tell the hook all the inputs that have to be copied to the - remote system. */ - CommonProto::write(worker.store, conn, inputPaths); - - /* Tell the hooks the missing outputs that have to be copied back - from the remote system. */ - { - StringSet missingOutputs; - for (auto & [outputName, status] : initialOutputs) { - // XXX: Does this include known CA outputs? - if (buildMode != bmCheck && status.known && status.known->isValid()) - continue; - missingOutputs.insert(outputName); - } - CommonProto::write(worker.store, conn, missingOutputs); - } - - hook->sink = FdSink(); - hook->toHook.writeSide.close(); - - /* Create the log file and pipe. */ - openLogFile(); - - std::set fds; - fds.insert(hook->fromHook.readSide.get()); - fds.insert(hook->builderOut.readSide.get()); - worker.childStarted(shared_from_this(), fds, false, false); - return rpAccept; #endif } 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 835a2754ead..47b04821289 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -78,11 +78,7 @@ private: /** * Is the build hook willing to perform the build? */ - HookReply tryBuildHook( - const std::map & initialOutputs, - const DerivationOptions & drvOptions, - std::unique_ptr & hook, - std::function openLogFile); + HookReply tryBuildHook(const DerivationOptions & drvOptions); Done doneFailureLogTooLong(BuildLog & buildLog); From 47ccf201fbd42227ad3ca6e24b431f9c46e6593c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 03:11:23 -0500 Subject: [PATCH 12/14] Make `DerivationBuildingGoal::inputPaths` local variable --- src/libstore/build/derivation-building-goal.cc | 8 +++++--- .../include/nix/store/build/derivation-building-goal.hh | 8 +------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 8a53cb247a5..e94931814dd 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -122,6 +122,8 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution(bool storeDerivation) writeDerivation(worker.store, *drv); } + StorePathSet inputPaths; + { /* If we get this far, we know no dynamic drvs inputs */ @@ -161,7 +163,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution(bool storeDerivation) slot to become available, since we don't need one if there is a build hook. */ co_await yield(); - co_return tryToBuild(); + co_return tryToBuild(std::move(inputPaths)); } /** @@ -177,7 +179,7 @@ struct LogFile ~LogFile(); }; -Goal::Co DerivationBuildingGoal::tryToBuild() +Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) { auto drvOptions = [&] { DerivationOptions temp; @@ -644,7 +646,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() if (curBuilds >= settings.maxBuildJobs) { outputLocks.unlock(); co_await waitForBuildSlot(); - co_return tryToBuild(); + co_return tryToBuild(std::move(inputPaths)); } if (!builder) { 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 47b04821289..ed9465a6ce1 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -57,12 +57,6 @@ private: * The remainder is state held during the build. */ - /** - * All input paths (that is, the union of FS closures of the - * immediate input paths). - */ - StorePathSet inputPaths; - BuildMode buildMode; std::unique_ptr> mcRunningBuilds; @@ -73,7 +67,7 @@ private: * The states. */ Co gaveUpOnSubstitution(bool storeDerivation); - Co tryToBuild(); + Co tryToBuild(StorePathSet inputPaths); /** * Is the build hook willing to perform the build? From cf5644ce99534d170519848e34a8984e39b88932 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 03:12:48 -0500 Subject: [PATCH 13/14] `DerivationBuildingGoal` Make some fields constant These are immutable parameters, not state, set in the constructor. --- .../include/nix/store/build/derivation-building-goal.hh | 6 +++--- 1 file changed, 3 insertions(+), 3 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 ed9465a6ce1..a509844f6f0 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -46,18 +46,18 @@ struct DerivationBuildingGoal : public Goal private: /** The path of the derivation. */ - StorePath drvPath; + const StorePath drvPath; /** * The derivation stored at drvPath. */ - std::unique_ptr drv; + const std::unique_ptr drv; /** * The remainder is state held during the build. */ - BuildMode buildMode; + const BuildMode buildMode; std::unique_ptr> mcRunningBuilds; From 1807dc78d6fc91fc42598a3b7095fb8121ce9160 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 14 Dec 2025 03:03:28 -0500 Subject: [PATCH 14/14] Split out separate coroutine functions for with/without hook This makes the logic much easier to follow. Unlike before, the use of separate functions is not making us pass a gazillion arguments or use the crutch of class variables. --- .../build/derivation-building-goal.cc | 400 +++++++++--------- .../store/build/derivation-building-goal.hh | 12 + 2 files changed, 219 insertions(+), 193 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index e94931814dd..f3b2d61fb14 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -240,46 +240,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) } checkPathValidity(initialOutputs); -#ifndef _WIN32 // TODO enable build hook on Windows - std::unique_ptr hook; -#endif - - std::unique_ptr buildLog; - std::unique_ptr logFile; - - auto openLogFile = [&]() { logFile = std::make_unique(worker.store, drvPath); }; - - auto closeLogFile = [&]() { logFile.reset(); }; - - auto started = [&]() { - auto msg = - fmt(buildMode == bmRepair ? "repairing outputs of '%s'" - : buildMode == bmCheck ? "checking outputs of '%s'" - : "building '%s'", - worker.store.printStorePath(drvPath)); -#ifndef _WIN32 // TODO enable build hook on Windows - if (hook) - msg += fmt(" on '%s'", hook->machineName); -#endif - buildLog = std::make_unique( - settings.logLines, - std::make_unique( - *logger, - lvlInfo, - actBuild, - msg, - Logger::Fields{ - worker.store.printStorePath(drvPath), -#ifndef _WIN32 // TODO enable build hook on Windows - hook ? hook->machineName : -#endif - "", - 1, - 1})); - mcRunningBuilds = std::make_unique>(worker.runningBuilds); - worker.updateProgress(); - }; - /** * Activity that denotes waiting for a lock. */ @@ -434,191 +394,225 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) actLock.reset(); if (useHook) { - hook = std::move(worker.hook); + co_return buildWithHook( + std::move(inputPaths), std::move(initialOutputs), std::move(drvOptions), std::move(outputLocks)); + } else { + co_return buildLocally( + std::move(inputPaths), + std::move(initialOutputs), + std::move(drvOptions), + std::move(outputLocks), + externalBuilder); + } +} - try { - hook->machineName = readLine(hook->fromHook.readSide.get()); - } catch (Error & e) { - e.addTrace({}, "while reading the machine name from the build hook"); - throw; - } +Goal::Co DerivationBuildingGoal::buildWithHook( + StorePathSet inputPaths, + std::map initialOutputs, + DerivationOptions drvOptions, + PathLocks outputLocks) +{ +#ifdef _WIN32 // TODO enable build hook on Windows + unreachable(); +#else + std::unique_ptr hook = std::move(worker.hook); - CommonProto::WriteConn conn{hook->sink}; + try { + hook->machineName = readLine(hook->fromHook.readSide.get()); + } catch (Error & e) { + e.addTrace({}, "while reading the machine name from the build hook"); + throw; + } - /* Tell the hook all the inputs that have to be copied to the - remote system. */ - CommonProto::write(worker.store, conn, inputPaths); + CommonProto::WriteConn conn{hook->sink}; - /* Tell the hooks the missing outputs that have to be copied back - from the remote system. */ - { - StringSet missingOutputs; - for (auto & [outputName, status] : initialOutputs) { - // XXX: Does this include known CA outputs? - if (buildMode != bmCheck && status.known && status.known->isValid()) - continue; - missingOutputs.insert(outputName); - } - CommonProto::write(worker.store, conn, missingOutputs); + /* Tell the hook all the inputs that have to be copied to the + remote system. */ + CommonProto::write(worker.store, conn, inputPaths); + + /* Tell the hooks the missing outputs that have to be copied back + from the remote system. */ + { + StringSet missingOutputs; + for (auto & [outputName, status] : initialOutputs) { + // XXX: Does this include known CA outputs? + if (buildMode != bmCheck && status.known && status.known->isValid()) + continue; + missingOutputs.insert(outputName); } + CommonProto::write(worker.store, conn, missingOutputs); + } - hook->sink = FdSink(); - hook->toHook.writeSide.close(); + hook->sink = FdSink(); + hook->toHook.writeSide.close(); - /* Create the log file and pipe. */ - openLogFile(); + /* Create the log file and pipe. */ + std::unique_ptr logFile = std::make_unique(worker.store, drvPath); - std::set fds; - fds.insert(hook->fromHook.readSide.get()); - fds.insert(hook->builderOut.readSide.get()); - worker.childStarted(shared_from_this(), fds, false, false); + std::set fds; + fds.insert(hook->fromHook.readSide.get()); + fds.insert(hook->builderOut.readSide.get()); + worker.childStarted(shared_from_this(), fds, false, false); - buildResult.startTime = time(0); // inexact - started(); + buildResult.startTime = time(0); // inexact -#ifndef _WIN32 - assert(hook); -#endif + auto msg = + fmt(buildMode == bmRepair ? "repairing outputs of '%s'" + : buildMode == bmCheck ? "checking outputs of '%s'" + : "building '%s'", + worker.store.printStorePath(drvPath)); + msg += fmt(" on '%s'", hook->machineName); + + std::unique_ptr buildLog = std::make_unique( + settings.logLines, + std::make_unique( + *logger, + lvlInfo, + actBuild, + msg, + Logger::Fields{worker.store.printStorePath(drvPath), hook->machineName, 1, 1})); + mcRunningBuilds = std::make_unique>(worker.runningBuilds); + worker.updateProgress(); - std::string currentHookLine; - uint64_t logSize = 0; + std::string currentHookLine; + uint64_t logSize = 0; - while (true) { - auto event = co_await WaitForChildEvent{}; - if (auto * output = std::get_if(&event)) { - auto & fd = output->fd; - auto & data = output->data; - if (fd == hook->builderOut.readSide.get()) { - logSize += data.size(); - if (settings.maxLogSize && logSize > settings.maxLogSize) { - hook.reset(); - co_return doneFailureLogTooLong(*buildLog); - } - (*buildLog)(data); - if (logFile->sink) - (*logFile->sink)(data); - } else if (fd == hook->fromHook.readSide.get()) { - for (auto c : data) - if (c == '\n') { - auto json = parseJSONMessage(currentHookLine, "the derivation builder"); - if (json) { - auto s = handleJSONLogMessage( - *json, worker.act, hook->activities, "the derivation builder", true); - // ensure that logs from a builder using `ssh-ng://` as protocol - // are also available to `nix log`. - if (s && logFile->sink) { - const auto type = (*json)["type"]; - const auto fields = (*json)["fields"]; - if (type == resBuildLogLine) { + while (true) { + auto event = co_await WaitForChildEvent{}; + if (auto * output = std::get_if(&event)) { + auto & fd = output->fd; + auto & data = output->data; + if (fd == hook->builderOut.readSide.get()) { + logSize += data.size(); + if (settings.maxLogSize && logSize > settings.maxLogSize) { + hook.reset(); + co_return doneFailureLogTooLong(*buildLog); + } + (*buildLog)(data); + if (logFile->sink) + (*logFile->sink)(data); + } else if (fd == hook->fromHook.readSide.get()) { + for (auto c : data) + if (c == '\n') { + auto json = parseJSONMessage(currentHookLine, "the derivation builder"); + if (json) { + auto s = handleJSONLogMessage( + *json, worker.act, hook->activities, "the derivation builder", true); + // ensure that logs from a builder using `ssh-ng://` as protocol + // are also available to `nix log`. + if (s && logFile->sink) { + const auto type = (*json)["type"]; + const auto fields = (*json)["fields"]; + if (type == resBuildLogLine) { + (*logFile->sink)((fields.size() > 0 ? fields[0].get() : "") + "\n"); + } else if (type == resSetPhase && !fields.is_null()) { + const auto phase = fields[0]; + if (!phase.is_null()) { + // nixpkgs' stdenv produces lines in the log to signal + // phase changes. + // We want to get the same lines in case of remote builds. + // The format is: + // @nix { "action": "setPhase", "phase": "$curPhase" } + const auto logLine = + nlohmann::json::object({{"action", "setPhase"}, {"phase", phase}}); (*logFile->sink)( - (fields.size() > 0 ? fields[0].get() : "") + "\n"); - } else if (type == resSetPhase && !fields.is_null()) { - const auto phase = fields[0]; - if (!phase.is_null()) { - // nixpkgs' stdenv produces lines in the log to signal - // phase changes. - // We want to get the same lines in case of remote builds. - // The format is: - // @nix { "action": "setPhase", "phase": "$curPhase" } - const auto logLine = - nlohmann::json::object({{"action", "setPhase"}, {"phase", phase}}); - (*logFile->sink)( - "@nix " - + logLine.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace) - + "\n"); - } + "@nix " + + logLine.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace) + + "\n"); } } } - currentHookLine.clear(); - } else - currentHookLine += c; - } - } else if (std::get_if(&event)) { - buildLog->flush(); - break; - } else if (auto * timeout = std::get_if(&event)) { - hook.reset(); - co_return doneFailure(std::move(*timeout)); + } + currentHookLine.clear(); + } else + currentHookLine += c; } + } else if (std::get_if(&event)) { + buildLog->flush(); + break; + } else if (auto * timeout = std::get_if(&event)) { + hook.reset(); + co_return doneFailure(std::move(*timeout)); } + } - 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 + trace("hook build done"); - debug("build hook for '%s' finished", worker.store.printStorePath(drvPath)); + /* 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 = hook->pid.kill(); - buildResult.timesBuilt++; - buildResult.stopTime = time(0); + debug("build hook for '%s' finished", worker.store.printStorePath(drvPath)); - /* So the child is gone now. */ - worker.childTerminated(this); + buildResult.timesBuilt++; + buildResult.stopTime = time(0); - /* 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 + /* So the child is gone now. */ + worker.childTerminated(this); - /* Close the log file. */ - closeLogFile(); + /* Close the read side of the logger pipe. */ + hook->builderOut.readSide.close(); + hook->fromHook.readSide.close(); - /* Check the exit status. */ - if (!statusOk(status)) { - auto e = fixupBuilderFailureErrorMessage({BuildResult::Failure::MiscFailure, status, ""}, *buildLog); + /* Close the log file. */ + logFile.reset(); - outputLocks.unlock(); + /* Check the exit status. */ + if (!statusOk(status)) { + auto e = fixupBuilderFailureErrorMessage({BuildResult::Failure::MiscFailure, status, ""}, *buildLog); - /* 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. + outputLocks.unlock(); - We can only early return when the outputs are known a priori. For - floating content-addressing derivations this isn't the case. + /* TODO (once again) support fine-grained error codes, see issue #12641. */ - Aborts if any output is not valid or corrupt, and otherwise - returns a 'SingleDrvOutputs' structure containing all outputs. - */ - [&] { - auto [allValid, validOutputs] = checkPathValidity(initialOutputs); - if (!allValid) - throw Error("some outputs are unexpectedly invalid"); - return validOutputs; - }(); + co_return doneFailure(std::move(e)); + } - StorePathSet outputPaths; - for (auto & [_, output] : builtOutputs) - outputPaths.insert(output.outPath); - runPostBuildHook(worker.store, *logger, drvPath, outputPaths); + /* 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. - /* 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(); + We can only early return when the outputs are known a priori. For + floating content-addressing derivations this isn't the case. - co_return doneSuccess(BuildResult::Success::Built, std::move(builtOutputs)); - } + Aborts if any output is not valid or corrupt, and otherwise + returns a 'SingleDrvOutputs' structure containing all outputs. + */ + [&] { + auto [allValid, validOutputs] = checkPathValidity(initialOutputs); + if (!allValid) + throw Error("some outputs are unexpectedly invalid"); + return validOutputs; + }(); + + 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::Success::Built, std::move(builtOutputs)); +#endif +} +Goal::Co DerivationBuildingGoal::buildLocally( + StorePathSet inputPaths, + std::map initialOutputs, + DerivationOptions drvOptions, + PathLocks outputLocks, + const ExternalBuilder * externalBuilder) +{ co_await yield(); if (!dynamic_cast(&worker.store)) { @@ -634,8 +628,28 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) #ifdef _WIN32 // TODO enable `DerivationBuilder` on Windows throw UnimplementedError("building derivations is not yet implemented on Windows"); #else - assert(!hook); + std::unique_ptr buildLog; + std::unique_ptr logFile; + + auto openLogFile = [&]() { logFile = std::make_unique(worker.store, drvPath); }; + + auto closeLogFile = [&]() { logFile.reset(); }; + auto started = [&]() { + auto msg = + fmt(buildMode == bmRepair ? "repairing outputs of '%s'" + : buildMode == bmCheck ? "checking outputs of '%s'" + : "building '%s'", + worker.store.printStorePath(drvPath)); + buildLog = std::make_unique( + settings.logLines, + std::make_unique( + *logger, lvlInfo, actBuild, msg, Logger::Fields{worker.store.printStorePath(drvPath), "", 1, 1})); + mcRunningBuilds = std::make_unique>(worker.runningBuilds); + worker.updateProgress(); + }; + + std::unique_ptr actLock; std::unique_ptr builder; Descriptor builderOut; 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 a509844f6f0..d8fb967aedf 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -16,6 +16,7 @@ namespace nix { using std::map; struct BuilderFailureError; +struct ExternalBuilder; #ifndef _WIN32 // TODO enable build hook on Windows struct HookInstance; struct DerivationBuilder; @@ -68,6 +69,17 @@ private: */ Co gaveUpOnSubstitution(bool storeDerivation); Co tryToBuild(StorePathSet inputPaths); + Co buildWithHook( + StorePathSet inputPaths, + std::map initialOutputs, + DerivationOptions drvOptions, + PathLocks outputLocks); + Co buildLocally( + StorePathSet inputPaths, + std::map initialOutputs, + DerivationOptions drvOptions, + PathLocks outputLocks, + const ExternalBuilder * externalBuilder); /** * Is the build hook willing to perform the build?