diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 1d1839a0cda..54468d9f84c 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -1,5 +1,6 @@ use std::collections::{HashMap, HashSet}; +use failure::bail; use log::{debug, trace}; use semver::VersionReq; use url::Url; @@ -79,7 +80,28 @@ pub struct PackageRegistry<'cfg> { patches_available: HashMap>, } -type LockedMap = HashMap)>>>; +/// 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` 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)>, + >, +>; #[derive(PartialEq, Eq, Clone, Copy)] enum Kind { @@ -275,6 +297,20 @@ impl<'cfg> PackageRegistry<'cfg> { .collect::>>() .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 @@ -579,7 +615,7 @@ fn lock(locked: &LockedMap, patches: &HashMap>, 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| { @@ -603,18 +639,56 @@ fn lock(locked: &LockedMap, patches: &HashMap>, 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; } } @@ -633,33 +707,6 @@ fn lock(locked: &LockedMap, patches: &HashMap>, 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 }) diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index e2863206131..8c1459c916c 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -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(); + 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(); + 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(); +}