Auto merge of #7214 - alexcrichton:json-diagnostics, r=ehuss

Add support for customizing JSON diagnostics from Cargo

Cargo has of #7143 enabled pipelined compilation by default which
affects how the compiler is invoked, especially with respect to JSON
messages. This, in some testing, has proven to cause quite a few issues
with rustbuild's current integration with Cargo. This commit is aimed at
adding features to Cargo to solve this issue.

This commit adds the ability to customize the stream of JSON messages
coming from Cargo. The new feature for Cargo is that it now also mirrors
rustc in how you can configure the JSON stream. Multiple
`--message-format` arguments are now supported and the value specified
is a comma-separated list of directives. In addition to the existing
`human`, `short`, and `json` directives these new directives have been
added:

* `json-render-diagnostics` - instructs Cargo to render rustc
  diagnostics and only print out JSON messages for artifacts and Cargo
  things.

* `json-diagnostic-short` - indicates that the `rendered` field of rustc
  diagnostics should use the "short" rendering.

* `json-diagnostic-rendered-ansi` - indicates that the `rendered` field of rustc
  diagnostics should embed ansi color codes.

The first option here, `json-render-diagnostics`, will be used by
rustbuild unconditionally. Additionally `json-diagnostic-short` will be
conditionally used based on the input to rustbuild itself.

This should be enough for external tools to customize how Cargo is
invoked and how all kinds of JSON diagnostics get printed, and it's
thought that we can relatively easily tweak this as necessary to extend
it and such.

Author: bors

CHANGELOG.md

@@ -5,10 +5,20 @@
 
 ### Added
 
+- Cargo build pipelining has been enabled by default to leverage more idle CPU
+  parallelism during builds.
+  [#7143](https://github.com/rust-lang/cargo/pull/7143)
+- The `--message-format` option to Cargo can now be specified multiple times and
+  accepts a comma-separated list of values. In addition to the previous values
+  it also now accepts `json-diagnostic-short` and
+  `json-diagnostic-rendered-ansi` which configures the output coming from rustc
+  in `json` message mode.
+  [#7214](https://github.com/rust-lang/cargo/pull/7214)
+
 ### Changed
 
 ### Fixed
-- (Nightly only): Fixed exponential blowup when using CARGO_BUILD_PIPELINING.
+- (Nightly only): Fixed exponential blowup when using `CARGO_BUILD_PIPELINING`.
   [#7062](https://github.com/rust-lang/cargo/pull/7062)
 - Fixed using the wrong directory when updating git repositories when using
   the `git-fetch-with-cli` config option, and the `GIT_DIR` environment

src/cargo/core/compiler/build_config.rs

@@ -112,7 +112,10 @@ impl BuildConfig {
     /// 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
+        match self.message_format {
+            MessageFormat::Json { .. } => true,
+            _ => false,
+        }
     }
 
     pub fn test(&self) -> bool {
@@ -123,7 +126,17 @@ impl BuildConfig {
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum MessageFormat {
     Human,
-    Json,
+    Json {
+        /// Whether rustc diagnostics are rendered by cargo or included into the
+        /// output stream.
+        render_diagnostics: bool,
+        /// Whether the `rendered` field of rustc diagnostics are using the
+        /// "short" rendering.
+        short: bool,
+        /// Whether the `rendered` field of rustc diagnostics embed ansi color
+        /// codes.
+        ansi: bool,
+    },
     Short,
 }
 

src/cargo/core/compiler/mod.rs

@@ -750,27 +750,47 @@ fn add_error_format_and_color(
         if pipelined {
             json.push_str(",artifacts");
         }
-        if cx.bcx.build_config.message_format == MessageFormat::Short {
-            json.push_str(",diagnostic-short");
+        match cx.bcx.build_config.message_format {
+            MessageFormat::Short | MessageFormat::Json { short: true, .. } => {
+                json.push_str(",diagnostic-short");
+            }
+            _ => {}
         }
         cmd.arg(json);
     } else {
+        let mut color = true;
         match cx.bcx.build_config.message_format {
             MessageFormat::Human => (),
-            MessageFormat::Json => {
+            MessageFormat::Json {
+                ansi,
+                short,
+                render_diagnostics,
+            } => {
                 cmd.arg("--error-format").arg("json");
+                // If ansi is explicitly requested, enable it. If we're
+                // rendering diagnostics ourselves then also enable it because
+                // we'll figure out what to do with the colors later.
+                if ansi || render_diagnostics {
+                    cmd.arg("--json=diagnostic-rendered-ansi");
+                }
+                if short {
+                    cmd.arg("--json=diagnostic-short");
+                }
+                color = false;
             }
             MessageFormat::Short => {
                 cmd.arg("--error-format").arg("short");
             }
         }
 
-        let color = if cx.bcx.config.shell().supports_color() {
-            "always"
-        } else {
-            "never"
-        };
-        cmd.args(&["--color", color]);
+        if color {
+            let color = if cx.bcx.config.shell().supports_color() {
+                "always"
+            } else {
+                "never"
+            };
+            cmd.args(&["--color", color]);
+        }
     }
     Ok(())
 }
@@ -1090,9 +1110,8 @@ 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,
+    /// What format we're emitting from Cargo itself.
+    format: MessageFormat,
     /// Look for JSON message that indicates .rmeta file is available for
     /// pipelined compilation.
     look_for_metadata_directive: bool,
@@ -1106,7 +1125,6 @@ struct OutputOptions {
 
 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() {
@@ -1118,7 +1136,7 @@ impl OutputOptions {
             None
         };
         OutputOptions {
-            extract_rendered_messages,
+            format: cx.bcx.build_config.message_format,
             look_for_metadata_directive,
             color,
             cache_cell,
@@ -1175,55 +1193,66 @@ fn on_stderr_line(
         }
     };
 
-    // In some modes of compilation Cargo switches the compiler to JSON mode
-    // but the user didn't request that so we still want to print pretty rustc
-    // 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 options.extract_rendered_messages {
-        #[derive(serde::Deserialize)]
-        struct CompilerMessage {
-            rendered: String,
+    // Depending on what we're emitting from Cargo itself, we figure out what to
+    // do with this JSON message.
+    match options.format {
+        // In the "human" output formats (human/short) or if diagnostic messages
+        // from rustc aren't being included in the output of Cargo's JSON
+        // messages then we extract the diagnostic (if present) here and handle
+        // it ourselves.
+        MessageFormat::Human
+        | MessageFormat::Short
+        | MessageFormat::Json {
+            render_diagnostics: true,
+            ..
+        } => {
+            #[derive(serde::Deserialize)]
+            struct CompilerMessage {
+                rendered: String,
+            }
+            if let Ok(mut error) = serde_json::from_str::<CompilerMessage>(compiler_message.get()) {
+                // state.stderr will add a newline
+                if error.rendered.ends_with('\n') {
+                    error.rendered.pop();
+                }
+                let rendered = if options.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"))
+                        .expect("strip should never fail")
+                };
+                state.stderr(rendered);
+                return Ok(());
+            }
         }
-        if let Ok(mut error) = serde_json::from_str::<CompilerMessage>(compiler_message.get()) {
-            // state.stderr will add a newline
-            if error.rendered.ends_with('\n') {
-                error.rendered.pop();
+
+        // Remove color information from the rendered string. When pipelining is
+        // enabled and/or when cached messages are enabled we're always asking
+        // for ANSI colors from rustc, so unconditionally postprocess here and
+        // remove ansi color codes.
+        MessageFormat::Json { ansi: false, .. } => {
+            #[derive(serde::Deserialize, serde::Serialize)]
+            struct CompilerMessage {
+                rendered: String,
+                #[serde(flatten)]
+                other: std::collections::BTreeMap<String, serde_json::Value>,
             }
-            let rendered = if options.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)
+            if let Ok(mut error) = serde_json::from_str::<CompilerMessage>(compiler_message.get()) {
+                error.rendered = strip_ansi_escapes::strip(&error.rendered)
                     .map(|v| String::from_utf8(v).expect("utf8"))
-                    .expect("strip should never fail")
-            };
-            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=diagnostic-rendered-ansi 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` or `--color` or `--message-format` flag).
-        #[derive(serde::Deserialize, serde::Serialize)]
-        struct CompilerMessage {
-            rendered: String,
-            #[serde(flatten)]
-            other: std::collections::BTreeMap<String, serde_json::Value>,
-        }
-        if let Ok(mut error) = serde_json::from_str::<CompilerMessage>(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::value::RawValue> = serde_json::from_str(&new_line)?;
-            compiler_message = new_msg;
+                    .unwrap_or(error.rendered);
+                let new_line = serde_json::to_string(&error)?;
+                let new_msg: Box<serde_json::value::RawValue> = serde_json::from_str(&new_line)?;
+                compiler_message = new_msg;
+            }
         }
+
+        // If ansi colors are desired then we should be good to go! We can just
+        // pass through this message as-is.
+        MessageFormat::Json { ansi: true, .. } => {}
     }
 
     // In some modes of execution we will execute rustc with `-Z
@@ -1274,12 +1303,8 @@ fn replay_output_cache(
     color: bool,
 ) -> Work {
     let target = target.clone();
-    let extract_rendered_messages = match format {
-        MessageFormat::Human | MessageFormat::Short => true,
-        MessageFormat::Json => false,
-    };
     let mut options = OutputOptions {
-        extract_rendered_messages,
+        format,
         look_for_metadata_directive: false,
         color,
         cache_cell: None,

src/cargo/util/command_prelude.rs

@@ -1,7 +1,3 @@
-use std::ffi::{OsStr, OsString};
-use std::fs;
-use std::path::PathBuf;
-
 use crate::core::compiler::{BuildConfig, MessageFormat};
 use crate::core::Workspace;
 use crate::ops::{CompileFilter, CompileOptions, NewOptions, Packages, VersionControl};
@@ -14,6 +10,10 @@ use crate::util::{
 };
 use crate::CargoResult;
 use clap::{self, SubCommand};
+use failure::bail;
+use std::ffi::{OsStr, OsString};
+use std::fs;
+use std::path::PathBuf;
 
 pub use crate::core::compiler::CompileMode;
 pub use crate::{CliError, CliResult, Config};
@@ -134,13 +134,7 @@ pub trait AppExt: Sized {
     }
 
     fn arg_message_format(self) -> Self {
-        self._arg(
-            opt("message-format", "Error format")
-                .value_name("FMT")
-                .case_insensitive(true)
-                .possible_values(&["human", "json", "short"])
-                .default_value("human"),
-        )
+        self._arg(multi_opt("message-format", "FMT", "Error format"))
     }
 
     fn arg_build_plan(self) -> Self {
@@ -301,23 +295,70 @@ pub trait ArgMatchesExt {
             self._values_of("package"),
         )?;
 
-        let message_format = match self._value_of("message-format") {
-            None => MessageFormat::Human,
-            Some(f) => {
-                if f.eq_ignore_ascii_case("json") {
-                    MessageFormat::Json
-                } else if f.eq_ignore_ascii_case("human") {
-                    MessageFormat::Human
-                } else if f.eq_ignore_ascii_case("short") {
-                    MessageFormat::Short
-                } else {
-                    panic!("Impossible message format: {:?}", f)
+        let mut message_format = None;
+        let default_json = MessageFormat::Json {
+            short: false,
+            ansi: false,
+            render_diagnostics: false,
+        };
+        for fmt in self._values_of("message-format") {
+            for fmt in fmt.split(',') {
+                let fmt = fmt.to_ascii_lowercase();
+                match fmt.as_str() {
+                    "json" => {
+                        if message_format.is_some() {
+                            bail!("cannot specify two kinds of `message-format` arguments");
+                        }
+                        message_format = Some(default_json);
+                    }
+                    "human" => {
+                        if message_format.is_some() {
+                            bail!("cannot specify two kinds of `message-format` arguments");
+                        }
+                        message_format = Some(MessageFormat::Human);
+                    }
+                    "short" => {
+                        if message_format.is_some() {
+                            bail!("cannot specify two kinds of `message-format` arguments");
+                        }
+                        message_format = Some(MessageFormat::Short);
+                    }
+                    "json-render-diagnostics" => {
+                        if message_format.is_none() {
+                            message_format = Some(default_json);
+                        }
+                        match &mut message_format {
+                            Some(MessageFormat::Json {
+                                render_diagnostics, ..
+                            }) => *render_diagnostics = true,
+                            _ => bail!("cannot specify two kinds of `message-format` arguments"),
+                        }
+                    }
+                    "json-diagnostic-short" => {
+                        if message_format.is_none() {
+                            message_format = Some(default_json);
+                        }
+                        match &mut message_format {
+                            Some(MessageFormat::Json { short, .. }) => *short = true,
+                            _ => bail!("cannot specify two kinds of `message-format` arguments"),
+                        }
+                    }
+                    "json-diagnostic-rendered-ansi" => {
+                        if message_format.is_none() {
+                            message_format = Some(default_json);
+                        }
+                        match &mut message_format {
+                            Some(MessageFormat::Json { ansi, .. }) => *ansi = true,
+                            _ => bail!("cannot specify two kinds of `message-format` arguments"),
+                        }
+                    }
+                    s => bail!("invalid message format specifier: `{}`", s),
                 }
             }
-        };
+        }
 
         let mut build_config = BuildConfig::new(config, self.jobs()?, &self.target(), mode)?;
-        build_config.message_format = message_format;
+        build_config.message_format = message_format.unwrap_or(MessageFormat::Human);
         build_config.release = self._is_present("release");
         build_config.build_plan = self._is_present("build-plan");
         if build_config.build_plan {