Skip to content

Commit

Permalink
Auto merge of rust-lang#14239 - epage:git, r=weihanglo
Browse files Browse the repository at this point in the history 
fix(source): Don't warn about unreferenced duplicate packages

### What does this PR try to resolve?

This also improves the message, consolidating multiple duplicates and saying which was loaded instead, as it naturally fell out of the design

Fixes rust-lang#10752

### How should we test and review this PR?

### Additional information

We're still subject to rust-lang#13724 and fully load every manifest, even if we don't use it.  I'm exploring that topic at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Redundant.20code.20in.20.60GitSouce.60.3F/near/450783427

This change builds on
- rust-lang#13993
- rust-lang#14169
- rust-lang#14231
- rust-lang#14234
  • Loading branch information
@bors
bors committed Jul 15, 2024
2 parents b31577d + ed56f1e commit 2d658f2
Show file tree
Hide file tree
Showing 2 changed files with 167 additions and 31 deletions.
84 changes: 60 additions & 24 deletions src/cargo/sources/path.rs
View file
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>>,
/// 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
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 = ["wycats@example.com"]
[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 = ["wycats@example.com"]
[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

0 comments on commit 2d658f2

Please sign in to comment.