Skip to content

Commit fe15457

Browse files
committed
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.
2 parents b0e8717 + 76a079c commit fe15457

File tree

5 files changed

+224
-84
lines changed

5 files changed

+224
-84
lines changed

Diff for: crates/resolver-tests/src/lib.rs

+2-3
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use std::rc::Rc;
1010
use std::time::Instant;
1111

1212
use cargo::core::dependency::DepKind;
13-
use cargo::core::resolver::{self, ResolveOpts};
13+
use cargo::core::resolver::{self, ResolveOpts, VersionPreferences};
1414
use cargo::core::source::{GitReference, SourceId};
1515
use cargo::core::Resolve;
1616
use cargo::core::{Dependency, PackageId, Registry, Summary};
@@ -183,8 +183,7 @@ pub fn resolve_with_config_raw(
183183
&[(summary, opts)],
184184
&[],
185185
&mut registry,
186-
&HashSet::new(),
187-
&HashMap::new(),
186+
&VersionPreferences::default(),
188187
Some(config),
189188
true,
190189
);

Diff for: src/cargo/core/resolver/dep_cache.rs

+14-37
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,22 @@ use crate::core::resolver::context::Context;
1313
use crate::core::resolver::errors::describe_path;
1414
use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
1515
use crate::core::resolver::{
16-
ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts,
16+
ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, VersionOrdering,
17+
VersionPreferences,
1718
};
1819
use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary};
1920
use crate::util::errors::CargoResult;
2021
use crate::util::interning::InternedString;
2122

2223
use anyhow::Context as _;
2324
use log::debug;
24-
use std::cmp::Ordering;
2525
use std::collections::{BTreeSet, HashMap, HashSet};
2626
use std::rc::Rc;
2727

2828
pub struct RegistryQueryer<'a> {
2929
pub registry: &'a mut (dyn Registry + 'a),
3030
replacements: &'a [(PackageIdSpec, Dependency)],
31-
try_to_use: &'a HashSet<PackageId>,
32-
prefer_patch_deps: &'a HashMap<InternedString, HashSet<Dependency>>,
31+
version_prefs: &'a VersionPreferences,
3332
/// If set the list of dependency candidates will be sorted by minimal
3433
/// versions first. That allows `cargo update -Z minimal-versions` which will
3534
/// specify minimum dependency versions to be used.
@@ -49,15 +48,13 @@ impl<'a> RegistryQueryer<'a> {
4948
pub fn new(
5049
registry: &'a mut dyn Registry,
5150
replacements: &'a [(PackageIdSpec, Dependency)],
52-
try_to_use: &'a HashSet<PackageId>,
53-
prefer_patch_deps: &'a HashMap<InternedString, HashSet<Dependency>>,
51+
version_prefs: &'a VersionPreferences,
5452
minimal_versions: bool,
5553
) -> Self {
5654
RegistryQueryer {
5755
registry,
5856
replacements,
59-
try_to_use,
60-
prefer_patch_deps,
57+
version_prefs,
6158
minimal_versions,
6259
registry_cache: HashMap::new(),
6360
summary_cache: HashMap::new(),
@@ -168,35 +165,15 @@ impl<'a> RegistryQueryer<'a> {
168165
}
169166

170167
// When we attempt versions for a package we'll want to do so in a sorted fashion to pick
171-
// the "best candidates" first. Currently we try prioritized summaries (those in
172-
// `try_to_use` or `prefer_patch_deps`) and failing that we list everything from the
173-
// maximum version to the lowest version.
174-
let should_prefer = |package_id: &PackageId| {
175-
self.try_to_use.contains(package_id)
176-
|| self
177-
.prefer_patch_deps
178-
.get(&package_id.name())
179-
.map(|deps| deps.iter().any(|d| d.matches_id(*package_id)))
180-
.unwrap_or(false)
181-
};
182-
ret.sort_unstable_by(|a, b| {
183-
let prefer_a = should_prefer(&a.package_id());
184-
let prefer_b = should_prefer(&b.package_id());
185-
let previous_cmp = prefer_a.cmp(&prefer_b).reverse();
186-
match previous_cmp {
187-
Ordering::Equal => {
188-
let cmp = a.version().cmp(b.version());
189-
if self.minimal_versions {
190-
// Lower version ordered first.
191-
cmp
192-
} else {
193-
// Higher version ordered first.
194-
cmp.reverse()
195-
}
196-
}
197-
_ => previous_cmp,
198-
}
199-
});
168+
// the "best candidates" first. VersionPreferences implements this notion.
169+
self.version_prefs.sort_summaries(
170+
&mut ret,
171+
if self.minimal_versions {
172+
VersionOrdering::MinimumVersionsFirst
173+
} else {
174+
VersionOrdering::MaximumVersionsFirst
175+
},
176+
);
200177

201178
let out = Rc::new(ret);
202179

Diff for: src/cargo/core/resolver/mod.rs

+7-18
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ use crate::core::PackageIdSpec;
5858
use crate::core::{Dependency, PackageId, Registry, Summary};
5959
use crate::util::config::Config;
6060
use crate::util::errors::CargoResult;
61-
use crate::util::interning::InternedString;
6261
use crate::util::profile;
6362

6463
use self::context::Context;
@@ -73,6 +72,7 @@ pub use self::errors::{ActivateError, ActivateResult, ResolveError};
7372
pub use self::features::{CliFeatures, ForceAllTargets, HasDevUnits};
7473
pub use self::resolve::{Resolve, ResolveVersion};
7574
pub use self::types::{ResolveBehavior, ResolveOpts};
75+
pub use self::version_prefs::{VersionOrdering, VersionPreferences};
7676

7777
mod conflict_cache;
7878
mod context;
@@ -82,6 +82,7 @@ mod errors;
8282
pub mod features;
8383
mod resolve;
8484
mod types;
85+
mod version_prefs;
8586

8687
/// Builds the list of all packages required to build the first argument.
8788
///
@@ -102,14 +103,8 @@ mod types;
102103
/// for the same query every time). Typically this is an instance of a
103104
/// `PackageRegistry`.
104105
///
105-
/// * `try_to_use` - this is a list of package IDs which were previously found
106-
/// in the lock file. We heuristically prefer the ids listed in `try_to_use`
107-
/// when sorting candidates to activate, but otherwise this isn't used
108-
/// anywhere else.
109-
///
110-
/// * `prefer_patch_deps` - this is a collection of dependencies on `[patch]`es, which
111-
/// should be preferred much like `try_to_use`, but are in the form of Dependencies
112-
/// and not PackageIds.
106+
/// * `version_prefs` - this represents a preference for some versions over others,
107+
/// based on the lock file or other reasons such as `[patch]`es.
113108
///
114109
/// * `config` - a location to print warnings and such, or `None` if no warnings
115110
/// should be printed
@@ -128,8 +123,7 @@ pub fn resolve(
128123
summaries: &[(Summary, ResolveOpts)],
129124
replacements: &[(PackageIdSpec, Dependency)],
130125
registry: &mut dyn Registry,
131-
try_to_use: &HashSet<PackageId>,
132-
prefer_patch_deps: &HashMap<InternedString, HashSet<Dependency>>,
126+
version_prefs: &VersionPreferences,
133127
config: Option<&Config>,
134128
check_public_visible_dependencies: bool,
135129
) -> CargoResult<Resolve> {
@@ -139,13 +133,8 @@ pub fn resolve(
139133
Some(config) => config.cli_unstable().minimal_versions,
140134
None => false,
141135
};
142-
let mut registry = RegistryQueryer::new(
143-
registry,
144-
replacements,
145-
try_to_use,
146-
prefer_patch_deps,
147-
minimal_versions,
148-
);
136+
let mut registry =
137+
RegistryQueryer::new(registry, replacements, version_prefs, minimal_versions);
149138
let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
150139

151140
let mut cksums = HashMap::new();

Diff for: src/cargo/core/resolver/version_prefs.rs

+181
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
//! This module implements support for preferring some versions of a package
2+
//! over other versions.
3+
4+
use std::cmp::Ordering;
5+
use std::collections::{HashMap, HashSet};
6+
7+
use crate::core::{Dependency, PackageId, Summary};
8+
use crate::util::interning::InternedString;
9+
10+
/// A collection of preferences for particular package versions.
11+
///
12+
/// This is built up with [`prefer_package_id`] and [`prefer_dep`], then used to sort the set of
13+
/// summaries for a package during resolution via [`sort_summaries`].
14+
///
15+
/// As written, a version is either "preferred" or "not preferred". Later extensions may
16+
/// introduce more granular preferences.
17+
#[derive(Default)]
18+
pub struct VersionPreferences {
19+
try_to_use: HashSet<PackageId>,
20+
prefer_patch_deps: HashMap<InternedString, HashSet<Dependency>>,
21+
}
22+
23+
pub enum VersionOrdering {
24+
MaximumVersionsFirst,
25+
MinimumVersionsFirst,
26+
}
27+
28+
impl VersionPreferences {
29+
/// Indicate that the given package (specified as a [`PackageId`]) should be preferred.
30+
pub fn prefer_package_id(&mut self, pkg_id: PackageId) {
31+
self.try_to_use.insert(pkg_id);
32+
}
33+
34+
/// Indicate that the given package (specified as a [`Dependency`]) should be preferred.
35+
pub fn prefer_dependency(&mut self, dep: Dependency) {
36+
self.prefer_patch_deps
37+
.entry(dep.package_name())
38+
.or_insert_with(HashSet::new)
39+
.insert(dep);
40+
}
41+
42+
/// Sort the given vector of summaries in-place, with all summaries presumed to be for
43+
/// the same package. Preferred versions appear first in the result, sorted by
44+
/// `version_ordering`, followed by non-preferred versions sorted the same way.
45+
pub fn sort_summaries(&self, summaries: &mut Vec<Summary>, version_ordering: VersionOrdering) {
46+
let should_prefer = |pkg_id: &PackageId| {
47+
self.try_to_use.contains(pkg_id)
48+
|| self
49+
.prefer_patch_deps
50+
.get(&pkg_id.name())
51+
.map(|deps| deps.iter().any(|d| d.matches_id(*pkg_id)))
52+
.unwrap_or(false)
53+
};
54+
summaries.sort_unstable_by(|a, b| {
55+
let prefer_a = should_prefer(&a.package_id());
56+
let prefer_b = should_prefer(&b.package_id());
57+
let previous_cmp = prefer_a.cmp(&prefer_b).reverse();
58+
match previous_cmp {
59+
Ordering::Equal => {
60+
let cmp = a.version().cmp(b.version());
61+
match version_ordering {
62+
VersionOrdering::MaximumVersionsFirst => cmp.reverse(),
63+
VersionOrdering::MinimumVersionsFirst => cmp,
64+
}
65+
}
66+
_ => previous_cmp,
67+
}
68+
});
69+
}
70+
}
71+
72+
#[cfg(test)]
73+
mod test {
74+
use super::*;
75+
use crate::core::SourceId;
76+
use crate::util::Config;
77+
use std::collections::BTreeMap;
78+
79+
fn pkgid(name: &str, version: &str) -> PackageId {
80+
let src_id =
81+
SourceId::from_url("registry+https://github.com/rust-lang/crates.io-index").unwrap();
82+
PackageId::new(name, version, src_id).unwrap()
83+
}
84+
85+
fn dep(name: &str, version: &str) -> Dependency {
86+
let src_id =
87+
SourceId::from_url("registry+https://github.com/rust-lang/crates.io-index").unwrap();
88+
Dependency::parse(name, Some(version), src_id).unwrap()
89+
}
90+
91+
fn summ(name: &str, version: &str) -> Summary {
92+
let pkg_id = pkgid(name, version);
93+
let config = Config::default().unwrap();
94+
let features = BTreeMap::new();
95+
Summary::new(&config, pkg_id, Vec::new(), &features, None::<&String>).unwrap()
96+
}
97+
98+
fn describe(summaries: &Vec<Summary>) -> String {
99+
let strs: Vec<String> = summaries
100+
.iter()
101+
.map(|summary| format!("{}/{}", summary.name(), summary.version()))
102+
.collect();
103+
strs.join(", ")
104+
}
105+
106+
#[test]
107+
fn test_prefer_package_id() {
108+
let mut vp = VersionPreferences::default();
109+
vp.prefer_package_id(pkgid("foo", "1.2.3"));
110+
111+
let mut summaries = vec![
112+
summ("foo", "1.2.4"),
113+
summ("foo", "1.2.3"),
114+
summ("foo", "1.1.0"),
115+
summ("foo", "1.0.9"),
116+
];
117+
118+
vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
119+
assert_eq!(
120+
describe(&summaries),
121+
"foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string()
122+
);
123+
124+
vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
125+
assert_eq!(
126+
describe(&summaries),
127+
"foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string()
128+
);
129+
}
130+
131+
#[test]
132+
fn test_prefer_dependency() {
133+
let mut vp = VersionPreferences::default();
134+
vp.prefer_dependency(dep("foo", "=1.2.3"));
135+
136+
let mut summaries = vec![
137+
summ("foo", "1.2.4"),
138+
summ("foo", "1.2.3"),
139+
summ("foo", "1.1.0"),
140+
summ("foo", "1.0.9"),
141+
];
142+
143+
vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
144+
assert_eq!(
145+
describe(&summaries),
146+
"foo/1.2.3, foo/1.2.4, foo/1.1.0, foo/1.0.9".to_string()
147+
);
148+
149+
vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
150+
assert_eq!(
151+
describe(&summaries),
152+
"foo/1.2.3, foo/1.0.9, foo/1.1.0, foo/1.2.4".to_string()
153+
);
154+
}
155+
156+
#[test]
157+
fn test_prefer_both() {
158+
let mut vp = VersionPreferences::default();
159+
vp.prefer_package_id(pkgid("foo", "1.2.3"));
160+
vp.prefer_dependency(dep("foo", "=1.1.0"));
161+
162+
let mut summaries = vec![
163+
summ("foo", "1.2.4"),
164+
summ("foo", "1.2.3"),
165+
summ("foo", "1.1.0"),
166+
summ("foo", "1.0.9"),
167+
];
168+
169+
vp.sort_summaries(&mut summaries, VersionOrdering::MaximumVersionsFirst);
170+
assert_eq!(
171+
describe(&summaries),
172+
"foo/1.2.3, foo/1.1.0, foo/1.2.4, foo/1.0.9".to_string()
173+
);
174+
175+
vp.sort_summaries(&mut summaries, VersionOrdering::MinimumVersionsFirst);
176+
assert_eq!(
177+
describe(&summaries),
178+
"foo/1.1.0, foo/1.2.3, foo/1.0.9, foo/1.2.4".to_string()
179+
);
180+
}
181+
}

0 commit comments

Comments
 (0)