From 206a5dbb8f4606a1a7b8d0179e018880b9b92575 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Mon, 24 Jun 2024 18:26:05 +0200 Subject: [PATCH 1/2] libmain/progress-bar: move implementation out of the header Change-Id: Ib4b42ebea290ee575294df6b2f17a38a5d850b80 --- src/libmain/progress-bar.cc | 20 ++++++++++++++++++++ src/libmain/progress-bar.hh | 21 +-------------------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index 28bb14863..e36bc0b01 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -13,6 +13,11 @@ namespace nix { +// 100 years ought to be enough for anyone (yet sufficiently smaller than max() to not cause signed integer overflow). +constexpr const auto A_LONG_TIME = std::chrono::duration_cast( + 100 * 365 * std::chrono::seconds(86400) +); + using namespace std::literals::chrono_literals; static std::string_view getS(const std::vector & fields, size_t n) @@ -36,6 +41,21 @@ static std::string_view storePathToName(std::string_view path) return i == std::string::npos ? base.substr(0, 0) : base.substr(i + 1); } +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)); + } + }); +} ProgressBar::~ProgressBar() { diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index 176e941e8..e682d75fe 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -8,11 +8,6 @@ namespace nix { -// 100 years ought to be enough for anyone (yet sufficiently smaller than max() to not cause signed integer overflow). -constexpr const auto A_LONG_TIME = std::chrono::duration_cast( - 100 * 365 * std::chrono::seconds(86400) -); - struct ProgressBar : public Logger { struct ActInfo @@ -68,21 +63,7 @@ struct ProgressBar : public Logger bool printMultiline = false; bool isTTY; - 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)); - } - }); - } + ProgressBar(bool isTTY); ~ProgressBar(); From 8eb2730d6d93f4847689148417b84889ccf4cbc5 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 16:53:12 -0600 Subject: [PATCH 2/2] repl: respect --print-build-logs & fix memory leak 243c0f18d[1] allowed the logger's progress bar to display during repl builds, but startProgressBar() re-creates the entire logger from scratch, discarding the value of printBuildLogs (and leaking the previous logger). In this commit, we instead make the update thread stopping solely conditional on quitCV, unset active, and manually reset activities and stored values in the progress bar's state, so stuff from multiple builds don't leak over into following ones. [1]: 243c0f18dae2a08ea0e46f7ff33277c63f7506d7 Change-Id: Ie734d1f638d45759d232805d7e3c2005f7dea483 --- src/libcmd/repl.cc | 7 +++-- src/libmain/progress-bar.cc | 57 +++++++++++++++++++++++++++++++++---- src/libmain/progress-bar.hh | 16 +++++++++++ 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 28341259c..d8d7ad3c0 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -300,7 +300,7 @@ ReplExitStatus NixRepl::mainLoop() /* Stop the progress bar because it interferes with the display of the repl. */ - stopProgressBar(); + deactivateProgressBar(); std::string input; @@ -684,11 +684,12 @@ 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(); + reactivateProgressBar(); Finally stopLogger([&]() { - stopProgressBar(); + deactivateProgressBar(); }); + state->store->buildPaths({ DerivedPath::Built { .drvPath = makeConstantStorePathRef(drvPath), diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index e36bc0b01..5da2d69c9 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -48,11 +48,24 @@ ProgressBar::ProgressBar(bool 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)); + + auto shouldQuit = [&]() -> bool { + auto const status = state.wait_for(quitCV, std::chrono::milliseconds(50)); + return status == std::cv_status::timeout; + }; + + while (!shouldQuit()) { + while (state->active) { + if (!state->haveUpdate) { + state.wait_for(updateCV, nextWakeup); + } + + nextWakeup = draw(*state, std::nullopt); + + // So we don't spin this loop too much while we're active but + // without updates. + shouldQuit(); + } } }); } @@ -554,6 +567,24 @@ void ProgressBar::setPrintMultiline(bool printMultiline) this->printMultiline = printMultiline; } +void ProgressBar::deactivateBar() +{ + auto state(state_.lock()); + bool const prevPaused = state->paused; + bool const prevHaveUpdate = state->haveUpdate; + *state = ProgressBar::State{ + .active = false, + .paused = prevPaused, + .haveUpdate = prevHaveUpdate, + // Default constructors for the rest. + }; +} + +void ProgressBar::reactivateBar() +{ + state_.lock()-> active = true; +} + Logger * makeProgressBar() { return new ProgressBar(shouldANSI()); @@ -571,4 +602,20 @@ void stopProgressBar() } +void deactivateProgressBar() +{ + auto progressBar = dynamic_cast(logger); + if (progressBar) { + progressBar->deactivateBar(); + } +} + +void reactivateProgressBar() +{ + auto progressBar = dynamic_cast(logger); + if (progressBar) { + progressBar->reactivateBar(); + } +} + } diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index e682d75fe..a35e348b8 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -109,6 +109,16 @@ struct ProgressBar : public Logger void setPrintBuildLogs(bool printBuildLogs) override; void setPrintMultiline(bool printMultiline) override; + + /** Deactivate the progress bar and reset activity statistics, but leave other + * logging alone (unlike pause() which pauses all logging). + */ + void deactivateBar(); + + /** Reactivate the progress bar after having previously been deactivated. + * No-op if already active. + */ + void reactivateBar(); }; Logger * makeProgressBar(); @@ -117,4 +127,10 @@ void startProgressBar(); void stopProgressBar(); +/** Call ProgressBar::deactivateBar() iff nix::Logger is a progress bar logger. */ +void deactivateProgressBar(); + +/** Call ProgressBar::reactivateBar() iff nix::Logger is a progress bar logger. */ +void reactivateProgressBar(); + }