Skip to content

/ servo

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

Tidy can now check for version conflicts... #7438

Merged
merged 1 commit into from Sep 1, 2015
Merged

Tidy can now check for version conflicts... #7438

merged 1 commit into from Sep 1, 2015

Conversation

@wafflespeanut
Copy link
Member

wafflespeanut commented Aug 28, 2015

fixes #7133

Review on Reviewable
@wafflespeanut
Copy link
Member Author

wafflespeanut commented Aug 28, 2015

NOTE: This doesn't work yet!

While this sort of fixes what the issue suggests, tidy's current suggestions are quite unhelpful.

screenshot

For now, glutin demands "gl_generator 0.0.26" and hence, that command shouldn't have any effect. So, we gotta wait until glutin updates its Cargo.toml and once all the versions match, we shall include the check_lock function here.
@mbrubeck
Copy link
Contributor

mbrubeck commented Aug 28, 2015

Maybe it should just print which packages depend on the older version, if it has that information? Frequently the solution is to upgrade that intermediate package, or submit a patch to it.
@wafflespeanut wafflespeanut force-pushed the wafflespeanut:cargo-tidy branch from cac20df to ed6bfa1 Aug 29, 2015
@wafflespeanut
Copy link
Member Author

wafflespeanut commented Aug 29, 2015

Update based on @mbrubeck's suggestion (/cc @larsbergstrom)...

screenshot

Again, this doesn't work yet! We have to include check_lock in the checking_functions when needed.
@wafflespeanut wafflespeanut changed the title Tidy could now check for version conflicts... Tidy can now check for version conflicts... Aug 29, 2015
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 29, 2015

@wafflespeanut This looks fantastic! I'm really excited by this. Is the "expected maximum version" the largest number seen so far? So, if you had three versions of gl_generator at 0.0.24, 0.0.26, and 0.0.27, there would be two errors for the 0.0.24 and 0.0.26?

In any case, I'm really happy with how this is coming out for preventing multiple conflicting packages from being brought in. cc @alexcrichton to show him the awesome.

Reviewed 2 of 2 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io
@wafflespeanut
Copy link
Member Author

wafflespeanut commented Aug 29, 2015

Thanks. And yeah, there would be two errors on 0.0.24 and 0.0.26 :)
@metajack
Copy link
Contributor

metajack commented Sep 1, 2015

So what's needed to land this?

Also, so happy to see this!
@jdm
Copy link
Member

jdm commented Sep 1, 2015

I'm hesitant to merge this code without it actually being called anywhere. Maybe we could just exceptions for the current cases that are broken and forbid any new ones from being added?
@wafflespeanut wafflespeanut force-pushed the wafflespeanut:cargo-tidy branch from ed6bfa1 to 05a8994 Sep 1, 2015
@wafflespeanut
Copy link
Member Author

wafflespeanut commented Sep 1, 2015

Done!
@wafflespeanut
Tidy could now check for version conflicts!
Loading status checks…
da1c858
@wafflespeanut wafflespeanut force-pushed the wafflespeanut:cargo-tidy branch from 05a8994 to da1c858 Sep 1, 2015
@jdm
Copy link
Member

jdm commented Sep 1, 2015

@bors-servo: r+
Thanks!
@wafflespeanut
Copy link
Member Author

wafflespeanut commented Sep 1, 2015

Thank you! :)
@bors-servo
Copy link
Contributor

bors-servo commented Sep 1, 2015

📌 Commit da1c858 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Sep 1, 2015

⌛ Testing commit da1c858 with merge 8b5418f...
bors-servo pushed a commit that referenced this pull request Sep 1, 2015
bors-servo
Auto merge of #7438 - Wafflespeanut:cargo-tidy, r=jdm
Loading status checks…
8b5418f 
Tidy can now check for version conflicts...

fixes #7133

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7438)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 1, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit da1c858 into servo:master Sep 1, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@wafflespeanut wafflespeanut deleted the wafflespeanut:cargo-tidy branch Sep 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@larsbergstrom larsbergstrom

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

`make tidy` should ensure that we do not have two versions of the same package in Cargo.lock
7 participants
@wafflespeanut @mbrubeck @larsbergstrom @metajack @jdm @bors-servo @highfive
You can’t perform that action at this time.