Add message caching. #6933

ehuss · 2019-05-12T20:19:40Z

The cache-messages feature causes Cargo to cache the messages generated by
the compiler. This is primarily useful if a crate compiles successfully with
warnings. Previously, re-running Cargo would not display any output. With the
cache-messages feature, it will quickly redisplay the previous warnings.

cargo +nightly check -Z cache-messages

Notes:

short messages do not work correctly.
rustdoc does not support --json-rendered=termcolor, so its output is currently uncolored.
This approach to rendering should address some output issues like Error messages from rustc interleaved #6848.

rust-highfive · 2019-05-12T20:19:51Z

r? @alexcrichton

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

appveyor.yml

alexcrichton

This is looking quite good to me!

One thing that strikes me though is that our output handling feels pretty convoluted, so I'm wondering if we can streamline it a bit and/or make it more understandable. We seem to be balancing a lot of concerns like:

If color is requested, we unconditionally turn on ansi colors for rustc and then we'll reinterpret them somewhere to Windows colors.
In general we want to present human-readable error messages by default, but this is balanced with:
- Using the pipelining feature we need JSON mode to see a notification and then we have to extract JSON errors. This is nightly-only though due to the flags we pass to rustc.
- In capture mode we also need JSON messages to extract (er but actually why does capture mode force JSON? Can't we just capture all output as usual?)
Dealing with color feels tricky where we're asking rustc to emit colors but then we strip them later from the rendered field because they accidentally weren't supposed to be in there.

I'm wondering if we can perhaps refactor this a bit to be a bit more clear on the intent? (I don't have anything in mind just curious if you have thoughts having just looked at it). Maybe something like an enum somewhere with clear match statements, and/or a struct instead of passing around a bunch of booleans everywhere (my bad).

src/cargo/core/compiler/build_config.rs

src/cargo/core/compiler/job_queue.rs

alexcrichton · 2019-05-15T15:30:32Z

src/cargo/core/compiler/mod.rs

@@ -139,8 +141,21 @@ fn compile<'a, 'cfg: 'a>(
            };
            work.then(link_targets(cx, unit, false)?)
        } else {
+            let work = if cx.bcx.build_config.cache_messages()
+                && cx.bcx.show_warnings(unit.pkg.package_id())


How come show_warnings is included here? I would imagine that if show_warnings were false we'd already in general have an empty cache, but there's been occasional warnings over time which bypass lint settings and warn even for transitive dependencies, and if we want to replay everything we may not want to suppress them.

The main reason is that if you run cargo build -vv, and then later run cargo build, you don't end up with a ton of warnings you can't silence (and are usually unactionable).

I would say the same logic applies to uncappable warnings. It would cause repeated noise that would unlikely be useful. I'm curious if there are any warnings like that today.

Of course, the reverse won't work (-vv won't show warnings from previous builds). Cargo could take over handling lint capping to make it possible to swap back and forth (with/without -vv), but I figure this is an extremely rare use case.

Hm this is a good point, and it sounds pretty unfortunate. One thing I'm worried about is that rustc may decide that not all warnings fall under --cap-lints. For example "please urgently fix this dependency since it will break soon" may be issued for upstream dependencies anyway. In that sense I don't think we can move --cap-lints into Cargo (not without more information in the json blob at least) easily.

That being said not spewing warnings on -vv followed by cargo build sounds good. The "failure case" of "less warnings on cargo build followed by -vv" seems no worse than today.

This all in all sounds like a good thing to note on a tracking issue for this :)

src/cargo/core/compiler/mod.rs

alexcrichton · 2019-05-15T15:33:32Z

src/cargo/core/compiler/mod.rs

-        MessageFormat::Human => (),
-        MessageFormat::Json => {
-            cmd.arg("--error-format").arg("json");
+fn add_error_format(


This is a pretty gnarly function, but to confirm, the end result is (as in in the limit of time):

Always pass --error-format=json

Always pass --json-rendered=termcolor

Maybe pass -Zemit-artifact-notifications depending on pipelining

And then we just figure out in Cargo what to do with all the output later. Does that sound right?

Could this future intention be added as a comment to the function in the interim?

src/cargo/core/compiler/mod.rs

ehuss

er but actually why does capture mode force JSON? Can't we just capture all output as usual?

There are a few reasons. One is to be able to seamlessly switch between JSON and human output, without having confusing behavior. The other is to fix some of the message interleaving problems (json messages are always atomic). Third, it seems like with things like pipelining it would be good to interact with rustc in a uniform manner.

Also, to be clear, this may need to be redesigned before stabilization due to the short problem. My primary goal was to see if this would work and how difficult it would be. We'll eventually need to make a high-level decision on how this should work. Some options I thought of:

"short" messages could be included just like "rendered" as a separate field.
JSON could be completely redesigned to match the Diagnostic struct, and have an external library which would generate the ascii-art messages. I'm vaguely aware of wg-diagnostics wanting to move everything to a separate library, but I suspect that will take a long time.

ehuss · 2019-05-15T16:16:04Z

src/cargo/core/compiler/mod.rs

@@ -139,8 +141,21 @@ fn compile<'a, 'cfg: 'a>(
            };
            work.then(link_targets(cx, unit, false)?)
        } else {
+            let work = if cx.bcx.build_config.cache_messages()
+                && cx.bcx.show_warnings(unit.pkg.package_id())


The main reason is that if you run cargo build -vv, and then later run cargo build, you don't end up with a ton of warnings you can't silence (and are usually unactionable).

I would say the same logic applies to uncappable warnings. It would cause repeated noise that would unlikely be useful. I'm curious if there are any warnings like that today.

Of course, the reverse won't work (-vv won't show warnings from previous builds). Cargo could take over handling lint capping to make it possible to swap back and forth (with/without -vv), but I figure this is an extremely rare use case.

ehuss · 2019-05-15T16:16:34Z

src/cargo/core/compiler/mod.rs

-        MessageFormat::Human => (),
-        MessageFormat::Json => {
-            cmd.arg("--error-format").arg("json");
+fn add_error_format(


src/cargo/core/compiler/mod.rs

alexcrichton · 2019-05-20T15:44:10Z

Ok sounds good to me! FWIW I think I like the idea of always invoking rustc in JSON mode. That does have a nice uniform feel to it and helps keep pressure on having JSON diagnostics match the actual printed versions. It does sort of obsolete a lot of color handling code in rustc and terminal printing, but I suppose that's not the end of the world :)

I'd be the first to say that I'd be delighted to remove the weird windows global lock code for printing diagnostics. AFAIK no other compiler does that and having Cargo do the synchronization would be fantastic...

bors · 2019-05-20T21:48:37Z

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

- Add some comments and cleanup. - Error out when using `short`.

pickfire · 2019-05-21T03:55:25Z

.travis.yml

@@ -39,6 +39,7 @@ matrix:

 before_script:
  - rustup target add $ALT
+  - rustup component add clippy || echo "clippy not available"


Should we do echo "clippy not available" here?

I don't understand the question. That's what it is doing. Or maybe you meant to comment on the appveyor changes?

I mean is it necessary to have echo "clippy not available"? Usually I only saw rustup component add clippy without the other part.

If not, the command may fail because of missing clippy, and the build will stop.

alexcrichton · 2019-05-21T15:04:44Z

@bors: r+

bors · 2019-05-21T15:04:45Z

📌 Commit 9ba6812 has been approved by alexcrichton

bors · 2019-05-21T15:04:54Z

⌛ Testing commit 9ba6812 with merge 5218d04...

Add message caching. The `cache-messages` feature causes Cargo to cache the messages generated by the compiler. This is primarily useful if a crate compiles successfully with warnings. Previously, re-running Cargo would not display any output. With the `cache-messages` feature, it will quickly redisplay the previous warnings. ``` cargo +nightly check -Z cache-messages ``` Notes: - `short` messages do not work correctly. - rustdoc does not support `--json-rendered=termcolor`, so its output is currently uncolored. - This approach to rendering should address some output issues like #6848.

bors · 2019-05-21T15:46:36Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 5218d04 to master...

Update cargo Update cargo 14 commits in c4fcfb725b4be00c72eb9cf30c7d8b095577c280..545f354259be4e9745ea00a524c0e4c51df01aa6 2019-05-15 19:48:47 +0000 to 2019-05-23 17:45:30 +0000 - Bump to 0.38.0 (rust-lang/cargo#6979) - cargo package: detect new empty directories (rust-lang/cargo#6973) - Add message caching. (rust-lang/cargo#6933) - Fix typo (rust-lang/cargo#6974) - Set `Finished` line correctly for debug=0. (rust-lang/cargo#6971) - Clippy fixes (rust-lang/cargo#6970) - Remove rustdoc `can_add_color_process`. (rust-lang/cargo#6968) - Document new `doctest` field. (rust-lang/cargo#6965) - Update some man pages that missed --offline. (rust-lang/cargo#6964) - add public & private prop tests. (rust-lang/cargo#6962) - zsh completion: Pull list of commands from cargo --list (rust-lang/cargo#6956) - Change docs "inequality" for semver requirement. (rust-lang/cargo#6963) - Update im-rc requirement from 12.1.0 to 13.0.0 (rust-lang/cargo#6959) - Add `doctest` field into metadata (rust-lang/cargo#6953)

rust-highfive assigned alexcrichton May 12, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 12, 2019

JeanMertz reviewed May 13, 2019

View reviewed changes

appveyor.yml Show resolved Hide resolved

alexcrichton reviewed May 15, 2019

View reviewed changes

ehuss commented May 19, 2019

View reviewed changes

ehuss added 4 commits May 20, 2019 15:22

Add message caching.

dcd4999

Cache-messages: address some review comments.

f6d30e9

- Add some comments and cleanup. - Error out when using `short`.

Pull out on_stderr_line options into a struct.

342f860

Add some more comments on add_error_format.

9ba6812

ehuss force-pushed the cache-output branch from 543e61c to 9ba6812 Compare May 20, 2019 22:22

pickfire reviewed May 21, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

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-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2019

bors merged commit 9ba6812 into rust-lang:master May 21, 2019

bors mentioned this pull request May 21, 2019

compiler: capture rustc error output if extra-verbose #6957

Closed

ehuss mentioned this pull request May 23, 2019

Update cargo rust-lang/rust#61095

Merged

ehuss mentioned this pull request May 27, 2019

Tracking issue for cache-messages #6986

Closed

4 tasks

ehuss mentioned this pull request Jun 17, 2019

Tracking issue for unstable --json-rendered flag rust-lang/rust#60987

Closed

3 tasks

ehuss added this to the 1.37.0 milestone Feb 6, 2022

epage mentioned this pull request Sep 26, 2023

Add --deny-warnings functionality for all commands. #8424

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add message caching. #6933

Add message caching. #6933

ehuss commented May 12, 2019

rust-highfive commented May 12, 2019

alexcrichton left a comment

alexcrichton May 15, 2019

ehuss May 15, 2019

alexcrichton May 20, 2019

alexcrichton May 15, 2019

ehuss May 15, 2019

alexcrichton May 20, 2019

ehuss left a comment

ehuss May 15, 2019

ehuss May 15, 2019

alexcrichton commented May 20, 2019

bors commented May 20, 2019

pickfire May 21, 2019

ehuss May 21, 2019

pickfire May 21, 2019

tesuji May 21, 2019

This comment has been minimized.

alexcrichton commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019

Add message caching. #6933

Add message caching. #6933

Conversation

ehuss commented May 12, 2019

rust-highfive commented May 12, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented May 20, 2019

bors commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

alexcrichton commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019