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

Deprecate using rustc_plugin without the rustc_driver dylib. #62727

Open
wants to merge 1 commit into
base: master
from

Conversation

@SimonSapin
Copy link
Contributor

commented Jul 16, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

r? @Zoxc

@rust-highfive rust-highfive assigned Zoxc and unassigned petrochenkov Jul 16, 2019

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

How does this trickery work?

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

At this point I think we should look to remove the plugin support from rustc.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

@Zoxc Do you mean this PR?

At first I tried adding #![rustc_deprecated] to the rustc_plugin crate so that depending on it directly causes a warning. But that caused every item to be deprecated as well. So instead this PR renames the crate to rustc_plugin_ (note the suffix, I was not inspired for another name) and creates a new crate with the old name that reexports all of its items. Only the new crate is deprecated.

Or do you mean the bug that it fixes? I’m not sure exactly. It looks like on Windows and macOS, failing to depend on the appropriate dylib crate results in there being two copies of a TLS variable or something. You found the same work-around in 7198687.


@Centril On one hand, we’ve arguably already been at that point for a couple years. On the other hand, Servo’s memory safety unfortunately still depends on a custom lint that checks that GC pointers on the stack are properly rooted: https://github.com/servo/servo/tree/cef98d2e5179/components/script_plugins

I would love to find another solution, this plugin is regularly giving us headaches. But until we do, if the plugin API is removed Servo will be unable to upgrade and will be stuck on an old Rust Nightly.

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

@Centril On one hand, we’ve arguably already been at that point for a couple years. On the other hand, Servo’s memory safety unfortunately still depends on a custom lint that checks that GC pointers on the stack are properly rooted: https://github.com/servo/servo/tree/cef98d2e5179/components/script_plugins

I would love to find another solution, this plugin is regularly giving us headaches. But until we do, if the plugin API is removed Servo will be unable to upgrade and will be stuck on an old Rust Nightly.

So there has been no movement since last time we discussed this. That's unfortunate. At some point however, I think it stops being reasonable for rustc to indefinitely maintain the plugin interface solely for the benefit of servo. What would it take to get rid of your reliance on this?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

What would it take to get rid of your reliance on this?

I’ve opened #62868 to discuss this, as I feel it doesn’t need to block this PR.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@Zoxc friendly review ping

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

☔️ The latest upstream changes (presumably #62086) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:plugins-tls-dylib branch from 9aa111e to 2025965 Jul 27, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

☔️ The latest upstream changes (presumably #63029) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnTitor

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Ping from triage: @Zoxc, waiting on your review. And @SimonSapin, you should resolve merge conflicts.

@joelpalmer

This comment has been minimized.

Copy link

commented Aug 12, 2019

Ping from triage: @Zoxc, any update on the review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.