RFC: UI fixes for --compact mode #693
Conversation
Signed-off-by: Austin Seipp <as@fastly.com>
Signed-off-by: Austin Seipp <as@fastly.com>
On the final tick, instead of simply moving the cursor back up several lines, make it emit a combination of up/clear escape codes for each line. This effectively resets the terminal cursor position completely. Signed-off-by: Austin Seipp <as@fastly.com>
Signed-off-by: Austin Seipp <as@fastly.com>
|
@ndmitchell ping, i've been using these patches for several weeks now and they work reasonably well. The only outstanding question is the |
|
@thoughtpolice new job and various holidays mean I'm way behind in my open source maintenance. Hopefully I'll clear the backlog in this coming week! |
There was a problem hiding this comment.
Looks good. The spurious HTTP dependency should go, and I suspect depending on terminal-size is the only sensible way to do terminal sizes (I use it in ghcid, it's been robust, and doing this in a Windows+Linux way is a pain).
As for the actual UI changes, there's no real way to review the code, only see the output, and if you say it's better this way I'll trust you 😄
Apologies for the disastrously long review time!
| @@ -111,7 +111,7 @@ library | |||
|
|
|||
| if flag(cloud) | |||
| cpp-options: -DNETWORK | |||
| build-depends: network, network-uri | |||
There was a problem hiding this comment.
Why is this part in there? Unrelated change that sneaked in?
| @@ -0,0 +1,48 @@ | |||
|
|
|||
| -- | Get terminal size with @ioctl@ | |||
| module Development.Shake.Internal.TermSize ( | |||
There was a problem hiding this comment.
Better to use https://hackage.haskell.org/package/terminal-size?
| maybe "" (", Failure! " ++) (isFailure p) | ||
|
|
||
| data RawProgress | ||
| = Starting |
There was a problem hiding this comment.
Do you need a special Starting message? Why not just Executing with no progress and no other information. Is it for compatibility with progressDisplay? (That seems reasonable)
| data RawProgress | ||
| = Starting | ||
| | Finished Double | ||
| | Executing Progress Double Double Double Int Int String |
There was a problem hiding this comment.
Running or Executing? I tend to think of executing as more "calling exec". I'm happy with whatever you prefer though.
|
I have a (400GB) .gif I shared with my colleagues of our own build system executing with these patches, but I can't reveal it. I'll see if I can use asciinema to record a version of something FOSS that is using CMake/Ninja... |
|
When I say "I'll trust you" I really mean it - no need to bother recording something! |
|
I added some inline comments, are you able to shed light on them? Particularly the addition of |
This introduces a number of fixes for the
--compactUX, making it much nicer on the eyes and more usable, IMO.Notably, it:
Ensures all traces outputted to the terminal are truncated to the terminal width. This avoids edge cases where a line is too long and wraps onto the next line in your terminal, which causes the remainder of the UX to become offset badly. (In our build system we have one traced command that prints a 200+ character-long string, which screws up the build formatting). Note: ninja also does this for the same reasons, and we use the same truncation logic (look up
ElideMiddlein util.cc. NOTE: in order to do this, we must introduce anTermSize.hscmodule to callioctl(STDOUT_FILENO, TIOCGWINSZ, ...)on Unix. A normal FFI import will not work, because we must be aware of the structure that theioctlfills; it does not simply return integer values. The.hscfile meanshsc2hsmust be available now.Clears and resets the cursor position for the line back to the original position it was in when you invoked
shake, an also clears all lines it printed on. This effectively restores the terminal back to the state it was when you ranshake. It also does not output anything on no-op builds, either.No more empty job lines: the UX expands dynamically based on whether or not a running thread is executing a job. By simply moving the line type to
Maybe Stringand usingmapMaybes, we get the benefit that empty lines with no jobs scheduled (e.g. because you are in a critical path, or because the build is about to end) are not printed. As noted, this is dynamically calculated, so this also has the nice side effect that e.g. if you have 4 threads running and the 3rd one in the UI list is finished, the 4th entry will "collapse" into the 3rd one on the next refresh, never showing an empty line.It colorizes the output a little differently now, including bolding commands that are executing for a rule (e.g. in the compact output
* foo/bar/baz.o (cc 25s),ccwill now be bolded and colorized red/yellow appropriately as well.)It refactors the
progressDisplayfunction into an internal helper which is now also used bycompactUI. This is necessary so that we can change the output in the compact UX more liberally in the future. Note:progressRawmay actually be useful to the user as an exposed API (after some cleanups/renaming), but for now it is hidden. Note that I useformatMessageetc which is also internal, hence why I left some of the code paths the same.Our cmake/ninja build at
$WORKeffectively ran into all of these cases when using theshakeexecutable in compact mode: ultra long column rule names, long paths that take 20+ seconds to build (invoking red/yellow status lines for the rules), and a job pool that shrinks entries as the build winds down and almost finishes. The extra stuff was just to make it all look a bit nicer.Please do not merge this just yet. There are still some tweaks I need to make (in particular the
ioctlstuff should be attributed to the place I got it from), and possibly tweaking the coloring/output formatting to be nicer. And there are some questions about how things should be structured. However, I have private build that I am using with these changes, for$WORK, and I have been successfully using them for the past day. I think the results are effectively much nicer.In particular, the introduction of an
.hscfile may mean that the Shake library can no longer trivially be loaded with just aghcicommand, which could be seen as a major downside. I did not experience this, since I just usecabal new-repl, but it is worth nothing since I think you use this workflow a lot, Neil. I can see two alternatives:Move this code into a separate package, so
shakeitself needs no.hscfiles directly. This would also mean we could possibly, eventually add support for Windows too, independently of Shake.Move this code into a
.cfile. This is easier but still suffers from the sameghcidrawback, since you'll have toforeign importit and the object file may not be compiled.However, at least 5d8d0e5 seems uncontroversial and an obvious fix. This fixes using
cabal new-replto load shake. You may just want to cherry pick that one change immediately right now. I can rebase the actual majority of the changes ontomasteragain later.