From 76a079cbd920830b337ef21eccaeb1ed33919ea7 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 18 Jul 2021 19:31:05 +0000 Subject: [PATCH] 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. --- crates/resolver-tests/src/lib.rs | 5 +- src/cargo/core/resolver/dep_cache.rs | 51 ++----- src/cargo/core/resolver/mod.rs | 25 +--- src/cargo/core/resolver/version_prefs.rs | 181 +++++++++++++++++++++++ src/cargo/ops/resolve.rs | 46 +++--- 5 files changed, 224 insertions(+), 84 deletions(-) create mode 100644 src/cargo/core/resolver/version_prefs.rs diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index a278b3f47..f47017d1d 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -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, ); diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index a604e0af8..82aec0b8f 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -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, - prefer_patch_deps: &'a HashMap>, + 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, - prefer_patch_deps: &'a HashMap>, + 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); diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 8edff2696..8f947594d 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -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, - prefer_patch_deps: &HashMap>, + version_prefs: &VersionPreferences, config: Option<&Config>, check_public_visible_dependencies: bool, ) -> CargoResult { @@ -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(); diff --git a/src/cargo/core/resolver/version_prefs.rs b/src/cargo/core/resolver/version_prefs.rs new file mode 100644 index 000000000..0add79d3c --- /dev/null +++ b/src/cargo/core/resolver/version_prefs.rs @@ -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, + prefer_patch_deps: HashMap>, +} + +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, 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) -> String { + let strs: Vec = 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() + ); + } +} diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index b3aff231c..6435ef6b1 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -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())