Enable pipelined compilation by default

alexcrichton · alexcrichton · commit daa1bce28340 · 2019-07-31T14:56:24.000-07:00
This commit enables pipelined compilation by default in Cargo now that the requisite support has been stablized in rust-lang/rust#62766. This involved minor updates in a number of locations here and there, but nothing of meat has changed from the original implementation (just tweaks to how rustc is called).
diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs
@@ -34,6 +34,7 @@ pub struct TargetInfo {
     pub rustflags: Vec<String>,
     /// Extra flags to pass to `rustdoc`, see `env_args`.
     pub rustdocflags: Vec<String>,
+    pub supports_pipelining: Option<bool>,
 }
 
 /// Kind of each file generated by a Unit, part of `FileType`.
@@ -98,6 +99,18 @@ impl TargetInfo {
             .args(&rustflags)
             .env_remove("RUSTC_LOG");
 
+        // NOTE: set this unconditionally to `true` once support for `--json`
+        // rides to stable.
+        //
+        // Also note that we only learn about this functionality for the host
+        // compiler since the host/target rustc are always the same.
+        let mut pipelining_test = process.clone();
+        pipelining_test.args(&["--error-format=json", "--json=artifacts"]);
+        let supports_pipelining = match kind {
+            Kind::Host => Some(rustc.cached_output(&pipelining_test).is_ok()),
+            Kind::Target => None,
+        };
+
         let target_triple = requested_target
             .as_ref()
             .map(|s| s.as_str())
@@ -179,6 +192,7 @@ impl TargetInfo {
                 "RUSTDOCFLAGS",
             )?,
             cfg,
+            supports_pipelining,
         })
     }
 
diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs
@@ -77,7 +77,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             .config
             .get_bool("build.pipelining")?
             .map(|t| t.val)
-            .unwrap_or(false);
+            .unwrap_or(bcx.host_info.supports_pipelining.unwrap());
 
         Ok(Self {
             bcx,
diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs
@@ -18,7 +18,7 @@ use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
-use failure::{bail, Error};
+use failure::Error;
 use lazycell::LazyCell;
 use log::debug;
 use same_file::is_same_file;
@@ -614,7 +614,6 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult
     rustdoc.arg("--crate-name").arg(&unit.target.crate_name());
     add_path_args(bcx, unit, &mut rustdoc);
     add_cap_lints(bcx, unit, &mut rustdoc);
-    add_color(bcx, &mut rustdoc);
 
     if unit.kind != Kind::Host {
         if let Some(ref target) = bcx.build_config.requested_target {
@@ -635,7 +634,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, false, false)?;
+    add_error_format_and_color(cx, &mut rustdoc, false)?;
 
     if let Some(args) = bcx.extra_args_for(unit) {
         rustdoc.args(args);
@@ -722,39 +721,20 @@ fn add_cap_lints(bcx: &BuildContext<'_, '_>, unit: &Unit<'_>, cmd: &mut ProcessB
     }
 }
 
-fn add_color(bcx: &BuildContext<'_, '_>, cmd: &mut ProcessBuilder) {
-    let shell = bcx.config.shell();
-    let color = if shell.supports_color() {
-        "always"
-    } else {
-        "never"
-    };
-    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.
+/// This is somewhat odd right now, but the general overview is that if
+/// `-Zcache-messages` or `pipelined` is enabled then 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.
 ///
-/// 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(
+/// It is intended in the future that Cargo *always* uses the JSON output (by
+/// turning on cache-messages by default), and this function can be simplified.
+fn add_error_format_and_color(
     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
@@ -769,26 +749,15 @@ fn add_error_format(
     // 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");
+        cmd.arg("--error-format=json");
+        let mut json = String::from("--json=diagnostic-rendered-ansi");
+        if pipelined {
+            json.push_str(",artifacts");
         }
         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");
+            json.push_str(",diagnostic-short");
         }
+        cmd.arg(json);
     } else {
         match cx.bcx.build_config.message_format {
             MessageFormat::Human => (),
@@ -799,6 +768,13 @@ fn add_error_format(
                 cmd.arg("--error-format").arg("short");
             }
         }
+
+        let color = if cx.bcx.config.shell().supports_color() {
+            "always"
+        } else {
+            "never"
+        };
+        cmd.args(&["--color", color]);
     }
     Ok(())
 }
@@ -829,8 +805,7 @@ fn build_base_args<'a, 'cfg>(
     cmd.arg("--crate-name").arg(&unit.target.crate_name());
 
     add_path_args(bcx, unit, cmd);
-    add_color(bcx, cmd);
-    add_error_format(cx, cmd, cx.rmeta_required(unit), true)?;
+    add_error_format_and_color(cx, cmd, cx.rmeta_required(unit))?;
 
     if !test {
         for crate_type in crate_types.iter() {
@@ -1234,11 +1209,11 @@ fn on_stderr_line(
     } 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
+        // 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-rendered` or `--color` or `--message-format` flag).
+        // `--json` or `--color` or `--message-format` flag).
         #[derive(serde::Deserialize, serde::Serialize)]
         struct CompilerMessage {
             rendered: String,
@@ -1304,10 +1279,8 @@ fn replay_output_cache(
 ) -> Work {
     let target = target.clone();
     let extract_rendered_messages = match format {
-        MessageFormat::Human => true,
+        MessageFormat::Human | MessageFormat::Short => true,
         MessageFormat::Json => false,
-        // FIXME: short not supported.
-        MessageFormat::Short => false,
     };
     let mut options = OutputOptions {
         extract_rendered_messages,
diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs
@@ -621,7 +621,7 @@ impl FixArgs {
                     ret.enabled_edition = Some(s[prefix.len()..].to_string());
                     continue;
                 }
-                if s.starts_with("--error-format=") || s.starts_with("--json-rendered=") {
+                if s.starts_with("--error-format=") || s.starts_with("--json=") {
                     // Cargo may add error-format in some cases, but `cargo
                     // fix` wants to add its own.
                     continue;
diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs
@@ -2155,6 +2155,11 @@ fn flags_go_into_tests() {
 
 #[cargo_test]
 fn diamond_passes_args_only_once() {
+    // FIXME: when pipelining rides to stable, enable this test on all channels.
+    if !crate::support::is_nightly() {
+        return;
+    }
+
     let p = project()
         .file(
             "Cargo.toml",
@@ -2229,7 +2234,7 @@ fn diamond_passes_args_only_once() {
 [COMPILING] a v0.5.0 ([..]
 [RUNNING] `rustc [..]`
 [COMPILING] foo v0.5.0 ([..]
-[RUNNING] `[..]rlib -L native=test`
+[RUNNING] `[..]rmeta -L native=test`
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
 ",
         )
diff --git a/tests/testsuite/cache_messages.rs b/tests/testsuite/cache_messages.rs
@@ -54,6 +54,52 @@ fn simple() {
     assert!(cargo_output2.stdout.is_empty());
 }
 
+// same as `simple`, except everything is using the short format
+#[cargo_test]
+fn simple_short() {
+    if !is_nightly() {
+        // --json-rendered is unstable
+        return;
+    }
+    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();
+
+    let rustc_output = process("rustc")
+        .cwd(p.root())
+        .args(&["--crate-type=lib", agnostic_path_s, "--error-format=short"])
+        .exec_with_output()
+        .expect("rustc to run");
+
+    assert!(rustc_output.stdout.is_empty());
+    assert!(rustc_output.status.success());
+
+    let cargo_output1 = p
+        .cargo("check -Zcache-messages -q --color=never --message-format=short")
+        .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());
+    let cargo_output2 = p
+        .cargo("check -Zcache-messages -q --message-format=short")
+        .masquerade_as_nightly_cargo()
+        .exec_with_output()
+        .expect("cargo to run");
+    println!("{}", String::from_utf8_lossy(&cargo_output2.stdout));
+    assert_eq!(as_str(&rustc_output.stderr), as_str(&cargo_output2.stderr));
+    assert!(cargo_output2.stdout.is_empty());
+}
+
 #[cargo_test]
 fn color() {
     if !is_nightly() {
@@ -334,15 +380,3 @@ fn very_verbose() {
         .with_stderr_contains("[..]not_used[..]")
         .run();
 }
-
-#[cargo_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();
-}
diff --git a/tests/testsuite/dep_info.rs b/tests/testsuite/dep_info.rs
@@ -511,6 +511,6 @@ fn canonical_path() {
     assert_deps_contains(
         &p,
         "target/debug/.fingerprint/foo-*/dep-lib-foo-*",
-        &[(1, "src/lib.rs"), (2, "debug/deps/libregdep-*.rlib")],
+        &[(1, "src/lib.rs"), (2, "debug/deps/libregdep-*.rmeta")],
     );
 }
diff --git a/tests/testsuite/profile_overrides.rs b/tests/testsuite/profile_overrides.rs
@@ -321,17 +321,17 @@ fn profile_override_hierarchy() {
     p.cargo("build -v").masquerade_as_nightly_cargo().with_stderr_unordered("\
 [COMPILING] m3 [..]
 [COMPILING] dep [..]
-[RUNNING] `rustc --crate-name m3 m3/src/lib.rs --color never --crate-type lib --emit=[..]link -C codegen-units=4 [..]
-[RUNNING] `rustc --crate-name dep [..]dep/src/lib.rs --color never --crate-type lib --emit=[..]link -C codegen-units=3 [..]
-[RUNNING] `rustc --crate-name m3 m3/src/lib.rs --color never --crate-type lib --emit=[..]link -C codegen-units=1 [..]
-[RUNNING] `rustc --crate-name build_script_build m1/build.rs --color never --crate-type bin --emit=[..]link -C codegen-units=4 [..]
+[RUNNING] `rustc --crate-name m3 m3/src/lib.rs [..] --crate-type lib --emit=[..]link -C codegen-units=4 [..]
+[RUNNING] `rustc --crate-name dep [..]dep/src/lib.rs [..] --crate-type lib --emit=[..]link -C codegen-units=3 [..]
+[RUNNING] `rustc --crate-name m3 m3/src/lib.rs [..] --crate-type lib --emit=[..]link -C codegen-units=1 [..]
+[RUNNING] `rustc --crate-name build_script_build m1/build.rs [..] --crate-type bin --emit=[..]link -C codegen-units=4 [..]
 [COMPILING] m2 [..]
-[RUNNING] `rustc --crate-name build_script_build m2/build.rs --color never --crate-type bin --emit=[..]link -C codegen-units=2 [..]
+[RUNNING] `rustc --crate-name build_script_build m2/build.rs [..] --crate-type bin --emit=[..]link -C codegen-units=2 [..]
 [RUNNING] `[..]/m1-[..]/build-script-build`
 [RUNNING] `[..]/m2-[..]/build-script-build`
-[RUNNING] `rustc --crate-name m2 m2/src/lib.rs --color never --crate-type lib --emit=[..]link -C codegen-units=2 [..]
+[RUNNING] `rustc --crate-name m2 m2/src/lib.rs [..] --crate-type lib --emit=[..]link -C codegen-units=2 [..]
 [COMPILING] m1 [..]
-[RUNNING] `rustc --crate-name m1 m1/src/lib.rs --color never --crate-type lib --emit=[..]link -C codegen-units=1 [..]
+[RUNNING] `rustc --crate-name m1 m1/src/lib.rs [..] --crate-type lib --emit=[..]link -C codegen-units=1 [..]
 [FINISHED] dev [unoptimized + debuginfo] [..]
 ",
         )
diff --git a/tests/testsuite/rustc_info_cache.rs b/tests/testsuite/rustc_info_cache.rs
@@ -4,6 +4,11 @@ use std::env;
 
 #[cargo_test]
 fn rustc_info_cache() {
+    // FIXME: when pipelining rides to stable, enable this test on all channels.
+    if !crate::support::is_nightly() {
+        return;
+    }
+
     let p = project()
         .file("src/main.rs", r#"fn main() { println!("hello"); }"#)
         .build();
diff --git a/tests/testsuite/rustdoc.rs b/tests/testsuite/rustdoc.rs
@@ -10,6 +10,7 @@ fn rustdoc_simple() {
 [DOCUMENTING] foo v0.0.1 ([CWD])
 [RUNNING] `rustdoc --crate-name foo src/lib.rs [..]\
         -o [CWD]/target/doc \
+        [..] \
         -L dependency=[CWD]/target/debug/deps`
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
 ",
@@ -27,6 +28,7 @@ fn rustdoc_args() {
 [DOCUMENTING] foo v0.0.1 ([CWD])
 [RUNNING] `rustdoc --crate-name foo src/lib.rs [..]\
         -o [CWD]/target/doc \
+        [..] \
         --cfg=foo \
         -L dependency=[CWD]/target/debug/deps`
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
@@ -66,6 +68,7 @@ fn rustdoc_foo_with_bar_dependency() {
 [DOCUMENTING] foo v0.0.1 ([CWD])
 [RUNNING] `rustdoc --crate-name foo src/lib.rs [..]\
         -o [CWD]/target/doc \
+        [..] \
         --cfg=foo \
         -L dependency=[CWD]/target/debug/deps \
         --extern [..]`
@@ -104,6 +107,7 @@ fn rustdoc_only_bar_dependency() {
 [DOCUMENTING] bar v0.0.1 ([..])
 [RUNNING] `rustdoc --crate-name bar [..]bar/src/lib.rs [..]\
         -o [CWD]/target/doc \
+        [..] \
         --cfg=foo \
         -L dependency=[CWD]/target/debug/deps`
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
@@ -125,6 +129,7 @@ fn rustdoc_same_name_documents_lib() {
 [DOCUMENTING] foo v0.0.1 ([..])
 [RUNNING] `rustdoc --crate-name foo src/lib.rs [..]\
         -o [CWD]/target/doc \
+        [..] \
         --cfg=foo \
         -L dependency=[CWD]/target/debug/deps`
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
@@ -168,6 +173,7 @@ fn rustdoc_target() {
 [RUNNING] `rustdoc --crate-name foo src/lib.rs [..]\
     --target x86_64-unknown-linux-gnu \
     -o [CWD]/target/x86_64-unknown-linux-gnu/doc \
+    [..] \
     -L dependency=[CWD]/target/x86_64-unknown-linux-gnu/debug/deps \
     -L dependency=[CWD]/target/debug/deps`
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",

Original file line number	Diff line number	Diff line change
`@@ -621,7 +621,7 @@ impl FixArgs {`
`621`	`621`	`ret.enabled_edition = Some(s[prefix.len()..].to_string());`
`622`	`622`	`continue;`
`623`	`623`	`}`
`624`		`- if s.starts_with("--error-format=") \|\| s.starts_with("--json-rendered=") {`
	`624`	`+ if s.starts_with("--error-format=") \|\| s.starts_with("--json=") {`
`625`	`625`	// Cargo may add error-format in some cases, but `cargo
`626`	`626`	// fix` wants to add its own.
`627`	`627`	`continue;`
Original file line number	Diff line number	Diff line change
`@@ -511,6 +511,6 @@ fn canonical_path() {`
`511`	`511`	`assert_deps_contains(`
`512`	`512`	`&p,`
`513`	`513`	`"target/debug/.fingerprint/foo-/dep-lib-foo-",`
`514`		`- &[(1, "src/lib.rs"), (2, "debug/deps/libregdep-*.rlib")],`
	`514`	`+ &[(1, "src/lib.rs"), (2, "debug/deps/libregdep-*.rmeta")],`
`515`	`515`	`);`
`516`	`516`	`}`