Skip to content

Commit

Permalink
Auto merge of #10130 - weihanglo:issue-8690, r=alexcrichton
Browse files Browse the repository at this point in the history 
Improve unused patch message when source URLs mismatched

Resolves #8690

`Resolve.unused_patches` does not contains info about which source
URLs they are going to patch. As a result, we cannot provide a precise
message but only list all possible URLs of the packages with the same
name in the resolved graph.

There is a little flaw that if multiple patches are patching the same
package, the source URL of the used one would be shown as a possible
URL in the warning.
  • Loading branch information
@bors
bors committed Nov 29, 2021
2 parents 5335120 + 91791c4 commit c5fddb0
Show file tree
Hide file tree
Showing 3 changed files with 185 additions and 64 deletions.
11 changes: 6 additions & 5 deletions src/cargo/core/registry.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -413,11 +413,12 @@ impl<'cfg> PackageRegistry<'cfg> {
self.patches_locked = true;
}

pub fn patches(&self) -> Vec<Summary> {
self.patches
.values()
.flat_map(|v| v.iter().cloned())
.collect()
/// Gets all patches grouped by the source URLS they are going to patch.
///
/// These patches are mainly collected from [`patch`](Self::patch).
/// They might not be the same as patches actually used during dependency resolving.
pub fn patches(&self) -> &HashMap<CanonicalUrl, Vec<Summary>> {
&self.patches
}

fn load(&mut self, source_id: SourceId, kind: Kind) -> CargoResult<()> {
Expand Down
106 changes: 87 additions & 19 deletions src/cargo/ops/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::util::errors::CargoResult;
use crate::util::{profile, CanonicalUrl};
use anyhow::Context as _;
use log::{debug, trace};
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};

/// Result for `resolve_ws_with_opts`.
pub struct WorkspaceResolve<'cfg> {
Expand Down Expand Up @@ -467,25 +467,17 @@ pub fn resolve_with_previous<'cfg>(
.require(Feature::public_dependency())
.is_ok(),
)?;
resolved.register_used_patches(&registry.patches());
if register_patches {
// It would be good if this warning was more targeted and helpful
// (such as showing close candidates that failed to match). However,
// that's not terribly easy to do, so just show a general help
// message.
let warnings: Vec<String> = resolved
.unused_patches()
.iter()
.map(|pkgid| format!("Patch `{}` was not used in the crate graph.", pkgid))
.collect();
if !warnings.is_empty() {
ws.config().shell().warn(format!(
"{}\n{}",
warnings.join("\n"),
UNUSED_PATCH_WARNING
))?;
}
let patches: Vec<_> = registry
.patches()
.values()
.flat_map(|v| v.iter().cloned())
.collect();
resolved.register_used_patches(&patches[..]);

if register_patches && !resolved.unused_patches().is_empty() {
emit_warnings_of_unused_patches(ws, &resolved, &registry)?;
}

if let Some(previous) = previous {
resolved.merge_from(previous)?;
}
Expand Down Expand Up @@ -757,3 +749,79 @@ fn master_branch_git_source(id: PackageId, resolve: &Resolve) -> Option<PackageI
}
None
}

/// Emits warnings of unused patches case by case.
///
/// This function does its best to provide more targeted and helpful
/// (such as showing close candidates that failed to match). However, that's
/// not terribly easy to do, so just show a general help message if we cannot.
fn emit_warnings_of_unused_patches(
ws: &Workspace<'_>,
resolve: &Resolve,
registry: &PackageRegistry<'_>,
) -> CargoResult<()> {
const MESSAGE: &str = "was not used in the crate graph.";

// Patch package with the source URLs being patch
let mut patch_pkgid_to_urls = HashMap::new();
for (url, summaries) in registry.patches().iter() {
for summary in summaries.iter() {
patch_pkgid_to_urls
.entry(summary.package_id())
.or_insert_with(HashSet::new)
.insert(url);
}
}

// pkg name -> all source IDs of under the same pkg name
let mut source_ids_grouped_by_pkg_name = HashMap::new();
for pkgid in resolve.iter() {
source_ids_grouped_by_pkg_name
.entry(pkgid.name())
.or_insert_with(HashSet::new)
.insert(pkgid.source_id());
}

let mut unemitted_unused_patches = Vec::new();
for unused in resolve.unused_patches().iter() {
// Show alternative source URLs if the source URLs being patch
// cannot not be found in the crate graph.
match (
source_ids_grouped_by_pkg_name.get(&unused.name()),
patch_pkgid_to_urls.get(unused),
) {
(Some(ids), Some(patched_urls))
if ids
.iter()
.all(|id| !patched_urls.contains(id.canonical_url())) =>
{
use std::fmt::Write;
let mut msg = String::new();
writeln!(&mut msg, "Patch `{}` {}", unused, MESSAGE)?;
write!(
&mut msg,
"Perhaps you misspell the source URL being patched.\n\
Possible URLs for `[patch.<URL>]`:",
)?;
for id in ids.iter() {
write!(&mut msg, "\n {}", id.display_registry_name())?;
}
ws.config().shell().warn(msg)?;
}
_ => unemitted_unused_patches.push(unused),
}
}

// Show general help message.
if !unemitted_unused_patches.is_empty() {
let warnings: Vec<_> = unemitted_unused_patches
.iter()
.map(|pkgid| format!("Patch `{}` {}", pkgid, MESSAGE))
.collect();
ws.config()
.shell()
.warn(format!("{}\n{}", warnings.join("\n"), UNUSED_PATCH_WARNING))?;
}

return Ok(());
}
132 changes: 92 additions & 40 deletions tests/testsuite/patch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,10 @@ fn unused() {
"\
[UPDATING] `dummy-registry` index
[WARNING] Patch `bar v0.2.0 ([CWD]/bar)` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.1.0 [..]
[COMPILING] bar v0.1.0
Expand All @@ -374,10 +374,10 @@ fn unused() {
.with_stderr(
"\
[WARNING] Patch `bar v0.2.0 ([CWD]/bar)` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[FINISHED] [..]
",
)
Expand All @@ -394,6 +394,60 @@ fn unused() {
);
}

#[cargo_test]
fn unused_with_mismatch_source_being_patched() {
registry::alt_init();
Package::new("bar", "0.1.0").publish();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
[dependencies]
bar = "0.1.0"
[patch.alternative]
bar = { path = "bar" }
[patch.crates-io]
bar = { path = "baz" }
"#,
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_manifest("bar", "0.2.0"))
.file("bar/src/lib.rs", "not rust code")
.file("baz/Cargo.toml", &basic_manifest("bar", "0.3.0"))
.file("baz/src/lib.rs", "not rust code")
.build();

p.cargo("build")
.with_stderr(
"\
[UPDATING] `dummy-registry` index
[WARNING] Patch `bar v0.2.0 ([CWD]/bar)` was not used in the crate graph.
Perhaps you misspell the source URL being patched.
Possible URLs for `[patch.<URL>]`:
crates-io
[WARNING] Patch `bar v0.3.0 ([CWD]/baz)` was not used in the crate graph.
Check that [..]
with the [..]
what is [..]
version. [..]
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.1.0 [..]
[COMPILING] bar v0.1.0
[COMPILING] foo v0.0.1 ([CWD])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
",
)
.run();
}

#[cargo_test]
fn prefer_patch_version() {
Package::new("bar", "0.1.2").publish();
Expand Down Expand Up @@ -477,10 +531,10 @@ fn unused_from_config() {
"\
[UPDATING] `dummy-registry` index
[WARNING] Patch `bar v0.2.0 ([CWD]/bar)` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.1.0 [..]
[COMPILING] bar v0.1.0
Expand All @@ -493,10 +547,10 @@ fn unused_from_config() {
.with_stderr(
"\
[WARNING] Patch `bar v0.2.0 ([CWD]/bar)` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[FINISHED] [..]
",
)
Expand Down Expand Up @@ -550,10 +604,10 @@ fn unused_git() {
[UPDATING] git repository `file://[..]`
[UPDATING] `dummy-registry` index
[WARNING] Patch `bar v0.2.0 ([..])` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.1.0 [..]
[COMPILING] bar v0.1.0
Expand All @@ -566,10 +620,10 @@ fn unused_git() {
.with_stderr(
"\
[WARNING] Patch `bar v0.2.0 ([..])` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[FINISHED] [..]
",
)
Expand Down Expand Up @@ -752,21 +806,21 @@ fn add_ignored_patch() {
.with_stderr(
"\
[WARNING] Patch `bar v0.1.1 ([CWD]/bar)` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",
)
.run();
p.cargo("build")
.with_stderr(
"\
[WARNING] Patch `bar v0.1.1 ([CWD]/bar)` was not used in the crate graph.
[..]
[..]
[..]
[..]
Check that [..]
with the [..]
what is [..]
version. [..]
[FINISHED] [..]",
)
.run();
Expand Down Expand Up @@ -1714,10 +1768,9 @@ fn two_semver_compatible() {
.with_stderr(
"\
warning: Patch `bar v0.1.1 [..]` was not used in the crate graph.
Check that [..]
with the [..]
what is [..]
version. [..]
Perhaps you misspell the source URL being patched.
Possible URLs for `[patch.<URL>]`:
[CWD]/bar
[FINISHED] [..]",
)
.run();
Expand Down Expand Up @@ -1769,10 +1822,9 @@ fn multipatch_select_big() {
.with_stderr(
"\
warning: Patch `bar v0.1.0 [..]` was not used in the crate graph.
Check that [..]
with the [..]
what is [..]
version. [..]
Perhaps you misspell the source URL being patched.
Possible URLs for `[patch.<URL>]`:
[CWD]/bar
[FINISHED] [..]",
)
.run();
Expand Down

0 comments on commit c5fddb0

Please sign in to comment.