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

Allow serde 0.8 and remove use of compiler plugins #216

Merged
merged 4 commits into from Aug 3, 2016
Merged

Conversation

@nox
Copy link
Member

nox commented Jul 30, 2016

This change is Reviewable

@nox nox mentioned this pull request Jul 30, 2016
23 of 23 tasks complete
@nox
Copy link
Member Author

nox commented Aug 2, 2016

The serde feature is actually used outside of Servo, and it would break at least feeds-to-pocket.

@nox
Copy link
Member Author

nox commented Aug 2, 2016

This would also break wolfram-alpha-rs and xkcd-rs.

@FraGag and @indiv0: Would you mind having to relax the serde bound in your own crates at the same time as we merge this here, so that we avoid a 2.0.0 bump in rust-url?

@nox nox force-pushed the nox:serde branch from 5693095 to 9f2f582 Aug 2, 2016
@FraGag
Copy link

FraGag commented Aug 3, 2016

Well this is annoying. If I relax the requirement on serde in feeds-to-pocket but stay on url 1.1.1, then feeds-to-pocket picks up serde 0.8.x while url stays on serde 0.7.x, which causes build errors. On the other hand, if I don't relax the requirement on serde in feeds-to-picket but update url to your branch, then it's the other way around: feeds-to-pocket stays on 0.7.x while url picks up 0.8.x, which also causes build errors. If I update both at once, then it's fine. :\ If rust-lang/cargo#2064 was fixed, then this wouldn't be an issue.

feeds-to-pocket is an executable, not a library, so the package comes with its Cargo.lock, and unless a user runs cargo update, in theory the crate should still build, because my Cargo.lock currently references url 1.1.1. Still, I'll publish an update with relaxed requirements after url 1.2.0 is published.

@nox
Copy link
Member Author

nox commented Aug 3, 2016

@FraGag Given feeds-to-pocket is an executable, there is no need for relaxed requirements there, even if you update rust-url after this is published, it won't make it use serde 0.8 as long as you don't also update serde AFAIK.

@SimonSapin
Copy link
Member

SimonSapin commented Aug 3, 2016

Even though publishing this change can break projects that don’t have a Cargo.lock file because Cargo over-optimitic in using the latest possible version of serde, as far as I understand it is possible then to downgrade with something like cargo update -p serde --precise 0.7.15.

As such, I believe this does not qualify as a breaking API change, and so 1.2.0 rather than 2.0.0 is fine. (Or even 1.1.2, but let’s not split hairs about this, Cargo treats them the same.)

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2016

📌 Commit 9f2f582 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2016

Testing commit 9f2f582 with merge 51d17bd...

bors-servo added a commit that referenced this pull request Aug 3, 2016
Allow serde 0.8 and remove use of compiler plugins

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/216)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit 9f2f582 into servo:master Aug 3, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the nox:serde branch Aug 3, 2016
@FraGag
Copy link

FraGag commented Sep 3, 2016

Just as a heads up (in particular to @nox): I just reinstalled feeds-to-pocket v0.1.1 locally with cargo install and now it fails to build. Apparently, crates uploaded to crates.io never contain Cargo.lock, even if it's not in the project's .gitignore. As a result, Cargo had to recalculate the dependencies and selected Serde 0.7 for feeds-to-pocket and Serde 0.8 for rust-url, which causes build errors because url::Url implements Serde 0.8's serde::Serialize trait, whereas feeds-to-pocket needs it to implement Serde 0.7's serde::Serialize trait.

So this did end up being a breaking change, but I agree with @SimonSapin that this kind of change should not be treated as a breaking change. Rather, Cargo could and should be improved such that situations like this do not cause breakage in the future (see e.g. rust-lang/cargo#2064, which I also mentioned above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.