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

refactor: make resolve_with_previous clearer #13727

Merged
merged 4 commits into from
Apr 10, 2024
Merged

refactor: make resolve_with_previous clearer #13727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e55c205
refactor: move to_avoid closure to `cargo update`
weihanglo Apr 9, 2024
57c814e
refactor: extract patch registration to a function
weihanglo Apr 9, 2024
f978671
refactor: unify naming of `keep` closure
weihanglo Apr 9, 2024
80d0d94
doc: add commment to `summary_for_patch`
weihanglo Apr 9, 2024
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
10 changes: 6 additions & 4 deletions src/cargo/core/registry.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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)
}