Auto merge of #9611 - ehuss:beta-1.54-backports, r=alexcrichton

Beta 1.54 backports

This backports the following changes to 1.54 beta:

* #9595 — Relax doc collision error.  This was an unintended regression where a warning was switched to an error in 1.54.
* #9563 — Change rustc-cdylib-link-arg error to a warning.  Validation was added in #9523 (1.54) that generated an error. However, it was unknown at the time that use of link args via dependencies was being used. Rather than regress those projects (which has been on stable since 1.50), this switches the validation to a warning to indicate this was not intended.
* #9604 — Disambiguate is_symlink. Required to get CI to pass.

crates/cargo-test-support/src/paths.rs

@@ -125,8 +125,6 @@ pub trait CargoPathExt {
     fn move_in_time<F>(&self, travel_amount: F)
     where
         F: Fn(i64, u32) -> (i64, u32);
-
-    fn is_symlink(&self) -> bool;
 }
 
 impl CargoPathExt for Path {
@@ -199,12 +197,14 @@ impl CargoPathExt for Path {
             });
         }
     }
+}
 
-    fn is_symlink(&self) -> bool {
-        fs::symlink_metadata(self)
-            .map(|m| m.file_type().is_symlink())
-            .unwrap_or(false)
-    }
+// Replace with std implementation when stabilized, see
+// https://github.com/rust-lang/rust/issues/85748
+pub fn is_symlink(path: &Path) -> bool {
+    fs::symlink_metadata(path)
+        .map(|m| m.file_type().is_symlink())
+        .unwrap_or(false)
 }
 
 fn do_op<F>(path: &Path, desc: &str, mut f: F)

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

@@ -507,17 +507,35 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             .collect::<Vec<_>>();
         // Sort for consistent error messages.
         keys.sort_unstable();
+        // These are kept separate to retain compatibility with older
+        // versions, which generated an error when there was a duplicate lib
+        // or bin (but the old code did not check bin<->lib collisions). To
+        // retain backwards compatibility, this only generates an error for
+        // duplicate libs or duplicate bins (but not both). Ideally this
+        // shouldn't be here, but since there isn't a complete workaround,
+        // yet, this retains the old behavior.
+        let mut doc_libs = HashMap::new();
+        let mut doc_bins = HashMap::new();
         for unit in keys {
+            if unit.mode.is_doc() && self.is_primary_package(unit) {
+                // These situations have been an error since before 1.0, so it
+                // is not a warning like the other situations.
+                if unit.target.is_lib() {
+                    if let Some(prev) = doc_libs.insert((unit.target.crate_name(), unit.kind), unit)
+                    {
+                        doc_collision_error(unit, prev)?;
+                    }
+                } else if let Some(prev) =
+                    doc_bins.insert((unit.target.crate_name(), unit.kind), unit)
+                {
+                    doc_collision_error(unit, prev)?;
+                }
+            }
             for output in self.outputs(unit)?.iter() {
                 if let Some(other_unit) = output_collisions.insert(output.path.clone(), unit) {
                     if unit.mode.is_doc() {
                         // See https://github.com/rust-lang/rust/issues/56169
                         // and https://github.com/rust-lang/rust/issues/61378
-                        if self.is_primary_package(unit) {
-                            // This has been an error since before 1.0, so it
-                            // is not a warning like the other situations.
-                            doc_collision_error(unit, other_unit)?;
-                        }
                         report_collision(unit, other_unit, &output.path, rustdoc_suggestion)?;
                     } else {
                         report_collision(unit, other_unit, &output.path, suggestion)?;

src/cargo/core/compiler/custom_build.rs

@@ -38,6 +38,9 @@ pub struct BuildOutput {
     /// Environment variables which, when changed, will cause a rebuild.
     pub rerun_if_env_changed: Vec<String>,
     /// Warnings generated by this build.
+    ///
+    /// These are only displayed if this is a "local" package, `-vv` is used,
+    /// or there is a build error for any target in this package.
     pub warnings: Vec<String>,
 }
 
@@ -571,13 +574,16 @@ impl BuildOutput {
                 "rustc-link-search" => library_paths.push(PathBuf::from(value)),
                 "rustc-link-arg-cdylib" | "rustc-cdylib-link-arg" => {
                     if !targets.iter().any(|target| target.is_cdylib()) {
-                        bail!(
-                            "invalid instruction `cargo:{}` from {}\n\
-                             The package {} does not have a cdylib target.",
-                            key,
-                            whence,
-                            pkg_descr
-                        );
+                        warnings.push(format!(
+                            "cargo:{} was specified in the build script of {}, \
+                             but that package does not contain a cdylib target\n\
+                             \n\
+                             Allowing this was an unintended change in the 1.50 \
+                             release, and may become an error in the future. \
+                             For more information, see \
+                             <https://github.com/rust-lang/cargo/issues/9562>.",
+                            key, pkg_descr
+                        ));
                     }
                     linker_args.push((LinkType::Cdylib, value))
                 }

src/cargo/core/compiler/mod.rs

@@ -394,7 +394,12 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
             }
 
             for (lt, arg) in &output.linker_args {
-                if lt.applies_to(target) {
+                // There was an unintentional change where cdylibs were
+                // allowed to be passed via transitive dependencies. This
+                // clause should have been kept in the `if` block above. For
+                // now, continue allowing it for cdylib only.
+                // See https://github.com/rust-lang/cargo/issues/9562
+                if lt.applies_to(target) && (key.0 == current_id || *lt == LinkType::Cdylib) {
                     rustc.arg("-C").arg(format!("link-arg={}", arg));
                 }
             }

tests/testsuite/build.rs

@@ -4505,6 +4505,7 @@ fn building_a_dependent_crate_witout_bin_should_fail() {
 #[cargo_test]
 #[cfg(any(target_os = "macos", target_os = "ios"))]
 fn uplift_dsym_of_bin_on_mac() {
+    use cargo_test_support::paths::is_symlink;
     let p = project()
         .file("src/main.rs", "fn main() { panic!(); }")
         .file("src/bin/b.rs", "fn main() { panic!(); }")
@@ -4517,7 +4518,7 @@ fn uplift_dsym_of_bin_on_mac() {
         .run();
     assert!(p.target_debug_dir().join("foo.dSYM").is_dir());
     assert!(p.target_debug_dir().join("b.dSYM").is_dir());
-    assert!(p.target_debug_dir().join("b.dSYM").is_symlink());
+    assert!(is_symlink(&p.target_debug_dir().join("b.dSYM")));
     assert!(p.target_debug_dir().join("examples/c.dSYM").is_dir());
     assert!(!p.target_debug_dir().join("c.dSYM").exists());
     assert!(!p.target_debug_dir().join("d.dSYM").exists());
@@ -4526,6 +4527,7 @@ fn uplift_dsym_of_bin_on_mac() {
 #[cargo_test]
 #[cfg(any(target_os = "macos", target_os = "ios"))]
 fn uplift_dsym_of_bin_on_mac_when_broken_link_exists() {
+    use cargo_test_support::paths::is_symlink;
     let p = project()
         .file("src/main.rs", "fn main() { panic!(); }")
         .build();
@@ -4544,7 +4546,7 @@ fn uplift_dsym_of_bin_on_mac_when_broken_link_exists() {
             .join("foo-baaaaaadbaaaaaad.dSYM"),
         &dsym,
     );
-    assert!(dsym.is_symlink());
+    assert!(is_symlink(&dsym));
     assert!(!dsym.exists());
 
     p.cargo("build").enable_mac_dsym().run();

tests/testsuite/build_script_extra_link_arg.rs

@@ -1,6 +1,11 @@
 //! Tests for -Zextra-link-arg.
 
-use cargo_test_support::{basic_bin_manifest, project};
+// NOTE: Many of these tests use `without_status()` when passing bogus flags
+// because MSVC link.exe just gives a warning on unknown flags (how helpful!),
+// and other linkers will return an error.
+
+use cargo_test_support::registry::Package;
+use cargo_test_support::{basic_bin_manifest, basic_manifest, project};
 
 #[cargo_test]
 fn build_script_extra_link_arg_bin() {
@@ -125,14 +130,16 @@ fn link_arg_missing_target() {
         )
         .build();
 
-    p.cargo("check")
-        .with_status(101)
-        .with_stderr("\
-[COMPILING] foo [..]
-error: invalid instruction `cargo:rustc-link-arg-cdylib` from build script of `foo v0.0.1 ([ROOT]/foo)`
-The package foo v0.0.1 ([ROOT]/foo) does not have a cdylib target.
-")
-        .run();
+    // TODO: Uncomment this if cdylib restriction is re-added (see
+    // cdylib_link_arg_transitive below).
+    //     p.cargo("check")
+    //         .with_status(101)
+    //         .with_stderr("\
+    // [COMPILING] foo [..]
+    // error: invalid instruction `cargo:rustc-link-arg-cdylib` from build script of `foo v0.0.1 ([ROOT]/foo)`
+    // The package foo v0.0.1 ([ROOT]/foo) does not have a cdylib target.
+    // ")
+    //         .run();
 
     p.change_file(
         "build.rs",
@@ -183,3 +190,113 @@ The instruction should have the form cargo:rustc-link-arg-bin=BIN=ARG
         )
         .run();
 }
+
+#[cargo_test]
+fn cdylib_link_arg_transitive() {
+    // There was an unintended regression in 1.50 where rustc-link-arg-cdylib
+    // arguments from dependencies were being applied in the parent package.
+    // Previously it was silently ignored.
+    // See https://github.com/rust-lang/cargo/issues/9562
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [lib]
+                crate-type = ["cdylib"]
+
+                [dependencies]
+                bar = {path="bar"}
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("bar", "1.0.0"))
+        .file("bar/src/lib.rs", "")
+        .file(
+            "bar/build.rs",
+            r#"
+                fn main() {
+                    println!("cargo:rustc-link-arg-cdylib=--bogus");
+                }
+            "#,
+        )
+        .build();
+    p.cargo("build -v")
+        .without_status()
+        .with_stderr_contains(
+            "\
+[COMPILING] bar v1.0.0 [..]
+[RUNNING] `rustc --crate-name build_script_build bar/build.rs [..]
+[RUNNING] `[..]build-script-build[..]
+warning: cargo:rustc-link-arg-cdylib was specified in the build script of bar v1.0.0 \
+([ROOT]/foo/bar), but that package does not contain a cdylib target
+
+Allowing this was an unintended change in the 1.50 release, and may become an error in \
+the future. For more information, see <https://github.com/rust-lang/cargo/issues/9562>.
+[RUNNING] `rustc --crate-name bar bar/src/lib.rs [..]
+[COMPILING] foo v0.1.0 [..]
+[RUNNING] `rustc --crate-name foo src/lib.rs [..]-C link-arg=--bogus[..]`
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn link_arg_transitive_not_allowed() {
+    // Verify that transitive dependencies don't pass link args.
+    //
+    // Note that rustc-link-arg doesn't have any errors or warnings when it is
+    // unused. Perhaps that could be more aggressive, but it is difficult
+    // since it could be used for test binaries.
+    Package::new("bar", "1.0.0")
+        .file("src/lib.rs", "")
+        .file(
+            "build.rs",
+            r#"
+                fn main() {
+                    println!("cargo:rustc-link-arg=--bogus");
+                }
+            "#,
+        )
+        .publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [lib]
+                crate-type = ["cdylib"]
+
+                [dependencies]
+                bar = "1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("build -v -Zextra-link-arg")
+        .masquerade_as_nightly_cargo()
+        .with_stderr(
+            "\
+[UPDATING] [..]
+[DOWNLOADING] [..]
+[DOWNLOADED] [..]
+[COMPILING] bar v1.0.0
+[RUNNING] `rustc --crate-name build_script_build [..]
+[RUNNING] `[..]/build-script-build[..]
+[RUNNING] `rustc --crate-name bar [..]
+[COMPILING] foo v0.1.0 [..]
+[RUNNING] `rustc --crate-name foo src/lib.rs [..]
+[FINISHED] dev [..]
+",
+        )
+        .with_stderr_does_not_contain("--bogus")
+        .run();
+}

tests/testsuite/clean.rs

@@ -1,6 +1,6 @@
 //! Tests for the `cargo clean` command.
 
-use cargo_test_support::paths::CargoPathExt;
+use cargo_test_support::paths::is_symlink;
 use cargo_test_support::registry::Package;
 use cargo_test_support::{basic_bin_manifest, basic_manifest, git, main_file, project, rustc_host};
 use std::env;
@@ -438,7 +438,7 @@ fn assert_all_clean(build_dir: &Path) {
         {
             continue;
         }
-        if path.is_symlink() || path.is_file() {
+        if is_symlink(path) || path.is_file() {
             panic!("{:?} was not cleaned", path);
         }
     }

tests/testsuite/doc.rs

@@ -271,14 +271,19 @@ fn doc_multiple_targets_same_name() {
         .build();
 
     p.cargo("doc --workspace")
-        .with_status(101)
-        .with_stderr(
+        .with_stderr_unordered(
             "\
-error: document output filename collision
-The bin `foo_lib` in package `foo v0.1.0 ([ROOT]/foo/foo)` has the same name as \
-the lib `foo_lib` in package `bar v0.1.0 ([ROOT]/foo/bar)`.
-Only one may be documented at once since they output to the same path.
-Consider documenting only one, renaming one, or marking one with `doc = false` in Cargo.toml.
+warning: output filename collision.
+The bin target `foo_lib` in package `foo v0.1.0 ([ROOT]/foo/foo)` \
+has the same output filename as the lib target `foo_lib` in package \
+`bar v0.1.0 ([ROOT]/foo/bar)`.
+Colliding filename is: [ROOT]/foo/target/doc/foo_lib/index.html
+The targets should have unique names.
+This is a known bug where multiple crates with the same name use
+the same path; see <https://github.com/rust-lang/cargo/issues/6313>.
+[DOCUMENTING] bar v0.1.0 ([ROOT]/foo/bar)
+[DOCUMENTING] foo v0.1.0 ([ROOT]/foo/foo)
+[FINISHED] [..]
 ",
         )
         .run();