Auto merge of #9703 - djmitche:versionprefs, r=alexcrichton

Factor version preferences into a struct

This concentrates all of the "prefer this version" logic previously
handled with `try_to_use` and `prefer_patch_deps` parameters into a
struct that hides both the reason a package version might be preferred
and the form that preference took (Dependency or PackageId).  Besides
simplifying `RegistryQuerier::query` slightly, this invites further
refinements to version preferences to support new cargo features.
This commit is contained in:
bors 2021-07-19 14:33:56 +00:00
commit fe15457291
5 changed files with 224 additions and 84 deletions

View File

@ -10,7 +10,7 @@ use std::rc::Rc;
use std::time::Instant;
use cargo::core::dependency::DepKind;
use cargo::core::resolver::{self, ResolveOpts};
use cargo::core::resolver::{self, ResolveOpts, VersionPreferences};
use cargo::core::source::{GitReference, SourceId};
use cargo::core::Resolve;
use cargo::core::{Dependency, PackageId, Registry, Summary};
@ -183,8 +183,7 @@ pub fn resolve_with_config_raw(
&[(summary, opts)],
&[],
&mut registry,
&HashSet::new(),
&HashMap::new(),
&VersionPreferences::default(),
Some(config),
true,
);

View File

@ -13,7 +13,8 @@ use crate::core::resolver::context::Context;
use crate::core::resolver::errors::describe_path;
use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
use crate::core::resolver::{
ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts,
ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, VersionOrdering,
VersionPreferences,
};
use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary};
use crate::util::errors::CargoResult;
@ -21,15 +22,13 @@ use crate::util::interning::InternedString;
use anyhow::Context as _;
use log::debug;
use std::cmp::Ordering;
use std::collections::{BTreeSet, HashMap, HashSet};
use std::rc::Rc;
pub struct RegistryQueryer<'a> {
pub registry: &'a mut (dyn Registry + 'a),
replacements: &'a [(PackageIdSpec, Dependency)],
try_to_use: &'a HashSet<PackageId>,
prefer_patch_deps: &'a HashMap<InternedString, HashSet<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.
@ -49,15 +48,13 @@ impl<'a> RegistryQueryer<'a> {
pub fn new(
registry: &'a mut dyn Registry,
replacements: &'a [(PackageIdSpec, Dependency)],
try_to_use: &'a HashSet<PackageId>,
prefer_patch_deps: &'a HashMap<InternedString, HashSet<Dependency>>,
version_prefs: &'a VersionPreferences,
minimal_versions: bool,
) -> Self {
RegistryQueryer {
registry,
replacements,
try_to_use,
prefer_patch_deps,
version_prefs,
minimal_versions,
registry_cache: HashMap::new(),
summary_cache: HashMap::new(),
@ -168,35 +165,15 @@ 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. Currently we try prioritized summaries (those in
// `try_to_use` or `prefer_patch_deps`) and failing that we list everything from the
// maximum version to the lowest version.
let should_prefer = |package_id: &PackageId| {
self.try_to_use.contains(package_id)
|| self
.prefer_patch_deps
.get(&package_id.name())
.map(|deps| deps.iter().any(|d| d.matches_id(*package_id)))
.unwrap_or(false)
};
ret.sort_unstable_by(|a, b| {
let prefer_a = should_prefer(&a.package_id());
let prefer_b = should_prefer(&b.package_id());
let previous_cmp = prefer_a.cmp(&prefer_b).reverse();
match previous_cmp {
Ordering::Equal => {
let cmp = a.version().cmp(b.version());
if self.minimal_versions {
// Lower version ordered first.
cmp
} else {
// Higher version ordered first.
cmp.reverse()
}
}
_ => previous_cmp,
}
});
// the "best candidates" first. VersionPreferences implements this notion.
self.version_prefs.sort_summaries(
&mut ret,
if self.minimal_versions {
VersionOrdering::MinimumVersionsFirst
} else {
VersionOrdering::MaximumVersionsFirst
},
);
let out = Rc::new(ret);

View File

@ -58,7 +58,6 @@ use crate::core::PackageIdSpec;
use crate::core::{Dependency, PackageId, Registry, Summary};
use crate::util::config::Config;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::profile;
use self::context::Context;
@ -73,6 +72,7 @@ pub use self::errors::{ActivateError, ActivateResult, ResolveError};
pub use self::features::{CliFeatures, ForceAllTargets, HasDevUnits};
pub use self::resolve::{Resolve, ResolveVersion};
pub use self::types::{ResolveBehavior, ResolveOpts};
pub use self::version_prefs::{VersionOrdering, VersionPreferences};
mod conflict_cache;
mod context;
@ -82,6 +82,7 @@ mod errors;
pub mod features;
mod resolve;
mod types;
mod version_prefs;
/// Builds the list of all packages required to build the first argument.
///
@ -102,14 +103,8 @@ mod types;
/// for the same query every time). Typically this is an instance of a
/// `PackageRegistry`.
///
/// * `try_to_use` - this is a list of package IDs which were previously found
/// in the lock file. We heuristically prefer the ids listed in `try_to_use`
/// when sorting candidates to activate, but otherwise this isn't used
/// anywhere else.
///
/// * `prefer_patch_deps` - this is a collection of dependencies on `[patch]`es, which
/// should be preferred much like `try_to_use`, but are in the form of Dependencies
/// and not PackageIds.
/// * `version_prefs` - this represents a preference for some versions over others,
/// based on the lock file or other reasons such as `[patch]`es.
///
/// * `config` - a location to print warnings and such, or `None` if no warnings
/// should be printed
@ -128,8 +123,7 @@ pub fn resolve(
summaries: &[(Summary, ResolveOpts)],
replacements: &[(PackageIdSpec, Dependency)],
registry: &mut dyn Registry,
try_to_use: &HashSet<PackageId>,
prefer_patch_deps: &HashMap<InternedString, HashSet<Dependency>>,
version_prefs: &VersionPreferences,
config: Option<&Config>,
check_public_visible_dependencies: bool,
) -> CargoResult<Resolve> {
@ -139,13 +133,8 @@ pub fn resolve(
Some(config) => config.cli_unstable().minimal_versions,
None => false,
};
let mut registry = RegistryQueryer::new(
registry,
replacements,
try_to_use,
prefer_patch_deps,
minimal_versions,
);
let mut registry =
RegistryQueryer::new(registry, replacements, version_prefs, minimal_versions);
let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
let mut cksums = HashMap::new();

View File

@ -0,0 +1,181 @@
//! This module implements support for preferring some versions of a package
//! over other versions.
use std::cmp::Ordering;
use std::collections::{HashMap, HashSet};
use crate::core::{Dependency, PackageId, Summary};
use crate::util::interning::InternedString;
/// A collection of preferences for particular package versions.
///
/// This is built up with [`prefer_package_id`] and [`prefer_dep`], then used to sort the set of
/// summaries for a package during resolution via [`sort_summaries`].
///
/// As written, a version is either "preferred" or "not preferred". Later extensions may
/// introduce more granular preferences.
#[derive(Default)]
pub struct VersionPreferences {
try_to_use: HashSet<PackageId>,
prefer_patch_deps: HashMap<InternedString, HashSet<Dependency>>,
}
pub enum VersionOrdering {
MaximumVersionsFirst,
MinimumVersionsFirst,
}
impl VersionPreferences {
/// Indicate that the given package (specified as a [`PackageId`]) should be preferred.
pub fn prefer_package_id(&mut self, pkg_id: PackageId) {
self.try_to_use.insert(pkg_id);
}
/// Indicate that the given package (specified as a [`Dependency`]) should be preferred.
pub fn prefer_dependency(&mut self, dep: Dependency) {
self.prefer_patch_deps
.entry(dep.package_name())
.or_insert_with(HashSet::new)
.insert(dep);
}
/// Sort the given vector of summaries in-place, with all summaries presumed to be for
/// the same package. Preferred versions appear first in the result, sorted by
/// `version_ordering`, followed by non-preferred versions sorted the same way.
pub fn sort_summaries(&self, summaries: &mut Vec<Summary>, version_ordering: VersionOrdering) {
let should_prefer = |pkg_id: &PackageId| {
self.try_to_use.contains(pkg_id)
|| self
.prefer_patch_deps
.get(&pkg_id.name())
.map(|deps| deps.iter().any(|d| d.matches_id(*pkg_id)))
.unwrap_or(false)
};
summaries.sort_unstable_by(|a, b| {
let prefer_a = should_prefer(&a.package_id());
let prefer_b = should_prefer(&b.package_id());
let previous_cmp = prefer_a.cmp(&prefer_b).reverse();
match previous_cmp {
Ordering::Equal => {
let cmp = a.version().cmp(b.version());
match version_ordering {
VersionOrdering::MaximumVersionsFirst => cmp.reverse(),
VersionOrdering::MinimumVersionsFirst => cmp,
}
}
_ => previous_cmp,
}
});
}
}
#[cfg(test)]
mod test {
use super::*;
use crate::core::SourceId;
use crate::util::Config;
use std::collections::BTreeMap;
fn pkgid(name: &str, version: &str) -> PackageId {
let src_id =
SourceId::from_url("registry+https://github.com/rust-lang/crates.io-index").unwrap();
PackageId::new(name, version, src_id).unwrap()
}
fn dep(name: &str, version: &str) -> Dependency {
let src_id =
SourceId::from_url("registry+https://github.com/rust-lang/crates.io-index").unwrap();
Dependency::parse(name, Some(version), src_id).unwrap()
}
fn summ(name: &str, version: &str) -> Summary {
let pkg_id = pkgid(name, version);
let config = Config::default().unwrap();
let features = BTreeMap::new();
Summary::new(&config, pkg_id, Vec::new(), &features, None::<&String>).unwrap()
}
fn describe(summaries: &Vec<Summary>) -> String {
let strs: Vec<String> = summaries
.iter()
.map(|summary| format!("{}/{}", summary.name(), summary.version()))
.collect();
strs.join(", ")
}
#[test]
fn test_prefer_package_id() {
let mut vp = VersionPreferences::default();
vp.prefer_package_id(pkgid("foo", "1.2.3"));
let mut summaries = vec![
summ("foo", "1.2.4"),
summ("foo", "1.2.3"),
summ("foo", "1.1.0"),
summ("foo", "1.0.9"),
];
vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
assert_eq!(
describe(&summaries),
"foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string()
);
vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
assert_eq!(
describe(&summaries),
"foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string()
);
}
#[test]
fn test_prefer_dependency() {
let mut vp = VersionPreferences::default();
vp.prefer_dependency(dep("foo", "=1.2.3"));
let mut summaries = vec![
summ("foo", "1.2.4"),
summ("foo", "1.2.3"),
summ("foo", "1.1.0"),
summ("foo", "1.0.9"),
];
vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
assert_eq!(
describe(&summaries),
"foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string()
);
vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
assert_eq!(
describe(&summaries),
"foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string()
);
}
#[test]
fn test_prefer_both() {
let mut vp = VersionPreferences::default();
vp.prefer_package_id(pkgid("foo", "1.2.3"));
vp.prefer_dependency(dep("foo", "=1.1.0"));
let mut summaries = vec![
summ("foo", "1.2.4"),
summ("foo", "1.2.3"),
summ("foo", "1.1.0"),
summ("foo", "1.0.9"),
];
vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
assert_eq!(
describe(&summaries),
"foo/1.2.3, foo/1.1.0, foo/1.2.4, foo/1.0.9".to_string()
);
vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
assert_eq!(
describe(&summaries),
"foo/1.1.0, foo/1.2.3, foo/1.0.9, foo/1.2.4".to_string()
);
}
}

View File

@ -15,7 +15,9 @@ use crate::core::registry::{LockedPatchDependency, PackageRegistry};
use crate::core::resolver::features::{
CliFeatures, FeatureOpts, FeatureResolver, ForceAllTargets, RequestedFeatures, ResolvedFeatures,
};
use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion};
use crate::core::resolver::{
self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion, VersionPreferences,
};
use crate::core::summary::Summary;
use crate::core::Feature;
use crate::core::{
@ -27,7 +29,7 @@ use crate::util::errors::CargoResult;
use crate::util::{profile, CanonicalUrl};
use anyhow::Context as _;
use log::{debug, trace};
use std::collections::{HashMap, HashSet};
use std::collections::HashSet;
/// Result for `resolve_ws_with_opts`.
pub struct WorkspaceResolve<'cfg> {
@ -242,22 +244,19 @@ pub fn resolve_with_previous<'cfg>(
}
};
// While registering patches, we will record preferences for particular versions
// of various packages.
let mut version_prefs = VersionPreferences::default();
// 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();
// This is a set of Dependency's of `[patch]` entries that should be preferred when searching
// dependencies.
let mut prefer_patch_deps = HashMap::new();
if register_patches {
for (url, patches) in ws.root_patch()?.iter() {
for patch in patches {
prefer_patch_deps
.entry(patch.package_name())
.or_insert_with(HashSet::new)
.insert(patch.clone());
version_prefs.prefer_dependency(patch.clone());
}
let previous = match previous {
Some(r) => r,
@ -365,7 +364,6 @@ pub fn resolve_with_previous<'cfg>(
}
}
debug!("avoid_patch_ids={:?}", avoid_patch_ids);
debug!("prefer_patch_deps={:?}", prefer_patch_deps);
let keep = |p: &PackageId| pre_patch_keep(p) && !avoid_patch_ids.contains(p);
@ -377,19 +375,16 @@ pub fn resolve_with_previous<'cfg>(
trace!("previous: {:?}", r);
register_previous_locks(ws, registry, r, &keep, dev_deps);
}
// Everything in the previous lock file we want to keep is prioritized
// in dependency selection if it comes up, aka we want to have
// conservative updates.
let try_to_use = previous
.map(|r| {
r.iter()
.filter(keep)
.inspect(|id| {
debug!("attempting to prefer {}", id);
})
.collect()
})
.unwrap_or_default();
// Prefer to use anything in the previous lock file, aka we want to have conservative updates.
for r in previous {
for id in r.iter() {
if keep(&id) {
debug!("attempting to prefer {}", id);
version_prefs.prefer_package_id(id);
}
}
}
if register_patches {
registry.lock_patches();
@ -438,8 +433,7 @@ pub fn resolve_with_previous<'cfg>(
&summaries,
&replace,
registry,
&try_to_use,
&prefer_patch_deps,
&version_prefs,
Some(ws.config()),
ws.unstable_features()
.require(Feature::public_dependency())