From a7bc1be03fd5ec40b92ec17379b13d3260e57051 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 18 Jan 2026 19:20:35 +0100 Subject: [PATCH] commands: RunPager RAII -> withPager wrapper RunPager is weird and confusing in that it replaces what stdout *is* depending on environmental conditions. this has not caused problems, but it's easy to imagine situations in which it would (eg if the stl decided to capture the stdout fd by duplicating it). using a wrapper for this also makes clear *what* actually goes into the pager; while the previous contract was semi-reasonable it was also very implicit, and with the proliferation of functions we had that printed directly to stdout it would have been easy to send wrong output to the pager. using a wrapper also makes process management much easier because we do not have to rely on destructors to always produce correct output. Change-Id: Ifd3760940af1ec719fe856158c913cbb9a5bf270 --- lix/legacy/nix-env.cc | 45 +++++++++++++++++++--------------- lix/legacy/nix-store.cc | 54 +++++++++++++++++++++-------------------- lix/libcmd/repl.cc | 39 +++++++++++++++-------------- lix/libmain/shared.cc | 17 +++++++++++++ lix/libmain/shared.hh | 26 ++++++++++++++++++++ lix/nix/log.cc | 35 ++++++++++++++------------ lix/nix/main.cc | 3 +-- lix/nix/why-depends.cc | 10 +++----- 8 files changed, 140 insertions(+), 89 deletions(-) diff --git a/lix/legacy/nix-env.cc b/lix/legacy/nix-env.cc index 307290cca..babcc8c62 100644 --- a/lix/legacy/nix-env.cc +++ b/lix/legacy/nix-env.cc @@ -1133,12 +1133,10 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) return; } - RunPager pager; - - { + withPager([&](Pager & pager) { Table table; - std::ostringstream dummy; - XMLWriter xml(true, *(xmlOutput ? &cout : &dummy)); + std::ostringstream xmlStream; + XMLWriter xml(true, xmlStream); XMLOpenElement xmlRoot(xml, "items"); for (auto & i : elems) { @@ -1369,9 +1367,11 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) } if (!xmlOutput) { - std::cout << printTable(table); + pager << printTable(table); + } else { + pager << xmlStream.str(); } - } + }); } static void opSwitchProfile(Globals & globals, Strings opFlags, Strings opArgs) @@ -1424,20 +1424,27 @@ static void opListGenerations(Globals & globals, Strings opFlags, Strings opArgs auto [gens, curGen] = findGenerations(globals.profile); - RunPager pager; - - for (auto & i : gens) { - tm t; - if (!localtime_r(&i.creationTime, &t)) throw Error("cannot convert time"); - logger->cout("%|4| %|4|-%|02|-%|02| %|02|:%|02|:%|02| %||", - i.number, - t.tm_year + 1900, t.tm_mon + 1, t.tm_mday, - t.tm_hour, t.tm_min, t.tm_sec, - i.number == curGen ? "(current)" : ""); - } + withPager([&](Pager & pager) { + for (auto & i : gens) { + tm t; + if (!localtime_r(&i.creationTime, &t)) { + throw Error("cannot convert time"); + } + pager << fmt( + "%|4| %|4|-%|02|-%|02| %|02|:%|02|:%|02| %||\n", + i.number, + t.tm_year + 1900, + t.tm_mon + 1, + t.tm_mday, + t.tm_hour, + t.tm_min, + t.tm_sec, + i.number == curGen ? "(current)" : "" + ); + } + }); } - static void opDeleteGenerations(Globals & globals, Strings opFlags, Strings opArgs) { if (opFlags.size() > 0) diff --git a/lix/legacy/nix-store.cc b/lix/legacy/nix-store.cc index c916152a8..13cd0f1ad 100644 --- a/lix/legacy/nix-store.cc +++ b/lix/legacy/nix-store.cc @@ -25,7 +25,9 @@ #include #include +#include #include +#include #include #include #include @@ -375,17 +377,14 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String if (!query) query = qOutputs; - RunPager pager; - - { + withPager([&](Pager & pager) { switch (*query) { - case qOutputs: { for (auto & i : opArgs) { auto outputs = aio.blockOn(maybeUseOutputs(store, store->followLinksToStorePath(i), true, forceRealise)); for (auto & outputPath : outputs) { - cout << fmt("%1%\n", store->printStorePath(outputPath)); + pager << fmt("%1%\n", store->printStorePath(outputPath)); } } break; @@ -420,7 +419,7 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String } auto sorted = aio.blockOn(store->topoSortPaths(paths)); for (StorePaths::reverse_iterator i = sorted.rbegin(); i != sorted.rend(); ++i) { - cout << fmt("%s\n", store->printStorePath(*i)); + pager << fmt("%s\n", store->printStorePath(*i)); } break; } @@ -428,7 +427,7 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String case qDeriver: for (auto & i : opArgs) { auto info = aio.blockOn(store->queryPathInfo(store->followLinksToStorePath(i))); - cout << fmt( + pager << fmt( "%s\n", info->deriver ? store->printStorePath(*info->deriver) : "unknown-deriver" ); } @@ -444,7 +443,7 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String } auto sorted = aio.blockOn(store->topoSortPaths(result)); for (StorePaths::reverse_iterator i = sorted.rbegin(); i != sorted.rend(); ++i) { - cout << fmt("%s\n", store->printStorePath(*i)); + pager << fmt("%s\n", store->printStorePath(*i)); } break; } @@ -461,7 +460,7 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String bindingName ); } - cout << fmt("%s\n", j->second); + pager << fmt("%s\n", j->second); } break; @@ -475,9 +474,9 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String auto info = aio.blockOn(store->queryPathInfo(j)); if (query == qHash) { assert(info->narHash.type == HashType::SHA256); - cout << fmt("%s\n", info->narHash.to_string(HashFormat::Base32)); + pager << fmt("%s\n", info->narHash.to_string(HashFormat::Base32)); } else if (query == qSize) { - cout << fmt("%d\n", info->narSize); + pager << fmt("%d\n", info->narSize); } } } @@ -486,7 +485,9 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String case qTree: { StorePathSet done; for (auto & i : opArgs) { - printTree(std::cout, store, aio, store->followLinksToStorePath(i), "", "", done); + std::stringstream tmp; + printTree(tmp, store, aio, store->followLinksToStorePath(i), "", "", done); + pager << tmp.str(); } break; } @@ -501,7 +502,7 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String roots.insert(j); } } - std::cout << aio.blockOn(printDotGraph(ref::unsafeFromPtr(store), std::move(roots))); + pager << aio.blockOn(printDotGraph(ref::unsafeFromPtr(store), std::move(roots))); break; } @@ -515,13 +516,13 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String roots.insert(j); } } - std::cout << aio.blockOn(printGraphML(ref::unsafeFromPtr(store), std::move(roots))); + pager << aio.blockOn(printGraphML(ref::unsafeFromPtr(store), std::move(roots))); break; } case qResolve: { for (auto & i : opArgs) { - cout << fmt("%s\n", store->printStorePath(store->followLinksToStorePath(i))); + pager << fmt("%s\n", store->printStorePath(store->followLinksToStorePath(i))); } break; } @@ -547,7 +548,7 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String for (auto & [target, links] : roots) { if (referrers.find(target) != referrers.end()) { for (auto & link : links) { - cout << fmt("%1% -> %2%\n", link, gcStore.printStorePath(target)); + pager << fmt("%1% -> %2%\n", link, gcStore.printStorePath(target)); } } } @@ -557,7 +558,7 @@ opQuery(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, String default: abort(); } - } + }); } static void @@ -593,15 +594,16 @@ opReadLog(std::shared_ptr store, AsyncIoRoot & aio, Strings opFlags, Stri auto & logStore = require(*store); - RunPager pager; - - for (auto & i : opArgs) { - auto path = logStore.followLinksToStorePath(i); - auto log = aio.blockOn(logStore.getBuildLog(path)); - if (!log) - throw Error("build log of derivation '%s' is not available", logStore.printStorePath(path)); - std::cout << *log; - } + withPager([&](Pager & pager) { + for (auto & i : opArgs) { + auto path = logStore.followLinksToStorePath(i); + auto log = aio.blockOn(logStore.getBuildLog(path)); + if (!log) { + throw Error("build log of derivation '%s' is not available", logStore.printStorePath(path)); + } + pager << *log; + } + }); } static void diff --git a/lix/libcmd/repl.cc b/lix/libcmd/repl.cc index cda830443..c72c60bbe 100644 --- a/lix/libcmd/repl.cc +++ b/lix/libcmd/repl.cc @@ -903,30 +903,29 @@ void NixRepl::initBuiltinCommands() subs.push_front(repl.evaluator.store); bool foundLog = false; - RunPager pager; - for (auto & sub : subs) { - auto * logSubP = dynamic_cast(&*sub); - if (!logSubP) { - printInfo( - "Skipped '%s' which does not support retrieving build logs", sub->getUri() - ); - continue; + withPager([&](Pager & pager) { + for (auto & sub : subs) { + auto * logSubP = dynamic_cast(&*sub); + if (!logSubP) { + printInfo("Skipped '%s' which does not support retrieving build logs", sub->getUri()); + continue; + } + auto & logSub = *logSubP; + + auto log = repl.state.aio.blockOn(logSub.getBuildLog(drvPath)); + if (log) { + printInfo("got build log for '%s' from '%s'", drvPathRaw, logSub.getUri()); + pager << *log; + foundLog = true; + break; + } } - auto & logSub = *logSubP; - auto log = repl.state.aio.blockOn(logSub.getBuildLog(drvPath)); - if (log) { - printInfo("got build log for '%s' from '%s'", drvPathRaw, logSub.getUri()); - logger->writeToStdout(*log); - foundLog = true; - break; + if (!foundLog) { + throw Error("build log of '%s' is not available", drvPathRaw); } - } - - if (!foundLog) { - throw Error("build log of '%s' is not available", drvPathRaw); - } + }); return ProcessLineResult::PromptAgain; }, diff --git a/lix/libmain/shared.cc b/lix/libmain/shared.cc index ed191d6fc..35ef20df0 100644 --- a/lix/libmain/shared.cc +++ b/lix/libmain/shared.cc @@ -419,6 +419,23 @@ RunPager::~RunPager() } } +void withPager(kj::Function fn) +{ + struct PagerImpl : Pager + { + Pager & operator<<(std::string_view data) override + { + std::cout.write(data.data(), data.size()); + return *this; + } + }; + + RunPager wrapper; + PagerImpl pager; + fn(pager); + std::cout.flush(); +} + PrintFreed::~PrintFreed() { // When in dry-run mode, print the paths on stdout diff --git a/lix/libmain/shared.hh b/lix/libmain/shared.hh index 199c203fb..e2aa68cf1 100644 --- a/lix/libmain/shared.hh +++ b/lix/libmain/shared.hh @@ -10,6 +10,8 @@ #include "lix/libutil/processes.hh" #include "lix/libutil/strings.hh" +#include + namespace nix { int handleExceptions(const std::string & programName, std::function fun); @@ -86,6 +88,30 @@ private: int std_out; }; +/** + * Represents a running pager if paging is available, or stdout if not. + */ +class Pager +{ +protected: + Pager() = default; + +public: + /** + * Writes some data to the pager (or stdout). Only the provided data + * is written, with no newlines are added or any formatting applied. + */ + virtual Pager & operator<<(std::string_view data) = 0; +}; + +/** + * Starts a pager if standard output is a terminal and $PAGER is set. The + * pager is provided as an argument to the callback; only data written to + * that object will be sent to the pager program. If no pager is started, + * e.g. if $PAGER is cleared, the pager object writes directly to stdout. + */ +void withPager(kj::Function fn); + /* GC helpers. */ std::string showBytes(uint64_t bytes); diff --git a/lix/nix/log.cc b/lix/nix/log.cc index 6277a01ab..f649dccb1 100644 --- a/lix/nix/log.cc +++ b/lix/nix/log.cc @@ -43,24 +43,27 @@ struct CmdLog : InstallableCommand }, }, b.path.raw()); - RunPager pager; - for (auto & sub : subs) { - auto * logSubP = dynamic_cast(&*sub); - if (!logSubP) { - printInfo("Skipped '%s' which does not support retrieving build logs", sub->getUri()); - continue; + withPager([&](Pager & pager) { + for (auto & sub : subs) { + auto * logSubP = dynamic_cast(&*sub); + if (!logSubP) { + printInfo("Skipped '%s' which does not support retrieving build logs", sub->getUri()); + continue; + } + auto & logSub = *logSubP; + + auto log = aio().blockOn(logSub.getBuildLog(path)); + if (!log) { + continue; + } + logger->pause(); + printInfo("got build log for '%s' from '%s'", installable->what(), logSub.getUri()); + pager << *log; + return; } - auto & logSub = *logSubP; - auto log = aio().blockOn(logSub.getBuildLog(path)); - if (!log) continue; - logger->pause(); - printInfo("got build log for '%s' from '%s'", installable->what(), logSub.getUri()); - writeFull(STDOUT_FILENO, *log); - return; - } - - throw Error("build log of '%s' is not available", installable->what()); + throw Error("build log of '%s' is not available", installable->what()); + }); } }; diff --git a/lix/nix/main.cc b/lix/nix/main.cc index 1ff86936a..7b823f3cc 100644 --- a/lix/nix/main.cc +++ b/lix/nix/main.cc @@ -381,8 +381,7 @@ static void showHelp(AsyncIoRoot & aio, std::vector subcommand, Nix auto markdown = state->forceString(attr->value, noPos, "while evaluating the lowdown help text"); - RunPager pager; - std::cout << renderMarkdownToTerminal(markdown) << "\n"; + withPager([&](Pager & pager) { pager << renderMarkdownToTerminal(markdown) << "\n"; }); } static NixArgs & getNixArgs(Command & cmd) diff --git a/lix/nix/why-depends.cc b/lix/nix/why-depends.cc index 7f99c99c7..b8bf23e48 100644 --- a/lix/nix/why-depends.cc +++ b/lix/nix/why-depends.cc @@ -101,13 +101,11 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions closure (i.e., that have a non-infinite distance to 'dependency'). Print every edge on a path between `package` and `dependency`. */ - RunPager pager; - logger->cout( - "%s", - aio().blockOn( + withPager([&](Pager & pager) { + pager << aio().blockOn( genGraphString(packagePath, dependencyPath, graphData, *store, all, precise) - ) - ); + ); + }); } };