Skip to content

Commit 2d658f2

Browse files
borsbors
committed
Auto merge of #14239 - epage:git, r=weihanglo
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 #10752 ### How should we test and review this PR? ### Additional information We're still subject to #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 - #13993 - #14169 - #14231 - #14234
2 parents b31577d + ed56f1e commit 2d658f2

File tree

2 files changed

+167
-31
lines changed

2 files changed

+167
-31
lines changed

‎src/cargo/sources/path.rs‎

Lines changed: 60 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,11 @@ pub struct RecursivePathSource<'gctx> {
226226
/// Whether this source has loaded all package information it may contain.
227227
loaded: bool,
228228
/// Packages that this sources has discovered.
229-
packages: HashMap<PackageId, Package>,
229+
///
230+
/// Tracking all packages for a given ID to warn on-demand for unused packages
231+
packages: HashMap<PackageId, Vec<Package>>,
232+
/// Avoid redundant unused package warnings
233+
warned_duplicate: HashSet<PackageId>,
230234
gctx: &'gctx GlobalContext,
231235
}
232236

@@ -245,6 +249,7 @@ impl<'gctx> RecursivePathSource<'gctx> {
245249
path: root.to_path_buf(),
246250
loaded: false,
247251
packages: Default::default(),
252+
warned_duplicate: Default::default(),
248253
gctx,
249254
}
250255
}
@@ -253,7 +258,13 @@ impl<'gctx> RecursivePathSource<'gctx> {
253258
/// filesystem if package information haven't yet loaded.
254259
pub fn read_packages(&mut self) -> CargoResult<Vec<Package>> {
255260
self.load()?;
256-
Ok(self.packages.iter().map(|(_, v)| v.clone()).collect())
261+
Ok(self
262+
.packages
263+
.iter()
264+
.map(|(pkg_id, v)| {
265+
first_package(*pkg_id, v, &mut self.warned_duplicate, self.gctx).clone()
266+
})
267+
.collect())
257268
}
258269

259270
/// List all files relevant to building this package inside this source.
@@ -311,7 +322,15 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
311322
f: &mut dyn FnMut(IndexSummary),
312323
) -> Poll<CargoResult<()>> {
313324
self.load()?;
314-
for s in self.packages.values().map(|p| p.summary()) {
325+
for s in self
326+
.packages
327+
.iter()
328+
.filter(|(pkg_id, _)| pkg_id.name() == dep.package_name())
329+
.map(|(pkg_id, pkgs)| {
330+
first_package(*pkg_id, pkgs, &mut self.warned_duplicate, self.gctx)
331+
})
332+
.map(|p| p.summary())
333+
{
315334
let matched = match kind {
316335
QueryKind::Exact => dep.matches(s),
317336
QueryKind::Alternatives => true,
@@ -340,7 +359,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
340359
trace!("getting packages; id={}", id);
341360
self.load()?;
342361
let pkg = self.packages.get(&id);
343-
pkg.cloned()
362+
pkg.map(|pkgs| first_package(id, pkgs, &mut self.warned_duplicate, self.gctx).clone())
344363
.map(MaybePackage::Ready)
345364
.ok_or_else(|| internal(format!("failed to find {} in path source", id)))
346365
}
@@ -384,6 +403,38 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
384403
}
385404
}
386405

406+
fn first_package<'p>(
407+
pkg_id: PackageId,
408+
pkgs: &'p Vec<Package>,
409+
warned_duplicate: &mut HashSet<PackageId>,
410+
gctx: &GlobalContext,
411+
) -> &'p Package {
412+
if pkgs.len() != 1 && warned_duplicate.insert(pkg_id) {
413+
let ignored = pkgs[1..]
414+
.iter()
415+
// We can assume a package with publish = false isn't intended to be seen
416+
// by users so we can hide the warning about those since the user is unlikely
417+
// to care about those cases.
418+
.filter(|pkg| pkg.publish().is_none())
419+
.collect::<Vec<_>>();
420+
if !ignored.is_empty() {
421+
use std::fmt::Write as _;
422+
423+
let plural = if ignored.len() == 1 { "" } else { "s" };
424+
let mut msg = String::new();
425+
let _ = writeln!(&mut msg, "skipping duplicate package{plural} `{pkg_id}`:");
426+
for ignored in ignored {
427+
let manifest_path = ignored.manifest_path().display();
428+
let _ = writeln!(&mut msg, " {manifest_path}");
429+
}
430+
let manifest_path = pkgs[0].manifest_path().display();
431+
let _ = writeln!(&mut msg, "in favor of {manifest_path}");
432+
let _ = gctx.shell().warn(msg);
433+
}
434+
}
435+
&pkgs[0]
436+
}
437+
387438
/// List all files relevant to building this package inside this source.
388439
///
389440
/// This function will use the appropriate methods to determine the
@@ -758,7 +809,7 @@ fn read_packages(
758809
path: &Path,
759810
source_id: SourceId,
760811
gctx: &GlobalContext,
761-
) -> CargoResult<HashMap<PackageId, Package>> {
812+
) -> CargoResult<HashMap<PackageId, Vec<Package>>> {
762813
let mut all_packages = HashMap::new();
763814
let mut visited = HashSet::<PathBuf>::new();
764815
let mut errors = Vec::<anyhow::Error>::new();
@@ -882,6 +933,8 @@ fn walk(path: &Path, callback: &mut dyn FnMut(&Path) -> CargoResult<bool>) -> Ca
882933
return Err(e.context(cx));
883934
}
884935
};
936+
let mut dirs = dirs.collect::<Vec<_>>();
937+
dirs.sort_unstable_by_key(|d| d.as_ref().ok().map(|d| d.file_name()));
885938
for dir in dirs {
886939
let dir = dir?;
887940
if dir.file_type()?.is_dir() {
@@ -897,7 +950,7 @@ fn has_manifest(path: &Path) -> bool {
897950

898951
fn read_nested_packages(
899952
path: &Path,
900-
all_packages: &mut HashMap<PackageId, Package>,
953+
all_packages: &mut HashMap<PackageId, Vec<Package>>,
901954
source_id: SourceId,
902955
gctx: &GlobalContext,
903956
visited: &mut HashSet<PathBuf>,
@@ -936,24 +989,7 @@ fn read_nested_packages(
936989
let pkg = Package::new(manifest, &manifest_path);
937990

938991
let pkg_id = pkg.package_id();
939-
use std::collections::hash_map::Entry;
940-
match all_packages.entry(pkg_id) {
941-
Entry::Vacant(v) => {
942-
v.insert(pkg);
943-
}
944-
Entry::Occupied(_) => {
945-
// We can assume a package with publish = false isn't intended to be seen
946-
// by users so we can hide the warning about those since the user is unlikely
947-
// to care about those cases.
948-
if pkg.publish().is_none() {
949-
let _ = gctx.shell().warn(format!(
950-
"skipping duplicate package `{}` found at `{}`",
951-
pkg.name(),
952-
path.display()
953-
));
954-
}
955-
}
956-
}
992+
all_packages.entry(pkg_id).or_default().push(pkg);
957993

958994
// Registry sources are not allowed to have `path=` dependencies because
959995
// they're all translated to actual registry dependencies.

‎tests/testsuite/git.rs‎

Lines changed: 107 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,31 +1132,125 @@ fn ambiguous_published_deps() {
11321132
let git_project = git::new("dep", |project| {
11331133
project
11341134
.file(
1135-
"aaa/Cargo.toml",
1135+
"duplicate1/Cargo.toml",
11361136
&format!(
11371137
r#"
11381138
[package]
1139-
name = "bar"
1139+
name = "duplicate"
1140+
version = "0.5.0"
1141+
edition = "2015"
1142+
publish = true
1143+
"#
1144+
),
1145+
)
1146+
.file("duplicate1/src/lib.rs", "")
1147+
.file(
1148+
"duplicate2/Cargo.toml",
1149+
&format!(
1150+
r#"
1151+
[package]
1152+
name = "duplicate"
1153+
version = "0.5.0"
1154+
edition = "2015"
1155+
publish = true
1156+
"#
1157+
),
1158+
)
1159+
.file("duplicate2/src/lib.rs", "")
1160+
});
1161+
1162+
let p = project
1163+
.file(
1164+
"Cargo.toml",
1165+
&format!(
1166+
r#"
1167+
[package]
1168+
1169+
name = "foo"
1170+
version = "0.5.0"
1171+
edition = "2015"
1172+
authors = ["[email protected]"]
1173+
1174+
[dependencies.duplicate]
1175+
git = '{}'
1176+
"#,
1177+
git_project.url()
1178+
),
1179+
)
1180+
.file("src/main.rs", "fn main() { }")
1181+
.build();
1182+
1183+
p.cargo("build").run();
1184+
p.cargo("run")
1185+
.with_stderr_data(str![[r#"
1186+
[WARNING] skipping duplicate package `duplicate v0.5.0 ([ROOTURL]/dep#[..])`:
1187+
[ROOT]/home/.cargo/git/checkouts/dep-[HASH]/[..]/duplicate2/Cargo.toml
1188+
in favor of [ROOT]/home/.cargo/git/checkouts/dep-[HASH]/[..]/duplicate1/Cargo.toml
1189+
1190+
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
1191+
[RUNNING] `target/debug/foo[EXE]`
1192+
1193+
"#]])
1194+
.run();
1195+
}
1196+
1197+
#[cargo_test]
1198+
fn unused_ambiguous_published_deps() {
1199+
let project = project();
1200+
let git_project = git::new("dep", |project| {
1201+
project
1202+
.file(
1203+
"unique/Cargo.toml",
1204+
&format!(
1205+
r#"
1206+
[package]
1207+
name = "unique"
1208+
version = "0.5.0"
1209+
edition = "2015"
1210+
publish = true
1211+
"#
1212+
),
1213+
)
1214+
.file("unique/src/lib.rs", "")
1215+
.file(
1216+
"duplicate1/Cargo.toml",
1217+
&format!(
1218+
r#"
1219+
[package]
1220+
name = "duplicate"
11401221
version = "0.5.0"
11411222
edition = "2015"
11421223
publish = true
11431224
"#
11441225
),
11451226
)
1146-
.file("aaa/src/lib.rs", "")
1227+
.file("duplicate1/src/lib.rs", "")
11471228
.file(
1148-
"bbb/Cargo.toml",
1229+
"duplicate2/Cargo.toml",
11491230
&format!(
11501231
r#"
11511232
[package]
1233+
name = "duplicate"
1234+
version = "0.5.0"
1235+
edition = "2015"
1236+
publish = true
1237+
"#
1238+
),
1239+
)
1240+
.file("duplicate2/src/lib.rs", "")
1241+
.file(
1242+
"invalid/Cargo.toml",
1243+
&format!(
1244+
r#"
1245+
[package
11521246
name = "bar"
11531247
version = "0.5.0"
11541248
edition = "2015"
11551249
publish = true
11561250
"#
11571251
),
11581252
)
1159-
.file("bbb/src/lib.rs", "")
1253+
.file("invalid/src/lib.rs", "")
11601254
});
11611255

11621256
let p = project
@@ -1171,7 +1265,7 @@ fn ambiguous_published_deps() {
11711265
edition = "2015"
11721266
authors = ["[email protected]"]
11731267
1174-
[dependencies.bar]
1268+
[dependencies.unique]
11751269
git = '{}'
11761270
"#,
11771271
git_project.url()
@@ -1183,7 +1277,13 @@ fn ambiguous_published_deps() {
11831277
p.cargo("build").run();
11841278
p.cargo("run")
11851279
.with_stderr_data(str![[r#"
1186-
[WARNING] skipping duplicate package `bar` found at `[ROOT]/home/.cargo/git/checkouts/dep-[HASH]/[..]`
1280+
[ERROR] invalid table header
1281+
expected `.`, `]`
1282+
--> ../home/.cargo/git/checkouts/dep-[HASH]/[..]/invalid/Cargo.toml:2:29
1283+
|
1284+
2 | [package
1285+
| ^
1286+
|
11871287
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
11881288
[RUNNING] `target/debug/foo[EXE]`
11891289

0 commit comments

Comments
 (0)