{"action":"created","issue":{"url":"https://api.github.com/repos/ndmitchell/shake/issues/693","repository_url":"https://api.github.com/repos/ndmitchell/shake","labels_url":"https://api.github.com/repos/ndmitchell/shake/issues/693/labels{/name}","comments_url":"https://api.github.com/repos/ndmitchell/shake/issues/693/comments","events_url":"https://api.github.com/repos/ndmitchell/shake/issues/693/events","html_url":"https://github.com/ndmitchell/shake/pull/693","id":469328203,"node_id":"MDExOlB1bGxSZXF1ZXN0Mjk4NTgxMjE2","number":693,"title":"RFC: UI fixes for --compact mode","user":{"login":"thoughtpolice","id":3416,"node_id":"MDQ6VXNlcjM0MTY=","avatar_url":"https://avatars0.githubusercontent.com/u/3416?v=4","gravatar_id":"","url":"https://api.github.com/users/thoughtpolice","html_url":"https://github.com/thoughtpolice","followers_url":"https://api.github.com/users/thoughtpolice/followers","following_url":"https://api.github.com/users/thoughtpolice/following{/other_user}","gists_url":"https://api.github.com/users/thoughtpolice/gists{/gist_id}","starred_url":"https://api.github.com/users/thoughtpolice/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/thoughtpolice/subscriptions","organizations_url":"https://api.github.com/users/thoughtpolice/orgs","repos_url":"https://api.github.com/users/thoughtpolice/repos","events_url":"https://api.github.com/users/thoughtpolice/events{/privacy}","received_events_url":"https://api.github.com/users/thoughtpolice/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2019-07-17T16:41:17Z","updated_at":"2019-09-11T18:02:58Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/ndmitchell/shake/pulls/693","html_url":"https://github.com/ndmitchell/shake/pull/693","diff_url":"https://github.com/ndmitchell/shake/pull/693.diff","patch_url":"https://github.com/ndmitchell/shake/pull/693.patch"},"body":"This introduces a number of fixes for the `--compact` UX, making it much nicer on the eyes and more usable, IMO.\r\n\r\nNotably, it:\r\n\r\n  1) 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 `ElideMiddle` in [util.cc](https://github.com/ninja-build/ninja/blob/1bcc689324bdee090eed035353724abc3fa7c909/src/util.cc#L596). **NOTE**: in order to do this, we must introduce an `TermSize.hsc` module to call `ioctl(STDOUT_FILENO, TIOCGWINSZ, ...)` on Unix. A normal FFI import will not work, because we must be aware of the structure that the `ioctl` fills; it does not simply return integer values. The `.hsc` file means `hsc2hs` must be available now.\r\n\r\n  2) 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 ran `shake`. It also does not output anything on no-op builds, either.\r\n\r\n  3) 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 String` and using `mapMaybes`, 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.\r\n\r\n  4) 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)`, `cc` will now be bolded and colorized red/yellow appropriately as well.)\r\n\r\n  5) It refactors the `progressDisplay` function into an internal helper which is now also used by `compactUI`. This is necessary so that we can change the output in the compact UX more liberally in the future. Note: `progressRaw` may actually be useful to the user as an exposed API (after some cleanups/renaming), but for now it is hidden. Note that I use `formatMessage` etc which is also internal, hence why I left some of the code paths the same.\r\n\r\n---\r\n\r\nOur cmake/ninja build at `$WORK` effectively ran into all of these cases when using the `shake` executable 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.\r\n\r\n---\r\n\r\nPlease do not merge this just yet. There are still some tweaks I need to make (in particular the `ioctl` stuff 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.\r\n\r\nIn particular, the introduction of an `.hsc` file may mean that the Shake library can no longer trivially be loaded with just a `ghci` command, which could be seen as a major downside. I did not experience this, since I just use `cabal new-repl`, but it is worth nothing since I think you use this workflow a lot, Neil. I can see two alternatives:\r\n\r\n  1) Move this code into a separate package, so `shake` itself needs no `.hsc` files directly. This would also mean we could possibly, eventually add support for Windows too, independently of Shake.\r\n\r\n  2) Move this code into a `.c` file. This is easier but still suffers from the same `ghci` drawback, since you'll have to `foreign import` it and the object file may not be compiled.\r\n\r\n---\r\n\r\nHowever, at least 5d8d0e509e489dbcf3a560b82c7be180379d937a seems uncontroversial and an obvious fix. This fixes using `cabal new-repl` to load shake. You may just want to cherry pick that one change immediately right now. I can rebase the actual majority of the changes onto `master` again later."},"comment":{"url":"https://api.github.com/repos/ndmitchell/shake/issues/comments/530496444","html_url":"https://github.com/ndmitchell/shake/pull/693#issuecomment-530496444","issue_url":"https://api.github.com/repos/ndmitchell/shake/issues/693","id":530496444,"node_id":"MDEyOklzc3VlQ29tbWVudDUzMDQ5NjQ0NA==","user":{"login":"thoughtpolice","id":3416,"node_id":"MDQ6VXNlcjM0MTY=","avatar_url":"https://avatars0.githubusercontent.com/u/3416?v=4","gravatar_id":"","url":"https://api.github.com/users/thoughtpolice","html_url":"https://github.com/thoughtpolice","followers_url":"https://api.github.com/users/thoughtpolice/followers","following_url":"https://api.github.com/users/thoughtpolice/following{/other_user}","gists_url":"https://api.github.com/users/thoughtpolice/gists{/gist_id}","starred_url":"https://api.github.com/users/thoughtpolice/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/thoughtpolice/subscriptions","organizations_url":"https://api.github.com/users/thoughtpolice/orgs","repos_url":"https://api.github.com/users/thoughtpolice/repos","events_url":"https://api.github.com/users/thoughtpolice/events{/privacy}","received_events_url":"https://api.github.com/users/thoughtpolice/received_events","type":"User","site_admin":false},"created_at":"2019-09-11T18:02:58Z","updated_at":"2019-09-11T18:02:58Z","author_association":"CONTRIBUTOR","body":"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..."}}
{"actor":{"display_login":"thoughtpolice"}}