Skip to content
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

feat(resolver): Add CLI option to resolve minimal version dependencies #5200

Merged
merged 7 commits into from Mar 23, 2018

Conversation

Projects
None yet
4 participants
@klausi
Copy link
Contributor

commented Mar 18, 2018

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.

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

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 in_previous first; then only the .summary.version() should be subject to this switch. @alexcrichton what do you think?

Also where is @rust-highfive to set:
r? @alexcrichton

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

I think we should always have in_previous first; then only the .summary.version() should be subject to this switch. @alexcrichton what do you think?

Agreed!

@klausi klausi force-pushed the klausi:minimal_versions branch from 776b572 to f2610df Mar 20, 2018

@klausi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Added a test case, this should now be ready for review.

I tried to follow your suggestion to only apply the switch when in_previous has been checked:

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 in_previous check? Or maybe we always need to reverse() as it is in the PR changes right now?

@klausi klausi changed the title [WIP] feat(resolver): Add CLI option to resolve minimal version dependencies feat(resolver): Add CLI option to resolve minimal version dependencies Mar 20, 2018

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

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.

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

Got it the old code was a.cmp(&b).reverse()! So that means the new code needs a .reverse() at the end asswell. (or any number of equivalent ways of writing it, if it makes things clearer. For example the .then_with( method may be easier to read, and match on a bool is somtimes cleaner as an if.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

@bors: r+

This looks great, thanks @klausi!

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

📌 Commit 5907a8b has been approved by alexcrichton

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

@alexcrichton This does not yet handle the lock file stufe correctly.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

@bors: r-

oh? How so? (I think I missed that...)

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

#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.

@Eh2406
Copy link
Contributor

left a comment

Overall, almost there.

@@ -45,6 +45,45 @@ fn resolve(
Ok(res)
}

// Clone of resolve() from above that also passes down a Config instance.

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Mar 21, 2018

Contributor

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).

@@ -320,6 +359,48 @@ fn test_resolving_maximum_version_with_transitive_deps() {
assert_that(&res, is_not(contains(names(&[("util", "1.1.1")]))));
}

#[test]
fn test_resolving_minimum_version_with_transitive_deps() {

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Mar 21, 2018

Contributor

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?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 23, 2018

Member

Oh sorry I missed this. Yeah @klausi mind adding a small integration test to ensure that the flag is plumbed to the right location?

This comment has been minimized.

Copy link
@klausi

klausi Mar 23, 2018

Author Contributor

done!

match self.minimal_versions {
true => a.cmp(&b),
false => a.cmp(&b).reverse(),
}

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Mar 21, 2018

Contributor

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.

@klausi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

Thanks a lot @Eh2406 , again :-)

Very good feedback, I think I covered your points now.

@klausi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

And now with integration test. tests/testsuite/generate_lockfile.rs might not be the best place to put it, any better suggestions?

@klausi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

Moved the integration test right after the resolver test, I guess it makes sense to just have them together there.

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

This looks good to me, r+! Thank you @klausi.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

@bors: delegate=Eh2406 r=Eh2406

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

📌 Commit a38184f has been approved by Eh2406

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

✌️ @Eh2406 can now approve this pull request

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

⌛️ Testing commit a38184f with merge bcd0300...

bors added a commit that referenced this pull request Mar 23, 2018

Auto merge of #5200 - klausi:minimal_versions, r=Eh2406
feat(resolver): Add CLI option to resolve minimal version dependencies

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.
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Eh2406
Pushing bcd0300 to master...

@bors bors merged commit a38184f into rust-lang:master Mar 23, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.