From 0dd1d8ca1cdccfc620644a7f690ed35bcd2d1e74 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Sat, 29 Jun 2024 15:03:44 +0200 Subject: [PATCH] tree-wide: unify progress bar inactive and paused states Previously, the progress bar had two subtly different states in which the bar would not actually render, both with their own shortcomings: inactive (which was irreversible) and paused (reversible, but swallowing logs). Furthermore, there was no way of resetting the statistics, so a very bad solution was implemented (243c0f18dae2a08ea0e46f7ff33277c63f7506d7) that would create a new logger for each line of the repl, leaking the previous one and discarding the value of printBuildLogs. Finally, if stderr was not attached to a TTY, the update thread was started even though the logger was not active, violating the invariant required by the destructor (which is not observed because the logger is leaked). In this commit, the two aforementioned states are unified into a single one, which can be exited again, correctly upholds the invariant that the update thread is only running while the progress bar is active, and does not swallow logs. The latter change in behavior is not expected to be a problems in the rare cases where the paused state was used before, since other loggers (like the simple one) don't exhibit it anyway. The startProgressBar/stopProgressBar API is removed due to being a footgun, and a new method for properly resetting the progress is added. Co-Authored-By: Qyriad Change-Id: I2b7c3eb17d439cd0c16f7b896cfb61239ac7ff3a --- src/libcmd/repl.cc | 8 ++-- src/libfetchers/git.cc | 5 +- src/libmain/progress-bar.cc | 75 ++++++++++++++---------------- src/libmain/progress-bar.hh | 13 ++---- src/libmain/shared.cc | 3 +- src/libutil/logging.hh | 3 +- src/nix-build/nix-build.cc | 4 +- src/nix/build.cc | 3 +- src/nix/cat.cc | 3 +- src/nix/develop.cc | 3 +- src/nix/dump-path.cc | 5 +- src/nix/edit.cc | 3 +- src/nix/eval.cc | 5 +- src/nix/log.cc | 3 +- src/nix/main.cc | 5 +- src/nix/prefetch.cc | 8 ++-- src/nix/run.cc | 3 +- src/nix/sigs.cc | 3 +- src/nix/upgrade-nix.cc | 5 +- src/nix/why-depends.cc | 3 +- tests/unit/libmain/progress-bar.cc | 3 +- 21 files changed, 68 insertions(+), 98 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 28341259c..39e89d999 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -32,7 +32,6 @@ #include "local-fs-store.hh" #include "signals.hh" #include "print.hh" -#include "progress-bar.hh" #include "gc-small-vector.hh" #include "users.hh" @@ -300,7 +299,7 @@ ReplExitStatus NixRepl::mainLoop() /* Stop the progress bar because it interferes with the display of the repl. */ - stopProgressBar(); + logger->pause(); std::string input; @@ -684,9 +683,10 @@ ProcessLineResult NixRepl::processLine(std::string line) // TODO: this only shows a progress bar for explicitly initiated builds, // not eval-time fetching or builds performed for IFD. // But we can't just show it everywhere, since that would erase partial output from evaluation. - startProgressBar(); + logger->resetProgress(); + logger->resume(); Finally stopLogger([&]() { - stopProgressBar(); + logger->pause(); }); state->store->buildPaths({ diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index b33d893b7..10e125207 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -403,11 +403,8 @@ struct GitInputScheme : InputScheme AutoDelete const _delete{msgPath}; writeFile(msgPath, *commitMsg); - // Pause the logger to allow for user input (such as a gpg passphrase) in `git commit` - logger->pause(); - Finally restoreLogger([]() { logger->resume(); }); runProgram("git", true, - { "-C", *root, "--git-dir", gitDir, "commit", std::string(path.rel()), "-F", msgPath }); + { "-C", *root, "--git-dir", gitDir, "commit", std::string(path.rel()), "-F", msgPath }, true); } } } diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index 68654c636..e4afcd829 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -4,7 +4,6 @@ #include "names.hh" #include "terminal.hh" -#include #include #include #include @@ -44,50 +43,56 @@ static std::string_view storePathToName(std::string_view path) ProgressBar::ProgressBar(bool isTTY) : isTTY(isTTY) { - state_.lock()->active = isTTY; - updateThread = std::thread([&]() { - auto state(state_.lock()); - auto nextWakeup = A_LONG_TIME; - while (state->active) { - if (!state->haveUpdate) - state.wait_for(updateCV, nextWakeup); - nextWakeup = draw(*state, {}); - state.wait_for(quitCV, std::chrono::milliseconds(50)); - } - }); + resume(); } ProgressBar::~ProgressBar() { - stop(); + pause(); } -/* Called by destructor, can't be overridden */ -void ProgressBar::stop() +void ProgressBar::pause() { + if (!isTTY) return; { auto state(state_.lock()); - if (!state->active) return; - state->active = false; - writeToStderr("\r\e[K"); + state->paused++; + if (state->paused > 1) return; // recursive pause, the update thread is already gone updateCV.notify_one(); quitCV.notify_one(); } updateThread.join(); } -void ProgressBar::pause() +void ProgressBar::resetProgress() { - state_.lock()->paused = true; - writeToStderr("\r\e[K"); + auto state(state_.lock()); + auto prevPaused = state->paused; + *state = ProgressBar::State { + .paused = prevPaused, + }; + update(*state); } void ProgressBar::resume() { - state_.lock()->paused = false; - writeToStderr("\r\e[K"); - state_.lock()->haveUpdate = true; - updateCV.notify_one(); + if (!isTTY) return; + auto state(state_.lock()); + assert(state->paused > 0); // should be paused + state->paused--; + if (state->paused > 0) return; // recursive pause, wait for the parents to resume too + state->haveUpdate = true; + updateThread = std::thread([&]() { + auto state(state_.lock()); + auto nextWakeup = A_LONG_TIME; + while (state->paused == 0) { + if (!state->haveUpdate) + state.wait_for(updateCV, nextWakeup); + nextWakeup = draw(*state, {}); + state.wait_for(quitCV, std::chrono::milliseconds(50)); + } + writeToStderr("\r\e[K"); + }); } bool ProgressBar::isVerbose() @@ -114,7 +119,7 @@ void ProgressBar::logEI(const ErrorInfo & ei) void ProgressBar::log(State & state, Verbosity lvl, std::string_view s) { - if (state.active) { + if (state.paused == 0) { draw(state, s); } else { auto s2 = s + ANSI_NORMAL "\n"; @@ -318,7 +323,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional 0) return nextWakeup; auto windowSize = getWindowSize(); auto width = windowSize.second; @@ -525,7 +530,7 @@ std::string ProgressBar::getStatus(State & state) void ProgressBar::writeToStdout(std::string_view s) { auto state(state_.lock()); - if (state->active) { + if (state->paused == 0) { Logger::writeToStdout(s); draw(*state, {}); } else { @@ -536,7 +541,7 @@ void ProgressBar::writeToStdout(std::string_view s) std::optional ProgressBar::ask(std::string_view msg) { auto state(state_.lock()); - if (!state->active || !isatty(STDIN_FILENO)) return {}; + if (state->paused > 0 || !isatty(STDIN_FILENO)) return {}; std::cerr << fmt("\r\e[K%s ", msg); auto s = trim(readLine(STDIN_FILENO)); if (s.size() != 1) return {}; @@ -559,16 +564,4 @@ Logger * makeProgressBar() return new ProgressBar(shouldANSI()); } -void startProgressBar() -{ - logger = makeProgressBar(); -} - -void stopProgressBar() -{ - auto progressBar = dynamic_cast(logger); - if (progressBar) progressBar->stop(); - -} - } diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index e682d75fe..ad500de6e 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -48,9 +48,8 @@ struct ProgressBar : public Logger uint64_t corruptedPaths = 0, untrustedPaths = 0; - bool active = true; - bool paused = false; - bool haveUpdate = true; + uint32_t paused = 1; + bool haveUpdate = false; }; Sync state_; @@ -67,10 +66,10 @@ struct ProgressBar : public Logger ~ProgressBar(); - void stop() override final; - void pause() override; + void resetProgress() override; + void resume() override; bool isVerbose() override; @@ -113,8 +112,4 @@ struct ProgressBar : public Logger Logger * makeProgressBar(); -void startProgressBar(); - -void stopProgressBar(); - } diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 29538a9ca..81ca204e3 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -4,7 +4,6 @@ #include "gc-store.hh" #include "signals.hh" #include "loggers.hh" -#include "progress-bar.hh" #include "current-process.hh" #include @@ -349,7 +348,7 @@ RunPager::RunPager() if (!pager) pager = getenv("PAGER"); if (pager && ((std::string) pager == "" || (std::string) pager == "cat")) return; - stopProgressBar(); + logger->pause(); Pipe toPager; toPager.create(); diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 3cead4296..7990ffce0 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -117,9 +117,8 @@ public: virtual ~Logger() { } - virtual void stop() { }; - virtual void pause() { }; + virtual void resetProgress() { }; virtual void resume() { }; // Whether the logger prints the whole build log diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 37b553dbb..4b8d7a2fa 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -543,7 +543,7 @@ static void main_nix_build(int argc, char * * argv) restoreProcessContext(); - logger->stop(); + logger->pause(); execvp(shell->c_str(), argPtrs.data()); @@ -606,7 +606,7 @@ static void main_nix_build(int argc, char * * argv) outPaths.push_back(outputPath); } - logger->stop(); + logger->pause(); for (auto & path : outPaths) std::cout << store->printStorePath(path) << '\n'; diff --git a/src/nix/build.cc b/src/nix/build.cc index 479100186..6de52c0b6 100644 --- a/src/nix/build.cc +++ b/src/nix/build.cc @@ -3,7 +3,6 @@ #include "shared.hh" #include "store-api.hh" #include "local-fs-store.hh" -#include "progress-bar.hh" #include @@ -143,7 +142,7 @@ struct CmdBuild : InstallablesCommand, MixDryRun, MixJSON, MixProfile createOutLinks(outLink, buildables, *store2); if (printOutputPaths) { - stopProgressBar(); + logger->pause(); for (auto & buildable : buildables) { std::visit(overloaded { [&](const BuiltPath::Opaque & bo) { diff --git a/src/nix/cat.cc b/src/nix/cat.cc index 678edd9a1..81c21e2ad 100644 --- a/src/nix/cat.cc +++ b/src/nix/cat.cc @@ -2,7 +2,6 @@ #include "store-api.hh" #include "fs-accessor.hh" #include "nar-accessor.hh" -#include "progress-bar.hh" using namespace nix; @@ -20,7 +19,7 @@ struct MixCat : virtual Args auto file = accessor->readFile(path); - stopProgressBar(); + logger->pause(); writeFull(STDOUT_FILENO, file); } }; diff --git a/src/nix/develop.cc b/src/nix/develop.cc index 353bf0110..fb144c904 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -6,7 +6,6 @@ #include "store-api.hh" #include "outputs-spec.hh" #include "derivations.hh" -#include "progress-bar.hh" #include "run.hh" #include @@ -690,7 +689,7 @@ struct CmdPrintDevEnv : Common, MixJSON { auto buildEnvironment = getBuildEnvironment(store, installable).first; - stopProgressBar(); + logger->pause(); if (json) { logger->writeToStdout(buildEnvironment.toJSON()); diff --git a/src/nix/dump-path.cc b/src/nix/dump-path.cc index fb32dddb7..99ff05dcc 100644 --- a/src/nix/dump-path.cc +++ b/src/nix/dump-path.cc @@ -1,7 +1,6 @@ #include "command.hh" #include "store-api.hh" #include "archive.hh" -#include "progress-bar.hh" using namespace nix; @@ -21,7 +20,7 @@ struct CmdDumpPath : StorePathCommand void run(ref store, const StorePath & storePath) override { - stopProgressBar(); + logger->pause(); FdSink sink(STDOUT_FILENO); store->narFromPath(storePath, sink); sink.flush(); @@ -57,7 +56,7 @@ struct CmdDumpPath2 : Command void run() override { - stopProgressBar(); + logger->pause(); FdSink sink(STDOUT_FILENO); dumpPath(path, sink); sink.flush(); diff --git a/src/nix/edit.cc b/src/nix/edit.cc index 2f701f145..8352c26e8 100644 --- a/src/nix/edit.cc +++ b/src/nix/edit.cc @@ -2,7 +2,6 @@ #include "shared.hh" #include "eval.hh" #include "attr-path.hh" -#include "progress-bar.hh" #include "editor-for.hh" #include "current-process.hh" @@ -42,7 +41,7 @@ struct CmdEdit : InstallableCommand } }(); - stopProgressBar(); + logger->pause(); auto args = editorFor(file, line); diff --git a/src/nix/eval.cc b/src/nix/eval.cc index 9f265930b..a027b9a58 100644 --- a/src/nix/eval.cc +++ b/src/nix/eval.cc @@ -6,7 +6,6 @@ #include "eval.hh" #include "eval-inline.hh" #include "value-to-json.hh" -#include "progress-bar.hh" #include @@ -76,7 +75,7 @@ struct CmdEval : MixJSON, InstallableCommand, MixReadOnlyOption } if (writeTo) { - stopProgressBar(); + logger->pause(); if (pathExists(*writeTo)) throw Error("path '%s' already exists", *writeTo); @@ -114,7 +113,7 @@ struct CmdEval : MixJSON, InstallableCommand, MixReadOnlyOption } else if (raw) { - stopProgressBar(); + logger->pause(); writeFull(STDOUT_FILENO, *state->coerceToString(noPos, *v, context, "while generating the eval command output")); } diff --git a/src/nix/log.cc b/src/nix/log.cc index 9a9bd30f9..b291489b5 100644 --- a/src/nix/log.cc +++ b/src/nix/log.cc @@ -3,7 +3,6 @@ #include "shared.hh" #include "store-api.hh" #include "log-store.hh" -#include "progress-bar.hh" using namespace nix; @@ -55,7 +54,7 @@ struct CmdLog : InstallableCommand auto log = logSub.getBuildLog(path); if (!log) continue; - stopProgressBar(); + logger->pause(); printInfo("got build log for '%s' from '%s'", installable->what(), logSub.getUri()); writeFull(STDOUT_FILENO, *log); return; diff --git a/src/nix/main.cc b/src/nix/main.cc index 55f8d59ba..2f52a352f 100644 --- a/src/nix/main.cc +++ b/src/nix/main.cc @@ -346,8 +346,6 @@ void mainWrapped(int argc, char * * argv) } #endif - Finally f([] { logger->stop(); }); - programPath = argv[0]; auto programName = std::string(baseNameOf(programPath)); @@ -363,7 +361,8 @@ void mainWrapped(int argc, char * * argv) evalSettings.pureEval = true; - setLogFormat("bar"); + setLogFormat(LogFormat::bar); + Finally f([] { logger->pause(); }); settings.verboseBuild = false; if (isatty(STDERR_FILENO)) { verbosity = lvlNotice; diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index cad70e726..13d94d645 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -1,10 +1,10 @@ #include "command.hh" #include "common-args.hh" +#include "loggers.hh" #include "shared.hh" #include "store-api.hh" #include "filetransfer.hh" #include "finally.hh" -#include "progress-bar.hh" #include "tarfile.hh" #include "attr-path.hh" #include "eval-inline.hh" @@ -180,10 +180,8 @@ static int main_nix_prefetch_url(int argc, char * * argv) if (args.size() > 2) throw UsageError("too many arguments"); - Finally f([]() { stopProgressBar(); }); - if (isatty(STDERR_FILENO)) - startProgressBar(); + setLogFormat(LogFormat::bar); auto store = openStore(); auto state = std::make_unique(myArgs.searchPath, store); @@ -237,7 +235,7 @@ static int main_nix_prefetch_url(int argc, char * * argv) auto [storePath, hash] = prefetchFile( store, resolveMirrorUrl(*state, url), name, ht, expectedHash, unpack, executable); - stopProgressBar(); + logger->pause(); if (!printPath) printInfo("path is '%s'", store->printStorePath(storePath)); diff --git a/src/nix/run.cc b/src/nix/run.cc index 1e4406df5..824201fdf 100644 --- a/src/nix/run.cc +++ b/src/nix/run.cc @@ -8,7 +8,6 @@ #include "local-store.hh" #include "finally.hh" #include "fs-accessor.hh" -#include "progress-bar.hh" #include "eval.hh" #include "build/personality.hh" #include "current-process.hh" @@ -31,7 +30,7 @@ void runProgramInStore(ref store, const Strings & args, std::optional system) { - stopProgressBar(); + logger->pause(); restoreProcessContext(); diff --git a/src/nix/sigs.cc b/src/nix/sigs.cc index eeb14e29a..948844e22 100644 --- a/src/nix/sigs.cc +++ b/src/nix/sigs.cc @@ -3,7 +3,6 @@ #include "store-api.hh" #include "thread-pool.hh" #include "signals.hh" -#include "progress-bar.hh" #include @@ -222,7 +221,7 @@ struct CmdKey : NixMultiCommand if (!command) throw UsageError("'nix key' requires a sub-command."); - stopProgressBar(); + logger->pause(); command->second->run(); } }; diff --git a/src/nix/upgrade-nix.cc b/src/nix/upgrade-nix.cc index c7f31f3fb..371879791 100644 --- a/src/nix/upgrade-nix.cc +++ b/src/nix/upgrade-nix.cc @@ -13,7 +13,6 @@ #include "eval-settings.hh" #include "attr-path.hh" #include "names.hh" -#include "progress-bar.hh" using namespace nix; @@ -88,7 +87,7 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand auto version = DrvName(storePath.name()).version; if (dryRun) { - stopProgressBar(); + logger->pause(); warn("would upgrade to version %s", version); return; } @@ -106,7 +105,7 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand throw Error("could not verify that '%s' works", program); } - stopProgressBar(); + logger->pause(); auto const fullStorePath = store->printStorePath(storePath); diff --git a/src/nix/why-depends.cc b/src/nix/why-depends.cc index 055cf6d0d..5bef11c4d 100644 --- a/src/nix/why-depends.cc +++ b/src/nix/why-depends.cc @@ -1,6 +1,5 @@ #include "command.hh" #include "store-api.hh" -#include "progress-bar.hh" #include "fs-accessor.hh" #include "shared.hh" @@ -110,7 +109,7 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions auto dependencyPath = *optDependencyPath; auto dependencyPathHash = dependencyPath.hashPart(); - stopProgressBar(); // FIXME + logger->pause(); // FIXME auto accessor = store->getFSAccessor(); diff --git a/tests/unit/libmain/progress-bar.cc b/tests/unit/libmain/progress-bar.cc index e44a8b37e..2f2c7dc77 100644 --- a/tests/unit/libmain/progress-bar.cc +++ b/tests/unit/libmain/progress-bar.cc @@ -2,6 +2,7 @@ #include "eval.hh" #include "progress-bar.hh" +#include "loggers.hh" #include "logging.hh" #include "shared.hh" @@ -23,7 +24,7 @@ namespace nix initNix(); initGC(); - startProgressBar(); + setLogFormat(LogFormat::bar); ASSERT_NE(dynamic_cast(logger), nullptr); ProgressBar & progressBar = dynamic_cast(*logger);