feat(resolver): Add CLI option to resolve minimal version dependencies#5200
feat(resolver): Add CLI option to resolve minimal version dependencies#5200bors merged 7 commits intorust-lang:masterfrom
Conversation
|
Thanks for working on this! Let me know when this is ready for review, or if there is anything else I can do to help? One complication is that @alexcrichton has recently #5180 add code that uses the sort to preserve the lock file. I think we should always have Also where is @rust-highfive to set: |
Agreed! |
776b572 to
f2610df
Compare
|
Added a test case, this should now be ready for review. I tried to follow your suggestion to only apply the switch when ret.sort_unstable_by(|a, b| {
let a_in_previous = self.try_to_use.contains(a.summary.package_id());
let b_in_previous = self.try_to_use.contains(b.summary.package_id());
let previous_cmp = a_in_previous.cmp(&b_in_previous);
match previous_cmp {
Ordering::Equal => {
let cmp = a.summary.version().cmp(&b.summary.version());
match self.minimal_versions {
true => cmp.reverse(),
false => cmp,
}
},
_ => previous_cmp,
}
});But that does not work and then my test case fails. Maybe I need to specify a more elaborate test case that would then drive around this |
|
Thanks for working on this. That looks like it should work. I wonder why the test fails with that code. I can experiment with it when I get home. |
|
Got it the old code was |
|
📌 Commit 5907a8b has been approved by |
|
@alexcrichton This does not yet handle the lock file stufe correctly. |
|
@bors: r- oh? How so? (I think I missed that...) |
|
#5200 (comment) suggested how to handle it correctly, but it was failing the test. So it was Not committed. #5200 (comment) figured out how to get it working, but that has not yet been incorporated. After that is fixed there are some nits, working on a revue now, then we will be good. |
tests/testsuite/resolve.rs
Outdated
| Ok(res) | ||
| } | ||
|
|
||
| // Clone of resolve() from above that also passes down a Config instance. |
There was a problem hiding this comment.
I am not fond of the copy/paste code duplication, but if @klausi thinks that it is the best solution I am ok with it.
As an alternative thought if this takes an Option<&Config> then resolve can just call resolve_with_config(...,None).
| } | ||
|
|
||
| #[test] | ||
| fn test_resolving_minimum_version_with_transitive_deps() { |
There was a problem hiding this comment.
I really like this test, it concisely demonstrates both properties of minimal-versions, that it chooses the smallest version while meeting all the requirements. Mabey a comment at the beginning explaining that for the next reader. @alexcrichton has been trying to teach me to leave more comments.
Also @alexcrichton, do you think this needs a full integration test that calls cargo -Z minimal-versions or is this sufficient?
There was a problem hiding this comment.
Oh sorry I missed this. Yeah @klausi mind adding a small integration test to ensure that the flag is plumbed to the right location?
| match self.minimal_versions { | ||
| true => a.cmp(&b), | ||
| false => a.cmp(&b).reverse(), | ||
| } |
There was a problem hiding this comment.
This needs your code from #5200 (comment). Also match on a bool is somtimes better as an if, and the .then_with( method may be helpfull. I leave it to your judgment.
|
Thanks a lot @Eh2406 , again :-) Very good feedback, I think I covered your points now. |
|
And now with integration test. tests/testsuite/generate_lockfile.rs might not be the best place to put it, any better suggestions? |
|
Moved the integration test right after the resolver test, I guess it makes sense to just have them together there. |
|
This looks good to me, r+! Thank you @klausi. |
|
@bors: delegate=Eh2406 r=Eh2406 |
|
📌 Commit a38184f has been approved by |
|
✌️ @Eh2406 can now approve this pull request |
|
☀️ Test successful - status-appveyor, status-travis |
Fixes #4100
Test cases are still missing. We need to come up with a plan what cases we want to cover.
Thanks a lot to @Eh2406 for very helpful instructions to kick this off.