Auto merge of #9065 - alexcrichton:fix-build-bug-again, r=ehuss

Fix `links` vars showing up for testing packages

If a package is tested and the library for the package wasn't built
(e.g. only tested or it wasn't present) then the `links` env vars from
dependencies weren't showing up to the build script by accident. This
was an accidental regression from #8969.

The intention of #8969 was to exclude connections to build scripts
connected via dev-dependencies, but it only applied a heuristic because
the unit graph doesn't retain information about dev-dependencies. The
fix here is to instead actually retain information about
dev-dependencies which is only used for constructing the unit graph and
connecting build script executions to one another.

Closes #9063

Author: bors

src/cargo/core/compiler/unit_dependencies.rs

@@ -47,6 +47,11 @@ struct State<'a, 'cfg> {
     target_data: &'a RustcTargetData,
     profiles: &'a Profiles,
     interner: &'a UnitInterner,
+
+    /// A set of edges in `unit_dependencies` where (a, b) means that the
+    /// dependency from a to b was added purely because it was a dev-dependency.
+    /// This is used during `connect_run_custom_build_deps`.
+    dev_dependency_edges: HashSet<(Unit, Unit)>,
 }
 
 pub fn build_unit_dependencies<'a, 'cfg>(
@@ -86,6 +91,7 @@ pub fn build_unit_dependencies<'a, 'cfg>(
         target_data,
         profiles,
         interner,
+        dev_dependency_edges: HashSet::new(),
     };
 
     let std_unit_deps = calc_deps_of_std(&mut state, std_roots)?;
@@ -98,7 +104,7 @@ pub fn build_unit_dependencies<'a, 'cfg>(
         attach_std_deps(&mut state, std_roots, std_unit_deps);
     }
 
-    connect_run_custom_build_deps(&mut state.unit_dependencies);
+    connect_run_custom_build_deps(&mut state);
 
     // Dependencies are used in tons of places throughout the backend, many of
     // which affect the determinism of the build itself. As a result be sure
@@ -258,7 +264,8 @@ fn compute_deps(
         });
 
     let mut ret = Vec::new();
-    for (id, _) in filtered_deps {
+    let mut dev_deps = Vec::new();
+    for (id, deps) in filtered_deps {
         let pkg = state.get(id);
         let lib = match pkg.targets().iter().find(|t| t.is_lib()) {
             Some(t) => t,
@@ -267,6 +274,7 @@ fn compute_deps(
         let mode = check_or_build_mode(unit.mode, lib);
         let dep_unit_for = unit_for.with_dependency(unit, lib);
 
+        let start = ret.len();
         if state.config.cli_unstable().dual_proc_macros && lib.proc_macro() && !unit.kind.is_host()
         {
             let unit_dep = new_unit_dep(state, unit, pkg, lib, dep_unit_for, unit.kind, mode)?;
@@ -286,7 +294,18 @@ fn compute_deps(
             )?;
             ret.push(unit_dep);
         }
+
+        // If the unit added was a dev-dependency unit, then record that in the
+        // dev-dependencies array. We'll add this to
+        // `state.dev_dependency_edges` at the end and process it later in
+        // `connect_run_custom_build_deps`.
+        if deps.iter().all(|d| !d.is_transitive()) {
+            for dep in ret[start..].iter() {
+                dev_deps.push((unit.clone(), dep.unit.clone()));
+            }
+        }
     }
+    state.dev_dependency_edges.extend(dev_deps);
 
     // If this target is a build script, then what we've collected so far is
     // all we need. If this isn't a build script, then it depends on the
@@ -617,26 +636,18 @@ fn new_unit_dep_with_profile(
 ///
 /// Here we take the entire `deps` map and add more dependencies from execution
 /// of one build script to execution of another build script.
-fn connect_run_custom_build_deps(unit_dependencies: &mut UnitGraph) {
+fn connect_run_custom_build_deps(state: &mut State<'_, '_>) {
     let mut new_deps = Vec::new();
 
     {
+        let state = &*state;
         // First up build a reverse dependency map. This is a mapping of all
         // `RunCustomBuild` known steps to the unit which depends on them. For
         // example a library might depend on a build script, so this map will
         // have the build script as the key and the library would be in the
         // value's set.
-        //
-        // Note that as an important part here we're skipping "test" units. Test
-        // units depend on the execution of a build script, but
-        // links-dependencies only propagate through `[dependencies]`, nothing
-        // else. We don't want to pull in a links-dependency through a
-        // dev-dependency since that could create a cycle.
         let mut reverse_deps_map = HashMap::new();
-        for (unit, deps) in unit_dependencies.iter() {
-            if unit.mode.is_any_test() {
-                continue;
-            }
+        for (unit, deps) in state.unit_dependencies.iter() {
             for dep in deps {
                 if dep.unit.mode == CompileMode::RunCustomBuild {
                     reverse_deps_map
@@ -656,7 +667,8 @@ fn connect_run_custom_build_deps(unit_dependencies: &mut UnitGraph) {
         // `links`, then we depend on that package's build script! Here we use
         // `dep_build_script` to manufacture an appropriate build script unit to
         // depend on.
-        for unit in unit_dependencies
+        for unit in state
+            .unit_dependencies
             .keys()
             .filter(|k| k.mode == CompileMode::RunCustomBuild)
         {
@@ -670,16 +682,34 @@ fn connect_run_custom_build_deps(unit_dependencies: &mut UnitGraph) {
             let to_add = reverse_deps
                 .iter()
                 // Get all sibling dependencies of `unit`
-                .flat_map(|reverse_dep| unit_dependencies[reverse_dep].iter())
+                .flat_map(|reverse_dep| {
+                    state.unit_dependencies[reverse_dep]
+                        .iter()
+                        .map(move |a| (reverse_dep, a))
+                })
                 // Only deps with `links`.
-                .filter(|other| {
+                .filter(|(_parent, other)| {
                     other.unit.pkg != unit.pkg
                         && other.unit.target.is_linkable()
                         && other.unit.pkg.manifest().links().is_some()
                 })
+                // Skip dependencies induced via dev-dependencies since
+                // connections between `links` and build scripts only happens
+                // via normal dependencies. Otherwise since dev-dependencies can
+                // be cyclic we could have cyclic build-script executions.
+                .filter_map(move |(parent, other)| {
+                    if state
+                        .dev_dependency_edges
+                        .contains(&((*parent).clone(), other.unit.clone()))
+                    {
+                        None
+                    } else {
+                        Some(other)
+                    }
+                })
                 // Get the RunCustomBuild for other lib.
                 .filter_map(|other| {
-                    unit_dependencies[&other.unit]
+                    state.unit_dependencies[&other.unit]
                         .iter()
                         .find(|other_dep| other_dep.unit.mode == CompileMode::RunCustomBuild)
                         .cloned()
@@ -695,7 +725,11 @@ fn connect_run_custom_build_deps(unit_dependencies: &mut UnitGraph) {
 
     // And finally, add in all the missing dependencies!
     for (unit, new_deps) in new_deps {
-        unit_dependencies.get_mut(&unit).unwrap().extend(new_deps);
+        state
+            .unit_dependencies
+            .get_mut(&unit)
+            .unwrap()
+            .extend(new_deps);
     }
 }
 

tests/testsuite/build_script.rs

@@ -4158,3 +4158,48 @@ fn rerun_if_directory() {
     dirty();
     fresh();
 }
+
+#[cargo_test]
+fn test_with_dep_metadata() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = { path = 'bar' }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "build.rs",
+            r#"
+                fn main() {
+                    assert_eq!(std::env::var("DEP_BAR_FOO").unwrap(), "bar");
+                }
+            "#,
+        )
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.1.0"
+                links = 'bar'
+            "#,
+        )
+        .file("bar/src/lib.rs", "")
+        .file(
+            "bar/build.rs",
+            r#"
+                fn main() {
+                    println!("cargo:foo=bar");
+                }
+            "#,
+        )
+        .build();
+    p.cargo("test --lib").run();
+}