From b6bcaf3318a7f71e0a0f39297f9508a2dcd34815 Mon Sep 17 00:00:00 2001 From: bors Date: Tue, 16 Mar 2021 18:23:15 +0000 Subject: [PATCH 1/2] Auto merge of #9276 - ehuss:fix-collision-root-removal, r=alexcrichton Fix doc duplicate removal of root units. The doc collision removal code didn't consider the possibility that there was a collision with a root unit. This caused problems in conjunction with #9142 where cargo would panic because a root unit got removed from the graph because of a filename collision. The solution here is to avoid removing root units during the doc collision sweep. This has a downside that this reintroduces a filename collision. An alternate solution would be to make the set of root units mutable, and remove from that list, but I figured this is simpler and more conservative. Fixes #9274 --- src/cargo/ops/cargo_compile.rs | 20 +++++------ tests/testsuite/collisions.rs | 66 ++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 981afe23580..dd5685b2693 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -1527,8 +1527,11 @@ fn remove_duplicate_doc( // Keep track of units to remove so that they can be efficiently removed // from the unit_deps. let mut removed_units: HashSet = HashSet::new(); - let mut remove = |units: Vec, reason: &str| { - for unit in units { + let mut remove = |units: Vec, reason: &str, cb: &dyn Fn(&Unit) -> bool| -> Vec { + let (to_remove, remaining_units): (Vec, Vec) = units + .into_iter() + .partition(|unit| cb(unit) && !root_units.contains(unit)); + for unit in to_remove { log::debug!( "removing duplicate doc due to {} for package {} target `{}`", reason, @@ -1538,6 +1541,7 @@ fn remove_duplicate_doc( unit_graph.remove(&unit); removed_units.insert(unit); } + remaining_units }; // Iterate over the duplicates and try to remove them from unit_graph. for (_crate_name, mut units) in all_docs { @@ -1550,14 +1554,11 @@ fn remove_duplicate_doc( .iter() .all(CompileKind::is_host) { - let (to_remove, remaining_units): (Vec, Vec) = - units.into_iter().partition(|unit| unit.kind.is_host()); // Note these duplicates may not be real duplicates, since they // might get merged in rebuild_unit_graph_shared. Either way, it // shouldn't hurt to remove them early (although the report in the // log might be confusing). - remove(to_remove, "host/target merger"); - units = remaining_units; + units = remove(units, "host/target merger", &|unit| unit.kind.is_host()); if units.len() == 1 { continue; } @@ -1579,10 +1580,9 @@ fn remove_duplicate_doc( units.sort_by(|a, b| a.pkg.version().partial_cmp(b.pkg.version()).unwrap()); // Remove any entries with version < newest. let newest_version = units.last().unwrap().pkg.version().clone(); - let (to_remove, keep_units): (Vec, Vec) = units - .into_iter() - .partition(|unit| unit.pkg.version() < &newest_version); - remove(to_remove, "older version"); + let keep_units = remove(units, "older version", &|unit| { + unit.pkg.version() < &newest_version + }); remaining_units.extend(keep_units); } else { remaining_units.extend(units); diff --git a/tests/testsuite/collisions.rs b/tests/testsuite/collisions.rs index d7deebc175c..2721746d6f6 100644 --- a/tests/testsuite/collisions.rs +++ b/tests/testsuite/collisions.rs @@ -478,3 +478,69 @@ fn collision_doc_target() { ) .run(); } + +#[cargo_test] +fn collision_with_root() { + // Check for a doc collision between a root package and a dependency. + // In this case, `foo-macro` comes from both the workspace and crates.io. + // This checks that the duplicate correction code doesn't choke on this + // by removing the root unit. + Package::new("foo-macro", "1.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["abc", "foo-macro"] + "#, + ) + .file( + "abc/Cargo.toml", + r#" + [package] + name = "abc" + version = "1.0.0" + + [dependencies] + foo-macro = "1.0" + "#, + ) + .file("abc/src/lib.rs", "") + .file( + "foo-macro/Cargo.toml", + r#" + [package] + name = "foo-macro" + version = "1.0.0" + + [lib] + proc-macro = true + + [dependencies] + abc = {path="../abc"} + "#, + ) + .file("foo-macro/src/lib.rs", "") + .build(); + + p.cargo("doc") + .with_stderr_unordered("\ +[UPDATING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] foo-macro v1.0.0 [..] +warning: output filename collision. +The lib target `foo-macro` in package `foo-macro v1.0.0` has the same output filename as the lib target `foo-macro` in package `foo-macro v1.0.0 [..]`. +Colliding filename is: [CWD]/target/doc/foo_macro/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 . +[CHECKING] foo-macro v1.0.0 +[DOCUMENTING] foo-macro v1.0.0 +[CHECKING] abc v1.0.0 [..] +[DOCUMENTING] foo-macro v1.0.0 [..] +[DOCUMENTING] abc v1.0.0 [..] +[FINISHED] [..] +") + .run(); +} From 77a13c03edb8830cee6f9e4050108fe587e20da5 Mon Sep 17 00:00:00 2001 From: bors Date: Tue, 16 Mar 2021 20:55:20 +0000 Subject: [PATCH 2/2] Auto merge of #9275 - ehuss:features-non-member, r=alexcrichton Fix --feature pkg/feat for V1 resolver for non-member. #8997 had an unintended regression where `-p foo --feature foo/feat` syntax where `foo` is an **optional non-member** fails with an error that `foo` did not match any packages. The issue is that the member/feature selection routine needed to slot this into the features for the package in the current working directory (it was incorrectly treating `foo` as a workspace member). V2 outright does not allow specifying features for non-workspace members. Fixes #9265 --- src/cargo/core/workspace.rs | 3 ++- tests/testsuite/package_features.rs | 31 +++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index e78814233b4..45276d45b58 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -1076,7 +1076,8 @@ impl<'cfg> Workspace<'cfg> { for feature in requested_features.features.iter() { if let Some(index) = feature.find('/') { let name = &feature[..index]; - if specs.iter().any(|spec| spec.name() == name) { + let is_member = self.members().any(|member| member.name() == name); + if is_member && specs.iter().any(|spec| spec.name() == name) { member_specific_features .entry(name) .or_default() diff --git a/tests/testsuite/package_features.rs b/tests/testsuite/package_features.rs index 0893e0c5bf3..15ed37e5cc9 100644 --- a/tests/testsuite/package_features.rs +++ b/tests/testsuite/package_features.rs @@ -458,3 +458,34 @@ fn resolver1_member_features() { .with_stdout("m1-feature set") .run(); } + +#[cargo_test] +fn resolver1_non_member_optional_feature() { + // --features x/y for an optional dependency `x` with the v1 resolver. + Package::new("bar", "1.0.0") + .feature("feat1", &[]) + .file( + "src/lib.rs", + r#" + #[cfg(not(feature = "feat1"))] + compile_error!("feat1 should be activated"); + "#, + ) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version="1.0", optional=true } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check -p bar --features bar/feat1").run(); +}