Merge "tree-wide: unify progress bar inactive and paused states" into main

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({

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);
             }
         }
     }

src/libmain/progress-bar.cc

@@ -4,7 +4,6 @@
 #include "names.hh"
 #include "terminal.hh"
 
-#include <atomic>
 #include <map>
 #include <thread>
 #include <sstream>
@@ -44,50 +43,56 @@ static std::string_view storePathToName(std::string_view path)
 ProgressBar::ProgressBar(bool isTTY)
     : isTTY(isTTY)
 {
-    state_.lock()->active = isTTY;
+    resume();
-    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));
-        }
-    });
 }
 
 ProgressBar::~ProgressBar()
 {
-    stop();
+    pause();
 }
 
-/* Called by destructor, can't be overridden */
+void ProgressBar::pause()
-void ProgressBar::stop()
 {
+    if (!isTTY) return;
     {
         auto state(state_.lock());
-        if (!state->active) return;
+        state->paused++;
-        state->active = false;
+        if (state->paused > 1) return; // recursive pause, the update thread is already gone
-        writeToStderr("\r\e[K");
         updateCV.notify_one();
         quitCV.notify_one();
     }
     updateThread.join();
 }
 
-void ProgressBar::pause()
+void ProgressBar::resetProgress()
 {
-    state_.lock()->paused = true;
+    auto state(state_.lock());
-    writeToStderr("\r\e[K");
+    auto prevPaused = state->paused;
+    *state = ProgressBar::State {
+        .paused = prevPaused,
+    };
+    update(*state);
 }
 
 void ProgressBar::resume()
 {
-    state_.lock()->paused = false;
+    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");
-    state_.lock()->haveUpdate = true;
+    });
-    updateCV.notify_one();
 }
 
 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<s
     auto nextWakeup = A_LONG_TIME;
 
     state.haveUpdate = false;
-    if (state.paused || !state.active) return nextWakeup;
+    if (state.paused > 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<char> 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<ProgressBar *>(logger);
-    if (progressBar) progressBar->stop();
-
-}
-
 }

src/libmain/progress-bar.hh

@@ -48,9 +48,8 @@ struct ProgressBar : public Logger
 
         uint64_t corruptedPaths = 0, untrustedPaths = 0;
 
-        bool active = true;
+        uint32_t paused = 1;
-        bool paused = false;
+        bool haveUpdate = false;
-        bool haveUpdate = true;
     };
 
     Sync<State> 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();
-
 }

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 <algorithm>
@@ -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();

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

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';

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 <nlohmann/json.hpp>
 
@@ -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) {

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);
     }
 };

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 <iterator>
@@ -690,7 +689,7 @@ struct CmdPrintDevEnv : Common, MixJSON
     {
         auto buildEnvironment = getBuildEnvironment(store, installable).first;
 
-        stopProgressBar();
+        logger->pause();
 
         if (json) {
             logger->writeToStdout(buildEnvironment.toJSON());

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> 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();

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);
 

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 <nlohmann/json.hpp>
 
@@ -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"));
         }
 

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;

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;

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<EvalState>(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));

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> store,
     const Strings & args,
     std::optional<std::string_view> system)
 {
-    stopProgressBar();
+    logger->pause();
 
     restoreProcessContext();
 

src/nix/sigs.cc

@@ -3,7 +3,6 @@
 #include "store-api.hh"
 #include "thread-pool.hh"
 #include "signals.hh"
-#include "progress-bar.hh"
 
 #include <atomic>
 
@@ -222,7 +221,7 @@ struct CmdKey : NixMultiCommand
         if (!command)
             throw UsageError("'nix key' requires a sub-command.");
 
-        stopProgressBar();
+        logger->pause();
         command->second->run();
     }
 };

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);
 

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();
 

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<ProgressBar *>(logger), nullptr);
         ProgressBar & progressBar = dynamic_cast<ProgressBar &>(*logger);