Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mini RFC: wasm-pack should use progressive enhancement in its output #298

Closed
fitzgen opened this issue Sep 11, 2018 · 10 comments
Closed

Mini RFC: wasm-pack should use progressive enhancement in its output #298

fitzgen opened this issue Sep 11, 2018 · 10 comments
Labels
bug Something isn't working enhancement New feature or request logging question Further information is requested refactor

Comments

@fitzgen
Copy link
Member

fitzgen commented Sep 11, 2018

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 to stdout.

We should use the atty crate and the $TERM environment variable to determine whether stdout is a fully featured terminal. If atty::is(atty::Stream::Stdout) is true and the $TERM environment variable is not set to "dumb", then wasm-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 some format!ed string messages used to construct some Error instances.

To move forward, we should create a new terminal module that encapsulates all terminal output, progressive enhancement, and feature detection.

  1. For styling text, we should refactor all existing usage of style(...).bold().dim() etc to use our own terminal utility function(s) that only apply the style(...) when the terminal supports it.

  2. For progress bars, we should have a utility in terminal that creates the progress bars for the caller. If stdout is not a tty or $TERM is "dumb", then it should return a progress bar created with indicatif::ProgressBar::hidden, which is a no-op progress bar.

Related Issues

References

@fitzgen fitzgen added bug Something isn't working enhancement New feature or request question Further information is requested refactor logging labels Sep 11, 2018
@fitzgen
Copy link
Member Author

fitzgen commented Sep 11, 2018

@fitzgen
Copy link
Member Author

fitzgen commented Sep 13, 2018

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.

@killercup
Copy link
Contributor

killercup commented Sep 22, 2018

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.)

What's the status quo?

Please bear with me while I describe some very general stuff first. I promise I get to concrete suggestions eventually!

  • People (i.e., "us developers") want to have emojis, progress bars, and probably also column/table layouts!

  • People tend to target "human in front of xterm256", but forget about

    • > dafuq.log
    • how Travis (and other CI systems) show it
    • weird tmux via ssh configs
    • having any kind of output that other tools can consume without (using first training a neural network as a parser)
  • 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.

    • Escape codes and resetting them,
    • detecting terminal sizes (for doing anything with layout correctly),
    • and buffered output and locking std{out,err} (and how to properly do that on Windows)

    …are just what comes to my mind right now. And people usually don't want to think about any of that.

  • Oh, and people use println! and macros like warn! (from some logging crate) in the same code

What to do about it

I can see the following:

  1. We have some data
  2. We want to show it in different ways

From a "classical" software engineering perspective I'd thus suggest the following:

  1. Model the data (i.e., writing a bunch of struct)
  2. Write presenters that can output the data in different contexts (i.e. in Rust, write a bunch of impls for different traits)

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.)

Writing a terminal output presenter

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.

Nested rendering

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

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.

Actually designing that TerminalFormatter thing

So 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?

  • A way to write to it
    • Actually, we need to push entities that can be rendered somehow to it. So, a 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.
  • A way to detect certain terminal features
    • color support, cf. https://github.com/rust-lang-nursery/cli-wg/issues/15
    • interactivity: clearing and replacing lines but also rate of updates to e.g. decide on the granularity of a progress bar (instead of writing 1000 lines of increasing percentages to a log file we should probably only add 1 line every x seconds or so)
    • …and also cache the detection of these features
  • A way to flush the internal buffer immediately to enable interactive printing, e.g. printing a dot (without a newline) for each test in a test suite
  • Optionally set/cache/reset global terminal settings
    • This might be required to support coloring on older Windows versions

Conclusion

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.

@killercup
Copy link
Contributor

Started some implementation work at https://github.com/killercup/output-rs. Very much WIP and subject to rapid change.

@alexcrichton
Copy link
Contributor

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

    • This is due to two parts, one is that wasm-pack currently logs all output of
      subcommands to log files. Another is that the usage of indcatif requires
      redirecting all output.
  • Lots of output quickly becomes boilerplate

    • for example "Checking 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.

    • After running wasm-pack build I see messages like:

      [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

    • Even for one-off projects informational warnings about missing 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

    • one comes from Cargo and one comes from wasm-pack, and it's sort of
      confusing to see that because it looks like wasm-pack is finishing twice
  • Indicatif output always seems somewhat buggy for me

    • For example my builds almost always end with:

      | :-) 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.

    • If you direct wasm-pack's output to a log file, all I see is an [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:

  1. Don't assume that a number of steps is known, don't print every step.

  2. Only print information if it's a warning, error, or informational about a
    step that's going to take a long time.

    • For example there's no need to print "Checking rustc version ..."
    • There's also no need to print "Adding WASM target..." unless it's not
      actually added already. wasm-pack could detect it's not installed and
      only print that if rustup will actually download something.
    • Messages like "Compiling to WASM..." don't need to be printed because Cargo
      will take care of all informational messages
  3. 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.

  4. 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:

    • Long-running downloads via rustup already have a progress indicator
    • Cargo now has a progress indicator
    • Downloads of tools can use indicatif on a one-off basis to provide a
      progress indicator for their downloads.
    • Almost nothing else is long enough running to need a progress indicator
  5. Output 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.

@fitzgen
Copy link
Member Author

fitzgen commented Nov 5, 2018

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 #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.

@alexcrichton
Copy link
Contributor

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!

@danwilhelm
Copy link
Member

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.

image

@alexcrichton
Copy link
Contributor

I believe this has all since been implemented, so closing!

@richard-delorenzi
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request logging question Further information is requested refactor
Projects
None yet
Development

No branches or pull requests

6 participants