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

Migrate output towards the 1.0 strategy #547

Merged
merged 10 commits into from
Mar 15, 2019

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Feb 19, 2019

fixes #520 #419

This commit is the first in what is hopefully a series to help move
wasm-pack's CLI output and interactions to a "1.0 status". This was
discussed at the recent Rust All Hands where the salient
points we ended up extracting were:

  • At all times if a user is waiting for wasm-pack to finish it should
    be clear what's being waited on.
    • As an example, Cargo's own output shows what crate is being built.
    • As another example, something that always takes only a handful of
      milliseconds to complete doesn't need an informational message.
  • The final output products of a command should always be clear and
    printed. For example the output location of artifacts should always be
    printed.
  • The wasm-pack CLI tool should use "progressive enhancement" to
    incrementally detect features of the output it can use (like colors,
    emoji, etc) but always work in the absence of these features. This'll
    help us support a wide range of use cases and terminals.

The goal of this commit is to not get us all the way there but start us
down the path to satisfying these goals. To that end the major change
here is to remove the dependency on indicatif. Using indicatif
requires that all output is piped through the indicatif crate itself,
which causes the third item here to not work for one of the main parts
of wasm-pack build, the cargo pieces. Cargo (and the Rust compiler)
are unable to use thir own tools for progressive enhancement when the
output is captured and sent through indicatif.

Lots more refactoring will be needed internally to fully polish off the
input/output to a "1.0 status", but this is hopefully a good start!

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for getting us started down the road towards the ideal output we agreed upon :)

@ashleygwilliams
Copy link
Member

looks like we have tests failing here, 1 because of a wasm-bindgen test and 1 because cargo fmt isn't able to be downloaded

@alexcrichton
Copy link
Contributor Author

@ashleygwilliams that should be easy enough to fix up, do you have comments on the PR contents?

@alexcrichton alexcrichton force-pushed the output-10 branch 2 times, most recently from 2284f6b to 24399eb Compare February 21, 2019 18:40
@alexcrichton
Copy link
Contributor Author

Still waiting on AppVeyor, but Travis should be green now

Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you supply a gif of the how the new output appears? this is not only for ease of review but also because we'll need to replace the demo.gif in the readme if this makes significant changes to the output (which i believe it does?) let me know if you need help doing it! i can also pull down this branch and add it myself if you prefer (i have asciinema all set up so it would not be too difficult). if we can get this done by end of day i could probably get this into the 0.7.0 release.

@alexcrichton
Copy link
Contributor Author

To be clear, I'd prefer that more changes were landed in succession after this to ensure that the motivation for this change is fully realized. For example this doesn't yet being to take advantage of compiler colors. Each aspect of the current UI will need a separate refactoring and will take some time to move towards this "1.0 status".

I'm hesitant to add too much to one PR though because it ends up not working out well historically. Additionally the pace here is somewhat slow which means it's difficult to iterate quickly and land incremental changes.

All that to say that the current output, which looks like the gif below, is not something I'd want to release. This is one incremental step and I would ideally like to move much more quickly on the future incremental steps.

asciicast

@alexcrichton
Copy link
Contributor Author

In the interest of time I've gone ahead and implemented a number of other changes moving us to where I believe is almost where we want to end up modulo final tweaks. The output now looks like this:

asciicast

Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing i'm noting about the newest demo.gif is that there are no "header sections". by this i mean, that if there is a long running task that dumps to stdout, it should have a header- e.g. cargo build - so i would expect to see a message above "updating crates.io" that says something to the effect of "compiling wasm..."

@alexcrichton
Copy link
Contributor Author

That's correct yes, but that's intentional.

If cargo blocks you know what Cargo is doing, and otherwise wasm-pack doesn't need to print out what it's doing before each step because it's redundant.

@ashleygwilliams ashleygwilliams modified the milestones: 0.8.0, 0.7.0 Mar 15, 2019
@ashleygwilliams ashleygwilliams mentioned this pull request Mar 15, 2019
@ashleygwilliams
Copy link
Member

hey @alexcrichton ! finally got to pull this locally and i think for the most part it's great, i have a few nits. if you can get to them today that's awesome, but we could potentially land this and fix nits for the next release since i think it is really close.

this is the output i got for build:
Screen Shot 2019-03-15 at 2 44 22 PM

i think the difference between the ℹ️ logs that show a step and the ones that show ℹ️ [INFO] are a little weird. perhaps we can have them all say [INFO] ? there's something there that feels a little weird to me, but i'm super open to suggestions. what do you thin?

@ashleygwilliams
Copy link
Member

also i am extremely ok fixing this in a separate PR but the pack command looks like this:
Screen Shot 2019-03-15 at 2 48 58 PM

i would expect a ℹ️ emoj on the success message but it's bare. perhaps we can just have the success messages and step messages not have the ℹ️ ? it might be simplest and i think it'd look less cluttered

@ashleygwilliams
Copy link
Member

happy to merge this and do the emoji tweaks in a separate PR, lemme know!

This commit is the first in what is hopefully a series to help move
`wasm-pack`'s CLI output and interactions to a "1.0 status". This was
[discussed at the recent Rust All Hands][discussion] where the salient
points we ended up extracting were:

* At all times if a user is waiting for `wasm-pack` to finish it should
  be clear what's being waited on.
  * As an example, Cargo's own output shows what crate is being built.
  * As another example, something that always takes only a handful of
    milliseconds to complete doesn't need an informational message.
* The final output products of a command should always be clear and
  printed. For example the output location of artifacts should always be
  printed.
* The `wasm-pack` CLI tool should use "progressive enhancement" to
  incrementally detect features of the output it can use (like colors,
  emoji, etc) but always work in the absence of these features. This'll
  help us support a wide range of use cases and terminals.

The goal of this commit is to not get us all the way there but start us
down the path to satisfying these goals. To that end the major change
here is to remove the dependency on `indicatif`. Using `indicatif`
requires that all output is piped through the `indicatif` crate itself,
which causes the third item here to not work for one of the main parts
of `wasm-pack build`, the `cargo` pieces. Cargo (and the Rust compiler)
are unable to use thir own tools for progressive enhancement when the
output is captured and sent through `indicatif`.

Lots more refactoring will be needed internally to fully polish off the
input/output to a "1.0 status", but this is hopefully a good start!

[discussion]: https://gist.github.com/fitzgen/23a62ebbd67574b9f6f72e5ac8eaeb67#file-road-to-wasm-pack-1-0-md
This commit moves wasm-pack further along the spectrum towards the 1.0
output previously discussed at the last work week. The changes here are:

* Steps which execute near instantaneously no longer print informational
  messages by default.
* Long-running steps like downloading chromedriver/geckodriver now only
  print if they actually perform a download.
* The "add wasm target" step now prints nothing if the wasm target is
  already installed.
* Child process output is no longer captured and is redirected natively
  to the terminal, granting colors from rustc/Cargo as well as Cargo's
  progress bar during a build.
Add a separate function for running a child and capturing stdout
While historically used to count steps in stages the count is no longer
present in the output so this should no longer be necessary.
* All messages are now printed onto stderr instead of some on stdout and
  some on stderr
* All messages uniformly are prefixed with an emoji with consistent
  spacing.
@ashleygwilliams ashleygwilliams changed the title Migrate output towards the 1.0 strategy [WIP] Migrate output towards the 1.0 strategy Mar 15, 2019
@ashleygwilliams
Copy link
Member

per chat, @alexcrichton is working on some updates and will post more soon!

@alexcrichton
Copy link
Contributor Author

@ashleygwilliams ok I believe I've applied your suggestions, and I don't have too many thoughts about the particulars of the output, so I'm down for w/e you feel looks good. For perfect output we may want to take a fresh look at the output again after the next release as it feels a little inconsistent right now (e.g. hasn't caught up with all the features added over time), but that's all future work for later

@ashleygwilliams
Copy link
Member

strong agree! i'll merge when green. thanks for the hard work!!

@ashleygwilliams ashleygwilliams removed the work in progress do not merge! label Mar 15, 2019
@ashleygwilliams ashleygwilliams changed the title [WIP] Migrate output towards the 1.0 strategy Migrate output towards the 1.0 strategy Mar 15, 2019
@ashleygwilliams ashleygwilliams merged commit 912b9b1 into rustwasm:master Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long lines in test output getting cut off
3 participants