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

Fully capture rustc and rustdoc output when -Zcompile-progress is passed #5862

Merged
merged 3 commits into from
Aug 6, 2018

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Aug 3, 2018

Fixes #5764 and #5695.

On Windows, we will parse the ANSI escape code into console commands via my fwdansi package, based on @ishitatsuyuki's idea in #5695 (comment). Outside of Windows the content is forwarded as-is.

JobQueue::drain_the_queue() could thus stop checking for `extra_verbose()`.
This prepares us for capturing output from these compilers.
@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@dwijnand
Copy link
Member

dwijnand commented Aug 3, 2018

Fixes #5764 and #5695.

Needs to be...

Fixes #5764
Fixes #5695

.. for GitHub to auto-close them both.

@ishitatsuyuki
Copy link
Contributor

Great work.

I'm not sure we should special case Windows here - at a little sacrifice of efficiency, we can just:

  • pass --color=always to downstream processes.
  • parse it with fwdansi and then let it handle all output cases with termcolor: Windows console, ANSI, no color.

@kennytm
Copy link
Member Author

kennytm commented Aug 4, 2018

@dwijnand they are already closed so I choose the human syntax instead 😄.

@kennytm kennytm changed the title [DO NOT MERGE YET] Fully capture rustc and rustdoc output when -Zcompile-progress is passed Fully capture rustc and rustdoc output when -Zcompile-progress is passed Aug 5, 2018
@kennytm
Copy link
Member Author

kennytm commented Aug 5, 2018

(rust-lang/rust#53003 has been merged so this is unblocked.)

@ishitatsuyuki
Copy link
Contributor

@kennytm Do you have some opinions on #5862 (comment)?

@kennytm
Copy link
Member Author

kennytm commented Aug 5, 2018

@ishitatsuyuki I don't see any advantage doing that. I'll let the reviewers decide.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I agree with @kennytm that not always passing --color always feels like the best strategy here.

@kennytm to confirm, when --color always is passed to rustc on windows when stderr is a pipe, that means that termcolor, the compiler's currently terminal coloring library (I think?), will turn on ANSI unconditionally, despite being on Windows?

I want to test this a bit locally to make sure it still works, it'll take me a bit though to run it on a Windows machine.

let shell = bcx.config.shell();
if capture_output || shell.color_choice() != ColorChoice::CargoAuto {
let color = if shell.supports_color() { "always" } else { "never" };
cmd.args(&["--color", color]);
Copy link
Member

Choose a reason for hiding this comment

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

This is slightly different than the previous invocation that was moved up from below, but I think it's equivalent, right?

Previously if I pass --color never to Cargo it'll forward that to rustc unconditionally. I think that'll happen here because stream.supports_color() is already set to return false unconditionally inside of termcolor, I think?

(and I believe the reverse is true for --color always)

Just wanted to make sure!

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously if I pass --color never to Cargo it'll forward that to rustc unconditionally. I think that'll happen here because stream.supports_color() is already set to return false unconditionally inside of termcolor, I think?

Yes. supports_color() will return false if the stream is "NoColor", and true otherwise.

When a stream is created, it will create the "NoColor" variant if ColorChoice::should_attempt_color() returns false, which is when --color never or --color auto with $TERM ≠ "dumb".

Thus --color never will imply supports_color() returns false unconditionally.

@kennytm
Copy link
Member Author

kennytm commented Aug 6, 2018

when --color always is passed to rustc on windows when stderr is a pipe, that means that termcolor, the compiler's currently terminal coloring library (I think?), will turn on ANSI unconditionally, despite being on Windows?

Yes. The behavior is the same with or without this PR.

screenshot_2018-08-06 21 35 44_gmqq9p-fs8

@alexcrichton
Copy link
Member

@bors: r+

Ok this looks great to me, thanks again @kennytm!

I think after this we're probably ready to turn this on by default, right?

@bors
Copy link
Contributor

bors commented Aug 6, 2018

📌 Commit 641f7ff has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 6, 2018

⌛ Testing commit 641f7ff with merge 2b6e996...

bors added a commit that referenced this pull request Aug 6, 2018
Fully capture rustc and rustdoc output when -Zcompile-progress is passed

Fixes #5764 and #5695.

On Windows, we will parse the ANSI escape code into console commands via my `fwdansi` package, based on @ishitatsuyuki's idea in #5695 (comment). Outside of Windows the content is forwarded as-is.
@kennytm
Copy link
Member Author

kennytm commented Aug 6, 2018

@alexcrichton Yes :)

@bors
Copy link
Contributor

bors commented Aug 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 2b6e996 to master...

@bors bors merged commit 641f7ff into rust-lang:master Aug 6, 2018
@kennytm kennytm deleted the capture-rustc-output branch August 6, 2018 16:04
bors added a commit to rust-lang/rust that referenced this pull request Aug 15, 2018
Update cargo

- Update transitioning url (rust-lang/cargo#5889)
- Resolve some clippy lint warnings (rust-lang/cargo#5884)
- Don't kill child processes on normal exit on Windows (rust-lang/cargo#5887)
- fix a bunch of clippy warnings (rust-lang/cargo#5876)
- Add support for rustc's --error-format short (rust-lang/cargo#5879)
- Support JSON with rustdoc. (rust-lang/cargo#5878)
- Fix rustfmt instructions in CONTRIBUTING.md (rust-lang/cargo#5880)
- Allow `cargo run` in workspaces. (rust-lang/cargo#5877)
- Change target filters in workspaces. (rust-lang/cargo#5873)
- Improve verbose console and log for finding git repo in package check (rust-lang/cargo#5858)
- Meta rename (rust-lang/cargo#5871)
- fetch: skip target tests when cross_compile is disabled (rust-lang/cargo#5870)
- Fully capture rustc and rustdoc output when -Zcompile-progress is passed (rust-lang/cargo#5862)
- Fix test --example docs. (rust-lang/cargo#5867)
- Add a feature to build a vendored OpenSSL (rust-lang/cargo#5865)
@alexcrichton
Copy link
Member

@kennytm I'd say this is probably ready to stabilize, but curious what you think?

@kennytm
Copy link
Member Author

kennytm commented Sep 8, 2018

SGTM, I'll prepare a PR.

@ehuss ehuss added this to the 1.30.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI nit: Progress bar partially overwritten by errors
7 participants