Skip to content

Commit b03182a

Browse files
borsbors
committed
Auto merge of #7450 - ehuss:stabilize-cache-messages, r=alexcrichton
Stabilize cache-messages This stabilizes the -Zcache-messages feature, making it always enabled. ## What is stabilized? This feature is intended to redisplay previous warnings on a "fresh" build instead of displaying no output. Users have occasionally indicated frustration when they know there are warnings, but no output is displayed when the build is fresh. This also improves the interaction between `cargo check` and `cargo clippy-preview`. This also simplifies the code, and opens more possibilities for `rustc` to send side-channel messages to Cargo. Cargo will now use JSON output from `rustc` and `rustdoc` 100% of the time (`rustdoc --test` does not use JSON). Previously Cargo would only use JSON for pipelined crates. Cargo will save the JSON output into a file named `output` in the `.fingerprint` directory. This file is only created when the compiler outputs a diagnostic message. If a crate is being recompiled, and Cargo considers it to be "fresh", it will replay the output file to the console. ## Notable changes in this PR - Fixed a bug where replays were erroneously including pipeline rmeta artifact json messages. - clippy-preview is now included in the metadata hash, to force its artifacts to be separate from `cargo check`. - clippy-preview is no longer force-enabled, under the assumption that caching and fingerprinting is accurate, and the cached messages will be replayed. - clippy-preview's arguments are included in the fingerprint now that it is not force-enabled. - Rustdoc colors and short messages were fixed when pipelining was stabilized, so updated tests. Closes #6986 Closes #6848 Closes #6664 Closes #2458 ## Concerns The only notable issue with this is that switching between short and long human messages only replays the format from the first invocation. That is, if you do `cargo build` and it generates warnings, then running again with `--message-format=short` will still show the full length human messages. I'm personally fine with that behavior, even though it is not ideal. I think this feature overall improves the situation (where before *no* output was displayed). Being able to re-render between short/long is a very difficult problem, and unlikely to be fixable in the foreseeable future. There was some concern expressed about being able to disable this. I think that would only be necessary if a severe bug is discovered. I do not feel that this change is risky enough to warrant a configurable option. If it does cause a problem, it can be quickly reverted with a one-line change to set `OutputOptions::cache_cell` to `None`. Since pipelining has been using JSON output for a while now without complaints, I feel pretty confident in it.
2 parents d5d7557 + bd73e8d commit b03182a

18 files changed

+242
-399
lines changed

‎src/bin/cargo/cli.rs

-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ Available unstable (nightly-only) flags:
3535
-Z unstable-options -- Allow the usage of unstable options such as --registry
3636
-Z config-profile -- Read profiles from .cargo/config files
3737
-Z install-upgrade -- `cargo install` will upgrade instead of failing
38-
-Z cache-messages -- Cache compiler messages
3938
-Z timings -- Display concurrency information
4039
-Z doctest-xcompile -- Compile and run doctests for non-host target using runner config
4140

‎src/bin/cargo/commands/clippy.rs

-1
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
7979
}
8080

8181
compile_opts.build_config.primary_unit_rustc = Some(wrapper);
82-
compile_opts.build_config.force_rebuild = true;
8382

8483
ops::compile(&ws, &compile_opts)?;
8584
Ok(())

‎src/cargo/core/compiler/build_config.rs

-8
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,6 @@ pub struct BuildConfig {
4343
/// An optional override of the rustc path for primary units only
4444
pub primary_unit_rustc: Option<ProcessBuilder>,
4545
pub rustfix_diagnostic_server: RefCell<Option<RustfixDiagnosticServer>>,
46-
/// Whether or not Cargo should cache compiler output on disk.
47-
cache_messages: bool,
4846
}
4947

5048
impl BuildConfig {
@@ -101,15 +99,9 @@ impl BuildConfig {
10199
build_plan: false,
102100
primary_unit_rustc: None,
103101
rustfix_diagnostic_server: RefCell::new(None),
104-
cache_messages: config.cli_unstable().cache_messages,
105102
})
106103
}
107104

108-
/// Whether or not Cargo should cache compiler messages on disk.
109-
pub fn cache_messages(&self) -> bool {
110-
self.cache_messages
111-
}
112-
113105
/// Whether or not the *user* wants JSON output. Whether or not rustc
114106
/// actually uses JSON is decided in `add_error_format`.
115107
pub fn emit_json(&self) -> bool {

‎src/cargo/core/compiler/context/compilation_files.rs

+8
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,14 @@ fn compute_metadata<'a, 'cfg>(
556556

557557
bcx.rustc.verbose_version.hash(&mut hasher);
558558

559+
if cx.is_primary_package(unit) {
560+
// This is primarily here for clippy. This ensures that the clippy
561+
// artifacts are separate from the `check` ones.
562+
if let Some(proc) = &cx.bcx.build_config.primary_unit_rustc {
563+
proc.get_program().hash(&mut hasher);
564+
}
565+
}
566+
559567
// Seed the contents of `__CARGO_DEFAULT_LIB_METADATA` to the hasher if present.
560568
// This should be the release channel, to get a different hash for each channel.
561569
if let Ok(ref channel) = __cargo_default_lib_metadata {

‎src/cargo/core/compiler/fingerprint.rs

+15-3
Original file line numberDiff line numberDiff line change
@@ -1083,11 +1083,23 @@ fn calculate_normal<'a, 'cfg>(
10831083
// Fill out a bunch more information that we'll be tracking typically
10841084
// hashed to take up less space on disk as we just need to know when things
10851085
// change.
1086-
let extra_flags = if unit.mode.is_doc() {
1086+
let mut extra_flags = if unit.mode.is_doc() {
10871087
cx.bcx.rustdocflags_args(unit)
10881088
} else {
10891089
cx.bcx.rustflags_args(unit)
1090-
};
1090+
}
1091+
.to_vec();
1092+
if cx.is_primary_package(unit) {
1093+
// This is primarily here for clippy arguments.
1094+
if let Some(proc) = &cx.bcx.build_config.primary_unit_rustc {
1095+
let args = proc
1096+
.get_args()
1097+
.iter()
1098+
.map(|s| s.to_string_lossy().to_string());
1099+
extra_flags.extend(args);
1100+
}
1101+
}
1102+
10911103
let profile_hash = util::hash_u64((&unit.profile, unit.mode, cx.bcx.extra_args_for(unit)));
10921104
// Include metadata since it is exposed as environment variables.
10931105
let m = unit.pkg.manifest().metadata();
@@ -1104,7 +1116,7 @@ fn calculate_normal<'a, 'cfg>(
11041116
local: Mutex::new(local),
11051117
memoized_hash: Mutex::new(None),
11061118
metadata,
1107-
rustflags: extra_flags.to_vec(),
1119+
rustflags: extra_flags,
11081120
fs_status: FsStatus::Stale,
11091121
outputs,
11101122
})

‎src/cargo/core/compiler/layout.rs

+3
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@
3636
//! # Detailed information used for logging the reason why
3737
//! # something is being recompiled.
3838
//! lib-$pkgname-$META.json
39+
//! # The console output from the compiler. This is cached
40+
//! # so that warnings can be redisplayed for "fresh" units.
41+
//! output
3942
//!
4043
//! # This is the root directory for all rustc artifacts except build
4144
//! # scripts, examples, and test and bench executables. Almost every

‎src/cargo/core/compiler/mod.rs

+53-95
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,7 @@ fn compile<'a, 'cfg: 'a>(
136136
};
137137
work.then(link_targets(cx, unit, false)?)
138138
} else {
139-
let work = if cx.bcx.build_config.cache_messages()
140-
&& cx.bcx.show_warnings(unit.pkg.package_id())
141-
{
139+
let work = if cx.bcx.show_warnings(unit.pkg.package_id()) {
142140
replay_output_cache(
143141
unit.pkg.package_id(),
144142
unit.target,
@@ -663,79 +661,31 @@ fn add_cap_lints(bcx: &BuildContext<'_, '_>, unit: &Unit<'_>, cmd: &mut ProcessB
663661

664662
/// Add error-format flags to the command.
665663
///
666-
/// This is somewhat odd right now, but the general overview is that if
667-
/// `-Zcache-messages` or `pipelined` is enabled then Cargo always uses JSON
668-
/// output. This has several benefits, such as being easier to parse, handles
669-
/// changing formats (for replaying cached messages), ensures atomic output (so
670-
/// messages aren't interleaved), etc.
671-
///
672-
/// It is intended in the future that Cargo *always* uses the JSON output (by
673-
/// turning on cache-messages by default), and this function can be simplified.
664+
/// Cargo always uses JSON output. This has several benefits, such as being
665+
/// easier to parse, handles changing formats (for replaying cached messages),
666+
/// ensures atomic output (so messages aren't interleaved), allows for
667+
/// intercepting messages like rmeta artifacts, etc. rustc includes a
668+
/// "rendered" field in the JSON message with the message properly formatted,
669+
/// which Cargo will extract and display to the user.
674670
fn add_error_format_and_color(
675671
cx: &Context<'_, '_>,
676672
cmd: &mut ProcessBuilder,
677673
pipelined: bool,
678674
) -> CargoResult<()> {
679-
// If this unit is producing a required rmeta file then we need to know
680-
// when the rmeta file is ready so we can signal to the rest of Cargo that
681-
// it can continue dependent compilations. To do this we are currently
682-
// required to switch the compiler into JSON message mode, but we still
683-
// want to present human readable errors as well. (this rabbit hole just
684-
// goes and goes)
685-
//
686-
// All that means is that if we're not already in JSON mode we need to
687-
// switch to JSON mode, ensure that rustc error messages can be rendered
688-
// prettily, and then when parsing JSON messages from rustc we need to
689-
// internally understand that we should extract the `rendered` field and
690-
// present it if we can.
691-
if cx.bcx.build_config.cache_messages() || pipelined {
692-
cmd.arg("--error-format=json");
693-
let mut json = String::from("--json=diagnostic-rendered-ansi");
694-
if pipelined {
695-
json.push_str(",artifacts");
696-
}
697-
match cx.bcx.build_config.message_format {
698-
MessageFormat::Short | MessageFormat::Json { short: true, .. } => {
699-
json.push_str(",diagnostic-short");
700-
}
701-
_ => {}
702-
}
703-
cmd.arg(json);
704-
} else {
705-
let mut color = true;
706-
match cx.bcx.build_config.message_format {
707-
MessageFormat::Human => (),
708-
MessageFormat::Json {
709-
ansi,
710-
short,
711-
render_diagnostics,
712-
} => {
713-
cmd.arg("--error-format").arg("json");
714-
// If ansi is explicitly requested, enable it. If we're
715-
// rendering diagnostics ourselves then also enable it because
716-
// we'll figure out what to do with the colors later.
717-
if ansi || render_diagnostics {
718-
cmd.arg("--json=diagnostic-rendered-ansi");
719-
}
720-
if short {
721-
cmd.arg("--json=diagnostic-short");
722-
}
723-
color = false;
724-
}
725-
MessageFormat::Short => {
726-
cmd.arg("--error-format").arg("short");
727-
}
728-
}
729-
730-
if color {
731-
let color = if cx.bcx.config.shell().supports_color() {
732-
"always"
733-
} else {
734-
"never"
735-
};
736-
cmd.args(&["--color", color]);
675+
cmd.arg("--error-format=json");
676+
let mut json = String::from("--json=diagnostic-rendered-ansi");
677+
if pipelined {
678+
// Pipelining needs to know when rmeta files are finished. Tell rustc
679+
// to emit a message that cargo will intercept.
680+
json.push_str(",artifacts");
681+
}
682+
match cx.bcx.build_config.message_format {
683+
MessageFormat::Short | MessageFormat::Json { short: true, .. } => {
684+
json.push_str(",diagnostic-short");
737685
}
686+
_ => {}
738687
}
688+
cmd.arg(json);
739689
Ok(())
740690
}
741691

@@ -1058,22 +1008,19 @@ struct OutputOptions {
10581008
color: bool,
10591009
/// Where to write the JSON messages to support playback later if the unit
10601010
/// is fresh. The file is created lazily so that in the normal case, lots
1061-
/// of empty files are not created. This is None if caching is disabled.
1011+
/// of empty files are not created. If this is None, the output will not
1012+
/// be cached (such as when replaying cached messages).
10621013
cache_cell: Option<(PathBuf, LazyCell<File>)>,
10631014
}
10641015

10651016
impl OutputOptions {
10661017
fn new<'a>(cx: &Context<'a, '_>, unit: &Unit<'a>) -> OutputOptions {
10671018
let look_for_metadata_directive = cx.rmeta_required(unit);
10681019
let color = cx.bcx.config.shell().supports_color();
1069-
let cache_cell = if cx.bcx.build_config.cache_messages() {
1070-
let path = cx.files().message_cache_path(unit);
1071-
// Remove old cache, ignore ENOENT, which is the common case.
1072-
drop(fs::remove_file(&path));
1073-
Some((path, LazyCell::new()))
1074-
} else {
1075-
None
1076-
};
1020+
let path = cx.files().message_cache_path(unit);
1021+
// Remove old cache, ignore ENOENT, which is the common case.
1022+
drop(fs::remove_file(&path));
1023+
let cache_cell = Some((path, LazyCell::new()));
10771024
OutputOptions {
10781025
format: cx.bcx.build_config.message_format,
10791026
look_for_metadata_directive,
@@ -1100,23 +1047,35 @@ fn on_stderr_line(
11001047
target: &Target,
11011048
options: &mut OutputOptions,
11021049
) -> CargoResult<()> {
1103-
// Check if caching is enabled.
1104-
if let Some((path, cell)) = &mut options.cache_cell {
1105-
// Cache the output, which will be replayed later when Fresh.
1106-
let f = cell.try_borrow_mut_with(|| File::create(path))?;
1107-
debug_assert!(!line.contains('\n'));
1108-
f.write_all(line.as_bytes())?;
1109-
f.write_all(&[b'\n'])?;
1050+
if on_stderr_line_inner(state, line, package_id, target, options)? {
1051+
// Check if caching is enabled.
1052+
if let Some((path, cell)) = &mut options.cache_cell {
1053+
// Cache the output, which will be replayed later when Fresh.
1054+
let f = cell.try_borrow_mut_with(|| File::create(path))?;
1055+
debug_assert!(!line.contains('\n'));
1056+
f.write_all(line.as_bytes())?;
1057+
f.write_all(&[b'\n'])?;
1058+
}
11101059
}
1060+
Ok(())
1061+
}
11111062

1063+
/// Returns true if the line should be cached.
1064+
fn on_stderr_line_inner(
1065+
state: &JobState<'_>,
1066+
line: &str,
1067+
package_id: PackageId,
1068+
target: &Target,
1069+
options: &mut OutputOptions,
1070+
) -> CargoResult<bool> {
11121071
// We primarily want to use this function to process JSON messages from
11131072
// rustc. The compiler should always print one JSON message per line, and
11141073
// otherwise it may have other output intermingled (think RUST_LOG or
11151074
// something like that), so skip over everything that doesn't look like a
11161075
// JSON message.
11171076
if !line.starts_with('{') {
11181077
state.stderr(line.to_string());
1119-
return Ok(());
1078+
return Ok(true);
11201079
}
11211080

11221081
let mut compiler_message: Box<serde_json::value::RawValue> = match serde_json::from_str(line) {
@@ -1128,7 +1087,7 @@ fn on_stderr_line(
11281087
Err(e) => {
11291088
debug!("failed to parse json: {:?}", e);
11301089
state.stderr(line.to_string());
1131-
return Ok(());
1090+
return Ok(true);
11321091
}
11331092
};
11341093

@@ -1164,14 +1123,13 @@ fn on_stderr_line(
11641123
.expect("strip should never fail")
11651124
};
11661125
state.stderr(rendered);
1167-
return Ok(());
1126+
return Ok(true);
11681127
}
11691128
}
11701129

1171-
// Remove color information from the rendered string. When pipelining is
1172-
// enabled and/or when cached messages are enabled we're always asking
1173-
// for ANSI colors from rustc, so unconditionally postprocess here and
1174-
// remove ansi color codes.
1130+
// Remove color information from the rendered string if color is not
1131+
// enabled. Cargo always asks for ANSI colors from rustc. This allows
1132+
// cached replay to enable/disable colors without re-invoking rustc.
11751133
MessageFormat::Json { ansi: false, .. } => {
11761134
#[derive(serde::Deserialize, serde::Serialize)]
11771135
struct CompilerMessage {
@@ -1213,7 +1171,7 @@ fn on_stderr_line(
12131171
log::debug!("looks like metadata finished early!");
12141172
state.rmeta_produced();
12151173
}
1216-
return Ok(());
1174+
return Ok(false);
12171175
}
12181176
}
12191177

@@ -1231,7 +1189,7 @@ fn on_stderr_line(
12311189
// instead. We want the stdout of Cargo to always be machine parseable as
12321190
// stderr has our colorized human-readable messages.
12331191
state.stdout(msg);
1234-
Ok(())
1192+
Ok(true)
12351193
}
12361194

12371195
fn replay_output_cache(
@@ -1244,7 +1202,7 @@ fn replay_output_cache(
12441202
let target = target.clone();
12451203
let mut options = OutputOptions {
12461204
format,
1247-
look_for_metadata_directive: false,
1205+
look_for_metadata_directive: true,
12481206
color,
12491207
cache_cell: None,
12501208
};

‎src/cargo/core/features.rs

-2
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,6 @@ pub struct CliUnstable {
335335
pub dual_proc_macros: bool,
336336
pub mtime_on_use: bool,
337337
pub install_upgrade: bool,
338-
pub cache_messages: bool,
339338
pub named_profiles: bool,
340339
pub binary_dep_depinfo: bool,
341340
pub build_std: Option<Vec<String>>,
@@ -393,7 +392,6 @@ impl CliUnstable {
393392
// can also be set in .cargo/config or with and ENV
394393
"mtime-on-use" => self.mtime_on_use = true,
395394
"install-upgrade" => self.install_upgrade = true,
396-
"cache-messages" => self.cache_messages = true,
397395
"named-profiles" => self.named_profiles = true,
398396
"binary-dep-depinfo" => self.binary_dep_depinfo = true,
399397
"build-std" => {

‎src/doc/src/reference/unstable.md

+3-22
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@ index each time.
2323
* Original Issue: [#6477](https://github.com/rust-lang/cargo/pull/6477)
2424
* Cache usage meta tracking issue: [#7150](https://github.com/rust-lang/cargo/issues/7150)
2525

26-
The `-Z mtime-on-use` flag is an experiment to have Cargo update the mtime of
27-
used files to make it easier for tools like cargo-sweep to detect which files
26+
The `-Z mtime-on-use` flag is an experiment to have Cargo update the mtime of
27+
used files to make it easier for tools like cargo-sweep to detect which files
2828
are stale. For many workflows this needs to be set on *all* invocations of cargo.
2929
To make this more practical setting the `unstable.mtime_on_use` flag in `.cargo/config`
30-
or the corresponding ENV variable will apply the `-Z mtime-on-use` to all
30+
or the corresponding ENV variable will apply the `-Z mtime-on-use` to all
3131
invocations of nightly cargo. (the config flag is ignored by stable)
3232

3333
### avoid-dev-deps
@@ -323,25 +323,6 @@ my_dep = { version = "1.2.3", public = true }
323323
private_dep = "2.0.0" # Will be 'private' by default
324324
```
325325

326-
### cache-messages
327-
* Tracking Issue: [#6986](https://github.com/rust-lang/cargo/issues/6986)
328-
329-
The `cache-messages` feature causes Cargo to cache the messages generated by
330-
the compiler. This is primarily useful if a crate compiles successfully with
331-
warnings. Previously, re-running Cargo would not display any output. With the
332-
`cache-messages` feature, it will quickly redisplay the previous warnings.
333-
334-
```
335-
cargo +nightly check -Z cache-messages
336-
```
337-
338-
This works with any command that runs the compiler (`build`, `check`, `test`,
339-
etc.).
340-
341-
This also changes the way Cargo interacts with the compiler, helping to
342-
prevent interleaved messages when multiple crates attempt to display a message
343-
at the same time.
344-
345326
### build-std
346327
* Tracking Repository: https://github.com/rust-lang/wg-cargo-std-aware
347328

0 commit comments

Comments
 (0)