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 (243c0f18da) 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 <qyriad@qyriad.me>
Change-Id: I2b7c3eb17d439cd0c16f7b896cfb61239ac7ff3a
This commit is contained in:
alois31 2024-06-29 15:03:44 +02:00
parent a55112898e
commit 0dd1d8ca1c
Signed by untrusted user: alois31
GPG key ID: E0F59EA5E5216914
21 changed files with 68 additions and 98 deletions

View file

@ -32,7 +32,6 @@
#include "local-fs-store.hh" #include "local-fs-store.hh"
#include "signals.hh" #include "signals.hh"
#include "print.hh" #include "print.hh"
#include "progress-bar.hh"
#include "gc-small-vector.hh" #include "gc-small-vector.hh"
#include "users.hh" #include "users.hh"
@ -300,7 +299,7 @@ ReplExitStatus NixRepl::mainLoop()
/* Stop the progress bar because it interferes with the display of /* Stop the progress bar because it interferes with the display of
the repl. */ the repl. */
stopProgressBar(); logger->pause();
std::string input; std::string input;
@ -684,9 +683,10 @@ ProcessLineResult NixRepl::processLine(std::string line)
// TODO: this only shows a progress bar for explicitly initiated builds, // TODO: this only shows a progress bar for explicitly initiated builds,
// not eval-time fetching or builds performed for IFD. // 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. // But we can't just show it everywhere, since that would erase partial output from evaluation.
startProgressBar(); logger->resetProgress();
logger->resume();
Finally stopLogger([&]() { Finally stopLogger([&]() {
stopProgressBar(); logger->pause();
}); });
state->store->buildPaths({ state->store->buildPaths({

View file

@ -403,11 +403,8 @@ struct GitInputScheme : InputScheme
AutoDelete const _delete{msgPath}; AutoDelete const _delete{msgPath};
writeFile(msgPath, *commitMsg); 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, 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);
} }
} }
} }

View file

@ -4,7 +4,6 @@
#include "names.hh" #include "names.hh"
#include "terminal.hh" #include "terminal.hh"
#include <atomic>
#include <map> #include <map>
#include <thread> #include <thread>
#include <sstream> #include <sstream>
@ -44,50 +43,56 @@ static std::string_view storePathToName(std::string_view path)
ProgressBar::ProgressBar(bool isTTY) ProgressBar::ProgressBar(bool isTTY)
: isTTY(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() ProgressBar::~ProgressBar()
{ {
stop(); pause();
} }
/* Called by destructor, can't be overridden */ void ProgressBar::pause()
void ProgressBar::stop()
{ {
if (!isTTY) return;
{ {
auto state(state_.lock()); 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(); updateCV.notify_one();
quitCV.notify_one(); quitCV.notify_one();
} }
updateThread.join(); 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() void ProgressBar::resume()
{ {
state_.lock()->paused = false; if (!isTTY) return;
writeToStderr("\r\e[K"); auto state(state_.lock());
state_.lock()->haveUpdate = true; assert(state->paused > 0); // should be paused
updateCV.notify_one(); 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() bool ProgressBar::isVerbose()
@ -114,7 +119,7 @@ void ProgressBar::logEI(const ErrorInfo & ei)
void ProgressBar::log(State & state, Verbosity lvl, std::string_view s) void ProgressBar::log(State & state, Verbosity lvl, std::string_view s)
{ {
if (state.active) { if (state.paused == 0) {
draw(state, s); draw(state, s);
} else { } else {
auto s2 = s + ANSI_NORMAL "\n"; 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; auto nextWakeup = A_LONG_TIME;
state.haveUpdate = false; state.haveUpdate = false;
if (state.paused || !state.active) return nextWakeup; if (state.paused > 0) return nextWakeup;
auto windowSize = getWindowSize(); auto windowSize = getWindowSize();
auto width = windowSize.second; auto width = windowSize.second;
@ -525,7 +530,7 @@ std::string ProgressBar::getStatus(State & state)
void ProgressBar::writeToStdout(std::string_view s) void ProgressBar::writeToStdout(std::string_view s)
{ {
auto state(state_.lock()); auto state(state_.lock());
if (state->active) { if (state->paused == 0) {
Logger::writeToStdout(s); Logger::writeToStdout(s);
draw(*state, {}); draw(*state, {});
} else { } else {
@ -536,7 +541,7 @@ void ProgressBar::writeToStdout(std::string_view s)
std::optional<char> ProgressBar::ask(std::string_view msg) std::optional<char> ProgressBar::ask(std::string_view msg)
{ {
auto state(state_.lock()); 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); std::cerr << fmt("\r\e[K%s ", msg);
auto s = trim(readLine(STDIN_FILENO)); auto s = trim(readLine(STDIN_FILENO));
if (s.size() != 1) return {}; if (s.size() != 1) return {};
@ -559,16 +564,4 @@ Logger * makeProgressBar()
return new ProgressBar(shouldANSI()); return new ProgressBar(shouldANSI());
} }
void startProgressBar()
{
logger = makeProgressBar();
}
void stopProgressBar()
{
auto progressBar = dynamic_cast<ProgressBar *>(logger);
if (progressBar) progressBar->stop();
}
} }

View file

@ -48,9 +48,8 @@ struct ProgressBar : public Logger
uint64_t corruptedPaths = 0, untrustedPaths = 0; 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_; Sync<State> state_;
@ -67,10 +66,10 @@ struct ProgressBar : public Logger
~ProgressBar(); ~ProgressBar();
void stop() override final;
void pause() override; void pause() override;
void resetProgress() override;
void resume() override; void resume() override;
bool isVerbose() override; bool isVerbose() override;
@ -113,8 +112,4 @@ struct ProgressBar : public Logger
Logger * makeProgressBar(); Logger * makeProgressBar();
void startProgressBar();
void stopProgressBar();
} }

View file

@ -4,7 +4,6 @@
#include "gc-store.hh" #include "gc-store.hh"
#include "signals.hh" #include "signals.hh"
#include "loggers.hh" #include "loggers.hh"
#include "progress-bar.hh"
#include "current-process.hh" #include "current-process.hh"
#include <algorithm> #include <algorithm>
@ -349,7 +348,7 @@ RunPager::RunPager()
if (!pager) pager = getenv("PAGER"); if (!pager) pager = getenv("PAGER");
if (pager && ((std::string) pager == "" || (std::string) pager == "cat")) return; if (pager && ((std::string) pager == "" || (std::string) pager == "cat")) return;
stopProgressBar(); logger->pause();
Pipe toPager; Pipe toPager;
toPager.create(); toPager.create();

View file

@ -117,9 +117,8 @@ public:
virtual ~Logger() { } virtual ~Logger() { }
virtual void stop() { };
virtual void pause() { }; virtual void pause() { };
virtual void resetProgress() { };
virtual void resume() { }; virtual void resume() { };
// Whether the logger prints the whole build log // Whether the logger prints the whole build log

View file

@ -543,7 +543,7 @@ static void main_nix_build(int argc, char * * argv)
restoreProcessContext(); restoreProcessContext();
logger->stop(); logger->pause();
execvp(shell->c_str(), argPtrs.data()); execvp(shell->c_str(), argPtrs.data());
@ -606,7 +606,7 @@ static void main_nix_build(int argc, char * * argv)
outPaths.push_back(outputPath); outPaths.push_back(outputPath);
} }
logger->stop(); logger->pause();
for (auto & path : outPaths) for (auto & path : outPaths)
std::cout << store->printStorePath(path) << '\n'; std::cout << store->printStorePath(path) << '\n';

View file

@ -3,7 +3,6 @@
#include "shared.hh" #include "shared.hh"
#include "store-api.hh" #include "store-api.hh"
#include "local-fs-store.hh" #include "local-fs-store.hh"
#include "progress-bar.hh"
#include <nlohmann/json.hpp> #include <nlohmann/json.hpp>
@ -143,7 +142,7 @@ struct CmdBuild : InstallablesCommand, MixDryRun, MixJSON, MixProfile
createOutLinks(outLink, buildables, *store2); createOutLinks(outLink, buildables, *store2);
if (printOutputPaths) { if (printOutputPaths) {
stopProgressBar(); logger->pause();
for (auto & buildable : buildables) { for (auto & buildable : buildables) {
std::visit(overloaded { std::visit(overloaded {
[&](const BuiltPath::Opaque & bo) { [&](const BuiltPath::Opaque & bo) {

View file

@ -2,7 +2,6 @@
#include "store-api.hh" #include "store-api.hh"
#include "fs-accessor.hh" #include "fs-accessor.hh"
#include "nar-accessor.hh" #include "nar-accessor.hh"
#include "progress-bar.hh"
using namespace nix; using namespace nix;
@ -20,7 +19,7 @@ struct MixCat : virtual Args
auto file = accessor->readFile(path); auto file = accessor->readFile(path);
stopProgressBar(); logger->pause();
writeFull(STDOUT_FILENO, file); writeFull(STDOUT_FILENO, file);
} }
}; };

View file

@ -6,7 +6,6 @@
#include "store-api.hh" #include "store-api.hh"
#include "outputs-spec.hh" #include "outputs-spec.hh"
#include "derivations.hh" #include "derivations.hh"
#include "progress-bar.hh"
#include "run.hh" #include "run.hh"
#include <iterator> #include <iterator>
@ -690,7 +689,7 @@ struct CmdPrintDevEnv : Common, MixJSON
{ {
auto buildEnvironment = getBuildEnvironment(store, installable).first; auto buildEnvironment = getBuildEnvironment(store, installable).first;
stopProgressBar(); logger->pause();
if (json) { if (json) {
logger->writeToStdout(buildEnvironment.toJSON()); logger->writeToStdout(buildEnvironment.toJSON());

View file

@ -1,7 +1,6 @@
#include "command.hh" #include "command.hh"
#include "store-api.hh" #include "store-api.hh"
#include "archive.hh" #include "archive.hh"
#include "progress-bar.hh"
using namespace nix; using namespace nix;
@ -21,7 +20,7 @@ struct CmdDumpPath : StorePathCommand
void run(ref<Store> store, const StorePath & storePath) override void run(ref<Store> store, const StorePath & storePath) override
{ {
stopProgressBar(); logger->pause();
FdSink sink(STDOUT_FILENO); FdSink sink(STDOUT_FILENO);
store->narFromPath(storePath, sink); store->narFromPath(storePath, sink);
sink.flush(); sink.flush();
@ -57,7 +56,7 @@ struct CmdDumpPath2 : Command
void run() override void run() override
{ {
stopProgressBar(); logger->pause();
FdSink sink(STDOUT_FILENO); FdSink sink(STDOUT_FILENO);
dumpPath(path, sink); dumpPath(path, sink);
sink.flush(); sink.flush();

View file

@ -2,7 +2,6 @@
#include "shared.hh" #include "shared.hh"
#include "eval.hh" #include "eval.hh"
#include "attr-path.hh" #include "attr-path.hh"
#include "progress-bar.hh"
#include "editor-for.hh" #include "editor-for.hh"
#include "current-process.hh" #include "current-process.hh"
@ -42,7 +41,7 @@ struct CmdEdit : InstallableCommand
} }
}(); }();
stopProgressBar(); logger->pause();
auto args = editorFor(file, line); auto args = editorFor(file, line);

View file

@ -6,7 +6,6 @@
#include "eval.hh" #include "eval.hh"
#include "eval-inline.hh" #include "eval-inline.hh"
#include "value-to-json.hh" #include "value-to-json.hh"
#include "progress-bar.hh"
#include <nlohmann/json.hpp> #include <nlohmann/json.hpp>
@ -76,7 +75,7 @@ struct CmdEval : MixJSON, InstallableCommand, MixReadOnlyOption
} }
if (writeTo) { if (writeTo) {
stopProgressBar(); logger->pause();
if (pathExists(*writeTo)) if (pathExists(*writeTo))
throw Error("path '%s' already exists", *writeTo); throw Error("path '%s' already exists", *writeTo);
@ -114,7 +113,7 @@ struct CmdEval : MixJSON, InstallableCommand, MixReadOnlyOption
} }
else if (raw) { else if (raw) {
stopProgressBar(); logger->pause();
writeFull(STDOUT_FILENO, *state->coerceToString(noPos, *v, context, "while generating the eval command output")); writeFull(STDOUT_FILENO, *state->coerceToString(noPos, *v, context, "while generating the eval command output"));
} }

View file

@ -3,7 +3,6 @@
#include "shared.hh" #include "shared.hh"
#include "store-api.hh" #include "store-api.hh"
#include "log-store.hh" #include "log-store.hh"
#include "progress-bar.hh"
using namespace nix; using namespace nix;
@ -55,7 +54,7 @@ struct CmdLog : InstallableCommand
auto log = logSub.getBuildLog(path); auto log = logSub.getBuildLog(path);
if (!log) continue; if (!log) continue;
stopProgressBar(); logger->pause();
printInfo("got build log for '%s' from '%s'", installable->what(), logSub.getUri()); printInfo("got build log for '%s' from '%s'", installable->what(), logSub.getUri());
writeFull(STDOUT_FILENO, *log); writeFull(STDOUT_FILENO, *log);
return; return;

View file

@ -346,8 +346,6 @@ void mainWrapped(int argc, char * * argv)
} }
#endif #endif
Finally f([] { logger->stop(); });
programPath = argv[0]; programPath = argv[0];
auto programName = std::string(baseNameOf(programPath)); auto programName = std::string(baseNameOf(programPath));
@ -363,7 +361,8 @@ void mainWrapped(int argc, char * * argv)
evalSettings.pureEval = true; evalSettings.pureEval = true;
setLogFormat("bar"); setLogFormat(LogFormat::bar);
Finally f([] { logger->pause(); });
settings.verboseBuild = false; settings.verboseBuild = false;
if (isatty(STDERR_FILENO)) { if (isatty(STDERR_FILENO)) {
verbosity = lvlNotice; verbosity = lvlNotice;

View file

@ -1,10 +1,10 @@
#include "command.hh" #include "command.hh"
#include "common-args.hh" #include "common-args.hh"
#include "loggers.hh"
#include "shared.hh" #include "shared.hh"
#include "store-api.hh" #include "store-api.hh"
#include "filetransfer.hh" #include "filetransfer.hh"
#include "finally.hh" #include "finally.hh"
#include "progress-bar.hh"
#include "tarfile.hh" #include "tarfile.hh"
#include "attr-path.hh" #include "attr-path.hh"
#include "eval-inline.hh" #include "eval-inline.hh"
@ -180,10 +180,8 @@ static int main_nix_prefetch_url(int argc, char * * argv)
if (args.size() > 2) if (args.size() > 2)
throw UsageError("too many arguments"); throw UsageError("too many arguments");
Finally f([]() { stopProgressBar(); });
if (isatty(STDERR_FILENO)) if (isatty(STDERR_FILENO))
startProgressBar(); setLogFormat(LogFormat::bar);
auto store = openStore(); auto store = openStore();
auto state = std::make_unique<EvalState>(myArgs.searchPath, store); 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( auto [storePath, hash] = prefetchFile(
store, resolveMirrorUrl(*state, url), name, ht, expectedHash, unpack, executable); store, resolveMirrorUrl(*state, url), name, ht, expectedHash, unpack, executable);
stopProgressBar(); logger->pause();
if (!printPath) if (!printPath)
printInfo("path is '%s'", store->printStorePath(storePath)); printInfo("path is '%s'", store->printStorePath(storePath));

View file

@ -8,7 +8,6 @@
#include "local-store.hh" #include "local-store.hh"
#include "finally.hh" #include "finally.hh"
#include "fs-accessor.hh" #include "fs-accessor.hh"
#include "progress-bar.hh"
#include "eval.hh" #include "eval.hh"
#include "build/personality.hh" #include "build/personality.hh"
#include "current-process.hh" #include "current-process.hh"
@ -31,7 +30,7 @@ void runProgramInStore(ref<Store> store,
const Strings & args, const Strings & args,
std::optional<std::string_view> system) std::optional<std::string_view> system)
{ {
stopProgressBar(); logger->pause();
restoreProcessContext(); restoreProcessContext();

View file

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

View file

@ -13,7 +13,6 @@
#include "eval-settings.hh" #include "eval-settings.hh"
#include "attr-path.hh" #include "attr-path.hh"
#include "names.hh" #include "names.hh"
#include "progress-bar.hh"
using namespace nix; using namespace nix;
@ -88,7 +87,7 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand
auto version = DrvName(storePath.name()).version; auto version = DrvName(storePath.name()).version;
if (dryRun) { if (dryRun) {
stopProgressBar(); logger->pause();
warn("would upgrade to version %s", version); warn("would upgrade to version %s", version);
return; return;
} }
@ -106,7 +105,7 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand
throw Error("could not verify that '%s' works", program); throw Error("could not verify that '%s' works", program);
} }
stopProgressBar(); logger->pause();
auto const fullStorePath = store->printStorePath(storePath); auto const fullStorePath = store->printStorePath(storePath);

View file

@ -1,6 +1,5 @@
#include "command.hh" #include "command.hh"
#include "store-api.hh" #include "store-api.hh"
#include "progress-bar.hh"
#include "fs-accessor.hh" #include "fs-accessor.hh"
#include "shared.hh" #include "shared.hh"
@ -110,7 +109,7 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions
auto dependencyPath = *optDependencyPath; auto dependencyPath = *optDependencyPath;
auto dependencyPathHash = dependencyPath.hashPart(); auto dependencyPathHash = dependencyPath.hashPart();
stopProgressBar(); // FIXME logger->pause(); // FIXME
auto accessor = store->getFSAccessor(); auto accessor = store->getFSAccessor();

View file

@ -2,6 +2,7 @@
#include "eval.hh" #include "eval.hh"
#include "progress-bar.hh" #include "progress-bar.hh"
#include "loggers.hh"
#include "logging.hh" #include "logging.hh"
#include "shared.hh" #include "shared.hh"
@ -23,7 +24,7 @@ namespace nix
initNix(); initNix();
initGC(); initGC();
startProgressBar(); setLogFormat(LogFormat::bar);
ASSERT_NE(dynamic_cast<ProgressBar *>(logger), nullptr); ASSERT_NE(dynamic_cast<ProgressBar *>(logger), nullptr);
ProgressBar & progressBar = dynamic_cast<ProgressBar &>(*logger); ProgressBar & progressBar = dynamic_cast<ProgressBar &>(*logger);