-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
fix(resolve): Improve multi-MSRV workspaces #14569
Changes from all commits
5f07992
698f26b
6b79647
3a47885
892ad41
64abeb2
94db932
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ pub struct VersionPreferences { | |
try_to_use: HashSet<PackageId>, | ||
prefer_patch_deps: HashMap<InternedString, HashSet<Dependency>>, | ||
version_ordering: VersionOrdering, | ||
max_rust_version: Option<PartialVersion>, | ||
rust_versions: Vec<PartialVersion>, | ||
} | ||
|
||
#[derive(Copy, Clone, Default, PartialEq, Eq, Hash, Debug)] | ||
|
@@ -49,8 +49,8 @@ impl VersionPreferences { | |
self.version_ordering = ordering; | ||
} | ||
|
||
pub fn max_rust_version(&mut self, ver: Option<PartialVersion>) { | ||
self.max_rust_version = ver; | ||
pub fn rust_versions(&mut self, vers: Vec<PartialVersion>) { | ||
self.rust_versions = vers; | ||
} | ||
|
||
/// Sort (and filter) the given vector of summaries in-place | ||
|
@@ -59,7 +59,7 @@ impl VersionPreferences { | |
/// | ||
/// Sort order: | ||
/// 1. Preferred packages | ||
/// 2. [`VersionPreferences::max_rust_version`] | ||
/// 2. Most compatible [`VersionPreferences::rust_versions`] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The unstable doc might need an update according, for the algorithm of determining workspace MSRV. However, from a user perspective, it is a bit hard to understand. I am not sure if we want to document it. It is also hard to determine from a human eye, if it is a large workspace and people are adding/removing members a lot that changes the workspace-wide MSRV. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the text. The unstable docs offer us an opportunity to evaluate the wording. Even once its stabilized, we can iterate as we see how people are dealing with it. |
||
/// 3. `first_version`, falling back to [`VersionPreferences::version_ordering`] when `None` | ||
/// | ||
/// Filtering: | ||
|
@@ -85,20 +85,11 @@ impl VersionPreferences { | |
return previous_cmp; | ||
} | ||
|
||
if let Some(max_rust_version) = &self.max_rust_version { | ||
let a_is_compat = a | ||
.rust_version() | ||
.map(|a| a.is_compatible_with(max_rust_version)) | ||
.unwrap_or(true); | ||
let b_is_compat = b | ||
.rust_version() | ||
.map(|b| b.is_compatible_with(max_rust_version)) | ||
.unwrap_or(true); | ||
match (a_is_compat, b_is_compat) { | ||
(true, true) => {} // fallback | ||
(false, false) => {} // fallback | ||
(true, false) => return Ordering::Less, | ||
(false, true) => return Ordering::Greater, | ||
if !self.rust_versions.is_empty() { | ||
let a_compat_count = self.msrv_compat_count(a); | ||
let b_compat_count = self.msrv_compat_count(b); | ||
if b_compat_count != a_compat_count { | ||
return b_compat_count.cmp(&a_compat_count); | ||
} | ||
} | ||
|
||
|
@@ -112,6 +103,17 @@ impl VersionPreferences { | |
let _ = summaries.split_off(1); | ||
} | ||
} | ||
|
||
fn msrv_compat_count(&self, summary: &Summary) -> usize { | ||
let Some(rust_version) = summary.rust_version() else { | ||
return self.rust_versions.len(); | ||
}; | ||
|
||
self.rust_versions | ||
.iter() | ||
.filter(|max| rust_version.is_compatible_with(max)) | ||
.count() | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
@@ -236,9 +238,9 @@ mod test { | |
} | ||
|
||
#[test] | ||
fn test_max_rust_version() { | ||
fn test_single_rust_version() { | ||
let mut vp = VersionPreferences::default(); | ||
vp.max_rust_version(Some("1.50".parse().unwrap())); | ||
vp.rust_versions(vec!["1.50".parse().unwrap()]); | ||
|
||
let mut summaries = vec![ | ||
summ("foo", "1.2.4", None), | ||
|
@@ -267,6 +269,38 @@ mod test { | |
); | ||
} | ||
|
||
#[test] | ||
fn test_multiple_rust_versions() { | ||
let mut vp = VersionPreferences::default(); | ||
vp.rust_versions(vec!["1.45".parse().unwrap(), "1.55".parse().unwrap()]); | ||
|
||
let mut summaries = vec![ | ||
summ("foo", "1.2.4", None), | ||
summ("foo", "1.2.3", Some("1.60")), | ||
summ("foo", "1.2.2", None), | ||
summ("foo", "1.2.1", Some("1.50")), | ||
summ("foo", "1.2.0", None), | ||
summ("foo", "1.1.0", Some("1.40")), | ||
summ("foo", "1.0.9", None), | ||
]; | ||
|
||
vp.version_ordering(VersionOrdering::MaximumVersionsFirst); | ||
vp.sort_summaries(&mut summaries, None); | ||
assert_eq!( | ||
describe(&summaries), | ||
"foo/1.2.4, foo/1.2.2, foo/1.2.0, foo/1.1.0, foo/1.0.9, foo/1.2.1, foo/1.2.3" | ||
.to_string() | ||
); | ||
|
||
vp.version_ordering(VersionOrdering::MinimumVersionsFirst); | ||
vp.sort_summaries(&mut summaries, None); | ||
assert_eq!( | ||
describe(&summaries), | ||
"foo/1.0.9, foo/1.1.0, foo/1.2.0, foo/1.2.2, foo/1.2.4, foo/1.2.1, foo/1.2.3" | ||
.to_string() | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_empty_summaries() { | ||
let vp = VersionPreferences::default(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation-wise looks good. I'd like to hear back from @VorpalBlade for their opinion on this new behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a step in the right direction, as I understand it, the majority wins, which would solve my issue.
(However, if you go ahead and open a pr in the middle of the night, and then merge it before morning there is little timely feedback I can give. Given timezones, work hours etc it is not reasonable to expect feedback in less than 24 hours.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was little time for feedback before merging but they noted elsewhere that your feedback didn't have to be blocking for merge as this is unstable. Its still important for stabilization!