Skip to content

Commit

Permalink
Auto merge of #13727 - weihanglo:patch, r=epage
Browse files Browse the repository at this point in the history 
refactor: make `resolve_with_previous` clearer
  • Loading branch information
@bors
bors committed Apr 10, 2024
2 parents f8a73f7 + 80d0d94 commit e366699
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 142 deletions.
10 changes: 6 additions & 4 deletions src/cargo/core/registry.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,12 @@ fn lock(
})
}

/// This is a helper for selecting the summary, or generating a helpful error message.
/// A helper for selecting the summary, or generating a helpful error message.
///
/// Returns a tuple that the first element is the summary selected. The second
/// is a package ID indicating that the patch entry should be unlocked. This
/// happens when a match cannot be found with the `locked` one, but found one
/// via the original patch, so we need to inform the resolver to "unlock" it.
fn summary_for_patch(
orig_patch: &Dependency,
locked: &Option<LockedPatchDependency>,
Expand Down Expand Up @@ -961,9 +966,6 @@ fn summary_for_patch(
let orig_matches = orig_matches.into_iter().map(|s| s.into_summary()).collect();

let summary = ready!(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 Poll::Ready(Ok((summary.0, Some(locked.package_id))));
}
// Try checking if there are *any* packages that match this by name.
Expand Down
17 changes: 16 additions & 1 deletion src/cargo/ops/cargo_generate_lockfile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,28 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
registry.add_sources(sources)?;
}

// Here we place an artificial limitation that all non-registry sources
// cannot be locked at more than one revision. This means that if a Git
// repository provides more than one package, they must all be updated in
// step when any of them are updated.
//
// TODO: this seems like a hokey reason to single out the registry as being
// different.
let to_avoid_sources: HashSet<_> = to_avoid
.iter()
.map(|p| p.source_id())
.filter(|s| !s.is_registry())
.collect();

let keep = |p: &PackageId| !to_avoid_sources.contains(&p.source_id()) && !to_avoid.contains(p);

let mut resolve = ops::resolve_with_previous(
&mut registry,
ws,
&CliFeatures::new_all(true),
HasDevUnits::Yes,
Some(&previous_resolve),
Some(&to_avoid),
Some(&keep),
&[],
true,
)?;
Expand Down
269 changes: 132 additions & 137 deletions src/cargo/ops/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,10 @@ fn resolve_with_registry<'gctx>(
/// Resolves all dependencies for a package using an optional previous instance
/// of resolve to guide the resolution process.
///
/// This also takes an optional hash set, `to_avoid`, which is a list of package
/// IDs that should be avoided when consulting the previous instance of resolve
/// (often used in pairings with updates).
/// This also takes an optional filter `keep_previous`, which informs the `registry`
/// which package ID should be locked to the previous instance of resolve
/// (often used in pairings with updates). See comments in [`register_previous_locks`]
/// for scenarios that might override this.
///
/// The previous resolve normally comes from a lock file. This function does not
/// read or write lock files from the filesystem.
Expand All @@ -283,7 +284,7 @@ pub fn resolve_with_previous<'gctx>(
cli_features: &CliFeatures,
has_dev_units: HasDevUnits,
previous: Option<&Resolve>,
to_avoid: Option<&HashSet<PackageId>>,
keep_previous: Option<&dyn Fn(&PackageId) -> bool>,
specs: &[PackageIdSpec],
register_patches: bool,
) -> CargoResult<Resolve> {
Expand All @@ -293,29 +294,8 @@ pub fn resolve_with_previous<'gctx>(
.gctx()
.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?;

// Here we place an artificial limitation that all non-registry sources
// cannot be locked at more than one revision. This means that if a Git
// repository provides more than one package, they must all be updated in
// step when any of them are updated.
//
// TODO: this seems like a hokey reason to single out the registry as being
// different.
let to_avoid_sources: HashSet<SourceId> = to_avoid
.map(|set| {
set.iter()
.map(|p| p.source_id())
.filter(|s| !s.is_registry())
.collect()
})
.unwrap_or_default();

let pre_patch_keep = |p: &PackageId| {
!to_avoid_sources.contains(&p.source_id())
&& match to_avoid {
Some(set) => !set.contains(p),
None => true,
}
};
// Try to keep all from previous resolve if no instruction given.
let keep_previous = keep_previous.unwrap_or(&|_| true);

// While registering patches, we will record preferences for particular versions
// of various packages.
Expand All @@ -327,117 +307,14 @@ pub fn resolve_with_previous<'gctx>(
version_prefs.max_rust_version(ws.rust_version().cloned().map(RustVersion::into_partial));
}

// This is a set of PackageIds of `[patch]` entries, and some related locked PackageIds, for
// which locking should be avoided (but which will be preferred when searching dependencies,
// via prefer_patch_deps below)
let mut avoid_patch_ids = HashSet::new();

if register_patches {
for (url, patches) in ws.root_patch()?.iter() {
for patch in patches {
version_prefs.prefer_dependency(patch.clone());
}
let Some(previous) = previous else {
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;
};

// 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_from(id_using_default.source_id()),
);

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,
}
}
};

registrations.push((dep, lock));
}

let canonical = CanonicalUrl::new(url)?;
for (orig_patch, unlock_id) in registry.patch(url, &registrations)? {
// 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 avoid_patch_ids = if register_patches {
register_patch_entries(registry, ws, previous, &mut version_prefs, keep_previous)?
} else {
HashSet::new()
};

let keep = |p: &PackageId| pre_patch_keep(p) && !avoid_patch_ids.contains(p);
// Refine `keep` with patches that should avoid locking.
let keep = |p: &PackageId| keep_previous(p) && !avoid_patch_ids.contains(p);

let dev_deps = ws.require_optional_deps() || has_dev_units == HasDevUnits::Yes;

Expand Down Expand Up @@ -880,3 +757,121 @@ fn emit_warnings_of_unused_patches(

return Ok(());
}

/// Informs `registry` and `version_pref` that `[patch]` entries are available
/// and preferable for the dependency resolution.
///
/// This returns a set of PackageIds of `[patch]` entries, and some related
/// locked PackageIds, for which locking should be avoided (but which will be
/// preferred when searching dependencies, via [`VersionPreferences::prefer_patch_deps`]).
#[tracing::instrument(level = "debug", skip_all, ret)]
fn register_patch_entries(
registry: &mut PackageRegistry<'_>,
ws: &Workspace<'_>,
previous: Option<&Resolve>,
version_prefs: &mut VersionPreferences,
keep_previous: &dyn Fn(&PackageId) -> bool,
) -> CargoResult<HashSet<PackageId>> {
let mut avoid_patch_ids = HashSet::new();
for (url, patches) in ws.root_patch()?.iter() {
for patch in patches {
version_prefs.prefer_dependency(patch.clone());
}
let Some(previous) = previous else {
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;
};

// 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(&keep_previous)
};

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_from(id_using_default.source_id()),
);

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,
}
}
};

registrations.push((dep, lock));
}

let canonical = CanonicalUrl::new(url)?;
for (orig_patch, unlock_id) in registry.patch(url, &registrations)? {
// 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
}));
}
}

Ok(avoid_patch_ids)
}

0 comments on commit e366699

Please sign in to comment.