Skip to content

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
Contributor

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
Contributor

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
Collaborator

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