From 4162a66cee97ec16c88d991ef9a6d9baa3740053 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 23:02:11 +0200 Subject: [PATCH] libutil: return sources from runProgram2 this much more closely mimics what is actually happening: we're reading data from somewhere else, actively, rather than passively waiting. with the data flow matching the underlying system interactions better we can remove a few sinkToSource calls that merely exists to undo the mismatch caused by not treating subprocess output as a data source to begin with Change-Id: If4abfc2f8398fb5e88c9b91a8bdefd5504bb2d11 --- src/libfetchers/git.cc | 15 ++++++------- src/libstore/build/derivation-goal.cc | 10 ++++++--- src/libutil/processes.cc | 31 ++++++++++++++++++--------- src/libutil/processes.hh | 9 ++++++-- 4 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 75ac926e2..81c579e84 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -688,17 +688,14 @@ struct GitInputScheme : InputScheme filter = isNotDotGitDirectory; } else { - // FIXME: should pipe this, or find some better way to extract a - // revision. - auto source = sinkToSource([&](Sink & sink) { - runProgram2({ - .program = "git", - .args = { "-C", repoDir, "--git-dir", gitDir, "archive", input.getRev()->gitRev() }, - .standardOut = &sink - }).wait(); + auto proc = runProgram2({ + .program = "git", + .args = { "-C", repoDir, "--git-dir", gitDir, "archive", input.getRev()->gitRev() }, + .captureStdout = true, }); + Finally const _wait([&] { proc.wait(); }); - unpackTarfile(*source, tmpDir); + unpackTarfile(*proc.stdout(), tmpDir); } auto storePath = store->addToStore(name, tmpDir, FileIngestionMethod::Recursive, htSHA256, filter); diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index a59984688..7c18f8b60 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -923,12 +923,16 @@ void runPostBuildHook( }; LogSink sink(act); - runProgram2({ + auto proc = runProgram2({ .program = settings.postBuildHook, .environment = hookEnvironment, - .standardOut = &sink, + .captureStdout = true, .mergeStderrToStdout = true, - }).wait(); + }); + Finally const _wait([&] { proc.wait(); }); + + // FIXME just process the data, without a wrapper sink class + proc.stdout()->drainInto(sink); } void DerivationGoal::buildDone() diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 3c353d82e..4fe3dcfb4 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -243,18 +243,28 @@ std::string runProgram(Path program, bool searchPath, const Strings & args, bool // Output = error code + "standard out" output stream std::pair runProgram(RunOptions && options) { - StringSink sink; - options.standardOut = &sink; + options.captureStdout = true; int status = 0; + std::string stdout; try { - runProgram2(options).wait(); + auto proc = runProgram2(options); + Finally const _wait([&] { proc.wait(); }); + stdout = proc.stdout()->drain(); } catch (ExecError & e) { status = e.status; } - return {status, std::move(sink.s)}; + return {status, std::move(stdout)}; +} + +RunningProgram::RunningProgram(PathView program, Pid pid, AutoCloseFD stdout) + : program(program) + , pid(std::move(pid)) + , stdoutSource(stdout ? std::make_unique(stdout.get()) : nullptr) + , stdout_(std::move(stdout)) +{ } RunningProgram::~RunningProgram() @@ -281,7 +291,7 @@ RunningProgram runProgram2(const RunOptions & options) /* Create a pipe. */ Pipe out; - if (options.standardOut) out.create(); + if (options.captureStdout) out.create(); ProcessOptions processOptions; @@ -299,7 +309,7 @@ RunningProgram runProgram2(const RunOptions & options) Pid pid{startProcess([&]() { if (options.environment) replaceEnv(*options.environment); - if (options.standardOut && dup2(out.writeSide.get(), STDOUT_FILENO) == -1) + if (options.captureStdout && dup2(out.writeSide.get(), STDOUT_FILENO) == -1) throw SysError("dupping stdout"); if (options.mergeStderrToStdout) if (dup2(STDOUT_FILENO, STDERR_FILENO) == -1) @@ -332,10 +342,11 @@ RunningProgram runProgram2(const RunOptions & options) out.writeSide.close(); - if (options.standardOut) - *options.standardOut << drainFDSource(out.readSide.get()); - - return RunningProgram{options.program, std::move(pid)}; + return RunningProgram{ + options.program, + std::move(pid), + options.captureStdout ? std::move(out.readSide) : AutoCloseFD{} + }; } std::string statusToString(int status) diff --git a/src/libutil/processes.hh b/src/libutil/processes.hh index 90b2d20af..f61b9696d 100644 --- a/src/libutil/processes.hh +++ b/src/libutil/processes.hh @@ -3,6 +3,7 @@ #include "types.hh" #include "error.hh" +#include "file-descriptor.hh" #include #include @@ -82,7 +83,7 @@ struct RunOptions std::optional gid; std::optional chdir; std::optional> environment; - Sink * standardOut = nullptr; + bool captureStdout = false; bool mergeStderrToStdout = false; bool isInteractive = false; }; @@ -94,14 +95,18 @@ struct [[nodiscard("you must call RunningProgram::wait()")]] RunningProgram private: Path program; Pid pid; + std::unique_ptr stdoutSource; + AutoCloseFD stdout_; - RunningProgram(Path program, Pid pid) : program(std::move(program)), pid(std::move(pid)) {} + RunningProgram(PathView program, Pid pid, AutoCloseFD stdout); public: RunningProgram() = default; ~RunningProgram(); void wait(); + + Source * stdout() const { return stdoutSource.get(); } }; std::pair runProgram(RunOptions && options);