From eebddd3723d1ce425cbdee6103221b7d32e0ccd4 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 16 Jan 2018 07:33:59 -0800 Subject: [PATCH] Fix `[patch]` sections depending on one another This commit fixes a bug in `[patch]` where the original source is updated too often (for example `Updating ...` being printed too much). This bug occurred when patches depended on each other (for example the dependencies of a resolved `[patch]` would actually resolve to a `[patch]` that hadn't been resolved yet). The ordering of resolution/locking wasn't happening correctly and wasn't ready to break these cycles! The process of adding `[patch]` sections to a registry has been updated to account for this bug. Instead of add-and-lock all in one go this commit instead splits the addition of `[patch]` into two phases. In the first we collect a bunch of unlocked patch summaries but record all the `PackageId` instances for each url we've scraped. After all `[patch]` sections have been processed in this manner we go back and lock all the summaries that were previously unlocked. The `lock` function was updated to *only* need the map of patches from URL to `PackageId` as it doesn't actually have access to the full `Summary` of patches during the `lock_patches` method. All in all this should correctly resolve dependencies here which means that processing of patches should proceed as usual, avoiding updating the registry too much! Closes #4941 --- src/cargo/core/dependency.rs | 5 + src/cargo/core/registry.rs | 113 +++++++++++++++++++---- src/cargo/ops/cargo_generate_lockfile.rs | 22 +++-- src/cargo/ops/resolve.rs | 67 +++++++++----- tests/patch.rs | 56 +++++++++++ 5 files changed, 213 insertions(+), 50 deletions(-) diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index f34ef2f14..2f75a0590 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -254,6 +254,11 @@ impl Dependency { pub fn lock_to(&mut self, id: &PackageId) -> &mut Dependency { assert_eq!(self.inner.source_id, *id.source_id()); assert!(self.inner.req.matches(id.version())); + trace!("locking dep from `{}` with `{}` at {} to {}", + self.name(), + self.version_req(), + self.source_id(), + id); self.set_version_req(VersionReq::exact(id.version())) .set_source_id(id.source_id().clone()) } diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 9257530ca..1a18bd011 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -88,7 +88,10 @@ pub struct PackageRegistry<'cfg> { locked: LockedMap, source_config: SourceConfigMap<'cfg>, + patches: HashMap>, + patches_locked: bool, + patches_available: HashMap>, } type LockedMap = HashMap)>>>; @@ -110,6 +113,8 @@ impl<'cfg> PackageRegistry<'cfg> { source_config: source_config, locked: HashMap::new(), patches: HashMap::new(), + patches_locked: false, + patches_available: HashMap::new(), }) } @@ -188,9 +193,50 @@ impl<'cfg> PackageRegistry<'cfg> { sub_vec.push((id, deps)); } + /// Insert a `[patch]` section into this registry. + /// + /// This method will insert a `[patch]` section for the `url` specified, + /// with the given list of dependencies. The `url` specified is the URL of + /// the source to patch (for example this is `crates-io` in the manifest). + /// The `deps` is an array of all the entries in the `[patch]` section of + /// 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). + /// + /// 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<()> { - let deps = deps.iter().map(|dep| { - let mut summaries = self.query_vec(dep)?.into_iter(); + // 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 + // (`patches_available` and `patches). Instead we're going straight to + // the source to load information from it. + // + // Remember that each dependency listed in `[patch]` has to resolve to + // precisely one package, so that's why we're just creating a flat list + // of summaries which should be the same length as `deps` above. + let unlocked_summaries = deps.iter().map(|dep| { + debug!("registring a patch for `{}` with `{}`", + url, + dep.name()); + + // Go straight to the source for resolving `dep`. Load it as we + // normally would and then ask it directly for the list of summaries + // corresponding to this `dep`. + self.ensure_loaded(dep.source_id(), Kind::Normal).chain_err(|| { + format_err!("failed to load source for a dependency \ + on `{}`", dep.name()) + })?; + + let mut summaries = 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 => { @@ -214,11 +260,38 @@ impl<'cfg> PackageRegistry<'cfg> { format_err!("failed to resolve patches for `{}`", url) })?; - self.patches.insert(url.clone(), deps); + // 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 + // `lock` method) and otherwise store the unlocked summaries in + // `patches` to get locked in a future call to `lock_patches`. + let ids = unlocked_summaries.iter() + .map(|s| s.package_id()) + .cloned() + .collect(); + self.patches_available.insert(url.clone(), ids); + self.patches.insert(url.clone(), unlocked_summaries); Ok(()) } + /// Lock all patch summaries added via `patch`, making them available to + /// resolution via `query`. + /// + /// This function will internally `lock` each summary added via `patch` + /// above now that the full set of `patch` packages are known. This'll allow + /// us to correctly resolve overridden dependencies between patches + /// hopefully! + pub fn lock_patches(&mut self) { + assert!(!self.patches_locked); + for summaries in self.patches.values_mut() { + for summary in summaries { + *summary = lock(&self.locked, &self.patches_available, summary.clone()); + } + } + self.patches_locked = true; + } + pub fn patches(&self) -> &HashMap> { &self.patches } @@ -271,7 +344,8 @@ impl<'cfg> PackageRegistry<'cfg> { /// possible. If we're unable to map a dependency though, we just pass it on /// through. pub fn lock(&self, summary: Summary) -> Summary { - lock(&self.locked, &self.patches, summary) + assert!(self.patches_locked); + lock(&self.locked, &self.patches_available, summary) } fn warn_bad_override(&self, @@ -323,6 +397,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> { fn query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> { + assert!(self.patches_locked); let (override_summary, n, to_warn) = { // Look for an override and get ready to query the real source. let override_summary = self.query_overrides(dep)?; @@ -357,8 +432,12 @@ impl<'cfg> Registry for PackageRegistry<'cfg> { } } else { if !patches.is_empty() { - debug!("found {} patches with an unlocked dep, \ - looking at sources", patches.len()); + debug!("found {} patches with an unlocked dep on `{}` at {} \ + with `{}`, \ + looking at sources", patches.len(), + dep.name(), + dep.source_id(), + dep.version_req()); } // Ensure the requested source_id is loaded @@ -387,7 +466,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> { // version as something in `patches` that we've // already selected, then we skip this `summary`. let locked = &self.locked; - let all_patches = &self.patches; + let all_patches = &self.patches_available; return source.query(dep, &mut |summary| { for patch in patches.iter() { let patch = patch.package_id().version(); @@ -437,7 +516,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> { } fn lock(locked: &LockedMap, - patches: &HashMap>, + patches: &HashMap>, summary: Summary) -> Summary { let pair = locked.get(summary.source_id()).and_then(|map| { map.get(summary.name()) @@ -504,24 +583,24 @@ fn lock(locked: &LockedMap, // this dependency. let v = patches.get(dep.source_id().url()).map(|vec| { let dep2 = dep.clone(); - let mut iter = vec.iter().filter(move |s| { - dep2.name() == s.package_id().name() && - dep2.version_req().matches(s.package_id().version()) + let mut iter = vec.iter().filter(move |p| { + dep2.name() == p.name() && + dep2.version_req().matches(p.version()) }); (iter.next(), iter) }); - if let Some((Some(summary), mut remaining)) = v { + if let Some((Some(patch_id), mut remaining)) = v { assert!(remaining.next().is_none()); - let patch_source = summary.package_id().source_id(); + let patch_source = patch_id.source_id(); let patch_locked = locked.get(patch_source).and_then(|m| { - m.get(summary.package_id().name()) + m.get(patch_id.name()) }).map(|list| { - list.iter().any(|&(ref id, _)| id == summary.package_id()) + list.iter().any(|&(ref id, _)| id == patch_id) }).unwrap_or(false); if patch_locked { - trace!("\tthird hit on {}", summary.package_id()); - let req = VersionReq::exact(summary.package_id().version()); + trace!("\tthird hit on {}", patch_id); + let req = VersionReq::exact(patch_id.version()); let mut dep = dep.clone(); dep.set_version_req(req); return dep diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 0d6ebefb7..4ace413e5 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -19,9 +19,14 @@ pub struct UpdateOptions<'a> { pub fn generate_lockfile(ws: &Workspace) -> CargoResult<()> { let mut registry = PackageRegistry::new(ws.config())?; - let resolve = ops::resolve_with_previous(&mut registry, ws, + let resolve = ops::resolve_with_previous(&mut registry, + ws, Method::Everything, - None, None, &[], true)?; + None, + None, + &[], + true, + true)?; ops::write_pkg_lockfile(ws, &resolve)?; Ok(()) } @@ -77,12 +82,13 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions) } let resolve = ops::resolve_with_previous(&mut registry, - ws, - Method::Everything, - Some(&previous_resolve), - Some(&to_avoid), - &[], - true)?; + ws, + Method::Everything, + Some(&previous_resolve), + Some(&to_avoid), + &[], + true, + true)?; // Summarize what is changing for the user. let print_change = |status: &str, msg: String, color: Color| { diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 2ccf3b394..8a49de85d 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -40,11 +40,13 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, if let Some(source) = source { registry.add_preloaded(source); } + let mut add_patches = true; let resolve = if ws.require_optional_deps() { // First, resolve the root_package's *listed* dependencies, as well as // downloading and updating all remotes and such. let resolve = resolve_with_registry(ws, &mut registry, false)?; + add_patches = false; // Second, resolve with precisely what we're doing. Filter out // transitive dependencies if necessary, specify features, handle @@ -77,9 +79,14 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, }; let resolved_with_overrides = - ops::resolve_with_previous(&mut registry, ws, - method, resolve.as_ref(), None, - specs, true)?; + ops::resolve_with_previous(&mut registry, + ws, + method, + resolve.as_ref(), + None, + specs, + add_patches, + true)?; let packages = get_resolved_packages(&resolved_with_overrides, registry); @@ -89,9 +96,14 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>, fn resolve_with_registry(ws: &Workspace, registry: &mut PackageRegistry, warn: bool) -> CargoResult { let prev = ops::load_pkg_lockfile(ws)?; - let resolve = resolve_with_previous(registry, ws, + let resolve = resolve_with_previous(registry, + ws, Method::Everything, - prev.as_ref(), None, &[], warn)?; + prev.as_ref(), + None, + &[], + true, + warn)?; if !ws.is_ephemeral() { ops::write_pkg_lockfile(ws, &resolve)?; @@ -115,6 +127,7 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, previous: Option<&'a Resolve>, to_avoid: Option<&HashSet<&'a PackageId>>, specs: &[PackageIdSpec], + register_patches: bool, warn: bool) -> CargoResult { // Here we place an artificial limitation that all non-registry sources @@ -169,27 +182,31 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry, } } - 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(); - 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 + if register_patches { + for (url, patches) in ws.root_patch() { + let previous = match previous { + Some(r) => r, + None => { + registry.patch(url, patches)?; + continue } - None => dep.clone(), - } - }).collect::>(); - registry.patch(url, &patches)?; + }; + let patches = patches.iter().map(|dep| { + let unused = previous.unused_patches(); + 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::>(); + registry.patch(url, &patches)?; + } + + registry.lock_patches(); } let mut summaries = Vec::new(); diff --git a/tests/patch.rs b/tests/patch.rs index c8d5e9ca6..c7f6bc1ab 100644 --- a/tests/patch.rs +++ b/tests/patch.rs @@ -796,3 +796,59 @@ fn patch_in_virtual() { [FINISHED] [..] ")); } + +#[test] +fn patch_depends_on_another_patch() { + Package::new("foo", "0.1.0") + .file("src/lib.rs", "broken code") + .publish(); + + Package::new("bar", "0.1.0") + .dep("foo", "0.1") + .file("src/lib.rs", "broken code") + .publish(); + + let p = project("p") + .file("Cargo.toml", r#" + [package] + name = "p" + authors = [] + version = "0.1.0" + + [dependencies] + foo = "0.1" + bar = "0.1" + + [patch.crates-io] + foo = { path = "foo" } + bar = { path = "bar" } + "#) + .file("src/lib.rs", "") + .file("foo/Cargo.toml", r#" + [package] + name = "foo" + version = "0.1.1" + authors = [] + "#) + .file("foo/src/lib.rs", r#""#) + .file("bar/Cargo.toml", r#" + [package] + name = "bar" + version = "0.1.1" + authors = [] + + [dependencies] + foo = "0.1" + "#) + .file("bar/src/lib.rs", r#""#) + .build(); + + assert_that(p.cargo("build"), + execs().with_status(0)); + + // Nothing should be rebuilt, no registry should be updated. + assert_that(p.cargo("build"), + execs().with_status(0).with_stderr("\ +[FINISHED] [..] +")); +} -- 2.30.2