feat(ghciwatch): add new experimental progress bar option to report compilation output on 1 line#380
feat(ghciwatch): add new experimental progress bar option to report compilation output on 1 line#380
Conversation
9999years
left a comment
There was a problem hiding this comment.
Hi! I think that getting this over the line as the default would be very challenging (see below), but I would be willing to merge this as an experimental feature (to help clarify to users that it's unsupported). If you'd like to put the work in, here's what I'd like to do:
- First, write a PR which adds a new
--experimental-featuresflag; we could use the existing--tuiflag as a proof-of-concept here. - Next, modify this PR to require e.g.
--experimental-features progress. - Replace the regex in
ProgressWriterwith the parser inghci/parse/ghc_message/compiling.rs. (Is there a reason you didn't use the parser in the first place? I couldn't figure it out.)
Then I think we could merge this. If you'd like, feel free to put ~30mins on my calendar and we can go over details.
Making this the new default behavior or shipping it as a non-experimental feature would require (I think) 1-2 months of work, and we just don't have the bandwidth for that right now. I know that's frustrating (it's frustrating for us as well) but we've been burned before and as a result we have a very high bar for changes to ghciwatch.
Some details, so that hopefully you can understand where we're coming from:
- We're not set up to test output rendering directly like this. I had a plan for implementing a test framework for this (#202), but I believe implementing it correctly and with good UX would take 2-4 weeks on its own.
- As a result, we have essentially zero tests for this feature. Testing is extremely important for ghciwatch; about a third of the code in the repo is test code, and a feature like this which stream-edits GHC output has a high blast radius and a high bar for testing.
- Because we're modifying the way GHC output is rendering, we need to be very confident that all GHC output renders correctly; this is very difficult, especially because GHCi sessions have user code and GHC itself share the same stdout handle. Some examples of ways this gets tricky:
- It is possible for users to spawn threads which write to stdout interleaved with compilation messages, and we do this in practice to run the backend server in the REPL.
- Similarly, error messages and compilation output can be interleaved. The tests sort of hint at this but don't feature (e.g.) an error message between two compilation messages.
- Another big source of complexity is that users often want to write and flush output without writing a newline (for example our test suite in CI which prints a single
.and no newline for each test case); I believe that this mechanism would break use-cases like this without any indication to the user that output is being hidden.
- Ghciwatch runs on everyone's computers, under dozens of setups. The variance is pretty huge, and when we ship new features we're consistently surprised by issues we weren't able to foresee or catch ahead of time. I expect that shipping this feature would add 1-2 weeks of reactive work (where we receive an unexpected issue report and have to drop our tasks to fix it immediately). Reactive work like this tends to be spread over time, so there's a good chance we will need to fix these bug reports when we're no longer feeling very familiar with the code.
- As a result of this, we're simply afraid of shipping changes like this to ghciwatch — they tend to incur significant cost that's difficult for us to bear. It's very frustrating for us when our users experience mysteriously unreliable software, so with a limited budget for maintenance, we've mostly settled on not shipping changes at all. I know this is frustrating to hear as a contributor as well, and I wish we could push this over the line quickly.
@9999years Thank you for the awesome PR review and for sharing so much context! I started on the first step to add a new experimental-features flag: #385 |
In `[N of M] Compiling ...` messages, parse `N` and `M` out for further inspection. Split off of #380
In `[N of M] Compiling ...` messages, parse `N` and `M` out for further inspection. Split off of #380
9999years
left a comment
There was a problem hiding this comment.
thanks for your work on this! I should be able to review this (early?) tomorrow
9999years
left a comment
There was a problem hiding this comment.
Left some small comments and a few questions, but I think this is almost ready to go!
9999years
left a comment
There was a problem hiding this comment.
lgtm, thanks for this! we may have to force merge because i don't want to write a ci pipeline
| use crate::ghci::parse::compiling; | ||
| use crate::ghci::parse::CompilingProgress; | ||
|
|
||
| /// Equivalent to `s[..s.floor_char_boundary(max_cols)]` (stable in Rust 1.82). |
There was a problem hiding this comment.
Oh, that's nice. We could potentially update the toolchain in this repo, just haven't bothered. We actually have a fairly recent Rust toolchain in mwb.
|
CI moment I'm afraid. |

Proposal implementation for: #21
patch,minor, ormajorto request a version bump when it's merged.docs/.tests/.I've been testing this one locally by pointing the local compilation to MWB via
GHCIWATCH_BIN.