Auto merge of #6432 - ehuss:fix-metabuild-json, r=alexcrichton

Fix metabuild compile errors with --message-format=json.

If an error occurs while compiling a metabuild target with `--message-format=json`, it would panic because it was unable to serialize `Target`. This change makes it so that it places a fake "metabuild.rs" string in the `src_path` in this situation.

I'm very unhappy with this solution, but I'm unable to think of something better. Changing `src_path` to an `Option` (or something) would break existing tools (this might break, but maybe not catastrophically?). I tried implementing something that resets the `src_path` to the correct path in the target dir after the workspace is configured, but it felt very brittle – you have to fix up after all dependencies are downloaded, and there's not a good place to ensure that happens correctly. Any alternate ideas?

This adds a `with_json_contains_unordered` to help with tests.

Author: bors

src/cargo/core/compiler/mod.rs

@@ -689,10 +689,9 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult
 // second is the cwd that rustc should operate in.
 fn path_args(bcx: &BuildContext<'_, '_>, unit: &Unit<'_>) -> (PathBuf, PathBuf) {
     let ws_root = bcx.ws.root();
-    let src = if unit.target.is_custom_build() && unit.pkg.manifest().metabuild().is_some() {
-        unit.pkg.manifest().metabuild_path(bcx.ws.target_dir())
-    } else {
-        unit.target.src_path().path().to_path_buf()
+    let src = match unit.target.src_path() {
+        TargetSourcePath::Path(path) => path.to_path_buf(),
+        TargetSourcePath::Metabuild => unit.pkg.manifest().metabuild_path(bcx.ws.target_dir()),
     };
     assert!(src.is_absolute());
     if unit.pkg.package_id().source_id().is_path() {

src/cargo/core/manifest.rs

@@ -222,10 +222,10 @@ pub enum TargetSourcePath {
 }
 
 impl TargetSourcePath {
-    pub fn path(&self) -> &Path {
+    pub fn path(&self) -> Option<&Path> {
         match self {
-            TargetSourcePath::Path(path) => path.as_ref(),
-            TargetSourcePath::Metabuild => panic!("metabuild not expected"),
+            TargetSourcePath::Path(path) => Some(path.as_ref()),
+            TargetSourcePath::Metabuild => None,
         }
     }
 
@@ -268,19 +268,25 @@ struct SerializedTarget<'a> {
     /// See https://doc.rust-lang.org/reference/linkage.html
     crate_types: Vec<&'a str>,
     name: &'a str,
-    src_path: &'a PathBuf,
+    src_path: Option<&'a PathBuf>,
     edition: &'a str,
     #[serde(rename = "required-features", skip_serializing_if = "Option::is_none")]
     required_features: Option<Vec<&'a str>>,
 }
 
 impl ser::Serialize for Target {
     fn serialize<S: ser::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
+        let src_path = match &self.src_path {
+            TargetSourcePath::Path(p) => Some(p),
+            // Unfortunately getting the correct path would require access to
+            // target_dir, which is not available here.
+            TargetSourcePath::Metabuild => None,
+        };
         SerializedTarget {
             kind: &self.kind,
             crate_types: self.rustc_crate_types(),
             name: &self.name,
-            src_path: &self.src_path.path().to_path_buf(),
+            src_path: src_path,
             edition: &self.edition.to_string(),
             required_features: self
                 .required_features
@@ -301,7 +307,7 @@ compact_debug! {
                             Target::lib_target(
                                 &self.name,
                                 kinds.clone(),
-                                self.src_path().path().to_path_buf(),
+                                self.src_path().path().unwrap().to_path_buf(),
                                 self.edition,
                             ),
                             format!("lib_target({:?}, {:?}, {:?}, {:?})",

src/cargo/ops/cargo_test.rs

@@ -148,7 +148,7 @@ fn run_doc_tests(
         config.shell().status("Doc-tests", target.name())?;
         let mut p = compilation.rustdoc_process(package, target)?;
         p.arg("--test")
-            .arg(target.src_path().path())
+            .arg(target.src_path().path().unwrap())
             .arg("--crate-name")
             .arg(&target.crate_name());
 

tests/testsuite/metabuild.rs

@@ -699,5 +699,78 @@ fn metabuild_json_artifact() {
     let p = basic_project();
     p.cargo("build --message-format=json")
         .masquerade_as_nightly_cargo()
+        .with_json_contains_unordered(
+            r#"
+{
+  "executable": null,
+  "features": [],
+  "filenames": [
+    "[..]/foo/target/debug/build/foo-[..]/metabuild-foo[EXE]"
+  ],
+  "fresh": false,
+  "package_id": "foo [..]",
+  "profile": "{...}",
+  "reason": "compiler-artifact",
+  "target": {
+    "crate_types": [
+      "bin"
+    ],
+    "edition": "2018",
+    "kind": [
+      "custom-build"
+    ],
+    "name": "metabuild-foo",
+    "src_path": "[..]/foo/target/.metabuild/metabuild-foo-[..].rs"
+  }
+}
+
+{
+  "cfgs": [],
+  "env": [],
+  "linked_libs": [],
+  "linked_paths": [],
+  "package_id": "foo [..]",
+  "reason": "build-script-executed"
+}
+"#,
+        )
+        .run();
+}
+
+#[test]
+fn metabuild_failed_build_json() {
+    let p = basic_project();
+    // Modify the metabuild dep so that it fails to compile.
+    p.change_file("mb/src/lib.rs", "");
+    p.cargo("build --message-format=json")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_json_contains_unordered(
+            r#"
+{
+  "message": {
+    "children": "{...}",
+    "code": "{...}",
+    "level": "error",
+    "message": "cannot find function `metabuild` in module `mb`",
+    "rendered": "[..]",
+    "spans": "{...}"
+  },
+  "package_id": "foo [..]",
+  "reason": "compiler-message",
+  "target": {
+    "crate_types": [
+      "bin"
+    ],
+    "edition": "2018",
+    "kind": [
+      "custom-build"
+    ],
+    "name": "metabuild-foo",
+    "src_path": null
+  }
+}
+"#,
+        )
         .run();
 }

tests/testsuite/support/mod.rs

@@ -550,6 +550,7 @@ pub struct Execs {
     expect_stderr_unordered: Vec<String>,
     expect_neither_contains: Vec<String>,
     expect_json: Option<Vec<Value>>,
+    expect_json_contains_unordered: Vec<Value>,
     stream_output: bool,
 }
 
@@ -689,6 +690,24 @@ impl Execs {
         self
     }
 
+    /// Verify JSON output contains the given objects (in any order) somewhere
+    /// in its output.
+    ///
+    /// CAUTION: Be very careful when using this. Make sure every object is
+    /// unique (not a subset of one another). Also avoid using objects that
+    /// could possibly match multiple output lines unless you're very sure of
+    /// what you are doing.
+    ///
+    /// See `with_json` for more detail.
+    pub fn with_json_contains_unordered(&mut self, expected: &str) -> &mut Self {
+        self.expect_json_contains_unordered.extend(
+            expected
+                .split("\n\n")
+                .map(|line| line.parse().expect("line to be a valid JSON value")),
+        );
+        self
+    }
+
     /// Forward subordinate process stdout/stderr to the terminal.
     /// Useful for printf debugging of the tests.
     /// CAUTION: CI will fail if you leave this in your test!
@@ -941,6 +960,33 @@ impl Execs {
                 self.match_json(obj, line)?;
             }
         }
+
+        if !self.expect_json_contains_unordered.is_empty() {
+            let stdout = str::from_utf8(&actual.stdout)
+                .map_err(|_| "stdout was not utf8 encoded".to_owned())?;
+            let mut lines = stdout
+                .lines()
+                .filter(|line| line.starts_with('{'))
+                .collect::<Vec<_>>();
+            for obj in &self.expect_json_contains_unordered {
+                match lines
+                    .iter()
+                    .position(|line| self.match_json(obj, line).is_ok())
+                {
+                    Some(index) => lines.remove(index),
+                    None => {
+                        return Err(format!(
+                            "Did not find expected JSON:\n\
+                             {}\n\
+                             Remaining available output:\n\
+                             {}\n",
+                            serde_json::to_string_pretty(obj).unwrap(),
+                            lines.join("\n")
+                        ));
+                    }
+                };
+            }
+        }
         Ok(())
     }
 
@@ -1322,6 +1368,7 @@ pub fn execs() -> Execs {
         expect_stderr_unordered: Vec::new(),
         expect_neither_contains: Vec::new(),
         expect_json: None,
+        expect_json_contains_unordered: Vec::new(),
         stream_output: false,
     }
 }