From 6ef861da961c62361c0d34383c845849ba3a26fa Mon Sep 17 00:00:00 2001 From: bors Date: Tue, 22 Jun 2021 19:27:25 +0000 Subject: [PATCH 1/3] Auto merge of #9563 - ehuss:link-cdylib-warning, r=alexcrichton Change `rustc-cdylib-link-arg` error to a warning. In #9523, an error was added if `cargo:rustc-cdylib-link-arg` was issued in a build script without actually having a cdylib target. This uncovered that there was an unintentional change in #8441 to cause those link args to be applied to transitive dependencies. This changes it so that the error is now a warning, with a note that this may become an error in the future. It also changes it so that the unstable `rustc-link-arg*` instructions only apply to the package that emitted them. --- src/cargo/core/compiler/custom_build.rs | 20 ++- src/cargo/core/compiler/mod.rs | 7 +- .../testsuite/build_script_extra_link_arg.rs | 135 ++++++++++++++++-- 3 files changed, 145 insertions(+), 17 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index ed13a9d3e..8fa6cb8b1 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/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, /// 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, } @@ -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 \ + .", + key, pkg_descr + )); } linker_args.push((LinkType::Cdylib, value)) } diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 0d72a9491..e5ea06598 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -394,7 +394,12 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc) -> 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)); } } diff --git a/tests/testsuite/build_script_extra_link_arg.rs b/tests/testsuite/build_script_extra_link_arg.rs index 6e8384b8a..79baee04d 100644 --- a/tests/testsuite/build_script_extra_link_arg.rs +++ b/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 . +[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(); +} From a9eb3efa19025110bf6df1536e8bf48045cc25b1 Mon Sep 17 00:00:00 2001 From: bors Date: Fri, 18 Jun 2021 03:12:08 +0000 Subject: [PATCH 2/3] Auto merge of #9595 - ehuss:ws-doc-collision-back-compat, r=alexcrichton Relax doc collision error. #9526 moved the `cargo doc` output name collision code to be shared with the other collision detection code. However, the way it detected collisions wasn't quite the same, and started generating errors for situations that were just warnings before. It was intended that the code should behave the same, so this PR relaxes the checks to be more like the original code. (It's still not 100% the same, but should be close enough.) Closes #9564 --- src/cargo/core/compiler/context/mod.rs | 28 +++++++++++++++++++++----- tests/testsuite/doc.rs | 19 ++++++++++------- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index cea171c7d..a32a68116 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -507,17 +507,35 @@ impl<'a, 'cfg> Context<'a, 'cfg> { .collect::>(); // 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)?; diff --git a/tests/testsuite/doc.rs b/tests/testsuite/doc.rs index 1dcf7735b..c61a27e62 100644 --- a/tests/testsuite/doc.rs +++ b/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 . +[DOCUMENTING] bar v0.1.0 ([ROOT]/foo/bar) +[DOCUMENTING] foo v0.1.0 ([ROOT]/foo/foo) +[FINISHED] [..] ", ) .run(); From 09ed3d2fcd77aa36372c4a3405a9f32ad1235288 Mon Sep 17 00:00:00 2001 From: bors Date: Mon, 21 Jun 2021 14:06:50 +0000 Subject: [PATCH 3/3] Auto merge of #9604 - ehuss:is_symlink, r=alexcrichton Disambiguate is_symlink. `Path::is_symlink` was added in https://github.com/rust-lang/rust/pull/85747 which triggers the `unstable_name_collisions` lint, breaking Cargo's CI. This switches it to a free function to avoid the collision. --- crates/cargo-test-support/src/paths.rs | 14 +++++++------- tests/testsuite/build.rs | 6 ++++-- tests/testsuite/clean.rs | 4 ++-- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/crates/cargo-test-support/src/paths.rs b/crates/cargo-test-support/src/paths.rs index 192657dae..fa32a0c2e 100644 --- a/crates/cargo-test-support/src/paths.rs +++ b/crates/cargo-test-support/src/paths.rs @@ -125,8 +125,6 @@ pub trait CargoPathExt { fn move_in_time(&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(path: &Path, desc: &str, mut f: F) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 277aa6b41..505c06525 100644 --- a/tests/testsuite/build.rs +++ b/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(); diff --git a/tests/testsuite/clean.rs b/tests/testsuite/clean.rs index 4d4475231..cabfd6410 100644 --- a/tests/testsuite/clean.rs +++ b/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); } }