Skip to content

Allow serde 0.8 and remove use of compiler plugins #216

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

Merged
merged 4 commits into from
Aug 3, 2016

Conversation

nox
Copy link
Contributor

@nox nox commented Jul 30, 2016

This change is Reviewable

@nox nox mentioned this pull request Jul 30, 2016
23 tasks
@nox
Copy link
Contributor 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
Contributor 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?

@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
Contributor 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

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

📌 Commit 9f2f582 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

⌛ Testing commit 9f2f582 with merge 51d17bd...

bors-servo pushed 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

☀️ Test successful - travis

@bors-servo bors-servo merged commit 9f2f582 into servo:master Aug 3, 2016
@nox nox deleted the serde branch August 3, 2016 17:21
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants