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

Jump to bottom

Fixes around multiple [patch] per crate #7303

Merged
merged 1 commit into from
Aug 27, 2019
Merged

Fixes around multiple [patch] per crate #7303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
117 changes: 82 additions & 35 deletions src/cargo/core/registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::{HashMap, HashSet};

use failure::bail;
use log::{debug, trace};
use semver::VersionReq;
use url::Url;
Expand Down Expand Up @@ -79,7 +80,28 @@ pub struct PackageRegistry<'cfg> {
patches_available: HashMap<Url, Vec<PackageId>>,
}

type LockedMap = HashMap<SourceId, HashMap<String, Vec<(PackageId, Vec<PackageId>)>>>;
/// A map of all "locked packages" which is filled in when parsing a lock file
/// and is used to guide dependency resolution by altering summaries as they're
/// queried from this source.
///
/// This map can be thought of as a glorified `Vec<MySummary>` where `MySummary`
/// has a `PackageId` for which package it represents as well as a list of
/// `PackageId` for the resolved dependencies. The hash map is otherwise
/// structured though for easy access throughout this registry.
type LockedMap = HashMap<
// The first level of key-ing done in this hash map is the source that
// dependencies come from, identified by a `SourceId`.
SourceId,
HashMap<
// This next level is keyed by the name of the package...
String,
// ... and the value here is a list of tuples. The first element of each
// tuple is a package which has the source/name used to get to this
// point. The second element of each tuple is the list of locked
// dependencies that the first element has.
Vec<(PackageId, Vec<PackageId>)>,
>,
>;

#[derive(PartialEq, Eq, Clone, Copy)]
enum Kind {
Expand Down Expand Up @@ -275,6 +297,20 @@ impl<'cfg> PackageRegistry<'cfg> {
.collect::<CargoResult<Vec<_>>>()
.chain_err(|| failure::format_err!("failed to resolve patches for `{}`", url))?;

let mut name_and_version = HashSet::new();
for summary in unlocked_summaries.iter() {
let name = summary.package_id().name();
let version = summary.package_id().version();
if !name_and_version.insert((name, version)) {
bail!(
"cannot have two `[patch]` entries which both resolve \
to `{} v{}`",
name,
version
);
}
}

// Note that we do not use `lock` here to lock summaries! That step
// happens later once `lock_patches` is invoked. In the meantime though
// we want to fill in the `patches_available` map (later used in the
Expand Down Expand Up @@ -579,7 +615,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum

// Lock the summary's ID if possible
let summary = match pair {
Some(&(ref precise, _)) => summary.override_id(precise.clone()),
Some((precise, _)) => summary.override_id(precise.clone()),
None => summary,
};
summary.map_dependencies(|dep| {
Expand All @@ -603,18 +639,56 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
// locked version because the dependency needs to be
// re-resolved.
//
// 3. We don't have a lock entry for this dependency, in which
// 3. We have a lock entry for this dependency, but it's from a
// different source than what's listed. This lock though happens
// through `[patch]`, so we want to preserve it.
//
// 4. We don't have a lock entry for this dependency, in which
// case it was likely an optional dependency which wasn't
// included previously so we just pass it through anyway.
//
// Cases 1/2 are handled by `matches_id` and case 3 is handled by
// falling through to the logic below.
if let Some(&(_, ref locked_deps)) = pair {
let locked = locked_deps.iter().find(|&&id| dep.matches_id(id));
// Cases 1/2 are handled by `matches_id`, case 3 is handled specially,
// and case 4 is handled by falling through to the logic below.
if let Some((_, locked_deps)) = pair {
let locked = locked_deps.iter().find(|&&id| {
// If the dependency matches the package id exactly then we've
// found a match, this is the id the dependency was previously
// locked to.
if dep.matches_id(id) {
return true;
}

// If the name/version doesn't match, then we definitely don't
// have a match whatsoever. Otherwise we need to check
// `[patch]`...
if !dep.matches_ignoring_source(id) {
return false;
}

// ... so here we look up the dependency url in the patches
// map, and we see if `id` is contained in the list of patches
// for that url. If it is then this lock is still valid,
// otherwise the lock is no longer valid.
match patches.get(dep.source_id().url()) {
Some(list) => list.contains(&id),
None => false,
}
});

if let Some(&locked) = locked {
trace!("\tfirst hit on {}", locked);
let mut dep = dep;
dep.lock_to(locked);

// If we found a locked version where the sources match, then
// we can `lock_to` to get an exact lock on this dependency.
// Otherwise we got a lock via `[patch]` so we only lock the
// version requirement, not the source.
if locked.source_id() == dep.source_id() {
dep.lock_to(locked);
} else {
let req = VersionReq::exact(locked.version());
dep.set_version_req(req);
}
return dep;
}
}
Expand All @@ -633,33 +707,6 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
return dep;
}

// Finally we check to see if any registered patches correspond to
// this dependency.
let v = patches.get(dep.source_id().url()).map(|vec| {
let dep2 = dep.clone();
let mut iter = vec
.iter()
.filter(move |&&p| dep2.matches_ignoring_source(p));
(iter.next(), iter)
});
if let Some((Some(patch_id), mut remaining)) = v {
assert!(remaining.next().is_none());
let patch_source = patch_id.source_id();
let patch_locked = locked
.get(&patch_source)
.and_then(|m| m.get(&*patch_id.name()))
.map(|list| list.iter().any(|&(ref id, _)| id == patch_id))
.unwrap_or(false);

if patch_locked {
trace!("\tthird hit on {}", patch_id);
let req = VersionReq::exact(patch_id.version());
let mut dep = dep;
dep.set_version_req(req);
return dep;
}
}

trace!("\tnope, unlocked");
dep
})
Expand Down
159 changes: 159 additions & 0 deletions tests/testsuite/patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,3 +1179,162 @@ fn multipatch() {

p.cargo("build").run();
}

#[cargo_test]
fn patch_same_version() {
let bar = git::repo(&paths::root().join("override"))
.file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
.file("src/lib.rs", "")
.build();

crate::support::registry::init();

let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.0.1"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = {{ path = "bar" }}
bar2 = {{ git = '{}', package = 'bar' }}
"#,
bar.url(),
),
)
.file("src/lib.rs", "")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.0"
"#,
)
.file("bar/src/lib.rs", "")
.build();

p.cargo("build")
.with_status(101)
.with_stderr(
"\
[UPDATING] [..]
error: cannot have two `[patch]` entries which both resolve to `bar v0.1.0`
",
)
.run();
}

#[cargo_test]
fn two_semver_compatible() {
let bar = git::repo(&paths::root().join("override"))
.file("Cargo.toml", &basic_manifest("bar", "0.1.1"))
.file("src/lib.rs", "")
.build();

crate::support::registry::init();

let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.0.1"
[dependencies]
bar = "0.1"
[patch.crates-io]
bar = {{ path = "bar" }}
bar2 = {{ git = '{}', package = 'bar' }}
"#,
bar.url(),
),
)
.file("src/lib.rs", "pub fn foo() { bar::foo() }")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.2"
"#,
)
.file("bar/src/lib.rs", "pub fn foo() {}")
.build();

// assert the build succeeds and doesn't panic anywhere, and then afterwards
// assert that the build succeeds again without updating anything or
// building anything else.
p.cargo("build").run();
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
p.cargo("build")
.with_stderr(
"\
warning: Patch `bar v0.1.1 [..]` was not used in the crate graph.
Check that [..]
with the [..]
what is [..]
version. [..]
[FINISHED] [..]",
)
.run();
}

#[cargo_test]
fn multipatch_select_big() {
let bar = git::repo(&paths::root().join("override"))
.file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
.file("src/lib.rs", "")
.build();

crate::support::registry::init();

let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.0.1"
[dependencies]
bar = "*"
[patch.crates-io]
bar = {{ path = "bar" }}
bar2 = {{ git = '{}', package = 'bar' }}
"#,
bar.url(),
),
)
.file("src/lib.rs", "pub fn foo() { bar::foo() }")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.2.0"
"#,
)
.file("bar/src/lib.rs", "pub fn foo() {}")
.build();

// assert the build succeeds, which is only possible if 0.2.0 is selected
// since 0.1.0 is missing the function we need. Afterwards assert that the
// build succeeds again without updating anything or building anything else.
p.cargo("build").run();
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
p.cargo("build")
.with_stderr(
"\
warning: Patch `bar v0.1.0 [..]` was not used in the crate graph.
Check that [..]
with the [..]
what is [..]
version. [..]
[FINISHED] [..]",
)
.run();
}