Closed fitzgen closed 5 years ago
cc @ashleygwilliams @alexcrichton @steveklabnik
There was some discussion of this at todays WG meeting. Consensus was that where possible, we should push this into upstream crates, but also generally do wahtever it takes to get the behavior described here at the end of the day.
I have a few thoughts on this; the topic of "nice CLI output" is one of the things the CLI WG is concerned with.
I have also not written any of this down publicly in a coherent way -- up until now. Well, actually, I don't know if this is going to be coherent. You be the judge of that. (Here are some earlier notes.)
Please bear with me while I describe some very general stuff first. I promise I get to concrete suggestions eventually!
> dafuq.log
In fact, there are a lot of edge cases one has to consider when doing even just slightly more than printing single-line ASCII strings to stdout.
…are just what comes to my mind right now. And people usually don't want to think about any of that.
println!
and macros like warn!
(from some logging crate) in the same codeI can see the following:
From a "classical" software engineering perspective I'd thus suggest the following:
Quick aside: The most trivial but valuable presenter is probably #[derive(Serialize)]
, used with --message-format=json
and a JSON-based logger. (We'll look at human-focussed presenters in the following.)
Assuming the data model is trivial (a string) or something you already have, the question quickly becomes: How do you write these presenters.
This is what @fitzgen's Details sections is getting at. He's suggesting to "create a new terminal
module that encapsulates all terminal output", containing "utility function(s) that only apply the style(...)
when the terminal supports it" as well as an abstraction around progress bars.
This is a good step in the right direction. I'd try and make this be based on some common traits instead of function that handle stuff. Let's introduce a TerminalOutput
trait (yes please bikeshed that name):
trait TerminalOutput {
type Handle = ();
fn output(&self, f: &mut TerminalFormatter) -> Result<Self::Handle, TerminalFormatterError>
}
In addition to the data in self
it also gets access to a formatter state, similarly to how std::fmt
's Display
and Debug
work. This formatter state can help abstract over and give helper methods for stuff like color support and interactivity.
The return type is a result that optionally gives you a handle to the instance of the presenter -- this is how I'd support progress bars and other interactive elements. Using the handle you can for example update the element on screen (or render a new line of output if that's what the formatter does). I'm not entirely sure how to best represent this, and would welcome suggestions here.
So far this hinges on "each data format has a different presenter trait", but "human output" is one such trait that then uses conditionals in its implementation to differentiate between environments and do something akin to progressive enhancement. Alternatively, we could split this into two: RawTextOutput and HumanTerminalOutput. I think this will yield a worse developer experience and introduce a maintenance burden because often you want to keep these two outputs in sync.
One more aspect of this I'd like share is the idea to nest rendering components: Instead of having an if f.supports_colors_and_stuff() { f.write(red("error: ")); } else { f.write("error: "); } f.write(&self.message);
over and over again, I'd like to describe the output I'd like to have as a chain of function calls: render![red("error: "), text(self.message)]
. (To make this work each of the functions in the macro call could be higher-order presenters that return a presentable object, but in the end that's an implementation detail. You could also make this macro look like XML/JSX if you wanted to make nested stuff like more readable than render![box(red("error: "), render![columns(self.errors)])]
.)
This would drastically reduce the need to think about what the current output supports and allows for progressive enhancement to be dealt with in a few code components. This is similar to what @fitzgen's proposed "terminal module" does.
Logging with the usual macros will need to be wrapped in a way so we can dynamically dispatch calls to the appropriate presenters. Same as above, JSON will be quite trivial (and I believe slog-json already supports Serializable types), but terminal output will need to use nested rendering or another form of composition, as well as have a good default adapter for external crates that use the regular "just write a string" way of logging.
TerminalFormatter
thingSo far we've seen an abstract design of a trait and some ideas how to work with it. What is also very important is the concrete design of the TerminalFormatter
type that I've used in the trait definition above. It is probably the one implementation detail that makes or breaks this whole idea.
What do we need to get from TerminalFormatter?
write
method will probably be generic. Maybe it can even be somewhat recursively generic by being fn write<T: TerminalOutput>(&mut self, x: T) -> Result<Self::Handle, TerminalFormatterError>
which is actually the same as TerminalFormatter::output
? This would enable nested rendering, but could lead to other problems down the road.x
seconds or so)I'd really like to try out the above design, and would appreciate your feedback. I know this is just a rough draft so far, and I'm sure we'll discover some issues along the way. Maybe this a good time to refactor wasm-pack in way that could be first prototype of this, and later make it a crate of its own? Let me know what you think.
Started some implementation work at https://github.com/killercup/output-rs. Very much WIP and subject to rapid change.
I wanted to try to kick the tires on this again recently, and it looks like changes to fix smaller bugs are starting to be blocked on this sort of larger scale revamp of the output. I wanted to jot down some notes that I've personally noticed, as well as an idea of how to make progress.
Some sample output I just saw from wasm-pack build
looked like:
[1/9] Checking `rustc` version...
[2/9] Checking crate configuration...
[3/9] Adding WASM target...
info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
[4/9] Compiling to WASM...
Finished release [optimized] target(s) in 0.02s
[5/9] Creating a pkg directory...
[6/9] Writing a package.json...
[INFO]: Optional fields missing from Cargo.toml: 'description', 'repository', and 'license'. These are not necessary, but recommended
[7/9] Copying over your README...
wasm-bindgen 0.2.27 (6dfbb4be8)
[8/9] wasm-bindgen already installed...
[9/9] Running WASM-bindgen...
:-) Done in 0 seconds
| :-) Your wasm pkg is ready to publish at "/home/alex/code/wasm-pack-template/pkg".
Some notes I'd have on this output are:
Compiler error messages and Cargo messages aren't colorized
Lots of output quickly becomes boilerplate
rustc
versoin..." gets printed on all invocations of
wasm-pack build
. Most of this informational output is relevant for maybe
the first run to see what's going on, but it quickly just becomes a lot of
boilerplate that keeps scrolling past.One-off messages from subprocesses aren't suppressed.
[3/9] Adding WASM target...
info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
[4/9] Compiling to WASM...
or
[7/9] Copying over your README...
wasm-bindgen 0.2.27 (6dfbb4be8)
[8/9] wasm-bindgen already installed...
these stray messages between the steps are "boilerplate informational" sometimes (like the rustup command) or sort of confusing and random (like the wasm-bindgen version getting printed)
Warnings are unconditionally printed and can't be suppressed
description
and such are printed, and there's not way to squelch the warning. This means
that for some projects the warnings are always and unconditionally printed
to the console and you've just got to get used to them being printed (which
reduces the usefulness of the warning).Two "Done in XX seconds" messages are printed
wasm-pack
, and it's sort of
confusing to see that because it looks like wasm-pack
is finishing twiceIndicatif output always seems somewhat buggy for me
| :-) Your wasm pkg is ready to publish at "/home/alex/code/wasm-pack-template/pkg".
I'm not sure why the spinner of |
always shows up at the beginning of the
line.
wasm-pack
produces practically zero output when output is redirected.
[INFO]
message for a missing description
and such, zero other progress
indicators.My personal opinion on what should be done to solve these issues is:
Don't assume that a number of steps is known, don't print every step.
Only print information if it's a warning, error, or informational about a step that's going to take a long time.
rustc
version ..."wasm-pack
could detect it's not installed and
only print that if rustup will actually download something.Never print warnings that unconditionally show up and can't be suppressed.
Ideally the warning about description
and such missing could be on a
publication step rather than the test/build steps.
Don't use indicatif for the entire build. This requires that all output flows through indicatif which is too painful of a restriction to implement a reasonable solution to many of the above points. While a spinner could be available for some steps, it's not necessary to have in general:
rustup
already have a progress indicatorOutput of commands should not make their way to log files. Like with indcatif for the entire build, this is just too difficult a restriction to work to implement solutions to many of the above pain points. I think the rationale for this is to have good crash logs, which while admirable I think we should figure out how to get in a different fashion.
Overall what I'd like to see is something like the following for a first run:
$ wasm-pack build
[*] Installing the Rust wasm target ...
# ... output of `rustup target add ...`
# raw output of `cargo build`, hooked up directly to the terminal
[*] Downloading `wasm-bindgen vX.Y.Z` ...
[*] Downloading `wasm-opt vX.Y.Z` ...
:-) Your wasm pkg is ready to publish at "..."
wasm-pack's primary output here is indicating that tools are being downloaded when necessary and an informational message when it's done. An incremental build would look like:
$ wasm-pack build
# raw output of `cargo build`, hooked up directly to the terminal
:-) Your wasm pkg is ready to publish at "..."
No extra informational messages are printed to the terminal, and it's largely
Cargo that's printing information here. wasm-pack
does, however, indicate
where the output is located after wasm-bindgen
has finished executing.
Some known problems with this strategy I can think of are:
Cargo prints "Finished ..." which isn't actually correct, as wasm-pack
has
more work to do. Similarly its timer doesn't include things like the rustup
download nor following tool downloads.
If wasm-bindgen
takes a long time then wasm-pack doesn't print much.
I think these issues are far more minor than the ones above, though, and are easy to fix in isolation.
I'm curious what others think about this though! If there's general consensus I'm fine signing myself up for implementing all this.
This is a great write up, thanks @alexcrichton!
I would like to add that I think we should also be doing a better job of testing output than we are right now. We do a pretty good job of testing functionality, but have basically zero tests for output that we emit.
We do have https://github.com/rustwasm/wasm-pack/issues/18 on file to start doing this sort of thing, but it looks like that crate has been deprecated since that issue was filed and assert_cmd
is the new hotness.
True! I would be more than willing to set up a test harness as well for the output. Fixing all of these associated issues will also be required to write tests, because if you try to write a test today the output will always be empty!
Just for reference for this issue, I am posting the wasm-pack output on Windows Console (containing many notdef glyphs). The latest Windows Console (and hence Powershell) does not display emoji glyphs - although support is coming soon.
I believe this has all since been implemented, so closing!
What version was this fixed in? I just downloaded 0.10.3 and still see no way to remove colour.
I did notice that piping it triggers no colour (is this the only way to control it). I note that it outputs to stderr, but if we connect stdout to a pipe (colour is turned off). If we connect stderr to the pipe colour is left on).
This is a strange, and as far as I can tell undocumented behaviour.
Summary
wasm-pack
should default to plain text output and use progressive enhancement to add bells and whistles such as colors, bold, and progress bars when the functionality is available.Motivation
wasm-pack
should work seamlessly with all terminals on all platforms, and when its output is piped to another process or a log file. Users should never see garbled characters or color codes.Details
By default,
wasm-pack
should emit only plain text output tostdout
.We should use the
atty
crate and the$TERM
environment variable to determine whetherstdout
is a fully featured terminal. Ifatty::is(atty::Stream::Stdout)
is true and the$TERM
environment variable is not set to"dumb"
, thenwasm-pack
should also emit colors, bold styles, and show progress bars when it makes sense.To enforce consistency with these rules across the code base, there should be one central place that manages whether colors are printed or progress bars are displayed. All other code should be able to assume that the colors are used and progress bars are displayed, and be none the wiser if that is not actually the case under the covers. Luckily, we already have most of the printing logic isolated into the
progressbar
module. The exception is someformat!
ed string messages used to construct someError
instances.To move forward, we should create a new
terminal
module that encapsulates all terminal output, progressive enhancement, and feature detection.For styling text, we should refactor all existing usage of
style(...).bold().dim()
etc to use our ownterminal
utility function(s) that only apply thestyle(...)
when the terminal supports it.For progress bars, we should have a utility in
terminal
that creates the progress bars for the caller. Ifstdout
is not a tty or$TERM
is"dumb"
, then it should return a progress bar created withindicatif::ProgressBar::hidden
, which is a no-op progress bar.Related Issues
References