Auto merge of #7185 - ehuss:targetinfo-error, r=alexcrichton

Clean up TargetInfo

- The main motivation here is to provide a better error message when collecting information from rustc fails (it now shows the command and the output).
- Remove `has_cfg_and_sysroot`. I think this dates back to when it was introduced in #2328, as a guard for older versions of rustc that did not know about `--print=cfg`. Unless I'm missing something, I don't think we need to retain this backwards compatibility.
- Add some documentation.
- Demote the rustc cache log messages to `debug` level. I haven't really seen any caching issues, so I don't think it needs to be info level.
- Some other misc cleanup (remove unused function, etc.).

src/cargo/core/compiler/build_context/mod.rs

@@ -127,7 +127,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
             Kind::Host => &self.host_info,
             Kind::Target => &self.target_info,
         };
-        info.cfg().unwrap_or(&[])
+        info.cfg()
     }
 
     /// Gets the host architecture triple.

src/cargo/core/compiler/build_context/target_info.rs

@@ -9,17 +9,34 @@ use crate::core::TargetKind;
 use crate::util::CfgExpr;
 use crate::util::{CargoResult, CargoResultExt, Cfg, Config, ProcessBuilder, Rustc};
 
+/// Information about the platform target gleaned from querying rustc.
+///
+/// The `BuildContext` keeps two of these, one for the host and one for the
+/// target. If no target is specified, it uses a clone from the host.
 #[derive(Clone)]
 pub struct TargetInfo {
-    crate_type_process: Option<ProcessBuilder>,
+    /// A base process builder for discovering crate type information. In
+    /// particular, this is used to determine the output filename prefix and
+    /// suffix for a crate type.
+    crate_type_process: ProcessBuilder,
+    /// Cache of output filename prefixes and suffixes.
+    ///
+    /// The key is the crate type name (like `cdylib`) and the value is
+    /// `Some((prefix, suffix))`, for example `libcargo.so` would be
+    /// `Some(("lib", ".so")). The value is `None` if the crate type is not
+    /// supported.
     crate_types: RefCell<HashMap<String, Option<(String, String)>>>,
-    cfg: Option<Vec<Cfg>>,
-    pub sysroot_libdir: Option<PathBuf>,
+    /// `cfg` information extracted from `rustc --print=cfg`.
+    cfg: Vec<Cfg>,
+    /// Path to the "lib" directory in the sysroot.
+    pub sysroot_libdir: PathBuf,
+    /// Extra flags to pass to `rustc`, see `env_args`.
     pub rustflags: Vec<String>,
+    /// Extra flags to pass to `rustdoc`, see `env_args`.
     pub rustdocflags: Vec<String>,
 }
 
-/// Type of each file generated by a Unit.
+/// Kind of each file generated by a Unit, part of `FileType`.
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub enum FileFlavor {
     /// Not a special file type.
@@ -30,9 +47,13 @@ pub enum FileFlavor {
     DebugInfo,
 }
 
+/// Type of each file generated by a Unit.
 pub struct FileType {
+    /// The kind of file.
     pub flavor: FileFlavor,
+    /// The suffix for the file (for example, `.rlib`).
     suffix: String,
+    /// The prefix for the file (for example, `lib`).
     prefix: String,
     // Wasm bin target will generate two files in deps such as
     // "web-stuff.js" and "web_stuff.wasm". Note the different usages of
@@ -92,60 +113,51 @@ impl TargetInfo {
             process.arg("--crate-type").arg(crate_type);
         }
 
-        let mut with_cfg = process.clone();
-        with_cfg.arg("--print=sysroot");
-        with_cfg.arg("--print=cfg");
+        process.arg("--print=sysroot");
+        process.arg("--print=cfg");
 
-        let mut has_cfg_and_sysroot = true;
         let (output, error) = rustc
-            .cached_output(&with_cfg)
-            .or_else(|_| {
-                has_cfg_and_sysroot = false;
-                rustc.cached_output(&process)
-            })
+            .cached_output(&process)
             .chain_err(|| "failed to run `rustc` to learn about target-specific information")?;
 
         let mut lines = output.lines();
         let mut map = HashMap::new();
         for crate_type in KNOWN_CRATE_TYPES {
-            let out = parse_crate_type(crate_type, &error, &mut lines)?;
+            let out = parse_crate_type(crate_type, &process, &output, &error, &mut lines)?;
             map.insert(crate_type.to_string(), out);
         }
 
-        let mut sysroot_libdir = None;
-        if has_cfg_and_sysroot {
-            let line = match lines.next() {
-                Some(line) => line,
-                None => failure::bail!(
-                    "output of --print=sysroot missing when learning about \
-                     target-specific information from rustc"
-                ),
-            };
-            let mut rustlib = PathBuf::from(line);
-            if kind == Kind::Host {
+        let line = match lines.next() {
+            Some(line) => line,
+            None => failure::bail!(
+                "output of --print=sysroot missing when learning about \
+                 target-specific information from rustc\n{}",
+                output_err_info(&process, &output, &error)
+            ),
+        };
+        let mut rustlib = PathBuf::from(line);
+        let sysroot_libdir = match kind {
+            Kind::Host => {
                 if cfg!(windows) {
                     rustlib.push("bin");
                 } else {
                     rustlib.push("lib");
                 }
-                sysroot_libdir = Some(rustlib);
-            } else {
+                rustlib
+            }
+            Kind::Target => {
                 rustlib.push("lib");
                 rustlib.push("rustlib");
                 rustlib.push(target_triple);
                 rustlib.push("lib");
-                sysroot_libdir = Some(rustlib);
+                rustlib
             }
-        }
-
-        let cfg = if has_cfg_and_sysroot {
-            Some(lines.map(Cfg::from_str).collect::<CargoResult<Vec<_>>>()?)
-        } else {
-            None
         };
 
+        let cfg = lines.map(Cfg::from_str).collect::<CargoResult<Vec<_>>>()?;
+
         Ok(TargetInfo {
-            crate_type_process: Some(crate_type_process),
+            crate_type_process,
             crate_types: RefCell::new(map),
             sysroot_libdir,
             // recalculate `rustflags` from above now that we have `cfg`
@@ -154,24 +166,24 @@ impl TargetInfo {
                 config,
                 requested_target,
                 &rustc.host,
-                cfg.as_ref().map(|v| v.as_ref()),
+                Some(&cfg),
                 kind,
                 "RUSTFLAGS",
             )?,
             rustdocflags: env_args(
                 config,
                 requested_target,
                 &rustc.host,
-                cfg.as_ref().map(|v| v.as_ref()),
+                Some(&cfg),
                 kind,
                 "RUSTDOCFLAGS",
             )?,
             cfg,
         })
     }
 
-    pub fn cfg(&self) -> Option<&[Cfg]> {
-        self.cfg.as_ref().map(|v| v.as_ref())
+    pub fn cfg(&self) -> &[Cfg] {
+        &self.cfg
     }
 
     pub fn file_types(
@@ -255,21 +267,26 @@ impl TargetInfo {
     }
 
     fn discover_crate_type(&self, crate_type: &str) -> CargoResult<Option<(String, String)>> {
-        let mut process = self.crate_type_process.clone().unwrap();
+        let mut process = self.crate_type_process.clone();
 
         process.arg("--crate-type").arg(crate_type);
 
         let output = process.exec_with_output().chain_err(|| {
             format!(
-                "failed to run `rustc` to learn about \
-                 crate-type {} information",
+                "failed to run `rustc` to learn about crate-type {} information",
                 crate_type
             )
         })?;
 
         let error = str::from_utf8(&output.stderr).unwrap();
         let output = str::from_utf8(&output.stdout).unwrap();
-        Ok(parse_crate_type(crate_type, error, &mut output.lines())?)
+        Ok(parse_crate_type(
+            crate_type,
+            &process,
+            output,
+            error,
+            &mut output.lines(),
+        )?)
     }
 }
 
@@ -284,6 +301,8 @@ impl TargetInfo {
 // are two files for bin (`.wasm` and `.js`)).
 fn parse_crate_type(
     crate_type: &str,
+    cmd: &ProcessBuilder,
+    output: &str,
     error: &str,
     lines: &mut str::Lines<'_>,
 ) -> CargoResult<Option<(String, String)>> {
@@ -297,24 +316,41 @@ fn parse_crate_type(
     let line = match lines.next() {
         Some(line) => line,
         None => failure::bail!(
-            "malformed output when learning about \
-             crate-type {} information",
-            crate_type
+            "malformed output when learning about crate-type {} information\n{}",
+            crate_type,
+            output_err_info(cmd, output, error)
         ),
     };
     let mut parts = line.trim().split("___");
     let prefix = parts.next().unwrap();
     let suffix = match parts.next() {
         Some(part) => part,
         None => failure::bail!(
-            "output of --print=file-names has changed in \
-             the compiler, cannot parse"
+            "output of --print=file-names has changed in the compiler, cannot parse\n{}",
+            output_err_info(cmd, output, error)
         ),
     };
 
     Ok(Some((prefix.to_string(), suffix.to_string())))
 }
 
+/// Helper for creating an error message when parsing rustc output fails.
+fn output_err_info(cmd: &ProcessBuilder, stdout: &str, stderr: &str) -> String {
+    let mut result = format!("command was: {}\n", cmd);
+    if !stdout.is_empty() {
+        result.push_str("\n--- stdout\n");
+        result.push_str(stdout);
+    }
+    if !stderr.is_empty() {
+        result.push_str("\n--- stderr\n");
+        result.push_str(stderr);
+    }
+    if stdout.is_empty() && stderr.is_empty() {
+        result.push_str("(no output received)");
+    }
+    result
+}
+
 /// Acquire extra flags to pass to the compiler from various locations.
 ///
 /// The locations are:

src/cargo/core/compiler/compilation.rs

@@ -46,10 +46,10 @@ pub struct Compilation<'cfg> {
     pub host_deps_output: PathBuf,
 
     /// The path to rustc's own libstd
-    pub host_dylib_path: Option<PathBuf>,
+    pub host_dylib_path: PathBuf,
 
     /// The path to libstd for the target
-    pub target_dylib_path: Option<PathBuf>,
+    pub target_dylib_path: PathBuf,
 
     /// Extra environment variables that were passed to compilations and should
     /// be passed to future invocations of programs.
@@ -189,14 +189,14 @@ impl<'cfg> Compilation<'cfg> {
     ) -> CargoResult<ProcessBuilder> {
         let mut search_path = if is_host {
             let mut search_path = vec![self.host_deps_output.clone()];
-            search_path.extend(self.host_dylib_path.clone());
+            search_path.push(self.host_dylib_path.clone());
             search_path
         } else {
             let mut search_path =
                 super::filter_dynamic_search_path(self.native_dirs.iter(), &self.root_output);
             search_path.push(self.deps_output.clone());
             search_path.push(self.root_output.clone());
-            search_path.extend(self.target_dylib_path.clone());
+            search_path.push(self.target_dylib_path.clone());
             search_path
         };
 
@@ -286,29 +286,27 @@ fn target_runner(bcx: &BuildContext<'_, '_>) -> CargoResult<Option<(PathBuf, Vec
     }
 
     // try target.'cfg(...)'.runner
-    if let Some(target_cfg) = bcx.target_info.cfg() {
-        if let Some(table) = bcx.config.get_table("target")? {
-            let mut matching_runner = None;
-
-            for key in table.val.keys() {
-                if CfgExpr::matches_key(key, target_cfg) {
-                    let key = format!("target.{}.runner", key);
-                    if let Some(runner) = bcx.config.get_path_and_args(&key)? {
-                        // more than one match, error out
-                        if matching_runner.is_some() {
-                            failure::bail!(
-                                "several matching instances of `target.'cfg(..)'.runner` \
-                                 in `.cargo/config`"
-                            )
-                        }
-
-                        matching_runner = Some(runner.val);
+    if let Some(table) = bcx.config.get_table("target")? {
+        let mut matching_runner = None;
+
+        for key in table.val.keys() {
+            if CfgExpr::matches_key(key, bcx.target_info.cfg()) {
+                let key = format!("target.{}.runner", key);
+                if let Some(runner) = bcx.config.get_path_and_args(&key)? {
+                    // more than one match, error out
+                    if matching_runner.is_some() {
+                        failure::bail!(
+                            "several matching instances of `target.'cfg(..)'.runner` \
+                             in `.cargo/config`"
+                        )
                     }
+
+                    matching_runner = Some(runner.val);
                 }
             }
-
-            return Ok(matching_runner);
         }
+
+        return Ok(matching_runner);
     }
 
     Ok(None)

src/cargo/core/dependency.rs

@@ -459,13 +459,10 @@ impl Dependency {
 }
 
 impl Platform {
-    pub fn matches(&self, name: &str, cfg: Option<&[Cfg]>) -> bool {
+    pub fn matches(&self, name: &str, cfg: &[Cfg]) -> bool {
         match *self {
             Platform::Name(ref p) => p == name,
-            Platform::Cfg(ref p) => match cfg {
-                Some(cfg) => p.matches(cfg),
-                None => false,
-            },
+            Platform::Cfg(ref p) => p.matches(cfg),
         }
     }
 }