diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 35ddee7b8b2..029dc49a099 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -107,6 +107,21 @@ enum Kind { Normal, } +/// Argument to `PackageRegistry::patch` which is information about a `[patch]` +/// directive that we found in a lockfile, if present. +pub struct LockedPatchDependency { + /// The original `Dependency` directive, except "locked" so it's version + /// requirement is `=foo` and its `SourceId` has a "precise" listed. + pub dependency: Dependency, + /// The `PackageId` that was previously found in a lock file which + /// `dependency` matches. + pub package_id: PackageId, + /// Something only used for backwards compatibility with the v2 lock file + /// format where `branch=master` is considered the same as `DefaultBranch`. + /// For more comments on this see the code in `ops/resolve.rs`. + pub alt_package_id: Option, +} + impl<'cfg> PackageRegistry<'cfg> { pub fn new(config: &'cfg Config) -> CargoResult> { let source_config = SourceConfigMap::new(config)?; @@ -240,7 +255,7 @@ impl<'cfg> PackageRegistry<'cfg> { pub fn patch( &mut self, url: &Url, - deps: &[(&Dependency, Option<(Dependency, PackageId)>)], + deps: &[(&Dependency, Option)], ) -> CargoResult> { // 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" @@ -268,7 +283,7 @@ impl<'cfg> PackageRegistry<'cfg> { 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, + Some(lock) => &lock.dependency, None => orig_patch, }; debug!( @@ -338,13 +353,36 @@ impl<'cfg> PackageRegistry<'cfg> { } } + // Calculate a list of all patches available for this source which is + // then used later during calls to `lock` to rewrite summaries to point + // directly at these patched entries. + // + // Note that this is somewhat subtle where the list of `ids` for a + // canonical URL is extend with possibly two ids per summary. This is done + // to handle the transition from the v2->v3 lock file format where in + // v2 DefeaultBranch was either DefaultBranch or Branch("master") for + // git dependencies. In this case if `summary.package_id()` is + // Branch("master") then alt_package_id will be DefaultBranch. This + // signifies that there's a patch available for either of those + // dependency directives if we see them in the dependency graph. + // + // This is a bit complicated and hopefully an edge case we can remove + // in the future, but for now it hopefully doesn't cause too much + // harm... + let mut ids = Vec::new(); + for (summary, (_, lock)) in unlocked_summaries.iter().zip(deps) { + ids.push(summary.package_id()); + if let Some(lock) = lock { + ids.extend(lock.alt_package_id); + } + } + self.patches_available.insert(canonical.clone(), ids); + // 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()).collect(); - self.patches_available.insert(canonical.clone(), ids); self.patches.insert(canonical, unlocked_summaries); Ok(unlock_patches) @@ -747,7 +785,7 @@ fn lock( /// 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)>, + locked: &Option, mut summaries: Vec, source: &mut dyn Source, ) -> CargoResult<(Summary, Option)> { @@ -779,7 +817,7 @@ fn summary_for_patch( } 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 { + if let Some(locked) = 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!( @@ -792,7 +830,7 @@ fn summary_for_patch( 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))); + return Ok((summary, Some(locked.package_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()); diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index e122f4e6778..b3f3b2486c4 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -11,7 +11,7 @@ //! providing the most power and flexibility. use crate::core::compiler::{CompileKind, RustcTargetData}; -use crate::core::registry::PackageRegistry; +use crate::core::registry::{LockedPatchDependency, PackageRegistry}; use crate::core::resolver::features::{ CliFeatures, FeatureOpts, FeatureResolver, ForceAllTargets, RequestedFeatures, ResolvedFeatures, }; @@ -257,26 +257,91 @@ pub fn resolve_with_previous<'cfg>( 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))) + + // This is a list of pairs where the first element of the pair is + // the raw `Dependency` which matches what's listed in `Cargo.toml`. + // The second element is, if present, the "locked" version of + // the `Dependency` as well as the `PackageId` that it previously + // resolved to. This second element is calculated by looking at the + // previous resolve graph, which is primarily what's done here to + // build the `registrations` list. + let mut registrations = Vec::new(); + for dep in patches { + let candidates = || { + previous + .iter() + .chain(previous.unused_patches().iter().cloned()) + .filter(&pre_patch_keep) + }; + + let lock = match candidates().find(|id| dep.matches_id(*id)) { + // If we found an exactly matching candidate in our list of + // candidates, then that's the one to use. + Some(package_id) => { + let mut locked_dep = dep.clone(); + locked_dep.lock_to(package_id); + Some(LockedPatchDependency { + dependency: locked_dep, + package_id, + alt_package_id: None, + }) + } + None => { + // If the candidate does not have a matching source id + // then we may still have a lock candidate. If we're + // loading a v2-encoded resolve graph and `dep` is a + // git dep with `branch = 'master'`, then this should + // also match candidates without `branch = 'master'` + // (which is now treated separately in Cargo). + // + // In this scenario we try to convert candidates located + // in the resolve graph to explicitly having the + // `master` branch (if they otherwise point to + // `DefaultBranch`). If this works and our `dep` + // matches that then this is something we'll lock to. + match candidates().find(|&id| { + match master_branch_git_source(id, previous) { + Some(id) => dep.matches_id(id), + None => false, + } + }) { + Some(id_using_default) => { + let id_using_master = id_using_default.with_source_id( + dep.source_id().with_precise( + id_using_default + .source_id() + .precise() + .map(|s| s.to_string()), + ), + ); + + let mut locked_dep = dep.clone(); + locked_dep.lock_to(id_using_master); + Some(LockedPatchDependency { + dependency: locked_dep, + package_id: id_using_master, + // Note that this is where the magic + // happens, where the resolve graph + // probably has locks pointing to + // DefaultBranch sources, and by including + // this here those will get transparently + // rewritten to Branch("master") which we + // have a lock entry for. + alt_package_id: Some(id_using_default), + }) + } + + // No locked candidate was found + None => None, } - None => (dep, None), } - }) - .collect::>(); + }; + + registrations.push((dep, lock)); + } + let canonical = CanonicalUrl::new(url)?; - for (orig_patch, unlock_id) in registry.patch(url, &patches)? { + for (orig_patch, unlock_id) in registry.patch(url, ®istrations)? { // Avoid the locked patch ID. avoid_patch_ids.insert(unlock_id); // Also avoid the thing it is patching. @@ -618,17 +683,8 @@ fn register_previous_locks( // Note that this is only applicable for loading older resolves now at // this point. All new lock files are encoded as v3-or-later, so this is // just compat for loading an old lock file successfully. - if resolve.version() <= ResolveVersion::V2 { - let source = node.source_id(); - if let Some(GitReference::DefaultBranch) = source.git_reference() { - let new_source = - SourceId::for_git(source.url(), GitReference::Branch("master".to_string())) - .unwrap() - .with_precise(source.precise().map(|s| s.to_string())); - - let node = node.with_source_id(new_source); - registry.register_lock(node, deps.clone()); - } + if let Some(node) = master_branch_git_source(node, resolve) { + registry.register_lock(node, deps.clone()); } registry.register_lock(node, deps); @@ -645,3 +701,17 @@ fn register_previous_locks( } } } + +fn master_branch_git_source(id: PackageId, resolve: &Resolve) -> Option { + if resolve.version() <= ResolveVersion::V2 { + let source = id.source_id(); + if let Some(GitReference::DefaultBranch) = source.git_reference() { + let new_source = + SourceId::for_git(source.url(), GitReference::Branch("master".to_string())) + .unwrap() + .with_precise(source.precise().map(|s| s.to_string())); + return Some(id.with_source_id(new_source)); + } + } + None +} diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index aebb4762601..24513ee2602 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -2320,3 +2320,80 @@ fn can_update_with_alt_reg() { ) .run(); } + +#[cargo_test] +fn old_git_patch() { + // Example where an old lockfile with an explicit branch="master" in Cargo.toml. + Package::new("bar", "1.0.0").publish(); + let (bar, bar_repo) = git::new_repo("bar", |p| { + p.file("Cargo.toml", &basic_manifest("bar", "1.0.0")) + .file("src/lib.rs", "") + }); + + let bar_oid = bar_repo.head().unwrap().target().unwrap(); + + let p = project() + .file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + + [patch.crates-io] + bar = {{ git = "{}", branch = "master" }} + "#, + bar.url() + ), + ) + .file( + "Cargo.lock", + &format!( + r#" +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +[[package]] +name = "bar" +version = "1.0.0" +source = "git+{}#{}" + +[[package]] +name = "foo" +version = "0.1.0" +dependencies = [ + "bar", +] + "#, + bar.url(), + bar_oid + ), + ) + .file("src/lib.rs", "") + .build(); + + bar.change_file("Cargo.toml", &basic_manifest("bar", "2.0.0")); + git::add(&bar_repo); + git::commit(&bar_repo); + + // This *should* keep the old lock. + p.cargo("tree") + // .env("CARGO_LOG", "trace") + .with_stderr( + "\ +[UPDATING] [..] +", + ) + // .with_status(1) + .with_stdout(format!( + "\ +foo v0.1.0 [..] +└── bar v1.0.0 (file:///[..]branch=master#{}) +", + &bar_oid.to_string()[..8] + )) + .run(); +}