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

Remove public dependency on Syntex #358

Closed
dtolnay opened this issue Jun 8, 2016 · 3 comments
Closed

Remove public dependency on Syntex #358

dtolnay opened this issue Jun 8, 2016 · 3 comments
Assignees
Milestone

Comments

@dtolnay
Copy link
Member

dtolnay commented Jun 8, 2016

Currently Syntex is a public dependency because the argument to serde_codegen::register is &mut syntex::Registry:

// build.rs
pub fn main() {
    let out_dir = env::var_os("OUT_DIR").unwrap();
    let src = Path::new("src/main.rs.in");
    let dst = Path::new(&out_dir).join("main.rs");

    let mut registry = syntex::Registry::new();
    serde_codegen::register(&mut registry);
    registry.expand("", &src, &dst).unwrap();
}

Let's see if we can do something more similar to this instead:

// build.rs
pub fn main() {
    let out_dir = env::var_os("OUT_DIR").unwrap();
    let src = Path::new("src/main.rs.in");
    let dst = Path::new(&out_dir).join("main.rs");

    // no more public Syntex dependency
    serde_codegen::expand(&src, &dst).unwrap();
}

The downside would be if someone wants to apply more than one compiler plugin, their code is parsed multiple times:

serde_codegen::expand(&src, &tmp1).unwrap();
other_plugin::expand(&tmp1, &tmp2).unwrap();
third_plugin::expand(&tmp2, &dst).unwrap();
@jimmycuadra
Copy link
Contributor

This seems like a worthwhile compromise to me. I'm betting that the number of projects that use more than one compiler plugin on stable is pretty small. (My main project actually does, but it's nightly-only, so this wouldn't affect me.)

homu added a commit that referenced this issue Jun 9, 2016
Add serde_codegen::expand to avoid public Syntex dependency

Required for #358. We can remove `serde_codegen::register` in the next breaking release.

This allows Syntex users to avoid being broken by Serde bumping its Syntex dependency.
homu added a commit that referenced this issue Jun 9, 2016
Add serde_codegen::expand to avoid public Syntex dependency

Required for #358. We can remove `serde_codegen::register` in the next breaking release.

This allows Syntex users to avoid being broken by Serde bumping its Syntex dependency.
@dtolnay
Copy link
Member Author

dtolnay commented Jun 10, 2016

The new serde_codegen::expand is in, but I am leaving this open until we either remove serde_codegen::register in a breaking release or decide to keep it.

@dtolnay dtolnay added this to the v0.8.0 milestone Jun 10, 2016
@erickt
Copy link
Member

erickt commented Jun 10, 2016

As I mentioned in in the other thread, I'm all for getting rid of serde_codegen::register.

@dtolnay dtolnay removed their assignment Jun 12, 2016
@dtolnay dtolnay self-assigned this Jun 13, 2016
plietar added a commit to plietar/librespot that referenced this issue Jul 6, 2016
This removes the direct dependency on syntex, meaning plugins can bump
their own versions independently (see serde-rs/serde#358)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants