Auto merge of #8969 - alexcrichton:fix-cycle, r=ehuss

Fix the unit dependency graph with dev-dependency `links`

This commit fixes #8966 by updating the unit generation logic to avoid
generating an erroneous circular dependency between the execution of two
build scripts. This has been present for Cargo in a long time since #5651
(an accidental regression), but the situation appears rare enough that
we didn't get to it until now!

Closes #8966

Author: bors

src/cargo/core/compiler/unit_dependencies.rs

@@ -630,8 +630,17 @@ fn connect_run_custom_build_deps(unit_dependencies: &mut UnitGraph) {
         // 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 dep in deps {
                 if dep.unit.mode == CompileMode::RunCustomBuild {
                     reverse_deps_map
@@ -655,15 +664,16 @@ fn connect_run_custom_build_deps(unit_dependencies: &mut UnitGraph) {
             .keys()
             .filter(|k| k.mode == CompileMode::RunCustomBuild)
         {
-            // This is the lib that runs this custom build.
+            // This list of dependencies all depend on `unit`, an execution of
+            // the build script.
             let reverse_deps = match reverse_deps_map.get(unit) {
                 Some(set) => set,
                 None => continue,
             };
 
             let to_add = reverse_deps
                 .iter()
-                // Get all deps for lib.
+                // Get all sibling dependencies of `unit`
                 .flat_map(|reverse_dep| unit_dependencies[reverse_dep].iter())
                 // Only deps with `links`.
                 .filter(|other| {

tests/testsuite/build_script.rs

@@ -4034,3 +4034,40 @@ Caused by:
     // Restore permissions so that the directory can be deleted.
     fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap();
 }
+
+#[cargo_test]
+fn dev_dep_with_links() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+                authors = []
+                links = "x"
+
+                [dev-dependencies]
+                bar = { path = "./bar" }
+            "#,
+        )
+        .file("build.rs", "fn main() {}")
+        .file("src/lib.rs", "")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.1.0"
+                authors = []
+                links = "y"
+
+                [dependencies]
+                foo = { path = ".." }
+            "#,
+        )
+        .file("bar/build.rs", "fn main() {}")
+        .file("bar/src/lib.rs", "")
+        .build();
+    p.cargo("check --tests").run()
+}