Skip to content

Commit

Permalink
Auto merge of #12930 - epage:msrv-refactor, r=Eh2406
Browse files Browse the repository at this point in the history 
refactor(resolver): Consolidate logic in `VersionPreferences`

### What does this PR try to resolve?

This makes customizing the resolver less intrusive by putting the logic in `VersionPreferences` and making it easy to add new priority cases to it.

In particular, this is prep for tweaking the MSRV resolver to prefer compatible versions, rather than require them.
See https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Alternative.20MSRV-aware.20resolver.20approach.3F

### How should we test and review this PR?

Each step is broken down into its own commit for easier browsing

### Additional information
  • Loading branch information
@bors
bors committed Nov 10, 2023
2 parents ac9d3ba + 46bb8ee commit 19a0404
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 134 deletions.
21 changes: 13 additions & 8 deletions crates/resolver-tests/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::task::Poll;
use std::time::Instant;

use cargo::core::dependency::DepKind;
use cargo::core::resolver::{self, ResolveOpts, VersionPreferences};
use cargo::core::resolver::{self, ResolveOpts, VersionOrdering, VersionPreferences};
use cargo::core::Resolve;
use cargo::core::{Dependency, PackageId, Registry, Summary};
use cargo::core::{GitReference, SourceId};
Expand Down Expand Up @@ -190,15 +190,17 @@ pub fn resolve_with_config_raw(
.unwrap();
let opts = ResolveOpts::everything();
let start = Instant::now();
let max_rust_version = None;
let mut version_prefs = VersionPreferences::default();
if config.cli_unstable().minimal_versions {
version_prefs.version_ordering(VersionOrdering::MinimumVersionsFirst)
}
let resolve = resolver::resolve(
&[(summary, opts)],
&[],
&mut registry,
&VersionPreferences::default(),
&version_prefs,
Some(config),
true,
max_rust_version,
);

// The largest test in our suite takes less then 30 sec.
Expand Down Expand Up @@ -982,14 +984,17 @@ fn meta_test_multiple_versions_strategy() {

/// Assert `xs` contains `elems`
#[track_caller]
pub fn assert_contains<A: PartialEq>(xs: &[A], elems: &[A]) {
pub fn assert_contains<A: PartialEq + std::fmt::Debug>(xs: &[A], elems: &[A]) {
for elem in elems {
assert!(xs.contains(elem));
assert!(
xs.contains(elem),
"missing element\nset: {xs:?}\nmissing: {elem:?}"
);
}
}

#[track_caller]
pub fn assert_same<A: PartialEq>(a: &[A], b: &[A]) {
assert_eq!(a.len(), b.len());
pub fn assert_same<A: PartialEq + std::fmt::Debug>(a: &[A], b: &[A]) {
assert_eq!(a.len(), b.len(), "not equal\n{a:?}\n{b:?}");
assert_contains(b, a);
}
77 changes: 27 additions & 50 deletions src/cargo/core/resolver/dep_cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry,
use crate::sources::source::QueryKind;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::RustVersion;

use anyhow::Context as _;
use std::collections::{BTreeSet, HashMap, HashSet};
Expand All @@ -32,16 +31,11 @@ pub struct RegistryQueryer<'a> {
pub registry: &'a mut (dyn Registry + 'a),
replacements: &'a [(PackageIdSpec, Dependency)],
version_prefs: &'a VersionPreferences,
/// If set the list of dependency candidates will be sorted by minimal
/// versions first. That allows `cargo update -Z minimal-versions` which will
/// specify minimum dependency versions to be used.
minimal_versions: bool,
max_rust_version: Option<RustVersion>,
/// a cache of `Candidate`s that fulfil a `Dependency` (and whether `first_minimal_version`)
registry_cache: HashMap<(Dependency, bool), Poll<Rc<Vec<Summary>>>>,
/// a cache of `Candidate`s that fulfil a `Dependency` (and whether `first_version`)
registry_cache: HashMap<(Dependency, Option<VersionOrdering>), Poll<Rc<Vec<Summary>>>>,
/// a cache of `Dependency`s that are required for a `Summary`
///
/// HACK: `first_minimal_version` is not kept in the cache key is it is 1:1 with
/// HACK: `first_version` is not kept in the cache key is it is 1:1 with
/// `parent.is_none()` (the first element of the cache key) as it doesn't change through
/// execution.
summary_cache: HashMap<
Expand All @@ -57,15 +51,11 @@ impl<'a> RegistryQueryer<'a> {
registry: &'a mut dyn Registry,
replacements: &'a [(PackageIdSpec, Dependency)],
version_prefs: &'a VersionPreferences,
minimal_versions: bool,
max_rust_version: Option<&RustVersion>,
) -> Self {
RegistryQueryer {
registry,
replacements,
version_prefs,
minimal_versions,
max_rust_version: max_rust_version.cloned(),
registry_cache: HashMap::new(),
summary_cache: HashMap::new(),
used_replacements: HashMap::new(),
Expand Down Expand Up @@ -106,23 +96,20 @@ impl<'a> RegistryQueryer<'a> {
pub fn query(
&mut self,
dep: &Dependency,
first_minimal_version: bool,
first_version: Option<VersionOrdering>,
) -> Poll<CargoResult<Rc<Vec<Summary>>>> {
let registry_cache_key = (dep.clone(), first_minimal_version);
let registry_cache_key = (dep.clone(), first_version);
if let Some(out) = self.registry_cache.get(&registry_cache_key).cloned() {
return out.map(Result::Ok);
}

let mut ret = Vec::new();
let ready = self.registry.query(dep, QueryKind::Exact, &mut |s| {
if self.max_rust_version.is_none() || s.rust_version() <= self.max_rust_version.as_ref()
{
ret.push(s);
}
ret.push(s);
})?;
if ready.is_pending() {
self.registry_cache
.insert((dep.clone(), first_minimal_version), Poll::Pending);
.insert((dep.clone(), first_version), Poll::Pending);
return Poll::Pending;
}
for summary in ret.iter() {
Expand All @@ -144,7 +131,7 @@ impl<'a> RegistryQueryer<'a> {
Poll::Ready(s) => s.into_iter(),
Poll::Pending => {
self.registry_cache
.insert((dep.clone(), first_minimal_version), Poll::Pending);
.insert((dep.clone(), first_version), Poll::Pending);
return Poll::Pending;
}
};
Expand Down Expand Up @@ -215,16 +202,8 @@ impl<'a> RegistryQueryer<'a> {
}
}

// When we attempt versions for a package we'll want to do so in a sorted fashion to pick
// the "best candidates" first. VersionPreferences implements this notion.
let ordering = if first_minimal_version || self.minimal_versions {
VersionOrdering::MinimumVersionsFirst
} else {
VersionOrdering::MaximumVersionsFirst
};
let first_version = first_minimal_version;
self.version_prefs
.sort_summaries(&mut ret, ordering, first_version);
let first_version = first_version;
self.version_prefs.sort_summaries(&mut ret, first_version);

let out = Poll::Ready(Rc::new(ret));

Expand All @@ -243,7 +222,7 @@ impl<'a> RegistryQueryer<'a> {
parent: Option<PackageId>,
candidate: &Summary,
opts: &ResolveOpts,
first_minimal_version: bool,
first_version: Option<VersionOrdering>,
) -> ActivateResult<Rc<(HashSet<InternedString>, Rc<Vec<DepInfo>>)>> {
// if we have calculated a result before, then we can just return it,
// as it is a "pure" query of its arguments.
Expand All @@ -263,24 +242,22 @@ impl<'a> RegistryQueryer<'a> {
let mut all_ready = true;
let mut deps = deps
.into_iter()
.filter_map(
|(dep, features)| match self.query(&dep, first_minimal_version) {
Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))),
Poll::Pending => {
all_ready = false;
// we can ignore Pending deps, resolve will be repeatedly called
// until there are none to ignore
None
}
Poll::Ready(Err(e)) => Some(Err(e).with_context(|| {
format!(
"failed to get `{}` as a dependency of {}",
dep.package_name(),
describe_path_in_context(cx, &candidate.package_id()),
)
})),
},
)
.filter_map(|(dep, features)| match self.query(&dep, first_version) {
Poll::Ready(Ok(candidates)) => Some(Ok((dep, candidates, features))),
Poll::Pending => {
all_ready = false;
// we can ignore Pending deps, resolve will be repeatedly called
// until there are none to ignore
None
}
Poll::Ready(Err(e)) => Some(Err(e).with_context(|| {
format!(
"failed to get `{}` as a dependency of {}",
dep.package_name(),
describe_path_in_context(cx, &candidate.package_id()),
)
})),
})
.collect::<CargoResult<Vec<DepInfo>>>()?;

// Attempt to resolve dependencies with fewer candidates before trying
Expand Down
54 changes: 16 additions & 38 deletions src/cargo/core/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ use crate::util::config::Config;
use crate::util::errors::CargoResult;
use crate::util::network::PollExt;
use crate::util::profile;
use crate::util::RustVersion;

use self::context::Context;
use self::dep_cache::RegistryQueryer;
Expand Down Expand Up @@ -139,39 +138,18 @@ pub fn resolve(
version_prefs: &VersionPreferences,
config: Option<&Config>,
check_public_visible_dependencies: bool,
mut max_rust_version: Option<&RustVersion>,
) -> CargoResult<Resolve> {
let _p = profile::start("resolving");
let minimal_versions = match config {
Some(config) => config.cli_unstable().minimal_versions,
None => false,
};
let direct_minimal_versions = match config {
Some(config) => config.cli_unstable().direct_minimal_versions,
None => false,
let first_version = match config {
Some(config) if config.cli_unstable().direct_minimal_versions => {
Some(VersionOrdering::MinimumVersionsFirst)
}
_ => None,
};
if !config
.map(|c| c.cli_unstable().msrv_policy)
.unwrap_or(false)
{
max_rust_version = None;
}
let mut registry = RegistryQueryer::new(
registry,
replacements,
version_prefs,
minimal_versions,
max_rust_version,
);
let mut registry = RegistryQueryer::new(registry, replacements, version_prefs);
let cx = loop {
let cx = Context::new(check_public_visible_dependencies);
let cx = activate_deps_loop(
cx,
&mut registry,
summaries,
direct_minimal_versions,
config,
)?;
let cx = activate_deps_loop(cx, &mut registry, summaries, first_version, config)?;
if registry.reset_pending() {
break cx;
} else {
Expand Down Expand Up @@ -223,7 +201,7 @@ fn activate_deps_loop(
mut cx: Context,
registry: &mut RegistryQueryer<'_>,
summaries: &[(Summary, ResolveOpts)],
direct_minimal_versions: bool,
first_version: Option<VersionOrdering>,
config: Option<&Config>,
) -> CargoResult<Context> {
let mut backtrack_stack = Vec::new();
Expand All @@ -241,7 +219,7 @@ fn activate_deps_loop(
registry,
None,
summary.clone(),
direct_minimal_versions,
first_version,
opts,
);
match res {
Expand Down Expand Up @@ -441,13 +419,13 @@ fn activate_deps_loop(
dep.package_name(),
candidate.version()
);
let direct_minimal_version = false; // this is an indirect dependency
let first_version = None; // this is an indirect dependency
let res = activate(
&mut cx,
registry,
Some((&parent, &dep)),
candidate,
direct_minimal_version,
first_version,
&opts,
);

Expand Down Expand Up @@ -659,7 +637,7 @@ fn activate(
registry: &mut RegistryQueryer<'_>,
parent: Option<(&Summary, &Dependency)>,
candidate: Summary,
first_minimal_version: bool,
first_version: Option<VersionOrdering>,
opts: &ResolveOpts,
) -> ActivateResult<Option<(DepsFrame, Duration)>> {
let candidate_pid = candidate.package_id();
Expand Down Expand Up @@ -716,7 +694,7 @@ fn activate(
parent.map(|p| p.0.package_id()),
&candidate,
opts,
first_minimal_version,
first_version,
)?;

// Record what list of features is active for this package.
Expand Down Expand Up @@ -905,14 +883,14 @@ fn generalize_conflicting(
})
{
for critical_parents_dep in critical_parents_deps.iter() {
// We only want `first_minimal_version=true` for direct dependencies of workspace
// We only want `first_version.is_some()` for direct dependencies of workspace
// members which isn't the case here as this has a `parent`
let first_minimal_version = false;
let first_version = None;
// A dep is equivalent to one of the things it can resolve to.
// Thus, if all the things it can resolve to have already ben determined
// to be conflicting, then we can just say that we conflict with the parent.
if let Some(others) = registry
.query(critical_parents_dep, first_minimal_version)
.query(critical_parents_dep, first_version)
.expect("an already used dep now error!?")
.expect("an already used dep now pending!?")
.iter()
Expand Down
Loading

0 comments on commit 19a0404

Please sign in to comment.