Auto merge of #9122 - ehuss:fix-multiple-run-custom-build, r=alexcric…

…hton

Fix env/cfg set for `cargo test` and `cargo run`.

There are some situations where a build script may need to run multiple times for the same package during the same `cargo` session.  There was a bug in that some of the values in the `Compilation` struct didn't handle this case.  The solution here is to be more careful about how this extra data is associated with `Unit`s, instead of assuming a package's build script only runs once.

The things that were not being handled properly:

* `Compilation::extra_env`, which is the output of `cargo:rustc-env` in build scripts.  The solution here is to use the `Metadata` hash which is used elsewhere for distinguishing build script outputs.
* `Compilation::cfgs`, which is the output of `cargo:rustc-cfg` in build scripts and the features to be set, and this is only used for doctests.  The solution here is to just add those `--cfg` flags directly in the `Doctest` struct.

The situations that cause a build script to be run multiple times:

* A package needed for both the host and target.  In the test here, this was accomplished with a proc-macro (which has to be `host`) and a cyclical dev dependency, but there are many other ways to trigger this.
* Something built with different features (with the new feature resolver), usually due to host/target differences.
* Something built with different profile settings, usually due to host/target differences.

Fixes #9100

src/cargo/core/compiler/compilation.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeSet, HashMap, HashSet};
+use std::collections::{BTreeSet, HashMap};
 use std::env;
 use std::ffi::{OsStr, OsString};
 use std::path::PathBuf;
@@ -7,9 +7,8 @@ use cargo_platform::CfgExpr;
 use semver::Version;
 
 use super::BuildContext;
-use crate::core::compiler::CompileKind;
-use crate::core::compiler::Unit;
-use crate::core::{Edition, Package, PackageId};
+use crate::core::compiler::{CompileKind, Metadata, Unit};
+use crate::core::{Edition, Package};
 use crate::util::{self, config, join_paths, process, CargoResult, Config, ProcessBuilder};
 
 /// Structure with enough information to run `rustdoc --test`.
@@ -22,20 +21,35 @@ pub struct Doctest {
     pub unstable_opts: bool,
     /// The -Clinker value to use.
     pub linker: Option<PathBuf>,
+    /// The script metadata, if this unit's package has a build script.
+    ///
+    /// This is used for indexing [`Compilation::extra_env`].
+    pub script_meta: Option<Metadata>,
+}
+
+/// Information about the output of a unit.
+#[derive(Ord, PartialOrd, Eq, PartialEq)]
+pub struct UnitOutput {
+    /// The unit that generated this output.
+    pub unit: Unit,
+    /// Path to the unit's primary output (an executable or cdylib).
+    pub path: PathBuf,
+    /// The script metadata, if this unit's package has a build script.
+    ///
+    /// This is used for indexing [`Compilation::extra_env`].
+    pub script_meta: Option<Metadata>,
 }
 
 /// A structure returning the result of a compilation.
 pub struct Compilation<'cfg> {
     /// An array of all tests created during this compilation.
-    /// `(unit, path_to_test_exe)` where `unit` contains information such as the
-    /// package, compile target, etc.
-    pub tests: Vec<(Unit, PathBuf)>,
+    pub tests: Vec<UnitOutput>,
 
     /// An array of all binaries created.
-    pub binaries: Vec<(Unit, PathBuf)>,
+    pub binaries: Vec<UnitOutput>,
 
     /// An array of all cdylibs created.
-    pub cdylibs: Vec<(Unit, PathBuf)>,
+    pub cdylibs: Vec<UnitOutput>,
 
     /// All directories for the output of native build commands.
     ///
@@ -60,17 +74,14 @@ pub struct Compilation<'cfg> {
 
     /// Extra environment variables that were passed to compilations and should
     /// be passed to future invocations of programs.
-    pub extra_env: HashMap<PackageId, Vec<(String, String)>>,
+    ///
+    /// The key is the build script metadata for uniquely identifying the
+    /// `RunCustomBuild` unit that generated these env vars.
+    pub extra_env: HashMap<Metadata, Vec<(String, String)>>,
 
     /// Libraries to test with rustdoc.
     pub to_doc_test: Vec<Doctest>,
 
-    /// Features per package enabled during this compilation.
-    pub cfgs: HashMap<PackageId, HashSet<String>>,
-
-    /// Flags to pass to rustdoc when invoked from cargo test, per package.
-    pub rustdocflags: HashMap<PackageId, Vec<String>>,
-
     /// The target host triple.
     pub host: String,
 
@@ -127,8 +138,6 @@ impl<'cfg> Compilation<'cfg> {
             cdylibs: Vec::new(),
             extra_env: HashMap::new(),
             to_doc_test: Vec::new(),
-            cfgs: HashMap::new(),
-            rustdocflags: HashMap::new(),
             config: bcx.config,
             host: bcx.host_triple().to_string(),
             rustc_process: rustc,
@@ -144,7 +153,13 @@ impl<'cfg> Compilation<'cfg> {
         })
     }
 
-    /// See `process`.
+    /// Returns a [`ProcessBuilder`] for running `rustc`.
+    ///
+    /// `is_primary` is true if this is a "primary package", which means it
+    /// was selected by the user on the command-line (such as with a `-p`
+    /// flag), see [`crate::core::compiler::Context::primary_packages`].
+    ///
+    /// `is_workspace` is true if this is a workspace member.
     pub fn rustc_process(
         &self,
         unit: &Unit,
@@ -160,14 +175,18 @@ impl<'cfg> Compilation<'cfg> {
         };
 
         let cmd = fill_rustc_tool_env(rustc, unit);
-        self.fill_env(cmd, &unit.pkg, unit.kind, true)
+        self.fill_env(cmd, &unit.pkg, None, unit.kind, true)
     }
 
-    /// See `process`.
-    pub fn rustdoc_process(&self, unit: &Unit) -> CargoResult<ProcessBuilder> {
+    /// Returns a [`ProcessBuilder`] for running `rustdoc`.
+    pub fn rustdoc_process(
+        &self,
+        unit: &Unit,
+        script_meta: Option<Metadata>,
+    ) -> CargoResult<ProcessBuilder> {
         let rustdoc = process(&*self.config.rustdoc()?);
         let cmd = fill_rustc_tool_env(rustdoc, unit);
-        let mut p = self.fill_env(cmd, &unit.pkg, unit.kind, true)?;
+        let mut p = self.fill_env(cmd, &unit.pkg, script_meta, unit.kind, true)?;
         if unit.target.edition() != Edition::Edition2015 {
             p.arg(format!("--edition={}", unit.target.edition()));
         }
@@ -179,25 +198,37 @@ impl<'cfg> Compilation<'cfg> {
         Ok(p)
     }
 
-    /// See `process`.
+    /// Returns a [`ProcessBuilder`] appropriate for running a process for the
+    /// host platform.
+    ///
+    /// This is currently only used for running build scripts. If you use this
+    /// for anything else, please be extra careful on how environment
+    /// variables are set!
     pub fn host_process<T: AsRef<OsStr>>(
         &self,
         cmd: T,
         pkg: &Package,
     ) -> CargoResult<ProcessBuilder> {
-        self.fill_env(process(cmd), pkg, CompileKind::Host, false)
+        self.fill_env(process(cmd), pkg, None, CompileKind::Host, false)
     }
 
     pub fn target_runner(&self, kind: CompileKind) -> Option<&(PathBuf, Vec<String>)> {
         self.target_runners.get(&kind).and_then(|x| x.as_ref())
     }
 
-    /// See `process`.
+    /// Returns a [`ProcessBuilder`] appropriate for running a process for the
+    /// target platform. This is typically used for `cargo run` and `cargo
+    /// test`.
+    ///
+    /// `script_meta` is the metadata for the `RunCustomBuild` unit that this
+    /// unit used for its build script. Use `None` if the package did not have
+    /// a build script.
     pub fn target_process<T: AsRef<OsStr>>(
         &self,
         cmd: T,
         kind: CompileKind,
         pkg: &Package,
+        script_meta: Option<Metadata>,
     ) -> CargoResult<ProcessBuilder> {
         let builder = if let Some((runner, args)) = self.target_runner(kind) {
             let mut builder = process(runner);
@@ -207,7 +238,7 @@ impl<'cfg> Compilation<'cfg> {
         } else {
             process(cmd)
         };
-        self.fill_env(builder, pkg, kind, false)
+        self.fill_env(builder, pkg, script_meta, kind, false)
     }
 
     /// Prepares a new process with an appropriate environment to run against
@@ -219,6 +250,7 @@ impl<'cfg> Compilation<'cfg> {
         &self,
         mut cmd: ProcessBuilder,
         pkg: &Package,
+        script_meta: Option<Metadata>,
         kind: CompileKind,
         is_rustc_tool: bool,
     ) -> CargoResult<ProcessBuilder> {
@@ -258,9 +290,11 @@ impl<'cfg> Compilation<'cfg> {
         let search_path = join_paths(&search_path, util::dylib_path_envvar())?;
 
         cmd.env(util::dylib_path_envvar(), &search_path);
-        if let Some(env) = self.extra_env.get(&pkg.package_id()) {
-            for &(ref k, ref v) in env {
-                cmd.env(k, v);
+        if let Some(meta) = script_meta {
+            if let Some(env) = self.extra_env.get(&meta) {
+                for (k, v) in env {
+                    cmd.env(k, v);
+                }
             }
         }
 

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

@@ -1,11 +1,12 @@
 use std::collections::{BTreeSet, HashMap, HashSet};
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 
 use filetime::FileTime;
 use jobserver::Client;
 
-use crate::core::compiler::{self, compilation, Unit};
+use crate::core::compiler::compilation::{self, UnitOutput};
+use crate::core::compiler::{self, Unit};
 use crate::core::PackageId;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::profile;
@@ -174,16 +175,16 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                 if unit.mode == CompileMode::Test {
                     self.compilation
                         .tests
-                        .push((unit.clone(), output.path.clone()));
+                        .push(self.unit_output(unit, &output.path));
                 } else if unit.target.is_executable() {
                     self.compilation
                         .binaries
-                        .push((unit.clone(), bindst.clone()));
+                        .push(self.unit_output(unit, bindst));
                 } else if unit.target.is_cdylib() {
-                    if !self.compilation.cdylibs.iter().any(|(u, _)| u == unit) {
+                    if !self.compilation.cdylibs.iter().any(|uo| uo.unit == *unit) {
                         self.compilation
                             .cdylibs
-                            .push((unit.clone(), bindst.clone()));
+                            .push(self.unit_output(unit, bindst));
                     }
                 }
             }
@@ -198,9 +199,10 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                             .build_script_out_dir(&dep.unit)
                             .display()
                             .to_string();
+                        let script_meta = self.get_run_build_script_metadata(&dep.unit);
                         self.compilation
                             .extra_env
-                            .entry(dep.unit.pkg.package_id())
+                            .entry(script_meta)
                             .or_insert_with(Vec::new)
                             .push(("OUT_DIR".to_string(), out_dir));
                     }
@@ -212,50 +214,36 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                 let mut unstable_opts = false;
                 let mut args = compiler::extern_args(&self, unit, &mut unstable_opts)?;
                 args.extend(compiler::lto_args(&self, unit));
+                for feature in &unit.features {
+                    args.push("--cfg".into());
+                    args.push(format!("feature=\"{}\"", feature).into());
+                }
+                let script_meta = self.find_build_script_metadata(unit);
+                if let Some(meta) = script_meta {
+                    if let Some(output) = self.build_script_outputs.lock().unwrap().get(meta) {
+                        for cfg in &output.cfgs {
+                            args.push("--cfg".into());
+                            args.push(cfg.into());
+                        }
+                    }
+                }
+                args.extend(self.bcx.rustdocflags_args(unit).iter().map(Into::into));
                 self.compilation.to_doc_test.push(compilation::Doctest {
                     unit: unit.clone(),
                     args,
                     unstable_opts,
                     linker: self.bcx.linker(unit.kind),
+                    script_meta,
                 });
             }
 
-            // Collect the enabled features.
-            let feats = &unit.features;
-            if !feats.is_empty() {
-                self.compilation
-                    .cfgs
-                    .entry(unit.pkg.package_id())
-                    .or_insert_with(|| {
-                        feats
-                            .iter()
-                            .map(|feat| format!("feature=\"{}\"", feat))
-                            .collect()
-                    });
-            }
-
-            // Collect rustdocflags.
-            let rustdocflags = self.bcx.rustdocflags_args(unit);
-            if !rustdocflags.is_empty() {
-                self.compilation
-                    .rustdocflags
-                    .entry(unit.pkg.package_id())
-                    .or_insert_with(|| rustdocflags.to_vec());
-            }
-
             super::output_depinfo(&mut self, unit)?;
         }
 
-        for (pkg_id, output) in self.build_script_outputs.lock().unwrap().iter() {
-            self.compilation
-                .cfgs
-                .entry(pkg_id)
-                .or_insert_with(HashSet::new)
-                .extend(output.cfgs.iter().cloned());
-
+        for (script_meta, output) in self.build_script_outputs.lock().unwrap().iter() {
             self.compilation
                 .extra_env
-                .entry(pkg_id)
+                .entry(*script_meta)
                 .or_insert_with(Vec::new)
                 .extend(output.env.iter().cloned());
 
@@ -352,11 +340,11 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
     /// Returns the RunCustomBuild Unit associated with the given Unit.
     ///
     /// If the package does not have a build script, this returns None.
-    pub fn find_build_script_unit(&self, unit: Unit) -> Option<Unit> {
+    pub fn find_build_script_unit(&self, unit: &Unit) -> Option<Unit> {
         if unit.mode.is_run_custom_build() {
-            return Some(unit);
+            return Some(unit.clone());
         }
-        self.bcx.unit_graph[&unit]
+        self.bcx.unit_graph[unit]
             .iter()
             .find(|unit_dep| {
                 unit_dep.unit.mode.is_run_custom_build()
@@ -369,7 +357,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
     /// the given unit.
     ///
     /// If the package does not have a build script, this returns None.
-    pub fn find_build_script_metadata(&self, unit: Unit) -> Option<Metadata> {
+    pub fn find_build_script_metadata(&self, unit: &Unit) -> Option<Metadata> {
         let script_unit = self.find_build_script_unit(unit)?;
         Some(self.get_run_build_script_metadata(&script_unit))
     }
@@ -398,6 +386,17 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         Ok(inputs.into_iter().collect())
     }
 
+    /// Returns a [`UnitOutput`] which represents some information about the
+    /// output of a unit.
+    pub fn unit_output(&self, unit: &Unit, path: &Path) -> UnitOutput {
+        let script_meta = self.find_build_script_metadata(unit);
+        UnitOutput {
+            unit: unit.clone(),
+            path: path.to_path_buf(),
+            script_meta,
+        }
+    }
+
     fn check_collisions(&self) -> CargoResult<()> {
         let mut output_collisions = HashMap::new();
         let describe_collision = |unit: &Unit, other_unit: &Unit, path: &PathBuf| -> String {