Progress indicator improvements #4296

edolstra · 2020-12-01T12:41:07Z

This replaces the current one-line progress bar with a progress indicator with the following features:

Interactive: you can hit the L key to enable display of build output, v to increase verbosity, r to show which paths are remaining to be built, etc.
Multi-line: rather than cramming a lot of info into a single line, it has separate progress bars for substitutions, builds, etc.
More logging: separately from the progress indicator (which only shows transient info), we log to stderr how long successful builds / substitutions took.
More use of colors.

SuperSandro2000 · 2020-12-01T13:01:26Z

Does this preserve color of log messages?

Can we customize the colors and used characters?

edolstra · 2020-12-01T13:26:05Z

If you mean color in build output, then yes (example).

There is no customization yet.

andir · 2020-12-01T15:10:58Z

src/libmain/progress-bar.cc

+        return stats;
+    }
+
+    enum StatusLineGroup {


Wouldn't it make sense to use a enum class here for some "free" type-safety?

mweinelt · 2020-12-01T15:10:59Z

I hope this can take some more inspiration from https://github.com/maralorn/nix-output-monitor. I particularly like the list of packages that are currently being built and on which machines that happens.

andir

Just did a quick read through some of the code and had a few remarks. I really like the prospect of having this (or something like it) in Nix. Having a better progress indicator for what Nix is doing is great. 👍

andir · 2020-12-01T15:48:36Z

src/libmain/progress-bar.cc

+                        if (errno == EINTR) continue;
+                        break;
+                    }
+                    c = std::tolower(c);


std::tolower is locale aware and at least cpp ref says "To use these functions safely with plain chars (or signed chars), the argument should first be converted to unsigned char".

andir · 2020-12-01T15:48:38Z

src/libmain/progress-bar.cc

+                    }
+                    c = std::tolower(c);
+
+                    if (c == 3 || c == 4 || c == 'q') {


Nit: for readability we could use some constant for ETX/EOT or add a comment here?

andir · 2020-12-01T15:56:34Z

src/libmain/progress-bar.cc

+
+            inputPipe.create();
+
+            inputThread = std::thread([this]() {


This whole function has become quite a beast. Would it make sense to move it into a function instead?

andir · 2020-12-01T16:00:53Z

src/libmain/progress-bar.cc

+                        }
+                    }
+
+                    if (c == 'l') {


These c == 'x' switches should probably have an explicit else if for all the other cases. That makes it explicit that only one of them will be active at a time. Making reading this code easier and more explicit. Another option would be a switch on c.

andir · 2020-12-01T16:07:58Z

src/libmain/progress-bar.cc

-    bool isTTY;
+    std::optional<struct termios> savedTermAttrs;
+
+    Pipe inputPipe;


inputPipe is only used to cancel the inputThread. Is that right? It didn't occur to me in the first passage of reading the thread that was the intend of it. It is a bit unfortunate to have the whole indirection through the pollfd.

andir · 2020-12-01T16:13:56Z

src/libmain/progress-bar.cc

@@ -184,8 +438,8 @@ class ProgressBar : public Logger
            auto sub = getS(fields, 1);
            i->s = fmt(
                hasPrefix(sub, "local")
-                ? "copying " ANSI_BOLD "%s" ANSI_NORMAL " from %s"
-                : "fetching " ANSI_BOLD "%s" ANSI_NORMAL " from %s",
+                ? ANSI_BOLD "%s" ANSI_NORMAL " from %s"


These two cases appear to be identical.

andir · 2020-12-01T16:25:48Z

src/libmain/progress-bar.cc

+        std::string s;
+
+        for (size_t i = 1; i < state.prevStatusLines; ++i)
+            s += "\r\e[K\e[A";


What are these escape sequences? Maybe drop a comment what they are doing and why they are here.

I was curious so I looked it up. Posting the answer in case anybody else wonders.

Anything of the form \e[A is an ansi escape, that is interpreted by the terminal. It's the same kind that is used to output color. '\r' is a carriage return, which puts the cursor back at the beginning of the line.

All put together we have:

\r return the cursor at the beginning of the line

\e[K erases the line from the cursor

\e[A moves the cursor one line up

So, for each line we had in the previous status line, we erase a full line and move the cursor up. Ie, we delete all we printed previously.

AmineChikhaoui · 2020-12-08T13:09:42Z

Looks interesting. Btw I tried this a bit and seems nix repl is broken with this branch.

nixos-discourse · 2020-12-17T15:11:00Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-5/10560/1

ryantm · 2020-12-30T03:57:04Z

Looks cool!

One concern I've had with the new nix shell commands is how they work with a dumb terminal like an Emacs inferior shell. They just look like they are hanging without any output until they finish.

It would be nice if we detected dumb terminals and reverted to basic output.

bqv · 2020-12-30T08:31:02Z

Fyi dumb terms are detected, but only by TERM it seems?

E.g. Try running your nix commands with env TERM=dumb, which is what I've been doing to workaround the emacs issue

During a build you can hit 'L' to enable/disable printing of build logs, 'v' or '+' to increase verbosity, and '-' to decrease verbosity.

Using '-L' caused another call to setLogFormat(), which caused another ProgressBar to be created. But the ProgressBar should be a singleton. To do: remove LogFormat::barWithLogs. '-L' should be a setting of the ProgressBar, not a different log format.

This will make it easier to add more settings to the progress bar.

nixos-discourse · 2023-08-01T05:35:24Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-rebuild-progress-bar/31149/4

Ericson2314 · 2023-08-18T12:20:28Z

Triaged in the Nix team meeting

@Ericson2314 enqueued this old stalled PR to the board because it seemed to come up in a number of other PRs, some of which were closed in lieu of this one.
alternative design we should consider: https://github.com/maralorn/nix-output-monitor

Will discuss further

nixos-discourse · 2023-08-23T07:20:14Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-08-18-nix-team-meeting-minutes-80/32081/1

fricklerhandwerk · 2023-11-13T14:02:09Z

Discussed in Nix team meeting:

idea approved as an additional progress bar
- it doesn't need to be guarded by experimental feature since there is no stability guarantee

Complete discussion

@thufschmitt: one option is to port nix-output-monitor into Nix, a lot of people use and like it
- would avoid much design discussion
- may have to rewrite it though, since it's in Haskell
  - @fricklerhandwerk: what's wrong with adding it as it is?
    - @edolstra @thufschmitt: it will bloat the closure a lot and make contributions harder because people will have to know another language
  - @Ericson2314: maintaining two languages in a codebase will make it a lot more expensive
@edolstra: something like n-o-m cannot possibly be accepted upstream because it will be bikeshedded to death
- @thufschmitt: the idea would be to accept it wholesale and possibly bikshed later
@fricklerhandwerk: alternative: provide a human-readable line-based interface and put effort into a structured line-based output, let people figure out a nice curses-like UI themselves (as with n-o-m)
- @thufschmitt: would make a crappy experience out of the box
  - @fricklerhandwerk: this is very subjective. it works okay for the stable CLI
  - the presentation has quite a high leverage on new users, probably not a good place to cut scope that way
@infinisil: CLI output doesn't have to be stable, so we can take any improvement
- @thufschmitt: not exactly any, we don't want to break the user interface, but generally yes
  - for the current progress bar, don't know what most of the numbers mean; breaking that shouldn't be much of a problem
@edolstra: could merge it as a new, separate progress bar to get feedback over time
- no reason why we couldn't have multiple
- @thufschmitt: generally feasible, but an ambient concern is that we're really good at adding more experimental features already
- @infinisil: if you ever wanted to remove the old progress bar, this is an behavior change users may not expect
- @edolstra: we may in fact want both: a minimal and a fancy one
@tomberek: A good progress bar can be a great explainer. Like documentation.
@thufschmitt: looking at the diff, could the tangential changes be merged independently?
- @edolstra: yes, there were some improvements to activities for the progress bar to work on
idea approved as an additional progress bar
- it doesn't need to be guarded by experimental feature since there is no stability guarantee

nixos-discourse · 2023-11-13T18:50:20Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-11-13-nix-team-meeting-minutes-103/35400/1

imincik · 2023-11-14T07:48:33Z

I very much appreciate any effort to improve nix command user interface, but I watched asciinema demo and think I like the output of nom (Nix output monitor) much more.

Since very long time has passed since this PR was submitted, did you consider taking some experience from this tool ?

infinisil · 2023-11-14T08:06:46Z

@imincik Expand the detailed discussion in the meeting notes, this was discussed. In short, nom being written in Haskell makes this very tricky, but if somebody had the resources to rewrite it in C++ and PR it, that would change the game.

szlend · 2023-12-20T10:48:12Z

Imo the default progress indicator should be simple (max 1-3 lines) and most importantly have a fixed output height so the output doesn't bounce around. This is a major usability issue with nom (example). It's fine to have a more advanced progress indicator, but please put it behind a flag (or toggle).

Taking another shot at nom: it doesn't do the most basic thing a progress indicator should do, namely: show progress.

Alizter · 2023-12-20T11:00:30Z

@szlend I think your concern is a valid one that I believe can be solved with the appropriate configuration, on top of some sane defaults. With UI (especially TUI) you can never satisfy every user. I believe a similar point appears in the meeting notes.

edolstra marked this pull request as draft December 1, 2020 12:41

edolstra force-pushed the progress-bar branch from cef088c to cbab288 Compare December 1, 2020 12:49

andir reviewed Dec 1, 2020

View reviewed changes

edolstra force-pushed the progress-bar branch from cbab288 to 9c02bdc Compare December 17, 2020 14:26

This was referenced Dec 22, 2020

Add a settings option for log-format #3961

Closed

progress-bar: multiple output lines #3972

Open

edolstra added 16 commits January 5, 2021 12:00

Interactive progress bar

2a2df85

During a build you can hit 'L' to enable/disable printing of build logs, 'v' or '+' to increase verbosity, and '-' to decrease verbosity.

Support multi-line status

304715d

Add separate progress bars for substituting and building

82bbb3a

Doh

e314119

Cleanup

55d3bdd

Style change

8f92b7f

Add activity for evaluation

dc0bac9

Show failure / evaluation

83f47e7

Put builds/substitutes under the right progress bar

256d642

Show duration of running builds

208425b

Show downloads

f90b120

Fix crash, tweaks

4b711bf

Disable the progress bar if stdout is redirected

29ada51

Replace LogFormat::barWithLogs with a setting

4979bd4

This will make it easier to add more settings to the progress bar.

Move method

2392688

edolstra modified the milestones: nix-2.15, nix-2.16, nix-2.17 May 26, 2023

roberth added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Jun 2, 2023

edolstra modified the milestones: nix-2.17, nix-2.18 Jul 24, 2023

edolstra modified the milestones: nix-2.18, nix-2.19 Sep 20, 2023

fricklerhandwerk added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Nov 13, 2023

edolstra modified the milestones: nix-2.19, nix-2.20 Nov 20, 2023

maralorn mentioned this pull request Dec 20, 2023

Can we have a fixed output height mode? maralorn/nix-output-monitor#126

Open

edolstra mentioned this pull request Feb 23, 2024

feat: show activity progress as a pipe #9906

Closed

edolstra modified the milestones: nix-2.20, nix-2.21 Mar 4, 2024

edolstra modified the milestones: nix-2.21, nix-2.22 Mar 11, 2024

edolstra modified the milestones: nix-2.22, nix-2.23 Apr 23, 2024

edolstra removed their assignment Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Progress indicator improvements #4296

Progress indicator improvements #4296

edolstra commented Dec 1, 2020 •

edited

SuperSandro2000 commented Dec 1, 2020

edolstra commented Dec 1, 2020

andir Dec 1, 2020

mweinelt commented Dec 1, 2020

andir left a comment

andir Dec 1, 2020

andir Dec 1, 2020

andir Dec 1, 2020

andir Dec 1, 2020

andir Dec 1, 2020

andir Dec 1, 2020

andir Dec 1, 2020

madjar Jan 19, 2021

AmineChikhaoui commented Dec 8, 2020

nixos-discourse commented Dec 17, 2020

ryantm commented Dec 30, 2020

bqv commented Dec 30, 2020

nixos-discourse commented Aug 1, 2023

Ericson2314 commented Aug 18, 2023

nixos-discourse commented Aug 23, 2023

fricklerhandwerk commented Nov 13, 2023

nixos-discourse commented Nov 13, 2023

imincik commented Nov 14, 2023

infinisil commented Nov 14, 2023

szlend commented Dec 20, 2023 •

edited

Alizter commented Dec 20, 2023

Progress indicator improvements #4296

Are you sure you want to change the base?

Progress indicator improvements #4296

Conversation

edolstra commented Dec 1, 2020 • edited

SuperSandro2000 commented Dec 1, 2020

edolstra commented Dec 1, 2020

Choose a reason for hiding this comment

mweinelt commented Dec 1, 2020

andir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmineChikhaoui commented Dec 8, 2020

nixos-discourse commented Dec 17, 2020

ryantm commented Dec 30, 2020

bqv commented Dec 30, 2020

nixos-discourse commented Aug 1, 2023

Ericson2314 commented Aug 18, 2023

nixos-discourse commented Aug 23, 2023

fricklerhandwerk commented Nov 13, 2023

nixos-discourse commented Nov 13, 2023

imincik commented Nov 14, 2023

infinisil commented Nov 14, 2023

szlend commented Dec 20, 2023 • edited

Alizter commented Dec 20, 2023

edolstra commented Dec 1, 2020 •

edited

szlend commented Dec 20, 2023 •

edited