From dcd4999d426ddbae733929a79da95d0936557705 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 11 May 2019 17:35:25 -0700 Subject: [PATCH 1/4] Add message caching. --- .travis.yml | 1 + Cargo.toml | 1 + appveyor.yml | 1 + src/bin/cargo/cli.rs | 1 + src/cargo/core/compiler/build_config.rs | 18 +- .../compiler/context/compilation_files.rs | 5 + src/cargo/core/compiler/custom_build.rs | 8 +- src/cargo/core/compiler/job_queue.rs | 8 +- src/cargo/core/compiler/mod.rs | 252 +++++++++---- src/cargo/core/features.rs | 2 + src/cargo/ops/fix.rs | 5 + src/cargo/util/process_builder.rs | 5 +- src/doc/src/reference/unstable.md | 21 +- tests/testsuite/cache_messages.rs | 336 ++++++++++++++++++ tests/testsuite/main.rs | 1 + 15 files changed, 590 insertions(+), 75 deletions(-) create mode 100644 tests/testsuite/cache_messages.rs diff --git a/.travis.yml b/.travis.yml index ba205f0d9fa..695d9317beb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -39,6 +39,7 @@ matrix: before_script: - rustup target add $ALT + - rustup component add clippy || echo "clippy not available" script: - cargo test --features=deny-warnings diff --git a/Cargo.toml b/Cargo.toml index 9abf06857e1..cf5aa2ff969 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,6 +54,7 @@ serde = { version = "1.0.82", features = ['derive'] } serde_ignored = "0.0.4" serde_json = { version = "1.0.30", features = ["raw_value"] } shell-escape = "0.1.4" +strip-ansi-escapes = "0.1.0" tar = { version = "0.4.18", default-features = false } tempfile = "3.0" termcolor = "1.0" diff --git a/appveyor.yml b/appveyor.yml index 0978669dde6..4173501e3a0 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -9,6 +9,7 @@ install: - rustup-init.exe -y --default-host x86_64-pc-windows-msvc --default-toolchain nightly - set PATH=%PATH%;C:\Users\appveyor\.cargo\bin - if defined OTHER_TARGET rustup target add %OTHER_TARGET% + - rustup component add clippy || exit 0 - rustc -V - cargo -V - git submodule update --init diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 422bdc4663c..36b479cc291 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -35,6 +35,7 @@ Available unstable (nightly-only) flags: -Z unstable-options -- Allow the usage of unstable options such as --registry -Z config-profile -- Read profiles from .cargo/config files -Z install-upgrade -- `cargo install` will upgrade instead of failing + -Z cache-messages -- Cache compiler messages Run with 'cargo -Z [FLAG] [SUBCOMMAND]'" ); diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 2056408097e..29555a712a4 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -27,6 +27,8 @@ pub struct BuildConfig { /// An optional wrapper, if any, used to wrap rustc invocations pub rustc_wrapper: Option, pub rustfix_diagnostic_server: RefCell>, + /// Whether or not Cargo should cache compiler output on disk. + cache_messages: bool, } impl BuildConfig { @@ -87,6 +89,7 @@ impl BuildConfig { } let cfg_jobs: Option = config.get("build.jobs")?; let jobs = jobs.or(cfg_jobs).unwrap_or(::num_cpus::get() as u32); + Ok(BuildConfig { requested_target: target, jobs, @@ -97,10 +100,23 @@ impl BuildConfig { build_plan: false, rustc_wrapper: None, rustfix_diagnostic_server: RefCell::new(None), + cache_messages: config.cli_unstable().cache_messages, }) } - pub fn json_messages(&self) -> bool { + /// Whether or not Cargo should cache compiler messages on disk. + pub fn cache_messages(&self) -> bool { + self.cache_messages + && match self.message_format { + MessageFormat::Human | MessageFormat::Json => true, + // short is currently not supported + MessageFormat::Short => false, + } + } + + /// Whether or not the *user* wants JSON output. Whether or not rustc + /// actually uses JSON is decided in `add_error_format`. + pub fn emit_json(&self) -> bool { self.message_format == MessageFormat::Json } diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs index b079307cd36..08176a81c9f 100644 --- a/src/cargo/core/compiler/context/compilation_files.rs +++ b/src/cargo/core/compiler/context/compilation_files.rs @@ -192,6 +192,11 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> { self.layout(unit.kind).fingerprint().join(dir) } + /// Path where compiler output is cached. + pub fn message_cache_path(&self, unit: &Unit<'a>) -> PathBuf { + self.fingerprint_dir(unit).join("output") + } + /// Returns the directory where a compiled build script is stored. /// `/path/to/target/{debug,release}/build/PKG-HASH` pub fn build_script_dir(&self, unit: &Unit<'a>) -> PathBuf { diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 327dc9358ab..7978d1d480e 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -112,7 +112,7 @@ fn emit_build_output(state: &JobState<'_>, output: &BuildOutput, package_id: Pac env: &output.env, } .to_json_string(); - state.stdout(&msg); + state.stdout(msg); } fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult { @@ -248,7 +248,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes ); let build_scripts = super::load_build_deps(cx, unit); let kind = unit.kind; - let json_messages = bcx.build_config.json_messages(); + let json_messages = bcx.build_config.emit_json(); let extra_verbose = bcx.config.extra_verbose(); let (prev_output, prev_script_out_dir) = prev_build_output(cx, unit); @@ -315,13 +315,13 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes .exec_with_streaming( &mut |stdout| { if extra_verbose { - state.stdout(&format!("{}{}", prefix, stdout)); + state.stdout(format!("{}{}", prefix, stdout)); } Ok(()) }, &mut |stderr| { if extra_verbose { - state.stderr(&format!("{}{}", prefix, stderr)); + state.stderr(format!("{}{}", prefix, stderr)); } Ok(()) }, diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index c78c55123ee..3a4f7d7ef49 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -105,12 +105,12 @@ impl<'a> JobState<'a> { .send(Message::BuildPlanMsg(module_name, cmd, filenames)); } - pub fn stdout(&self, stdout: &str) { - drop(self.tx.send(Message::Stdout(stdout.to_string()))); + pub fn stdout(&self, stdout: String) { + drop(self.tx.send(Message::Stdout(stdout))); } - pub fn stderr(&self, stderr: &str) { - drop(self.tx.send(Message::Stderr(stderr.to_string()))); + pub fn stderr(&self, stderr: String) { + drop(self.tx.send(Message::Stderr(stderr))); } /// A method used to signal to the coordinator thread that the rmeta file diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 7fc8f1badfd..74e19071a27 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -13,11 +13,13 @@ mod unit; use std::env; use std::ffi::{OsStr, OsString}; -use std::fs; +use std::fs::{self, File}; +use std::io::{BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; use std::sync::Arc; use failure::{bail, Error}; +use lazycell::LazyCell; use log::debug; use same_file::is_same_file; use serde::Serialize; @@ -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()) + { + replay_output_cache( + unit.pkg.package_id(), + unit.target, + cx.files().message_cache_path(unit), + cx.bcx.build_config.message_format, + cx.bcx.config.shell().supports_color(), + ) + } else { + Work::noop() + }; // Need to link targets on both the dirty and fresh. - link_targets(cx, unit, true)? + work.then(link_targets(cx, unit, true)?) }); job @@ -202,6 +217,9 @@ fn rustc<'a, 'cfg>( let dep_info_loc = fingerprint::dep_info_loc(cx, unit); rustc.args(cx.bcx.rustflags_args(unit)); + let emit_json = cx.bcx.build_config.emit_json(); + let mut cache_cell = new_cache_cell(cx, unit); + let color = cx.bcx.config.shell().supports_color(); let package_id = unit.pkg.package_id(); let target = unit.target.clone(); let mode = unit.mode; @@ -218,47 +236,6 @@ fn rustc<'a, 'cfg>( let fingerprint_dir = cx.files().fingerprint_dir(unit); let rmeta_produced = cx.rmeta_required(unit); - // If this unit is producing a required rmeta file then we need to know - // when the rmeta file is ready so we can signal to the rest of Cargo that - // it can continue dependant compilations. To do this we are currently - // required to switch the compiler into JSON message mode, but we still - // want to present human readable errors as well. (this rabbit hole just - // goes and goes) - // - // All that means is that if we're not already in JSON mode we need to - // switch to JSON mode, ensure that rustc error messages can be rendered - // prettily, and then when parsing JSON messages from rustc we need to - // internally understand that we should extract the `rendered` field and - // present it if we can. - let extract_rendered_errors = if rmeta_produced { - match cx.bcx.build_config.message_format { - MessageFormat::Json => { - rustc.arg("-Zemit-artifact-notifications"); - false - } - MessageFormat::Human => { - rustc - .arg("--error-format=json") - .arg("--json-rendered=termcolor") - .arg("-Zunstable-options") - .arg("-Zemit-artifact-notifications"); - true - } - - // FIXME(rust-lang/rust#60419): right now we have no way of turning - // on JSON messages from the compiler and also asking the rendered - // field to be in the `short` format. - MessageFormat::Short => { - bail!( - "currently `--message-format short` is incompatible with \ - pipelined compilation" - ); - } - } - } else { - false - }; - return Ok(Work::new(move |state| { // Only at runtime have we discovered what the extra -L and -l // arguments are for native libraries, so we process those here. We @@ -325,8 +302,10 @@ fn rustc<'a, 'cfg>( line, package_id, &target, - extract_rendered_errors, + !emit_json, rmeta_produced, + color, + &mut cache_cell, ) }, ) @@ -440,7 +419,7 @@ fn link_targets<'a, 'cfg>( .into_iter() .map(|s| s.to_owned()) .collect(); - let json_messages = bcx.build_config.json_messages(); + let json_messages = bcx.build_config.emit_json(); let executable = cx.get_executable(unit)?; let mut target = unit.target.clone(); if let TargetSourcePath::Metabuild = target.src_path() { @@ -500,7 +479,7 @@ fn link_targets<'a, 'cfg>( fresh, } .to_json_string(); - state.stdout(&msg); + state.stdout(msg); } Ok(()) })) @@ -657,7 +636,7 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult rustdoc.arg("--cfg").arg(&format!("feature=\"{}\"", feat)); } - add_error_format(cx, &mut rustdoc); + add_error_format(cx, &mut rustdoc, false, false)?; if let Some(args) = bcx.extra_args_for(unit) { rustdoc.args(args); @@ -670,8 +649,11 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult let name = unit.pkg.name().to_string(); let build_state = cx.build_state.clone(); let key = (unit.pkg.package_id(), unit.kind); + let emit_json = bcx.build_config.emit_json(); + let color = bcx.config.shell().supports_color(); let package_id = unit.pkg.package_id(); let target = unit.target.clone(); + let mut cache_cell = new_cache_cell(cx, unit); Ok(Work::new(move |state| { if let Some(output) = build_state.outputs.lock().unwrap().get(&key) { @@ -687,7 +669,18 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult rustdoc .exec_with_streaming( &mut |line| on_stdout_line(state, line, package_id, &target), - &mut |line| on_stderr_line(state, line, package_id, &target, false, false), + &mut |line| { + on_stderr_line( + state, + line, + package_id, + &target, + !emit_json, + false, + color, + &mut cache_cell, + ) + }, false, ) .chain_err(|| format!("Could not document `{}`.", name))?; @@ -753,16 +746,53 @@ fn add_color(bcx: &BuildContext<'_, '_>, cmd: &mut ProcessBuilder) { cmd.args(&["--color", color]); } -fn add_error_format(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder) { - match cx.bcx.build_config.message_format { - MessageFormat::Human => (), - MessageFormat::Json => { - cmd.arg("--error-format").arg("json"); +fn add_error_format( + cx: &Context<'_, '_>, + cmd: &mut ProcessBuilder, + pipelined: bool, + supports_termcolor: bool, +) -> CargoResult<()> { + // If this unit is producing a required rmeta file then we need to know + // when the rmeta file is ready so we can signal to the rest of Cargo that + // it can continue dependent compilations. To do this we are currently + // required to switch the compiler into JSON message mode, but we still + // want to present human readable errors as well. (this rabbit hole just + // goes and goes) + // + // All that means is that if we're not already in JSON mode we need to + // switch to JSON mode, ensure that rustc error messages can be rendered + // prettily, and then when parsing JSON messages from rustc we need to + // internally understand that we should extract the `rendered` field and + // present it if we can. + if cx.bcx.build_config.cache_messages() || pipelined { + cmd.arg("--error-format=json").arg("-Zunstable-options"); + if supports_termcolor { + cmd.arg("--json-rendered=termcolor"); + } + if pipelined { + cmd.arg("-Zemit-artifact-notifications"); + if cx.bcx.build_config.message_format == MessageFormat::Short { + // FIXME(rust-lang/rust#60419): right now we have no way of + // turning on JSON messages from the compiler and also asking + // the rendered field to be in the `short` format. + bail!( + "currently `--message-format short` is incompatible with \ + pipelined compilation" + ); + } } - MessageFormat::Short => { - cmd.arg("--error-format").arg("short"); + } else { + match cx.bcx.build_config.message_format { + MessageFormat::Human => (), + MessageFormat::Json => { + cmd.arg("--error-format").arg("json"); + } + MessageFormat::Short => { + cmd.arg("--error-format").arg("short"); + } } } + Ok(()) } fn build_base_args<'a, 'cfg>( @@ -792,7 +822,7 @@ fn build_base_args<'a, 'cfg>( add_path_args(bcx, unit, cmd); add_color(bcx, cmd); - add_error_format(cx, cmd); + add_error_format(cx, cmd, cx.rmeta_required(unit), true)?; if !test { for crate_type in crate_types.iter() { @@ -1086,7 +1116,7 @@ fn on_stdout_line( _package_id: PackageId, _target: &Target, ) -> CargoResult<()> { - state.stdout(line); + state.stdout(line.to_string()); Ok(()) } @@ -1097,25 +1127,36 @@ fn on_stderr_line( target: &Target, extract_rendered_messages: bool, look_for_metadata_directive: bool, + color: bool, + cache_cell: &mut Option<(PathBuf, LazyCell)>, ) -> CargoResult<()> { + if let Some((path, cell)) = cache_cell { + // The file is created lazily so that in the normal case, lots of + // empty files are not created. + let f = cell.try_borrow_mut_with(|| File::create(path))?; + f.write_all(line.as_bytes())?; + f.write_all(&[b'\n'])?; + } + // We primarily want to use this function to process JSON messages from // rustc. The compiler should always print one JSON message per line, and // otherwise it may have other output intermingled (think RUST_LOG or // something like that), so skip over everything that doesn't look like a // JSON message. if !line.starts_with('{') { - state.stderr(line); + state.stderr(line.to_string()); return Ok(()); } - let compiler_message: Box = match serde_json::from_str(line) { + let mut compiler_message: Box = match serde_json::from_str(line) { Ok(msg) => msg, // If the compiler produced a line that started with `{` but it wasn't // valid JSON, maybe it wasn't JSON in the first place! Forward it along // to stderr. - Err(_) => { - state.stderr(line); + Err(e) => { + debug!("failed to parse json: {:?}", e); + state.stderr(line.to_string()); return Ok(()); } }; @@ -1130,10 +1171,43 @@ fn on_stderr_line( struct CompilerMessage { rendered: String, } - if let Ok(error) = serde_json::from_str::(compiler_message.get()) { - state.stderr(&error.rendered); + if let Ok(mut error) = serde_json::from_str::(compiler_message.get()) { + // state.stderr will add a newline + if error.rendered.ends_with('\n') { + error.rendered.pop(); + } + let rendered = if color { + error.rendered + } else { + strip_ansi_escapes::strip(&error.rendered) + .map(|v| String::from_utf8(v).expect("utf8")) + .unwrap_or(error.rendered) + }; + state.stderr(rendered); return Ok(()); } + } else { + // Remove color information from the rendered string. rustc has not + // included color in the past, so to avoid breaking anything, strip it + // out when --json-rendered=termcolor is used. This runs + // unconditionally under the assumption that Cargo will eventually + // move to this as the default mode. Perhaps in the future, cargo + // could allow the user to enable/disable color (such as with a + // `--json-rendered` or `--color` or `--message-format` flag). + #[derive(serde::Deserialize, serde::Serialize)] + struct CompilerMessage { + rendered: String, + #[serde(flatten)] + other: std::collections::BTreeMap, + } + if let Ok(mut error) = serde_json::from_str::(compiler_message.get()) { + error.rendered = strip_ansi_escapes::strip(&error.rendered) + .map(|v| String::from_utf8(v).expect("utf8")) + .unwrap_or(error.rendered); + let new_line = serde_json::to_string(&error)?; + let new_msg: Box = serde_json::from_str(&new_line)?; + compiler_message = new_msg; + } } // In some modes of execution we will execute rustc with `-Z @@ -1172,6 +1246,56 @@ fn on_stderr_line( // Switch json lines from rustc/rustdoc that appear on stderr to stdout // instead. We want the stdout of Cargo to always be machine parseable as // stderr has our colorized human-readable messages. - state.stdout(&msg); + state.stdout(msg); Ok(()) } + +fn replay_output_cache( + package_id: PackageId, + target: &Target, + path: PathBuf, + format: MessageFormat, + color: bool, +) -> Work { + let target = target.clone(); + let extract_rendered_messages = match format { + MessageFormat::Human => true, + MessageFormat::Json => false, + // FIXME: short not supported. + MessageFormat::Short => false, + }; + Work::new(move |state| { + if path.exists() { + let mut f = BufReader::new(File::open(&path)?); + let mut line = String::new(); + loop { + if f.read_line(&mut line)? == 0 { + break; + } + on_stderr_line( + state, + &line, + package_id, + &target, + extract_rendered_messages, + false, // look_for_metadata_directive + color, + &mut None, + )?; + line.clear(); + } + } + Ok(()) + }) +} + +fn new_cache_cell(cx: &Context<'_, '_>, unit: &Unit<'_>) -> Option<(PathBuf, LazyCell)> { + if cx.bcx.build_config.cache_messages() { + let path = cx.files().message_cache_path(unit); + // Remove old cache, ignore ENOENT, which is the common case. + drop(fs::remove_file(&path)); + Some((path, LazyCell::new())) + } else { + None + } +} diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 91564645a76..ccb4808ee2e 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -331,6 +331,7 @@ pub struct CliUnstable { pub dual_proc_macros: bool, pub mtime_on_use: bool, pub install_upgrade: bool, + pub cache_messages: bool, } impl CliUnstable { @@ -375,6 +376,7 @@ impl CliUnstable { "dual-proc-macros" => self.dual_proc_macros = true, "mtime-on-use" => self.mtime_on_use = true, "install-upgrade" => self.install_upgrade = true, + "cache-messages" => self.cache_messages = true, _ => failure::bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs index 76452630884..013bb9e1da7 100644 --- a/src/cargo/ops/fix.rs +++ b/src/cargo/ops/fix.rs @@ -592,6 +592,11 @@ impl FixArgs { ret.enabled_edition = Some(s[prefix.len()..].to_string()); continue; } + if s.starts_with("--error-format=") || s.starts_with("--json-rendered=") { + // Cargo may add error-format in some cases, but `cargo + // fix` wants to add its own. + continue; + } } ret.other.push(path.into()); } diff --git a/src/cargo/util/process_builder.rs b/src/cargo/util/process_builder.rs index ddd92be7380..9ac8ff1cbcd 100644 --- a/src/cargo/util/process_builder.rs +++ b/src/cargo/util/process_builder.rs @@ -212,7 +212,10 @@ impl ProcessBuilder { /// /// If any invocations of these function return an error, it will be propagated. /// - /// Optionally, output can be passed to errors using `print_output` + /// If `capture_output` is true, then all the output will also be buffered + /// and stored in the returned `Output` object. If it is false, no caching + /// is done, and the callbacks are solely responsible for handling the + /// output. pub fn exec_with_streaming( &self, on_stdout_line: &mut dyn FnMut(&str) -> CargoResult<()>, diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 227309cd55d..bb2921043ac 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -255,7 +255,7 @@ installed. ### public-dependency * Tracking Issue: [#44663](https://github.com/rust-lang/rust/issues/44663) -The 'public-dependency' features allows marking dependencies as 'public' +The 'public-dependency' feature allows marking dependencies as 'public' or 'private'. When this feature is enabled, additional information is passed to rustc to allow the 'exported_private_dependencies' lint to function properly. @@ -268,3 +268,22 @@ cargo-features = ["public-dependency"] my_dep = { version = "1.2.3", public = true } private_dep = "2.0.0" # Will be 'private' by default ``` + +### cache-messages +* Tracking Issue: TODO + +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 +``` + +This works with any command that runs the compiler (`build`, `check`, `test`, +etc.). + +This also changes the way Cargo interacts with the compiler, helping to +prevent interleaved messages when multiple crates attempt to display a message +at the same time. diff --git a/tests/testsuite/cache_messages.rs b/tests/testsuite/cache_messages.rs new file mode 100644 index 00000000000..900efb9dc89 --- /dev/null +++ b/tests/testsuite/cache_messages.rs @@ -0,0 +1,336 @@ +use crate::support::{is_nightly, process, project, registry::Package}; +use std::path::Path; + +fn as_str(bytes: &[u8]) -> &str { + std::str::from_utf8(bytes).expect("valid utf-8") +} + +#[test] +fn simple() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + // A simple example that generates two warnings (unused functions). + let p = project() + .file( + "src/lib.rs", + " + fn a() {} + fn b() {} + ", + ) + .build(); + + let agnostic_path = Path::new("src").join("lib.rs"); + let agnostic_path_s = agnostic_path.to_str().unwrap(); + + // Capture what rustc actually emits. This is done to avoid relying on the + // exact message formatting in rustc. + let rustc_output = process("rustc") + .cwd(p.root()) + .args(&["--crate-type=lib", agnostic_path_s]) + .exec_with_output() + .expect("rustc to run"); + + assert!(rustc_output.stdout.is_empty()); + assert!(rustc_output.status.success()); + + // -q so the output is the same as rustc (no "Compiling" or "Finished"). + let cargo_output1 = p + .cargo("check -Zcache-messages -q --color=never") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("cargo to run"); + assert_eq!(as_str(&rustc_output.stderr), as_str(&cargo_output1.stderr)); + assert!(cargo_output1.stdout.is_empty()); + // Check that the cached version is exactly the same. + let cargo_output2 = p + .cargo("check -Zcache-messages -q") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("cargo to run"); + assert_eq!(as_str(&rustc_output.stderr), as_str(&cargo_output2.stderr)); + assert!(cargo_output2.stdout.is_empty()); +} + +#[test] +fn color() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + // Check enabling/disabling color. + let p = project().file("src/lib.rs", "fn a() {}").build(); + + let agnostic_path = Path::new("src").join("lib.rs"); + let agnostic_path_s = agnostic_path.to_str().unwrap(); + // Capture the original color output. + let rustc_output = process("rustc") + .cwd(p.root()) + .args(&["--crate-type=lib", agnostic_path_s, "--color=always"]) + .exec_with_output() + .expect("rustc to run"); + assert!(rustc_output.status.success()); + let rustc_color = as_str(&rustc_output.stderr); + assert!(rustc_color.contains("\x1b[")); + + // Capture the original non-color output. + let rustc_output = process("rustc") + .cwd(p.root()) + .args(&["--crate-type=lib", agnostic_path_s]) + .exec_with_output() + .expect("rustc to run"); + let rustc_nocolor = as_str(&rustc_output.stderr); + assert!(!rustc_nocolor.contains("\x1b[")); + + // First pass, non-cached, with color, should be the same. + let cargo_output1 = p + .cargo("check -Zcache-messages -q --color=always") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("cargo to run"); + assert_eq!(rustc_color, as_str(&cargo_output1.stderr)); + + // Replay cached, with color. + let cargo_output2 = p + .cargo("check -Zcache-messages -q --color=always") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("cargo to run"); + assert_eq!(rustc_color, as_str(&cargo_output2.stderr)); + + // Replay cached, no color. + let cargo_output_nocolor = p + .cargo("check -Zcache-messages -q --color=never") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("cargo to run"); + assert_eq!(rustc_nocolor, as_str(&cargo_output_nocolor.stderr)); +} + +#[test] +fn cached_as_json() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + // Check that cached JSON output is the same. + let p = project().file("src/lib.rs", "fn a() {}").build(); + + // Grab the non-cached output, feature disabled. + // NOTE: When stabilizing, this will need to be redone. + let cargo_output = p + .cargo("check --message-format=json") + .exec_with_output() + .expect("cargo to run"); + assert!(cargo_output.status.success()); + let orig_cargo_out = as_str(&cargo_output.stdout); + assert!(orig_cargo_out.contains("compiler-message")); + p.cargo("clean").run(); + + // Check JSON output, not fresh. + let cargo_output1 = p + .cargo("check -Zcache-messages --message-format=json") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("cargo to run"); + assert_eq!(as_str(&cargo_output1.stdout), orig_cargo_out); + + // Check JSON output, fresh. + let cargo_output2 = p + .cargo("check -Zcache-messages --message-format=json") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("cargo to run"); + // The only difference should be this field. + let fix_fresh = as_str(&cargo_output2.stdout).replace("\"fresh\":true", "\"fresh\":false"); + assert_eq!(fix_fresh, orig_cargo_out); +} + +#[test] +fn clears_cache_after_fix() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + // Make sure the cache is invalidated when there is no output. + let p = project().file("src/lib.rs", "fn asdf() {}").build(); + // Fill the cache. + p.cargo("check -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]asdf[..]") + .run(); + let cpath = p + .glob("target/debug/.fingerprint/foo-*/output") + .next() + .unwrap() + .unwrap(); + assert!(std::fs::read_to_string(cpath).unwrap().contains("asdf")); + + // Fix it. + p.change_file("src/lib.rs", ""); + + p.cargo("check -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stdout("") + .with_stderr( + "\ +[CHECKING] foo [..] +[FINISHED] [..] +", + ) + .run(); + assert_eq!(p.glob("target/debug/.fingerprint/foo-*/output").count(), 0); + + // And again, check the cache is correct. + p.cargo("check -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stdout("") + .with_stderr( + "\ +[FINISHED] [..] +", + ) + .run(); +} + +#[test] +fn rustdoc() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + // Create a warning in rustdoc. + let p = project() + .file( + "src/lib.rs", + " + #![warn(private_doc_tests)] + /// asdf + /// ``` + /// let x = 1; + /// ``` + fn f() {} + ", + ) + .build(); + + // At this time, rustdoc does not support --json-rendered=termcolor. So it + // will always be uncolored with -Zcache-messages. + let rustdoc_output = p + .cargo("doc -Zcache-messages -q") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("rustdoc to run"); + assert!(rustdoc_output.status.success()); + let rustdoc_stderr = as_str(&rustdoc_output.stderr); + assert!(rustdoc_stderr.contains("private")); + // Invert this when --json-rendered is added. + assert!(!rustdoc_stderr.contains("\x1b[")); + assert_eq!(p.glob("target/debug/.fingerprint/foo-*/output").count(), 1); + + // Check the cached output. + let rustdoc_output = p + .cargo("doc -Zcache-messages -q") + .masquerade_as_nightly_cargo() + .exec_with_output() + .expect("rustdoc to run"); + assert_eq!(as_str(&rustdoc_output.stderr), rustdoc_stderr); +} + +#[test] +fn clippy() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() { + eprintln!("clippy-driver not available, skipping clippy test"); + eprintln!("{:?}", e); + return; + } + + // Caching clippy output. + // This is just a random clippy lint (assertions_on_constants) that + // hopefully won't change much in the future. + let p = project() + .file("src/lib.rs", "pub fn f() { assert!(true); }") + .build(); + + p.cargo("clippy-preview -Zunstable-options -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") + .run(); + + // Again, reading from the cache. + p.cargo("clippy-preview -Zunstable-options -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") + .run(); + + // FIXME: Unfortunately clippy is sharing the same hash with check. This + // causes the cache to be reused when it shouldn't. + p.cargo("check -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]assert!(true)[..]") // This should not be here. + .run(); +} + +#[test] +fn fix() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + // Make sure `fix` is not broken by caching. + let p = project().file("src/lib.rs", "pub fn try() {}").build(); + + p.cargo("fix --edition --allow-no-vcs -Zcache-messages") + .masquerade_as_nightly_cargo() + .run(); + + assert_eq!(p.read_file("src/lib.rs"), "pub fn r#try() {}"); +} + +#[test] +fn very_verbose() { + if !is_nightly() { + // --json-rendered is unstable + return; + } + // Handle cap-lints in dependencies. + Package::new("bar", "1.0.0") + .file("src/lib.rs", "fn not_used() {}") + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check -Zcache-messages -vv") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]not_used[..]") + .run(); + + p.cargo("check -Zcache-messages") + .masquerade_as_nightly_cargo() + .with_stderr("[FINISHED] [..]") + .run(); + + p.cargo("check -Zcache-messages -vv") + .masquerade_as_nightly_cargo() + .with_stderr_contains("[..]not_used[..]") + .run(); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 5dcf126ec82..a34277f3414 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -19,6 +19,7 @@ mod build_lib; mod build_plan; mod build_script; mod build_script_env; +mod cache_messages; mod cargo_alias_config; mod cargo_command; mod cargo_features; From f6d30e91558bf9dda50156fc419282986b1f8b41 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 16 May 2019 12:52:03 -0700 Subject: [PATCH 2/4] Cache-messages: address some review comments. - Add some comments and cleanup. - Error out when using `short`. --- src/cargo/core/compiler/build_config.rs | 5 -- src/cargo/core/compiler/mod.rs | 66 ++++++++++++++----------- tests/testsuite/cache_messages.rs | 12 +++++ 3 files changed, 48 insertions(+), 35 deletions(-) diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs index 29555a712a4..c2c3d76bd7c 100644 --- a/src/cargo/core/compiler/build_config.rs +++ b/src/cargo/core/compiler/build_config.rs @@ -107,11 +107,6 @@ impl BuildConfig { /// Whether or not Cargo should cache compiler messages on disk. pub fn cache_messages(&self) -> bool { self.cache_messages - && match self.message_format { - MessageFormat::Human | MessageFormat::Json => true, - // short is currently not supported - MessageFormat::Short => false, - } } /// Whether or not the *user* wants JSON output. Whether or not rustc diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 74e19071a27..103c4549ff6 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -14,7 +14,7 @@ mod unit; use std::env; use std::ffi::{OsStr, OsString}; use std::fs::{self, File}; -use std::io::{BufRead, BufReader, Write}; +use std::io::Write; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -769,17 +769,21 @@ fn add_error_format( if supports_termcolor { cmd.arg("--json-rendered=termcolor"); } + if cx.bcx.build_config.message_format == MessageFormat::Short { + // FIXME(rust-lang/rust#60419): right now we have no way of + // turning on JSON messages from the compiler and also asking + // the rendered field to be in the `short` format. + bail!( + "currently `--message-format short` is incompatible with {}", + if pipelined { + "pipelined compilation" + } else { + "cached output" + } + ); + } if pipelined { cmd.arg("-Zemit-artifact-notifications"); - if cx.bcx.build_config.message_format == MessageFormat::Short { - // FIXME(rust-lang/rust#60419): right now we have no way of - // turning on JSON messages from the compiler and also asking - // the rendered field to be in the `short` format. - bail!( - "currently `--message-format short` is incompatible with \ - pipelined compilation" - ); - } } } else { match cx.bcx.build_config.message_format { @@ -1130,10 +1134,13 @@ fn on_stderr_line( color: bool, cache_cell: &mut Option<(PathBuf, LazyCell)>, ) -> CargoResult<()> { + // Check if caching is enabled. if let Some((path, cell)) = cache_cell { + // Cache the output, which will be replayed later when Fresh. // The file is created lazily so that in the normal case, lots of // empty files are not created. let f = cell.try_borrow_mut_with(|| File::create(path))?; + debug_assert!(!line.contains('\n')); f.write_all(line.as_bytes())?; f.write_all(&[b'\n'])?; } @@ -1179,9 +1186,11 @@ fn on_stderr_line( let rendered = if color { error.rendered } else { + // Strip only fails if the the Writer fails, which is Cursor + // on a Vec, which should never fail. strip_ansi_escapes::strip(&error.rendered) .map(|v| String::from_utf8(v).expect("utf8")) - .unwrap_or(error.rendered) + .expect("strip should never fail") }; state.stderr(rendered); return Ok(()); @@ -1265,25 +1274,22 @@ fn replay_output_cache( MessageFormat::Short => false, }; Work::new(move |state| { - if path.exists() { - let mut f = BufReader::new(File::open(&path)?); - let mut line = String::new(); - loop { - if f.read_line(&mut line)? == 0 { - break; - } - on_stderr_line( - state, - &line, - package_id, - &target, - extract_rendered_messages, - false, // look_for_metadata_directive - color, - &mut None, - )?; - line.clear(); - } + if !path.exists() { + // No cached output, probably didn't emit anything. + return Ok(()); + } + let contents = fs::read_to_string(&path)?; + for line in contents.lines() { + on_stderr_line( + state, + &line, + package_id, + &target, + extract_rendered_messages, + false, // look_for_metadata_directive + color, + &mut None, + )?; } Ok(()) }) diff --git a/tests/testsuite/cache_messages.rs b/tests/testsuite/cache_messages.rs index 900efb9dc89..c86e9a279cc 100644 --- a/tests/testsuite/cache_messages.rs +++ b/tests/testsuite/cache_messages.rs @@ -334,3 +334,15 @@ fn very_verbose() { .with_stderr_contains("[..]not_used[..]") .run(); } + +#[test] +fn short_incompatible() { + let p = project().file("src/lib.rs", "").build(); + p.cargo("check -Zcache-messages --message-format=short") + .masquerade_as_nightly_cargo() + .with_stderr( + "[ERROR] currently `--message-format short` is incompatible with cached output", + ) + .with_status(101) + .run(); +} From 342f860829e9f9670e06e7dddec3049e3c8a2982 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 18 May 2019 18:16:57 -0700 Subject: [PATCH 3/4] Pull out on_stderr_line options into a struct. --- src/cargo/core/compiler/mod.rs | 115 +++++++++++++++------------------ 1 file changed, 53 insertions(+), 62 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 103c4549ff6..4e2e095a589 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -217,9 +217,7 @@ fn rustc<'a, 'cfg>( let dep_info_loc = fingerprint::dep_info_loc(cx, unit); rustc.args(cx.bcx.rustflags_args(unit)); - let emit_json = cx.bcx.build_config.emit_json(); - let mut cache_cell = new_cache_cell(cx, unit); - let color = cx.bcx.config.shell().supports_color(); + let mut output_options = OutputOptions::new(cx, unit); let package_id = unit.pkg.package_id(); let target = unit.target.clone(); let mode = unit.mode; @@ -234,7 +232,6 @@ fn rustc<'a, 'cfg>( .unwrap_or_else(|| cx.bcx.config.cwd()) .to_path_buf(); let fingerprint_dir = cx.files().fingerprint_dir(unit); - let rmeta_produced = cx.rmeta_required(unit); return Ok(Work::new(move |state| { // Only at runtime have we discovered what the extra -L and -l @@ -296,18 +293,7 @@ fn rustc<'a, 'cfg>( &target, mode, &mut |line| on_stdout_line(state, line, package_id, &target), - &mut |line| { - on_stderr_line( - state, - line, - package_id, - &target, - !emit_json, - rmeta_produced, - color, - &mut cache_cell, - ) - }, + &mut |line| on_stderr_line(state, line, package_id, &target, &mut output_options), ) .map_err(internal_if_simple_exit_code) .chain_err(|| format!("Could not compile `{}`.", name))?; @@ -649,11 +635,9 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult let name = unit.pkg.name().to_string(); let build_state = cx.build_state.clone(); let key = (unit.pkg.package_id(), unit.kind); - let emit_json = bcx.build_config.emit_json(); - let color = bcx.config.shell().supports_color(); let package_id = unit.pkg.package_id(); let target = unit.target.clone(); - let mut cache_cell = new_cache_cell(cx, unit); + let mut output_options = OutputOptions::new(cx, unit); Ok(Work::new(move |state| { if let Some(output) = build_state.outputs.lock().unwrap().get(&key) { @@ -669,18 +653,7 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult rustdoc .exec_with_streaming( &mut |line| on_stdout_line(state, line, package_id, &target), - &mut |line| { - on_stderr_line( - state, - line, - package_id, - &target, - !emit_json, - false, - color, - &mut cache_cell, - ) - }, + &mut |line| on_stderr_line(state, line, package_id, &target, &mut output_options), false, ) .chain_err(|| format!("Could not document `{}`.", name))?; @@ -1114,6 +1087,43 @@ impl Kind { } } +struct OutputOptions { + /// Get the `"rendered"` field from the JSON output and display it on + /// stderr instead of the JSON message. + extract_rendered_messages: bool, + /// Look for JSON message that indicates .rmeta file is available for + /// pipelined compilation. + look_for_metadata_directive: bool, + /// Whether or not to display messages in color. + color: bool, + /// Where to write the JSON messages to support playback later if the unit + /// is fresh. The file is created lazily so that in the normal case, lots + /// of empty files are not created. This is None if caching is disabled. + cache_cell: Option<(PathBuf, LazyCell)>, +} + +impl OutputOptions { + fn new<'a>(cx: &Context<'a, '_>, unit: &Unit<'a>) -> OutputOptions { + let extract_rendered_messages = cx.bcx.build_config.message_format != MessageFormat::Json; + let look_for_metadata_directive = cx.rmeta_required(unit); + let color = cx.bcx.config.shell().supports_color(); + let cache_cell = if cx.bcx.build_config.cache_messages() { + let path = cx.files().message_cache_path(unit); + // Remove old cache, ignore ENOENT, which is the common case. + drop(fs::remove_file(&path)); + Some((path, LazyCell::new())) + } else { + None + }; + OutputOptions { + extract_rendered_messages, + look_for_metadata_directive, + color, + cache_cell, + } + } +} + fn on_stdout_line( state: &JobState<'_>, line: &str, @@ -1129,16 +1139,11 @@ fn on_stderr_line( line: &str, package_id: PackageId, target: &Target, - extract_rendered_messages: bool, - look_for_metadata_directive: bool, - color: bool, - cache_cell: &mut Option<(PathBuf, LazyCell)>, + options: &mut OutputOptions, ) -> CargoResult<()> { // Check if caching is enabled. - if let Some((path, cell)) = cache_cell { + if let Some((path, cell)) = &mut options.cache_cell { // Cache the output, which will be replayed later when Fresh. - // The file is created lazily so that in the normal case, lots of - // empty files are not created. let f = cell.try_borrow_mut_with(|| File::create(path))?; debug_assert!(!line.contains('\n')); f.write_all(line.as_bytes())?; @@ -1173,7 +1178,7 @@ fn on_stderr_line( // colorized diagnostics. In those cases (`extract_rendered_messages`) we // take a look at the JSON blob we go, see if it's a relevant diagnostics, // and if so forward just that diagnostic for us to print. - if extract_rendered_messages { + if options.extract_rendered_messages { #[derive(serde::Deserialize)] struct CompilerMessage { rendered: String, @@ -1183,7 +1188,7 @@ fn on_stderr_line( if error.rendered.ends_with('\n') { error.rendered.pop(); } - let rendered = if color { + let rendered = if options.color { error.rendered } else { // Strip only fails if the the Writer fails, which is Cursor @@ -1227,7 +1232,7 @@ fn on_stderr_line( // // In these cases look for a matching directive and inform Cargo internally // that a metadata file has been produced. - if look_for_metadata_directive { + if options.look_for_metadata_directive { #[derive(serde::Deserialize)] struct ArtifactNotification { artifact: String, @@ -1273,6 +1278,12 @@ fn replay_output_cache( // FIXME: short not supported. MessageFormat::Short => false, }; + let mut options = OutputOptions { + extract_rendered_messages, + look_for_metadata_directive: false, + color, + cache_cell: None, + }; Work::new(move |state| { if !path.exists() { // No cached output, probably didn't emit anything. @@ -1280,28 +1291,8 @@ fn replay_output_cache( } let contents = fs::read_to_string(&path)?; for line in contents.lines() { - on_stderr_line( - state, - &line, - package_id, - &target, - extract_rendered_messages, - false, // look_for_metadata_directive - color, - &mut None, - )?; + on_stderr_line(state, &line, package_id, &target, &mut options)?; } Ok(()) }) } - -fn new_cache_cell(cx: &Context<'_, '_>, unit: &Unit<'_>) -> Option<(PathBuf, LazyCell)> { - if cx.bcx.build_config.cache_messages() { - let path = cx.files().message_cache_path(unit); - // Remove old cache, ignore ENOENT, which is the common case. - drop(fs::remove_file(&path)); - Some((path, LazyCell::new())) - } else { - None - } -} From 9ba68125fcc80164714e0aff140d08ecfafbbf58 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 20 May 2019 09:40:12 -0700 Subject: [PATCH 4/4] Add some more comments on add_error_format. --- src/cargo/core/compiler/mod.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 4e2e095a589..e8d14ec220b 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -719,6 +719,24 @@ fn add_color(bcx: &BuildContext<'_, '_>, cmd: &mut ProcessBuilder) { cmd.args(&["--color", color]); } +/// Add error-format flags to the command. +/// +/// This is rather convoluted right now. The general overview is: +/// - If -Zcache-messages or `build.pipelining` is enabled, Cargo always uses +/// JSON output. This has several benefits, such as being easier to parse, +/// handles changing formats (for replaying cached messages), ensures +/// atomic output (so messages aren't interleaved), etc. +/// - `supports_termcolor` is a temporary flag. rustdoc does not yet support +/// the `--json-rendered` flag, but it is intended to fix that soon. +/// - `short` output is not yet supported for JSON output. We haven't yet +/// decided how this problem will be resolved. Probably either adding +/// "short" to the JSON output, or more ambitiously moving diagnostic +/// rendering to an external library that Cargo can share with rustc. +/// +/// It is intended in the future that Cargo *always* uses the JSON output, and +/// this function can be simplified. The above issues need to be resolved, the +/// flags need to be stabilized, and we need more testing to ensure there +/// aren't any regressions. fn add_error_format( cx: &Context<'_, '_>, cmd: &mut ProcessBuilder,