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.

Sign up for GitHub

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

Jump to bottom

Switch to serde_derive #430

Merged
merged 1 commit into from
Oct 7, 2016
Merged

Switch to serde_derive #430

merged 1 commit into from
Oct 7, 2016

Conversation

nox
Copy link
Contributor

@nox nox commented Oct 4, 2016 •
edited by larsbergstrom
Loading

This change is Reviewable

@nox nox force-pushed the derive branch 3 times, most recently from 4ba8175 to 30b37b7 Compare October 4, 2016 17:11
@nox nox changed the title Switch to serde_derive (Do not merge) Switch to serde_derive Oct 4, 2016
@nox
Copy link
Contributor Author

nox commented Oct 4, 2016 •
edited
Loading

@alexcrichton There is an issue with default features again, somehow running cargo build --no-default-features --features serde_derive in webrender_traits enables the with-syntex feature of serde_codegen. Any idea whether this is fixable?

@nox nox mentioned this pull request Oct 4, 2016
Closed
@alexcrichton
Copy link

@nox features are unioned amongst crates, so it's likely that some other crate is enabling the with-syntex feature of serde_codegen. As to debugging which crate is enabling that, unfortunately we don't have a lot of tooling for that right now.

@nox
Copy link
Contributor Author

nox commented Oct 4, 2016

so it's likely that some other crate is enabling the with-syntex feature of serde_codegen

AFAIK no, if I comment the default and codegen features of webrender_traits, with-syntex isn't enabled anymore. Will look again though.

@nox nox mentioned this pull request Oct 5, 2016
Closed
@nox
Copy link
Contributor Author

nox commented Oct 5, 2016

@alexcrichton Did you check the Cargo.lock file? I just noticed that while syntex_syntax is present in it, I see no "Compiling syntex_syntax" message.

@nox nox changed the title (Do not merge) Switch to serde_derive Switch to serde_derive Oct 7, 2016
Manishearth
Manishearth approved these changes Oct 7, 2016
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ but I'm unsure of the semimajor version bump

webrender/Cargo.toml
@@ -1,13 +1,13 @@
[package]
name = "webrender"
version = "0.5.1"
version = "0.6.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this version bump is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the features. No one cares anyway because it's not published.

@nox
Copy link
Contributor Author

nox commented Oct 7, 2016

@bors-servo r=Manishearth

As discussed on IRC.

@bors-servo
Copy link
Contributor

📌 Commit 94437c4 has been approved by Manishearth

@bors-servo
Copy link
Contributor

⌛ Testing commit 94437c4 with merge 3597a9d...

bors-servo pushed a commit that referenced this pull request Oct 7, 2016
Auto merge of #430 - servo:derive, r=Manishearth
3597a9d 
Switch to serde_derive

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

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit 94437c4 into master Oct 7, 2016
@nox nox deleted the derive branch October 7, 2016 11:04
@paulrouget paulrouget mentioned this pull request Oct 8, 2016
Closed
@samuknet samuknet mentioned this pull request Nov 8, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth Manishearth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nox @alexcrichton @bors-servo @Manishearth