itsscb/cargo
1
0
Fork 0
mirror of https://github.com/rust-lang/cargo.git synced 2025-10-01 11:30:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #8248 - ehuss:patch-err-help, r=alexcrichton

Browse Source 
Provide better error messages for a bad `patch`.

This attempts to provide more user-friendly error messages for some situations with a bad `patch`.  This is a follow-up to #8243.

I think this more or less covers all the issues from #4678.  I imagine there are other corner cases, but those will need to wait for another day. The main one I can think of is when the patch location is missing required features.  Today you get a "blah was not used in the crate graph." warning, with some suggestions added in #6470, but it doesn't actually check if there is a feature mismatch.

Closes #4678
This commit is contained in:
bors 2020-05-21 14:29:50 +00:00
commit d662f2559b
3 changed files with 689 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

171
src/cargo/core/registry.rs
View File

@ -222,16 +222,35 @@ impl<'cfg> PackageRegistry<'cfg> {
/// the manifest.
///
/// Here the `deps` will be resolved to a precise version and stored
/// internally for future calls to `query` below. It's expected that `deps`
/// have had `lock_to` call already, if applicable. (e.g., if a lock file was
/// already present).
/// internally for future calls to `query` below. `deps` should be a tuple
/// where the first element is the patch definition straight from the
/// manifest, and the second element is an optional variant where the
/// patch has been locked. This locked patch is the patch locked to
/// a specific version found in Cargo.lock. This will be `None` if
/// `Cargo.lock` doesn't exist, or the patch did not match any existing
/// entries in `Cargo.lock`.
///
/// Note that the patch list specified here *will not* be available to
/// `query` until `lock_patches` is called below, which should be called
/// once all patches have been added.
pub fn patch(&mut self, url: &Url, deps: &[Dependency]) -> CargoResult<()> {
///
/// The return value is a `Vec` of patches that should *not* be locked.
/// This happens when the patch is locked, but the patch has been updated
/// so the locked value is no longer correct.
pub fn patch(
&mut self,
url: &Url,
deps: &[(&Dependency, Option<(Dependency, PackageId)>)],
) -> CargoResult<Vec<(Dependency, PackageId)>> {
// NOTE: None of this code is aware of required features. If a patch
// is missing a required feature, you end up with an "unused patch"
// warning, which is very hard to understand. Ideally the warning
// would be tailored to indicate *why* it is unused.
let canonical = CanonicalUrl::new(url)?;
// Return value of patches that shouldn't be locked.
let mut unlock_patches = Vec::new();
// First up we need to actually resolve each `deps` specification to
// precisely one summary. We're not using the `query` method below as it
// internally uses maps we're building up as part of this method
@ -243,7 +262,15 @@ impl<'cfg> PackageRegistry<'cfg> {
// of summaries which should be the same length as `deps` above.
let unlocked_summaries = deps
.iter()
.map(|dep| {
.map(|(orig_patch, locked)| {
// Remove double reference in orig_patch. Is there maybe a
// magic pattern that could avoid this?
let orig_patch = *orig_patch;
// Use the locked patch if it exists, otherwise use the original.
let dep = match locked {
Some((locked_patch, _locked_id)) => locked_patch,
None => orig_patch,
};
debug!(
"registering a patch for `{}` with `{}`",
url,
@ -261,30 +288,27 @@ impl<'cfg> PackageRegistry<'cfg> {
)
})?;
let mut summaries = self
let source = self
.sources
.get_mut(dep.source_id())
.expect("loaded source not present")
.query_vec(dep)?
.into_iter();
let summary = match summaries.next() {
Some(summary) => summary,
None => anyhow::bail!(
"patch for `{}` in `{}` did not resolve to any crates. If this is \
unexpected, you may wish to consult: \
https://github.com/rust-lang/cargo/issues/4678",
dep.package_name(),
url
),
};
if summaries.next().is_some() {
anyhow::bail!(
"patch for `{}` in `{}` resolved to more than one candidate",
dep.package_name(),
url
.expect("loaded source not present");
let summaries = source.query_vec(dep)?;
let (summary, should_unlock) =
summary_for_patch(orig_patch, &locked, summaries, source).chain_err(|| {
format!(
"patch for `{}` in `{}` failed to resolve",
orig_patch.package_name(),
url,
)
})?;
debug!(
"patch summary is {:?} should_unlock={:?}",
summary, should_unlock
);
if let Some(unlock_id) = should_unlock {
unlock_patches.push((orig_patch.clone(), unlock_id));
}
if *summary.package_id().source_id().canonical_url() == canonical {
anyhow::bail!(
"patch for `{}` in `{}` points to the same source, but \
@ -321,7 +345,7 @@ impl<'cfg> PackageRegistry<'cfg> {
self.patches_available.insert(canonical.clone(), ids);
self.patches.insert(canonical, unlocked_summaries);
Ok(())
Ok(unlock_patches)
}
/// Lock all patch summaries added via `patch`, making them available to
@ -335,6 +359,7 @@ impl<'cfg> PackageRegistry<'cfg> {
assert!(!self.patches_locked);
for summaries in self.patches.values_mut() {
for summary in summaries {
debug!("locking patch {:?}", summary);
*summary = lock(&self.locked, &self.patches_available, summary.clone());
}
}
@ -718,3 +743,97 @@ fn lock(
dep
})
}
/// This is a helper for selecting the summary, or generating a helpful error message.
fn summary_for_patch(
orig_patch: &Dependency,
locked: &Option<(Dependency, PackageId)>,
mut summaries: Vec<Summary>,
source: &mut dyn Source,
) -> CargoResult<(Summary, Option<PackageId>)> {
if summaries.len() == 1 {
return Ok((summaries.pop().unwrap(), None));
}
if summaries.len() > 1 {
// TODO: In the future, it might be nice to add all of these
// candidates so that version selection would just pick the
// appropriate one. However, as this is currently structured, if we
// added these all as patches, the unselected versions would end up in
// the "unused patch" listing, and trigger a warning. It might take a
// fair bit of restructuring to make that work cleanly, and there
// isn't any demand at this time to support that.
let mut vers: Vec<_> = summaries.iter().map(|summary| summary.version()).collect();
vers.sort();
let versions: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
anyhow::bail!(
"patch for `{}` in `{}` resolved to more than one candidate\n\
Found versions: {}\n\
Update the patch definition to select only one package.\n\
For example, add an `=` version requirement to the patch definition, \
such as `version = \"={}\"`.",
orig_patch.package_name(),
orig_patch.source_id(),
versions.join(", "),
versions.last().unwrap()
);
}
assert!(summaries.is_empty());
// No summaries found, try to help the user figure out what is wrong.
if let Some((_locked_patch, locked_id)) = locked {
// Since the locked patch did not match anything, try the unlocked one.
let orig_matches = source.query_vec(orig_patch).unwrap_or_else(|e| {
log::warn!(
"could not determine unlocked summaries for dep {:?}: {:?}",
orig_patch,
e
);
Vec::new()
});
let (summary, _) = summary_for_patch(orig_patch, &None, orig_matches, source)?;
// The unlocked version found a match. This returns a value to
// indicate that this entry should be unlocked.
return Ok((summary, Some(*locked_id)));
}
// Try checking if there are *any* packages that match this by name.
let name_only_dep = Dependency::new_override(orig_patch.package_name(), orig_patch.source_id());
let name_summaries = source.query_vec(&name_only_dep).unwrap_or_else(|e| {
log::warn!(
"failed to do name-only summary query for {:?}: {:?}",
name_only_dep,
e
);
Vec::new()
});
let mut vers = name_summaries
.iter()
.map(|summary| summary.version())
.collect::<Vec<_>>();
let found = match vers.len() {
0 => format!(""),
1 => format!("version `{}`", vers[0]),
_ => {
vers.sort();
let strs: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
format!("versions `{}`", strs.join(", "))
}
};
if found.is_empty() {
anyhow::bail!(
"The patch location `{}` does not appear to contain any packages \
matching the name `{}`.",
orig_patch.source_id(),
orig_patch.package_name()
);
} else {
anyhow::bail!(
"The patch location `{}` contains a `{}` package with {}, but the patch \
definition requires `{}`.\n\
Check that the version in the patch location is what you expect, \
and update the patch definition to match.",
orig_patch.source_id(),
orig_patch.package_name(),
found,
orig_patch.version_req()
);
}
}

79
src/cargo/ops/resolve.rs
View File

@ -20,7 +20,7 @@ use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Worksp
use crate::ops;
use crate::sources::PathSource;
use crate::util::errors::{CargoResult, CargoResultExt};
use crate::util::profile;
use crate::util::{profile, CanonicalUrl};
use log::{debug, trace};
use std::collections::HashSet;
@ -224,7 +224,7 @@ pub fn resolve_with_previous<'cfg>(
);
}
let keep = |p: &PackageId| {
let pre_patch_keep = |p: &PackageId| {
!to_avoid_sources.contains(&p.source_id())
&& match to_avoid {
Some(set) => !set.contains(p),
@ -232,6 +232,55 @@ pub fn resolve_with_previous<'cfg>(
}
};
// This is a set of PackageIds of `[patch]` entries that should not be
// locked.
let mut avoid_patch_ids = HashSet::new();
if register_patches {
for (url, patches) in ws.root_patch() {
let previous = match previous {
Some(r) => r,
None => {
let patches: Vec<_> = patches.iter().map(|p| (p, None)).collect();
let unlock_ids = registry.patch(url, &patches)?;
// Since nothing is locked, this shouldn't possibly return anything.
assert!(unlock_ids.is_empty());
continue;
}
};
let patches = patches
.iter()
.map(|dep| {
let unused = previous.unused_patches().iter().cloned();
let candidates = previous.iter().chain(unused);
match candidates
.filter(pre_patch_keep)
.find(|&id| dep.matches_id(id))
{
Some(id) => {
let mut locked_dep = dep.clone();
locked_dep.lock_to(id);
(dep, Some((locked_dep, id)))
}
None => (dep, None),
}
})
.collect::<Vec<_>>();
let canonical = CanonicalUrl::new(url)?;
for (orig_patch, unlock_id) in registry.patch(url, &patches)? {
// Avoid the locked patch ID.
avoid_patch_ids.insert(unlock_id);
// Also avoid the thing it is patching.
avoid_patch_ids.extend(previous.iter().filter(|id| {
orig_patch.matches_ignoring_source(*id)
&& *id.source_id().canonical_url() == canonical
}));
}
}
}
debug!("avoid_patch_ids={:?}", avoid_patch_ids);
let keep = |p: &PackageId| pre_patch_keep(p) && !avoid_patch_ids.contains(p);
// In the case where a previous instance of resolve is available, we
// want to lock as many packages as possible to the previous version
// without disturbing the graph structure.
@ -249,32 +298,6 @@ pub fn resolve_with_previous<'cfg>(
}
if register_patches {
for (url, patches) in ws.root_patch() {
let previous = match previous {
Some(r) => r,
None => {
registry.patch(url, patches)?;
continue;
}
};
let patches = patches
.iter()
.map(|dep| {
let unused = previous.unused_patches().iter().cloned();
let candidates = previous.iter().chain(unused);
match candidates.filter(keep).find(|&id| dep.matches_id(id)) {
Some(id) => {
let mut dep = dep.clone();
dep.lock_to(id);
dep
}
None => dep.clone(),
}
})
.collect::<Vec<_>>();
registry.patch(url, &patches)?;
}
registry.lock_patches();
}

493
tests/testsuite/patch.rs
View File

@ -1498,7 +1498,25 @@ fn update_unused_new_version() {
// Create a backup so we can test it with different options.
fs::copy(p.root().join("Cargo.lock"), p.root().join("Cargo.lock.bak")).unwrap();
// Try with `-p`.
// Try to build again, this should automatically update Cargo.lock.
p.cargo("build")
.with_stderr(
"\
[UPDATING] `[..]/registry` index
[COMPILING] bar v0.1.6 ([..]/bar)
[COMPILING] foo v0.0.1 ([..]/foo)
[FINISHED] [..]
",
)
.run();
// This should not update any registry.
p.cargo("build").with_stderr("[FINISHED] [..]").run();
assert!(!p.read_lockfile().contains("unused"));
// Restore the lock file, and see if `update` will work, too.
fs::copy(p.root().join("Cargo.lock.bak"), p.root().join("Cargo.lock")).unwrap();
// Try `update -p`.
p.cargo("update -p bar")
.with_stderr(
"\
@ -1521,3 +1539,476 @@ fn update_unused_new_version() {
)
.run();
}
#[cargo_test]
fn too_many_matches() {
// The patch locations has multiple versions that match.
Package::new("bar", "0.1.0").publish();
Package::new("bar", "0.1.0").alternative(true).publish();
Package::new("bar", "0.1.1").alternative(true).publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = { version = "0.1", registry = "alternative" }
"#,
)
.file("src/lib.rs", "")
.build();
// Picks 0.1.1, the most recent version.
p.cargo("check")
.with_status(101)
.with_stderr(
"\
[UPDATING] `[..]/alternative-registry` index
[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
Caused by:
patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
Caused by:
patch for `bar` in `registry `[..]/alternative-registry`` resolved to more than one candidate
Found versions: 0.1.0, 0.1.1
Update the patch definition to select only one package.
For example, add an `=` version requirement to the patch definition, such as `version = \"=0.1.1\"`.
",
)
.run();
}
#[cargo_test]
fn no_matches() {
// A patch to a location that does not contain the named package.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = { path = "bar" }
"#,
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_manifest("abc", "0.1.0"))
.file("bar/src/lib.rs", "")
.build();
p.cargo("check")
.with_status(101)
.with_stderr(
"\
error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
Caused by:
patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
Caused by:
The patch location `[..]/foo/bar` does not appear to contain any packages matching the name `bar`.
",
)
.run();
}
#[cargo_test]
fn mismatched_version() {
// A patch to a location that has an old version.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1.1"
[patch.crates-io]
bar = { path = "bar", version = "0.1.1" }
"#,
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"))
.file("bar/src/lib.rs", "")
.build();
p.cargo("check")
.with_status(101)
.with_stderr(
"\
[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
Caused by:
patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
Caused by:
The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, \
but the patch definition requires `^0.1.1`.
Check that the version in the patch location is what you expect, \
and update the patch definition to match.
",
)
.run();
}
#[cargo_test]
fn patch_walks_backwards() {
// Starting with a locked patch, change the patch so it points to an older version.
Package::new("bar", "0.1.0").publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = {path="bar"}
"#,
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_manifest("bar", "0.1.1"))
.file("bar/src/lib.rs", "")
.build();
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/registry` index
[CHECKING] bar v0.1.1 ([..]/foo/bar)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
// Somehow the user changes the version backwards.
p.change_file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"));
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/registry` index
[CHECKING] bar v0.1.0 ([..]/foo/bar)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
}
#[cargo_test]
fn patch_walks_backwards_restricted() {
// This is the same as `patch_walks_backwards`, but the patch contains a
// `version` qualifier. This is unusual, just checking a strange edge case.
Package::new("bar", "0.1.0").publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = {path="bar", version="0.1.1"}
"#,
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_manifest("bar", "0.1.1"))
.file("bar/src/lib.rs", "")
.build();
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/registry` index
[CHECKING] bar v0.1.1 ([..]/foo/bar)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
// Somehow the user changes the version backwards.
p.change_file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"));
p.cargo("check")
.with_status(101)
.with_stderr(
"\
error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
Caused by:
patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
Caused by:
The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, but the patch definition requires `^0.1.1`.
Check that the version in the patch location is what you expect, and update the patch definition to match.
",
)
.run();
}
#[cargo_test]
fn patched_dep_new_version() {
// What happens when a patch is locked, and then one of the patched
// dependencies needs to be updated. In this case, the baz requirement
// gets updated from 0.1.0 to 0.1.1.
Package::new("bar", "0.1.0").dep("baz", "0.1.0").publish();
Package::new("baz", "0.1.0").publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = {path="bar"}
"#,
)
.file("src/lib.rs", "")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.0"
[dependencies]
baz = "0.1"
"#,
)
.file("bar/src/lib.rs", "")
.build();
// Lock everything.
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/registry` index
[DOWNLOADING] crates ...
[DOWNLOADED] baz v0.1.0 [..]
[CHECKING] baz v0.1.0
[CHECKING] bar v0.1.0 ([..]/foo/bar)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
Package::new("baz", "0.1.1").publish();
// Just the presence of the new version should not have changed anything.
p.cargo("check").with_stderr("[FINISHED] [..]").run();
// Modify the patch so it requires the new version.
p.change_file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.0"
[dependencies]
baz = "0.1.1"
"#,
);
// Should unlock and update cleanly.
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/registry` index
[DOWNLOADING] crates ...
[DOWNLOADED] baz v0.1.1 (registry `[..]/registry`)
[CHECKING] baz v0.1.1
[CHECKING] bar v0.1.0 ([..]/foo/bar)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
}
#[cargo_test]
fn patch_update_doesnt_update_other_sources() {
// Very extreme edge case, make sure a patch update doesn't update other
// sources.
Package::new("bar", "0.1.0").publish();
Package::new("bar", "0.1.0").alternative(true).publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
bar_alt = { version = "0.1", registry = "alternative", package = "bar" }
[patch.crates-io]
bar = { path = "bar" }
"#,
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"))
.file("bar/src/lib.rs", "")
.build();
p.cargo("check")
.with_stderr_unordered(
"\
[UPDATING] `[..]/registry` index
[UPDATING] `[..]/alternative-registry` index
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.1.0 (registry `[..]/alternative-registry`)
[CHECKING] bar v0.1.0 (registry `[..]/alternative-registry`)
[CHECKING] bar v0.1.0 ([..]/foo/bar)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
// Publish new versions in both sources.
Package::new("bar", "0.1.1").publish();
Package::new("bar", "0.1.1").alternative(true).publish();
// Since it is locked, nothing should change.
p.cargo("check").with_stderr("[FINISHED] [..]").run();
// Require new version on crates.io.
p.change_file("bar/Cargo.toml", &basic_manifest("bar", "0.1.1"));
// This should not update bar_alt.
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/registry` index
[CHECKING] bar v0.1.1 ([..]/foo/bar)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
}
#[cargo_test]
fn can_update_with_alt_reg() {
// A patch to an alt reg can update.
Package::new("bar", "0.1.0").publish();
Package::new("bar", "0.1.0").alternative(true).publish();
Package::new("bar", "0.1.1").alternative(true).publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = { version = "=0.1.1", registry = "alternative" }
"#,
)
.file("src/lib.rs", "")
.build();
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/alternative-registry` index
[UPDATING] `[..]/registry` index
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.1.1 (registry `[..]/alternative-registry`)
[CHECKING] bar v0.1.1 (registry `[..]/alternative-registry`)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
Package::new("bar", "0.1.2").alternative(true).publish();
// Should remain locked.
p.cargo("check").with_stderr("[FINISHED] [..]").run();
// This does nothing, due to `=` requirement.
p.cargo("update -p bar")
.with_stderr(
"\
[UPDATING] `[..]/alternative-registry` index
[UPDATING] `[..]/registry` index
",
)
.run();
// Bump to 0.1.2.
p.change_file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = { version = "=0.1.2", registry = "alternative" }
"#,
);
p.cargo("check")
.with_stderr(
"\
[UPDATING] `[..]/alternative-registry` index
[UPDATING] `[..]/registry` index
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.1.2 (registry `[..]/alternative-registry`)
[CHECKING] bar v0.1.2 (registry `[..]/alternative-registry`)
[CHECKING] foo v0.1.0 ([..]/foo)
[FINISHED] [..]
",
)
.run();
}