Auto merge of #49053 - alexcrichton:fail-if-build-cargo-twice, r=Mark-Simulacrum

rustbuild: Fail the build if we build Cargo twice

This commit updates the `ToolBuild` step to stream Cargo's JSON messages, parse
them, and record all libraries built. If we build anything twice (aka Cargo)
it'll most likely happen due to dependencies being recompiled which is caught by
this check.

Author: bors

src/Cargo.lock

@@ -16,6 +16,7 @@
 //! compiler. This module is also responsible for assembling the sysroot as it
 //! goes along from the output of the previous stage.
 
+use std::borrow::Cow;
 use std::env;
 use std::fs::{self, File};
 use std::io::BufReader;
@@ -996,24 +997,6 @@ fn stderr_isatty() -> bool {
 pub fn run_cargo(build: &Build, cargo: &mut Command, stamp: &Path, is_check: bool)
     -> Vec<PathBuf>
 {
-    // Instruct Cargo to give us json messages on stdout, critically leaving
-    // stderr as piped so we can get those pretty colors.
-    cargo.arg("--message-format").arg("json")
-         .stdout(Stdio::piped());
-
-    if stderr_isatty() && build.ci_env == CiEnv::None {
-        // since we pass message-format=json to cargo, we need to tell the rustc
-        // wrapper to give us colored output if necessary. This is because we
-        // only want Cargo's JSON output, not rustcs.
-        cargo.env("RUSTC_COLOR", "1");
-    }
-
-    build.verbose(&format!("running: {:?}", cargo));
-    let mut child = match cargo.spawn() {
-        Ok(child) => child,
-        Err(e) => panic!("failed to execute command: {:?}\nerror: {}", cargo, e),
-    };
-
     // `target_root_dir` looks like $dir/$target/release
     let target_root_dir = stamp.parent().unwrap();
     // `target_deps_dir` looks like $dir/$target/release/deps
@@ -1028,46 +1011,33 @@ pub fn run_cargo(build: &Build, cargo: &mut Command, stamp: &Path, is_check: boo
     // files we need to probe for later.
     let mut deps = Vec::new();
     let mut toplevel = Vec::new();
-    let stdout = BufReader::new(child.stdout.take().unwrap());
-    for line in stdout.lines() {
-        let line = t!(line);
-        let json: serde_json::Value = if line.starts_with("{") {
-            t!(serde_json::from_str(&line))
-        } else {
-            // If this was informational, just print it out and continue
-            println!("{}", line);
-            continue
+    let ok = stream_cargo(build, cargo, &mut |msg| {
+        let filenames = match msg {
+            CargoMessage::CompilerArtifact { filenames, .. } => filenames,
+            _ => return,
         };
-        if json["reason"].as_str() != Some("compiler-artifact") {
-            if build.config.rustc_error_format.as_ref().map_or(false, |e| e == "json") {
-                // most likely not a cargo message, so let's send it out as well
-                println!("{}", line);
-            }
-            continue
-        }
-        for filename in json["filenames"].as_array().unwrap() {
-            let filename = filename.as_str().unwrap();
+        for filename in filenames {
             // Skip files like executables
             if !filename.ends_with(".rlib") &&
                !filename.ends_with(".lib") &&
                !is_dylib(&filename) &&
                !(is_check && filename.ends_with(".rmeta")) {
-                continue
+                return;
             }
 
-            let filename = Path::new(filename);
+            let filename = Path::new(&*filename);
 
             // If this was an output file in the "host dir" we don't actually
             // worry about it, it's not relevant for us.
             if filename.starts_with(&host_root_dir) {
-                continue;
+                return;
             }
 
             // If this was output in the `deps` dir then this is a precise file
             // name (hash included) so we start tracking it.
             if filename.starts_with(&target_deps_dir) {
                 deps.push(filename.to_path_buf());
-                continue;
+                return;
             }
 
             // Otherwise this was a "top level artifact" which right now doesn't
@@ -1088,15 +1058,10 @@ pub fn run_cargo(build: &Build, cargo: &mut Command, stamp: &Path, is_check: boo
 
             toplevel.push((file_stem, extension, expected_len));
         }
-    }
+    });
 
-    // Make sure Cargo actually succeeded after we read all of its stdout.
-    let status = t!(child.wait());
-    if !status.success() {
-        panic!("command did not execute successfully: {:?}\n\
-                expected success, got: {}",
-               cargo,
-               status);
+    if !ok {
+        panic!("cargo must succeed");
     }
 
     // Ok now we need to actually find all the files listed in `toplevel`. We've
@@ -1167,3 +1132,63 @@ pub fn run_cargo(build: &Build, cargo: &mut Command, stamp: &Path, is_check: boo
     t!(t!(File::create(stamp)).write_all(&new_contents));
     deps
 }
+
+pub fn stream_cargo(
+    build: &Build,
+    cargo: &mut Command,
+    cb: &mut FnMut(CargoMessage),
+) -> bool {
+    // Instruct Cargo to give us json messages on stdout, critically leaving
+    // stderr as piped so we can get those pretty colors.
+    cargo.arg("--message-format").arg("json")
+         .stdout(Stdio::piped());
+
+    if stderr_isatty() && build.ci_env == CiEnv::None {
+        // since we pass message-format=json to cargo, we need to tell the rustc
+        // wrapper to give us colored output if necessary. This is because we
+        // only want Cargo's JSON output, not rustcs.
+        cargo.env("RUSTC_COLOR", "1");
+    }
+
+    build.verbose(&format!("running: {:?}", cargo));
+    let mut child = match cargo.spawn() {
+        Ok(child) => child,
+        Err(e) => panic!("failed to execute command: {:?}\nerror: {}", cargo, e),
+    };
+
+    // Spawn Cargo slurping up its JSON output. We'll start building up the
+    // `deps` array of all files it generated along with a `toplevel` array of
+    // files we need to probe for later.
+    let stdout = BufReader::new(child.stdout.take().unwrap());
+    for line in stdout.lines() {
+        let line = t!(line);
+        match serde_json::from_str::<CargoMessage>(&line) {
+            Ok(msg) => cb(msg),
+            // If this was informational, just print it out and continue
+            Err(_) => println!("{}", line)
+        }
+    }
+
+    // Make sure Cargo actually succeeded after we read all of its stdout.
+    let status = t!(child.wait());
+    if !status.success() {
+        println!("command did not execute successfully: {:?}\n\
+                  expected success, got: {}",
+                 cargo,
+                 status);
+    }
+    status.success()
+}
+
+#[derive(Deserialize)]
+#[serde(tag = "reason", rename_all = "kebab-case")]
+pub enum CargoMessage<'a> {
+    CompilerArtifact {
+        package_id: Cow<'a, str>,
+        features: Vec<Cow<'a, str>>,
+        filenames: Vec<Cow<'a, str>>,
+    },
+    BuildScriptExecuted {
+        package_id: Cow<'a, str>,
+    }
+}

src/bootstrap/compile.rs

@@ -254,6 +254,10 @@ pub struct Build {
     ci_env: CiEnv,
     delayed_failures: RefCell<Vec<String>>,
     prerelease_version: Cell<Option<u32>>,
+    tool_artifacts: RefCell<HashMap<
+        Interned<String>,
+        HashMap<String, (&'static str, PathBuf, Vec<String>)>
+    >>,
 }
 
 #[derive(Debug)]
@@ -353,6 +357,7 @@ impl Build {
             ci_env: CiEnv::current(),
             delayed_failures: RefCell::new(Vec::new()),
             prerelease_version: Cell::new(None),
+            tool_artifacts: Default::default(),
         }
     }
 

src/bootstrap/lib.rs

@@ -117,7 +117,80 @@ impl Step for ToolBuild {
 
         let _folder = build.fold_output(|| format!("stage{}-{}", compiler.stage, tool));
         println!("Building stage{} tool {} ({})", compiler.stage, tool, target);
-        let is_expected = build.try_run(&mut cargo);
+        let mut duplicates = Vec::new();
+        let is_expected = compile::stream_cargo(build, &mut cargo, &mut |msg| {
+            // Only care about big things like the RLS/Cargo for now
+            if tool != "rls" && tool != "cargo" {
+                return
+            }
+            let (id, features, filenames) = match msg {
+                compile::CargoMessage::CompilerArtifact {
+                    package_id,
+                    features,
+                    filenames
+                } => {
+                    (package_id, features, filenames)
+                }
+                _ => return,
+            };
+            let features = features.iter().map(|s| s.to_string()).collect::<Vec<_>>();
+
+            for path in filenames {
+                let val = (tool, PathBuf::from(&*path), features.clone());
+                // we're only interested in deduplicating rlibs for now
+                if val.1.extension().and_then(|s| s.to_str()) != Some("rlib") {
+                    continue
+                }
+
+                // Don't worry about libs that turn out to be host dependencies
+                // or build scripts, we only care about target dependencies that
+                // are in `deps`.
+                if let Some(maybe_target) = val.1
+                    .parent()                   // chop off file name
+                    .and_then(|p| p.parent())   // chop off `deps`
+                    .and_then(|p| p.parent())   // chop off `release`
+                    .and_then(|p| p.file_name())
+                    .and_then(|p| p.to_str())
+                {
+                    if maybe_target != &*target {
+                        continue
+                    }
+                }
+
+                let mut artifacts = build.tool_artifacts.borrow_mut();
+                let prev_artifacts = artifacts
+                    .entry(target)
+                    .or_insert_with(Default::default);
+                if let Some(prev) = prev_artifacts.get(&*id) {
+                    if prev.1 != val.1 {
+                        duplicates.push((
+                            id.to_string(),
+                            val,
+                            prev.clone(),
+                        ));
+                    }
+                    return
+                }
+                prev_artifacts.insert(id.to_string(), val);
+            }
+        });
+
+        if is_expected && duplicates.len() != 0 {
+            println!("duplicate artfacts found when compiling a tool, this \
+                      typically means that something was recompiled because \
+                      a transitive dependency has different features activated \
+                      than in a previous build:\n");
+            for (id, cur, prev) in duplicates {
+                println!("  {}", id);
+                println!("    `{}` enabled features {:?} at {:?}",
+                         cur.0, cur.2, cur.1);
+                println!("    `{}` enabled features {:?} at {:?}",
+                         prev.0, prev.2, prev.1);
+            }
+            println!("");
+            panic!("tools should not compile multiple copies of the same crate");
+        }
+
         build.save_toolstate(tool, if is_expected {
             ToolState::TestFail
         } else {

src/bootstrap/tool.rs

@@ -56,3 +56,4 @@ byteorder = { version = "1.1", features = ["i128"]}
 #        later crate stop compiling. If you can remove this and everything
 #        compiles, then please feel free to do so!
 flate2 = "1.0"
+tempdir = "0.3"

src/librustc/Cargo.toml

@@ -7,3 +7,7 @@ license = "MIT/Apache-2.0"
 
 [dependencies]
 tidy = { path = "../tidy" }
+
+# not actually needed but required for now to unify the feature selection of
+# `num-traits` between this and `rustbook`
+num-traits = "0.2"