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 non-sysroot sources from rust-src component #69631

Merged
merged 1 commit into from
Mar 8, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 2, 2020

See #69592 (comment): these were likely added in #58269 for the sake of compiler plugins, but those are being entirely phased out, so there is no good reason to ship these sources.

OTOH, @eddyb wrote

Yeah, my question is why librustc_plugin specifically? Everything else makes sense.

So maybe there is some good reason to keep these? Then we should have a comment explaining that reason.

Cc @eddyb @taeguk @Mark-Simulacrum

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2020
@eddyb
Copy link
Member

eddyb commented Mar 2, 2020

Keep in mind that since then, libsyntax and librustc were split into a handful of crates, and something happened to librustc_plugin so it's possible this has been broken for plugin authors for a while.

When I wrote "Everything else makes sense." I probably imagined we could have most of src/ in rust-src but now that we have rustc-dev we should probably have a similar rustc-src component if we want to provide all of those libraries' source.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2020

Yeah, having just the sources of these two crates without all their dependencies seems pretty useless. Hence this PR.

@Mark-Simulacrum
Copy link
Member

I am in favor of removing these, particularly as I believe it's highly likely that they're not doing too much good since we've stripped so much out of them these days into other crates.

I think the best way to do so would be to land this change now-ish and then if people complain evaluate readding them (perhaps into the rustc-dev component?). I doubt that people are aware that they are using them if they are.

cc @matklad as well, in case IntelliJ or other IDE tooling wants these for some reason.

@matklad
Copy link
Member

matklad commented Mar 5, 2020

No, neither IntelliJ nor rust-analyzer needs librustc

@Mark-Simulacrum
Copy link
Member

I'm going to approve this in that case. If someone feels we should get wider consensus, though, please feel free to unapprove.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 5, 2020

📌 Commit cbf52b1 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Mar 6, 2020

@bors rollup

Centril added a commit to Centril/rust that referenced this pull request Mar 8, 2020
remove non-sysroot sources from rust-src component

See rust-lang#69592 (comment): these were likely added in rust-lang#58269 for the sake of compiler plugins, but those are being entirely phased out, so there is no good reason to ship these sources.

OTOH, @eddyb [wrote](rust-lang#58269 (comment))

> Yeah, my question is why librustc_plugin specifically? Everything else makes sense.

So maybe there is some good reason to keep these? Then we should have a comment explaining that reason.

Cc @eddyb @taeguk @Mark-Simulacrum
bors added a commit that referenced this pull request Mar 8, 2020
Rollup of 8 pull requests

Successful merges:

 - #69631 (remove non-sysroot sources from rust-src component)
 - #69646 (Miri visitor: detect primitive types based on type, not layout (also, more tests))
 - #69651 (Try to ensure usize marker does not get merged)
 - #69668 (More documentation and simplification of BTreeMap's internals)
 - #69685 (unix: Don't override existing SIGSEGV/BUS handlers)
 - #69771 (Cleanup E0390 explanation)
 - #69777 (Add missing ` in doc for File::with_options())
 - #69812 (Refactorings to method/probe.rs and CrateId)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Mar 8, 2020
Rollup of 7 pull requests

Successful merges:

 - #69631 (remove non-sysroot sources from rust-src component)
 - #69646 (Miri visitor: detect primitive types based on type, not layout (also, more tests))
 - #69651 (Try to ensure usize marker does not get merged)
 - #69668 (More documentation and simplification of BTreeMap's internals)
 - #69771 (Cleanup E0390 explanation)
 - #69777 (Add missing ` in doc for File::with_options())
 - #69812 (Refactorings to method/probe.rs and CrateId)

Failed merges:

r? @ghost
@bors bors merged commit b3c405c into rust-lang:master Mar 8, 2020
@RalfJung RalfJung deleted the rust-src branch March 8, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants