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

Add proc-macro feature to control the runtime dependency on rustc toolchain libs #62

Merged
merged 1 commit into from
Mar 31, 2018

Conversation

staktrace
Copy link
Contributor

@dtolnay Would you consider merging something like this?

Root problem is rust-lang/rust#47931 - my patch at rust-lang/rust#48217 would have solved it but is stalled. So as a workaround to get unblocked we extracted libproc_macro and published it as a standalone rustc-ap-proc_macro crate on crates.io, similar to how other libraries inside rustc are published as standalone crates (although I did it manually instead of scripting it, see alexcrichton/rustc-auto-publish#1). We then also forked proc-macro2, quote, and syn to standalone variants (now on crates.io as standalone-proc-macro2, standalone-quote, and standalone-syn) and updated cbindgen to use those. This solved our immediate problem, but is a pretty bad solution overall.

To make it a little better, it should be possible to just have a feature flag on proc-macro2, quote, and syn that does the same job, instead of having to fork the entire crate. This PR shows what the feature would look like for quote - if this looks acceptable to you I can do one for syn as well. Thoughts/comments?

Note that I haven't verified this actually does what I want it to (in terms of making cbindgen use this feature) but if you have no objections to the general idea then I can do that prior to merging this.

@alexcrichton
Copy link
Collaborator

FWIW my preferred route to solve this is here - dtolnay/proc-macro2#65 - notably that syn/quote would reexport the ability to turn off the linking to proc_macro

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I cannot accept a standalone feature because Cargo features are required to be additive -- as implemented, this would break the ability to use both standalone and non-standalone quote in the same dependency graph. For example code like the following would break in confusing ways after adding a transitive dependency on some crate that uses standalone quote.

let tokens: proc_macro2::TokenStream = /* ... */;
syn::parse2(tokens)

I published Alex's PR as proc-macro2 0.2.3. Please update this PR to use that approach instead.

@staktrace staktrace force-pushed the standalone branch 2 times, most recently from 78c9adb to 19d3160 Compare February 22, 2018 21:56
@staktrace
Copy link
Contributor Author

@dtolnay makes sense, thanks for the explanation! I've updated the PR to use proc-macro2 0.2.3 and add a default-enabled proc-macro feature. Let me know if it looks good. I have a similar one ready for syn but I need a version number of quote published on crates.io before I can finalize that patch. I can still put up the WIP PR if you want.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! This approach seems reasonable to me.

One obstacle with doing the equivalent thing in Syn is that lots of libraries depend on syn = { version = "0.12", default-features = false, features = ["derive", "parsing"] } and expect to be able to call syn::parse which takes a proc_macro type as input. That code would break if syn::parse were to require an additional feature.

If possible I would prefer to introduce these "proc-macro" flags only in the next breaking release. What do you think?

@staktrace
Copy link
Contributor Author

staktrace commented Feb 23, 2018

Yup that sounds reasonable. Can we just make the introduction of proc-macro come with a breaking version bump? Or did you want to wait until there's enough "breaking changes" accumulated before bumping the version?

Edit: fix typo

@dtolnay
Copy link
Owner

dtolnay commented Feb 23, 2018

Let's batch it up with the next round of breaking changes. These updates tend to be a large amount of work for a project like Servo where they need to coordinate an update across dozens of repos. We try to make it worth their time when it happens. The best time may be when the next round of libproc_macro breaking changes rolls out -- tracked in rust-lang/rust#47786.

Could you make a Syn PR as well so we have that ready to go?

@dtolnay dtolnay changed the title (WIP) Add a standalone feature that breaks the runtime dependency on rustc toolchain libs Add proc-macro feature to control the runtime dependency on rustc toolchain libs Feb 23, 2018
@staktrace
Copy link
Contributor Author

Syn PR is at dtolnay/syn#363

@dtolnay
Copy link
Owner

dtolnay commented Mar 4, 2018

Thanks! I continue to be on board with this change but we are not prepared to do a round of breaking changes right now. I filed #63 to make sure we reopen and merge this PR when ready. Until then I would recommend using a fork of quote published under a different name.

This feature is enabled by default for backwards compatibility, but
it can be disabled in order to break the runtime dependency on
the dynamic library libproc_macro in the rustc toolchain.
@dtolnay dtolnay merged commit 9efc2e5 into dtolnay:master Mar 31, 2018
@staktrace staktrace deleted the standalone branch May 31, 2018 13:45
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.

3 participants