Rustdoc test compiler output color #74293

GuillaumeGomez · 2020-07-13T13:37:22Z

We just need to be sure it doesn't break rustdoc doctests' compilation checks. Maybe some other unforeseen consequences too?

r? @ehuss
cc @rust-lang/rustdoc

Mark-Simulacrum · 2020-07-13T15:14:51Z

color=always seems wrong -- we should try to thread the color status of rustdoc into rustc, rather, I suspect. In particular that should fix the tests here which shouldn't contain the ANSI codes (at least because I think that'll be a problem on Windows).

GuillaumeGomez · 2020-07-13T15:23:02Z

I expected the executed rustc to handle that internally through its own termcolor. However, it might indeed be a good idea to not display colors if rustc isnt always or auto. I'll update.

Mark-Simulacrum · 2020-07-13T15:25:49Z

rustc cannot handle that correctly -- you've told it to emit color independently of the normal heuristic. The normal heuristic doesn't work because stdout/stderr are connected to a pipe rather than the tty, so the detection concludes that colored output isn't the right choice.

GuillaumeGomez · 2020-07-13T15:27:57Z

So we definitely have to rely on internal checks. @ehuss said that the only issue would be on older windows, so it has be taken into account. So to sum things up: I need to add a check to see that we're in a mode where colors are expected and that we're not on a too old windows.

ehuss · 2020-07-14T16:14:37Z

I'm concerned this approach doesn't work for a few cases, such as not connected to a TTY, or on older versions of windows.

The only way I can think of to handle this properly is to query the Emitter. There's a lot of plumbing to get that information out, but seems to be the best way.

Since JSON is currently not supported, I'll ignore it for now, but it should probably be supported someday.

I would expect:

Windows consoles without virtual terminal processing should always be --color=never.
--color=always translates to --color=always
--color=never translates to --color=never
--color=auto will translate to always or never depending on if the emitter has detected color support.

GuillaumeGomez · 2020-07-15T11:52:48Z

I looked at how termcolor was doing and apparently, old windows console seem to support ansi color codes, this is why I didn't make the check.

ehuss · 2020-07-15T15:33:20Z

ANSI support was added in Windows 10 Creators Update (mid 2017). here is where termcolor queries if ANSI is supported. The is_synchronous function is probably the best way to detect if this is the case.

GuillaumeGomez · 2020-07-16T18:33:02Z

Updated!

src/librustdoc/lib.rs

ehuss · 2020-07-20T15:19:29Z

src/librustdoc/test.rs

+                            compiler.arg("--color").arg("always");
+                        }
+                    }
+                    #[cfg(not(windows))]
+                    {
+                        compiler.arg("--color").arg("always");
+                    }


I think this is missing the check that this is a TTY in both the windows and non-windows case. That's why I was suggesting to query the Emitter which has already figured these things out (and to avoid duplicating that logic).

The Emitter doesn't provide such information, that's why I had to add these checks performed in termcolor. I didn't see something in librustc_errors that allowed me to know: "can I print color here?". But maybe I just missed it? If you have a tip, it'd be very helpful!

It would need to be added as a new method to Emitter.

For EmitterWriter, it would need to check the Destination to determine whether or not the destination supports color. For Terminal(StandardStream), it would check supports_color. For Buffered(BufferWriter), it would need to call buffer().supports_color(). For Destination::Raw, it would be the 2nd tuple value (the bool indicating color support).

For JsonEmitter, it would need to call new_emitter and do the same checks as above.

Other emitters would need similar treatment, though I think the rest of them are special (like SilentEmitter which could just return false).

crlf0710 · 2020-08-21T11:55:06Z

There's merge conflict now. @GuillaumeGomez

@ehuss

Add a comment why `extern crate` is necessary for rustdoc r? @ehuss From rust-lang#74293 (comment).

@ehuss

Add a comment why `extern crate` is necessary for rustdoc r? @ehuss From rust-lang#74293 (comment).

bors · 2020-11-17T01:18:31Z

☔ The latest upstream changes (presumably #79104) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

… the color escape characters

GuillaumeGomez · 2020-11-17T09:34:02Z

@bors: r=jyn514

bors · 2020-11-17T09:34:03Z

📌 Commit ec10824 has been approved by jyn514

Rollup of 8 pull requests Successful merges: - rust-lang#74293 (Rustdoc test compiler output color) - rust-lang#78702 ([self-profiling] Include the estimated size of each cgu in the profile) - rust-lang#79069 (Get rid of `highlight::Class::None`) - rust-lang#79072 (Fix exhaustiveness in case a byte string literal is used at slice type) - rust-lang#79120 (update rustfmt to v1.4.27) - rust-lang#79125 (Get rid of clean::{Method, TyMethod}) - rust-lang#79126 (Remove duplicate `Trait::auto` field) - rust-lang#79130 (extend macro braces test) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

pickfire · 2020-11-18T03:08:21Z

@GuillaumeGomez How do we prevent regression for this?

jyn514 · 2020-11-18T03:12:38Z

Good question 😆 I'm open to ideas!

pickfire · 2020-11-18T07:13:34Z

The most accurate but hardest way is probably snapshot testing, like taking the image snapshot of the colors. Maybe we can just set the environment variables, and detect the colors ascii codes in standard output?

jyn514 · 2020-11-18T13:51:13Z

@pickfire would you be interested in working on that? That sounds like we might even be able to put it in rustdoc-ui without too many changes to the testing framework.

pickfire · 2020-11-18T15:52:07Z

No, not for this. I have quite a few backlog I want to do and never did.

ehuss · 2020-11-23T16:00:58Z

Thanks for fixing this @GuillaumeGomez! I know I was being a little picky that it properly supports all versions of Windows, but I think good Windows support is important, and I appreciate that you took the time to get it working.

GuillaumeGomez · 2020-11-23T16:07:04Z

@ehuss You were right in doing so! If merged too quickly, we only get bugs later on. I personally don't use windows so input from windows users are very welcome.

rust-highfive assigned ehuss Jul 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2020

GuillaumeGomez force-pushed the rustdoc-test-compiler-output-color branch 2 times, most recently from 149a69e to 2e37e4b Compare July 14, 2020 12:44

GuillaumeGomez force-pushed the rustdoc-test-compiler-output-color branch from 8b6eb7f to 7a5a446 Compare July 16, 2020 15:02

ehuss reviewed Jul 20, 2020

View reviewed changes

ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 24, 2020

Mark-Simulacrum mentioned this pull request Jul 30, 2020

Rename "--display-warnings" to "--display-doctest-warnings" #73314

Merged

This comment has been minimized.

Sign in to view

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2020

GuillaumeGomez force-pushed the rustdoc-test-compiler-output-color branch from 7a5a446 to ccd2639 Compare August 22, 2020 15:44

This comment has been minimized.

Sign in to view

jyn514 mentioned this pull request Sep 15, 2020

Add a comment why extern crate is necessary for rustdoc #76728

Merged

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 16, 2020

Rollup merge of rust-lang#76728 - jyn514:rustdoc-extern-crate, r=ehuss

36bf0c9

Add a comment why `extern crate` is necessary for rustdoc r? @ehuss From rust-lang#74293 (comment).

tmandry added a commit to tmandry/rust that referenced this pull request Sep 16, 2020

Rollup merge of rust-lang#76728 - jyn514:rustdoc-extern-crate, r=ehuss

6e57eec

Add a comment why `extern crate` is necessary for rustdoc r? @ehuss From rust-lang#74293 (comment).

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 17, 2020

GuillaumeGomez added 9 commits November 17, 2020 10:32

Add color in rustdoc --test output

96a6a5f

Update error code detection in compile_fail doctests

54e8216

Add check to get windows console type to decide to use colors or not

57bab5e

Update lock file

704001b

Simplfy color availability check

32d64ed

Update doctest tests

fd4a33c

Correctly detect color support

95ee1fc

Add comment explaining why we can't split on error[{}]: because of…

63785c8

… the color escape characters

Remove unused import

ec10824

GuillaumeGomez force-pushed the rustdoc-test-compiler-output-color branch from bdcd6da to ec10824 Compare November 17, 2020 09:33

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2020

m-ou-se mentioned this pull request Nov 17, 2020

Rollup of 8 pull requests #79138

Merged

bors merged commit 81f9feb into rust-lang:master Nov 17, 2020

rustbot added this to the 1.50.0 milestone Nov 17, 2020

GuillaumeGomez deleted the rustdoc-test-compiler-output-color branch November 17, 2020 18:00

jyn514 mentioned this pull request Nov 18, 2020

Test that rustdoc preserves color output in doctests #79170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustdoc test compiler output color #74293

Rustdoc test compiler output color #74293

GuillaumeGomez commented Jul 13, 2020

Mark-Simulacrum commented Jul 13, 2020

GuillaumeGomez commented Jul 13, 2020

Mark-Simulacrum commented Jul 13, 2020

GuillaumeGomez commented Jul 13, 2020

ehuss commented Jul 14, 2020

GuillaumeGomez commented Jul 15, 2020

ehuss commented Jul 15, 2020

GuillaumeGomez commented Jul 16, 2020

ehuss Jul 20, 2020

GuillaumeGomez Aug 22, 2020

ehuss Aug 22, 2020

This comment has been minimized.

crlf0710 commented Aug 21, 2020

This comment has been minimized.

bors commented Nov 17, 2020

GuillaumeGomez commented Nov 17, 2020

bors commented Nov 17, 2020

pickfire commented Nov 18, 2020

jyn514 commented Nov 18, 2020

pickfire commented Nov 18, 2020 •

edited

Loading

jyn514 commented Nov 18, 2020

pickfire commented Nov 18, 2020

ehuss commented Nov 23, 2020

GuillaumeGomez commented Nov 23, 2020

Rustdoc test compiler output color #74293

Rustdoc test compiler output color #74293

Conversation

GuillaumeGomez commented Jul 13, 2020

Mark-Simulacrum commented Jul 13, 2020

GuillaumeGomez commented Jul 13, 2020

Mark-Simulacrum commented Jul 13, 2020

GuillaumeGomez commented Jul 13, 2020

ehuss commented Jul 14, 2020

GuillaumeGomez commented Jul 15, 2020

ehuss commented Jul 15, 2020

GuillaumeGomez commented Jul 16, 2020

ehuss Jul 20, 2020

Choose a reason for hiding this comment

GuillaumeGomez Aug 22, 2020

Choose a reason for hiding this comment

ehuss Aug 22, 2020

Choose a reason for hiding this comment

This comment has been minimized.

crlf0710 commented Aug 21, 2020

This comment has been minimized.

bors commented Nov 17, 2020

GuillaumeGomez commented Nov 17, 2020

bors commented Nov 17, 2020

pickfire commented Nov 18, 2020

jyn514 commented Nov 18, 2020

pickfire commented Nov 18, 2020 • edited Loading

jyn514 commented Nov 18, 2020

pickfire commented Nov 18, 2020

ehuss commented Nov 23, 2020

GuillaumeGomez commented Nov 23, 2020

pickfire commented Nov 18, 2020 •

edited

Loading