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

Servo forks serde with [patch] #19588

Closed
DustinByfuglien opened this issue Dec 17, 2017 · 9 comments
Closed

Servo forks serde with [patch] #19588

DustinByfuglien opened this issue Dec 17, 2017 · 9 comments

Comments

@DustinByfuglien
Copy link

@DustinByfuglien DustinByfuglien commented Dec 17, 2017

I'm trying a build simple servo app with cargo build (rustc 1.24.0-nightly (3bee2b44c 2017-12-16)):

extern crate servo;

fn main() {
    println!("Servo version: {}", servo::config::servo_version());
}

Cargo.toml includes:

[dependencies]
servo = { git = "https://github.com/servo/servo" }

But building is terminating with exit code 101:

error: Package serde_derive v1.0.23 does not have these features: deserialize_from

Windows 7x64.

Help please.

@DustinByfuglien DustinByfuglien changed the title build error Build error: Package serde_derive v1.0.23 does not have these features: deserialize_from Dec 17, 2017
@fabricedesre
Copy link
Contributor

@fabricedesre fabricedesre commented Dec 17, 2017

Servo uses a serde fork, the current version in Cargo.lock is https://github.com/gankro/serde?branch=deserialize_from_enums3#fc611736

Does that match yours? Note how in https://github.com/paulrouget/servo-embedding-example they recommend to copy Servo's Cargo.lock in your project to avoid these kind of issues.

@DustinByfuglien
Copy link
Author

@DustinByfuglien DustinByfuglien commented Dec 17, 2017

Thank you for the link.
Now I understand wat was wrong.

@SimonSapin SimonSapin changed the title Build error: Package serde_derive v1.0.23 does not have these features: deserialize_from Servo forks serde with [replace] Dec 17, 2017
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Dec 17, 2017

Servo has this section in its root Cargo.toml, which you probably need to copy in yours:

[replace]
"serde:1.0.23" = { git = "https://github.com/gankro/serde", branch = "deserialize_from_enums3" }
"serde_derive:1.0.23" = { git = "https://github.com/gankro/serde", branch = "deserialize_from_enums3", feature="deserialize_from" }

This also seems to make using [patch] impossible. @gankro, is there a plan to upstream these serde features, or somehow stop using [replace]?

https://github.com/rust-lang/cargo/blob/0.23.0/src/cargo/util/toml/mod.rs#L743-L745

        if self.patch.is_some() && self.replace.is_some() {
            bail!("cannot specify both [replace] and [patch]");
        }
@SimonSapin SimonSapin reopened this Dec 17, 2017
@SimonSapin SimonSapin changed the title Servo forks serde with [replace] Servo forks serde with [patch] Dec 18, 2017
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Dec 18, 2017

#19590 replaces [replace] with [patch] which fixes some problems, but this is still not ideal. When [patch]’ing another library which depends on serde = "1.0", cargo insisted on upgrading that dependency to 1.0.24 from crates.io instead of 1.0.23 from the forked git repo, even after explicitly downgrading it: cargo update -p serde:1.0.24 --precise 1.0.23 would succeed, but then the next build would "upgrade" again.

This may be a Cargo bug, but [patch] / [replace] is still something that should not be in master IMO.

@dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Dec 18, 2017

I will be publishing a Serde version with most of the changes from the fork in the next few days. As I understand it, at that point Servo will be able to drop the [patch] of serde but wants to keep using a fork of serde_derive -- so we need to upstream the rest of the changes somehow or figure out a long-term solution for the fork.

@Gankra
Copy link
Contributor

@Gankra Gankra commented Dec 18, 2017

With the serde work upstreamed servo could remove both patches, as the serde_derive fork “only” introduces a transparent optimization. Performance of gecko is more critical (in the short term), so we would continue to apply the derive patch there.

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jan 5, 2018

More breakage cause by this: #19698. Though I think part of the reason is that the fork’s version number is behind crates.io. Doing either of:

  • Keep the fork up to date whenever upstream publishes a new version
  • Or change the fork’s version number from 1.0.23 to something like 1.0.1023

might help. What do think @gankro ?

@Gankra
Copy link
Contributor

@Gankra Gankra commented Jan 5, 2018

You can remove the fork in the next webrender update, it’s been upstreamed. Webrender still forks serde_derive but it’s for a transparent optimization, failing to include it just means you’re a bit slower.

@jdm
Copy link
Member

@jdm jdm commented Jan 5, 2018

The fork was indeed removed in #19614.

@jdm jdm closed this Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.