Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(source): Don't warn about unreferenced duplicate packages #14239

Merged
merged 5 commits into from
Jul 15, 2024
Merged

fix(source): Don't warn about unreferenced duplicate packages #14239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a25400
test(git): Show unused duplicate package warning
epage May 30, 2024
6c0f14c
fix(source): Make duplicate selection predictable
epage Jul 12, 2024
58ee635
fix(source): Consolidate duplicate package warnings
epage Jul 10, 2024
3124b0c
refactor(source): Delay the duplicate name check
epage Jul 11, 2024
ed56f1e
fix(source): Don't warn about unreferenced duplicate packages
epage Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 60 additions & 24 deletions src/cargo/sources/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,11 @@ pub struct RecursivePathSource<'gctx> {
/// Whether this source has loaded all package information it may contain.
loaded: bool,
/// Packages that this sources has discovered.
packages: HashMap<PackageId, Package>,
///
/// Tracking all packages for a given ID to warn on-demand for unused packages
packages: HashMap<PackageId, Vec<Package>>,
epage marked this conversation as resolved.
Show resolved Hide resolved
/// Avoid redundant unused package warnings
warned_duplicate: HashSet<PackageId>,
gctx: &'gctx GlobalContext,
}

Expand All @@ -245,6 +249,7 @@ impl<'gctx> RecursivePathSource<'gctx> {
path: root.to_path_buf(),
loaded: false,
packages: Default::default(),
warned_duplicate: Default::default(),
gctx,
}
}
Expand All @@ -253,7 +258,13 @@ impl<'gctx> RecursivePathSource<'gctx> {
/// filesystem if package information haven't yet loaded.
pub fn read_packages(&mut self) -> CargoResult<Vec<Package>> {
self.load()?;
Ok(self.packages.iter().map(|(_, v)| v.clone()).collect())
Ok(self
.packages
.iter()
.map(|(pkg_id, v)| {
first_package(*pkg_id, v, &mut self.warned_duplicate, self.gctx).clone()
})
.collect())
}

/// List all files relevant to building this package inside this source.
Expand Down Expand Up @@ -311,7 +322,15 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>> {
self.load()?;
for s in self.packages.values().map(|p| p.summary()) {
for s in self
.packages
.iter()
.filter(|(pkg_id, _)| pkg_id.name() == dep.package_name())
.map(|(pkg_id, pkgs)| {
first_package(*pkg_id, pkgs, &mut self.warned_duplicate, self.gctx)
})
.map(|p| p.summary())
{
let matched = match kind {
QueryKind::Exact => dep.matches(s),
QueryKind::Alternatives => true,
Expand Down Expand Up @@ -340,7 +359,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
trace!("getting packages; id={}", id);
self.load()?;
let pkg = self.packages.get(&id);
pkg.cloned()
pkg.map(|pkgs| first_package(id, pkgs, &mut self.warned_duplicate, self.gctx).clone())
.map(MaybePackage::Ready)
.ok_or_else(|| internal(format!("failed to find {} in path source", id)))
}
Expand Down Expand Up @@ -384,6 +403,38 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
}
}

fn first_package<'p>(
pkg_id: PackageId,
pkgs: &'p Vec<Package>,
warned_duplicate: &mut HashSet<PackageId>,
gctx: &GlobalContext,
) -> &'p Package {
if pkgs.len() != 1 && warned_duplicate.insert(pkg_id) {
let ignored = pkgs[1..]
.iter()
// We can assume a package with publish = false isn't intended to be seen
// by users so we can hide the warning about those since the user is unlikely
// to care about those cases.
.filter(|pkg| pkg.publish().is_none())
.collect::<Vec<_>>();
if !ignored.is_empty() {
use std::fmt::Write as _;

let plural = if ignored.len() == 1 { "" } else { "s" };
let mut msg = String::new();
let _ = writeln!(&mut msg, "skipping duplicate package{plural} `{pkg_id}`:");
for ignored in ignored {
let manifest_path = ignored.manifest_path().display();
let _ = writeln!(&mut msg, " {manifest_path}");
}
let manifest_path = pkgs[0].manifest_path().display();
let _ = writeln!(&mut msg, "in favor of {manifest_path}");
let _ = gctx.shell().warn(msg);
}
}
&pkgs[0]
}

/// List all files relevant to building this package inside this source.
///
/// This function will use the appropriate methods to determine the
Expand Down Expand Up @@ -758,7 +809,7 @@ fn read_packages(
path: &Path,
source_id: SourceId,
gctx: &GlobalContext,
) -> CargoResult<HashMap<PackageId, Package>> {
) -> CargoResult<HashMap<PackageId, Vec<Package>>> {
let mut all_packages = HashMap::new();
let mut visited = HashSet::<PathBuf>::new();
let mut errors = Vec::<anyhow::Error>::new();
Expand Down Expand Up @@ -882,6 +933,8 @@ fn walk(path: &Path, callback: &mut dyn FnMut(&Path) -> CargoResult<bool>) -> Ca
return Err(e.context(cx));
}
};
let mut dirs = dirs.collect::<Vec<_>>();
dirs.sort_unstable_by_key(|d| d.as_ref().ok().map(|d| d.file_name()));
for dir in dirs {
let dir = dir?;
if dir.file_type()?.is_dir() {
Expand All @@ -897,7 +950,7 @@ fn has_manifest(path: &Path) -> bool {

fn read_nested_packages(
path: &Path,
all_packages: &mut HashMap<PackageId, Package>,
all_packages: &mut HashMap<PackageId, Vec<Package>>,
source_id: SourceId,
gctx: &GlobalContext,
visited: &mut HashSet<PathBuf>,
Expand Down Expand Up @@ -936,24 +989,7 @@ fn read_nested_packages(
let pkg = Package::new(manifest, &manifest_path);

let pkg_id = pkg.package_id();
use std::collections::hash_map::Entry;
match all_packages.entry(pkg_id) {
Entry::Vacant(v) => {
v.insert(pkg);
}
Entry::Occupied(_) => {
// We can assume a package with publish = false isn't intended to be seen
// by users so we can hide the warning about those since the user is unlikely
// to care about those cases.
if pkg.publish().is_none() {
let _ = gctx.shell().warn(format!(
"skipping duplicate package `{}` found at `{}`",
pkg.name(),
path.display()
));
}
}
}
all_packages.entry(pkg_id).or_default().push(pkg);

// Registry sources are not allowed to have `path=` dependencies because
// they're all translated to actual registry dependencies.
Expand Down
114 changes: 107 additions & 7 deletions tests/testsuite/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,31 +1132,125 @@ fn ambiguous_published_deps() {
let git_project = git::new("dep", |project| {
project
.file(
"aaa/Cargo.toml",
"duplicate1/Cargo.toml",
&format!(
r#"
[package]
name = "bar"
name = "duplicate"
version = "0.5.0"
edition = "2015"
publish = true
"#
),
)
.file("duplicate1/src/lib.rs", "")
.file(
"duplicate2/Cargo.toml",
&format!(
r#"
[package]
name = "duplicate"
version = "0.5.0"
edition = "2015"
publish = true
"#
),
)
.file("duplicate2/src/lib.rs", "")
});

let p = project
.file(
"Cargo.toml",
&format!(
r#"
[package]

name = "foo"
version = "0.5.0"
edition = "2015"
authors = ["[email protected]"]

[dependencies.duplicate]
git = '{}'
"#,
git_project.url()
),
)
.file("src/main.rs", "fn main() { }")
.build();

p.cargo("build").run();
p.cargo("run")
.with_stderr_data(str![[r#"
[WARNING] skipping duplicate package `duplicate v0.5.0 ([ROOTURL]/dep#[..])`:
[ROOT]/home/.cargo/git/checkouts/dep-[HASH]/[..]/duplicate2/Cargo.toml
in favor of [ROOT]/home/.cargo/git/checkouts/dep-[HASH]/[..]/duplicate1/Cargo.toml

[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[RUNNING] `target/debug/foo[EXE]`

"#]])
.run();
}

#[cargo_test]
fn unused_ambiguous_published_deps() {
let project = project();
let git_project = git::new("dep", |project| {
project
.file(
"unique/Cargo.toml",
&format!(
r#"
[package]
name = "unique"
version = "0.5.0"
edition = "2015"
publish = true
"#
),
)
.file("unique/src/lib.rs", "")
.file(
"duplicate1/Cargo.toml",
&format!(
r#"
[package]
name = "duplicate"
version = "0.5.0"
edition = "2015"
publish = true
"#
),
)
.file("aaa/src/lib.rs", "")
.file("duplicate1/src/lib.rs", "")
.file(
"bbb/Cargo.toml",
"duplicate2/Cargo.toml",
&format!(
r#"
[package]
name = "duplicate"
version = "0.5.0"
edition = "2015"
publish = true
"#
),
)
.file("duplicate2/src/lib.rs", "")
.file(
"invalid/Cargo.toml",
&format!(
r#"
[package
name = "bar"
version = "0.5.0"
edition = "2015"
publish = true
"#
),
)
.file("bbb/src/lib.rs", "")
.file("invalid/src/lib.rs", "")
});

let p = project
Expand All @@ -1171,7 +1265,7 @@ fn ambiguous_published_deps() {
edition = "2015"
authors = ["[email protected]"]

[dependencies.bar]
[dependencies.unique]
git = '{}'
"#,
git_project.url()
Expand All @@ -1183,7 +1277,13 @@ fn ambiguous_published_deps() {
p.cargo("build").run();
p.cargo("run")
.with_stderr_data(str![[r#"
[WARNING] skipping duplicate package `bar` found at `[ROOT]/home/.cargo/git/checkouts/dep-[HASH]/[..]`
[ERROR] invalid table header
expected `.`, `]`
--> ../home/.cargo/git/checkouts/dep-[HASH]/[..]/invalid/Cargo.toml:2:29
|
2 | [package
| ^
|
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[RUNNING] `target/debug/foo[EXE]`

Expand Down