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

Shrink the rust-src component #41546

Closed
wants to merge 3 commits into from
Closed

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 25, 2017

Before this change, the installable rust-src component had essentially the same contents as the rustc-src dist tarball, just additionally wrapped in a rust-installer. As discussed on internals, rust-src is only meant to support uses for the standard library, so it doesn't really need the rest of the compiler sources.

Now rust-src only contains libstd and its path dependencies, which roughly matches the set of crates that have rust-analysis data. The result is significantly smaller, from 36MB to 1.3MB compressed, and from 247MB to 8.5MB uncompressed.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@malbarbo
Copy link
Contributor

malbarbo commented Apr 26, 2017

@cuviper What do you think about this rust-std component issue?

@cuviper
Copy link
Member Author

cuviper commented Apr 26, 2017

@malbarbo To paraphrase, you're asking about the fact that rust-std may or may not include rustc crates? I'm not actually referring to rust-std for the chosen crates here, but only the proper std crate and its dependencies, including those that are arch-specific like the sanitizers.

If we want to include rustc crates in rust-src, we could, but I don't think that's necessary. All of those crates are unstable, and folks who really want that can still checkout rust.git for use by racer et al.

@alexcrichton
Copy link
Member

Thanks for the PR @cuviper! This seems like the kind of list that's highly susceptible to breakage over time, I wonder if we could enhance distcheck to verify that the rust-src component is valid? Maybe something like executing cargo metadata or something like that in the un-tar'd version to verify that all dependencies are there at least?

Also cc @jonathandturner and @nrc, I believe the rls uses the rust-src component

@petrochenkov
Copy link
Contributor

I need to check if this breaks intellisense for people developing rustc itself using editors with plugins depending on rust-src component (e.g. VSCode).

@petrochenkov
Copy link
Contributor

I need to check if this breaks intellisense ...

Ok, everything still seems to work normally.

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2017
@cuviper
Copy link
Member Author

cuviper commented Apr 26, 2017

@alexcrichton I agree that the list is a bit fragile. Using cargo metadata as a sanity check sounds like a good idea! Maybe I can even use that to generate the list in the first place, picking out the path+file:///... locations. I'll try it out.

I think rls primarily uses this through racer, but I'm not certain. I don't usually use rls or vscode, but it seems like it works, and I did make sure racer directly works too.

@cuviper
Copy link
Member Author

cuviper commented Apr 26, 2017

Maybe I can even use that to generate the list in the first place, picking out the path+file:///... locations.

Well at first glance, it's not that easy, because cargo metadata in the full source gives me data on the entire workspace, including all the rustc bits. I guess that would have to be more clever and actually walk the dependencies in the metadata starting from std.

Just having the distcheck as your suggested is probably an 80% solution, and it's not like this list changes frequently.

@alexcrichton
Copy link
Member

So far this list has changed at least once a cycle for I think the past 3 or 4 cycles? Which means it'd otherwise have broken once per release so far for the past few months :(

@cuviper
Copy link
Member Author

cuviper commented Apr 26, 2017

OK, there's a distcheck. I double-checked that if the source list has something bogus then rust-src doesn't build successfully, and if it's missing something (say libcore) then the distcheck catches that.

@alexcrichton
Copy link
Member

@bors: r+

Awesome, thanks @cuviper!

@bors
Copy link
Contributor

bors commented Apr 26, 2017

📌 Commit b9bdb17 has been approved by alexcrichton

arielb1 pushed a commit to arielb1/rust that referenced this pull request Apr 26, 2017
…chton

Shrink the rust-src component

Before this change, the installable rust-src component had essentially the same contents as the rustc-src dist tarball, just additionally wrapped in a rust-installer.  As discussed on [internals], rust-src is only meant to support uses for the standard library, so it doesn't really need the rest of the compiler sources.

Now rust-src only contains libstd and its path dependencies, which roughly matches the set of crates that have rust-analysis data.  The result is **significantly** smaller, from 36MB to 1.3MB compressed, and from 247MB to 8.5MB uncompressed.

[internals]: https://internals.rust-lang.org/t/minimizing-the-rust-src-component/5117
arielb1 pushed a commit to arielb1/rust that referenced this pull request Apr 26, 2017
…chton

Shrink the rust-src component

Before this change, the installable rust-src component had essentially the same contents as the rustc-src dist tarball, just additionally wrapped in a rust-installer.  As discussed on [internals], rust-src is only meant to support uses for the standard library, so it doesn't really need the rest of the compiler sources.

Now rust-src only contains libstd and its path dependencies, which roughly matches the set of crates that have rust-analysis data.  The result is **significantly** smaller, from 36MB to 1.3MB compressed, and from 247MB to 8.5MB uncompressed.

[internals]: https://internals.rust-lang.org/t/minimizing-the-rust-src-component/5117
bors added a commit that referenced this pull request Apr 27, 2017
Rollup of 9 pull requests

- Successful merges: #41370, #41456, #41493, #41499, #41501, #41524, #41546, #41550, #41552
- Failed merges:
@cuviper
Copy link
Member Author

cuviper commented Apr 27, 2017

Hmm, it looks like this was partially merged in rollup #41567, but without the distcheck commit. Then rollup #41585 says it's including this too, but doesn't have any of my commits. Is @bors confused?

@alexcrichton
Copy link
Member

@frewsxcv did you happen to manually back this out of #41585?

@cuviper thankfully the way we do rollups means that a PR is only closed if it merges all commits! In that sense I'm hoping that if @bors naturally gets around to this PR it'll merge and end up picking up the last commit.

@frewsxcv
Copy link
Member

I did not do any manual adjustments to the rollup. Everything that's committed in there is through the bors queue interface. I feel like we've had this happen before; it's kind of a scary bug.

bors added a commit that referenced this pull request Apr 28, 2017
Rollup of 7 pull requests

- Successful merges: #41438, #41523, #41526, #41546, #41556, #41572, #41578
- Failed merges:
This was referenced Apr 28, 2017
bors added a commit that referenced this pull request Apr 28, 2017
Rollup of 4 pull requests

- Successful merges: #41534, #41546, #41571, #41583
- Failed merges:
@cuviper
Copy link
Member Author

cuviper commented Apr 28, 2017

📌 Commit b9bdb17 has been approved by alexcrichton

I just noticed, that's not the tail commit -- it should be c5cd4cb. @alexcrichton maybe r+ it explicitly?

@alexcrichton
Copy link
Member

@bors: r+ c5cd4cb

@bors
Copy link
Contributor

bors commented Apr 28, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Apr 28, 2017

🙀 c5cd4cb51e1a77cd4f10ce2d04fc29621354afc9 is not a valid commit SHA. Please try again with b9bdb17.

@alexcrichton
Copy link
Member

Hm sure enough homu is now wedged with this, closing to test out.

@alexcrichton
Copy link
Member

Hm @cuviper mind sending up a follow-up PR? Sorry for the automation hiccup :(

@cuviper
Copy link
Member Author

cuviper commented Apr 28, 2017

No problem, I was already getting that ready. :) #41608

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 29, 2017
…richton

Add a distcheck for rust-src completeness

This is for the last commit of rust-lang#41546.  For some reason, @bors only saw the first two commits, and wouldn't approve the last even when explicitly directed so.

r? @alexcrichton
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 30, 2017
…richton

Add a distcheck for rust-src completeness

This is for the last commit of rust-lang#41546.  For some reason, @bors only saw the first two commits, and wouldn't approve the last even when explicitly directed so.

r? @alexcrichton
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 30, 2017
…richton

Add a distcheck for rust-src completeness

This is for the last commit of rust-lang#41546.  For some reason, @bors only saw the first two commits, and wouldn't approve the last even when explicitly directed so.

r? @alexcrichton
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 26, 2017
rust-src: include everything needed to compile libstd with jemalloc

I am not very happy about all this `Path::new`, but did not find a nice way to avoid it. Also, this shouldn't be very performance-critical.

With this patch, rust-src-1.19.0-dev.tar.gz grows from 1.4 to 3.1 MiB (new uncompressed size: 15.5 MiB). Not great, but shipping incomplete sources is also not great, and this is still much smaller than pre-rust-lang#41546. Excluding the entire `src/jemalloc/test` does not work, unfortunately; there is a file in there that is needed to build libstd. (And anyway there's just 190 KiB uncompressed left in that folder.)

In principle, we could try excluding the Rust test suite directories (that would be `libcore/tests` and `libcollection/tests`). I don't know enough about how this component is used to judge whether that would cause any problems. Anyway this is just 600 KiB uncompressed.

Fixes rust-lang#41952
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 27, 2017
rust-src: include everything needed to compile libstd with jemalloc

I am not very happy about all this `Path::new`, but did not find a nice way to avoid it. Also, this shouldn't be very performance-critical.

With this patch, rust-src-1.19.0-dev.tar.gz grows from 1.4 to 3.1 MiB (new uncompressed size: 15.5 MiB). Not great, but shipping incomplete sources is also not great, and this is still much smaller than pre-rust-lang#41546. Excluding the entire `src/jemalloc/test` does not work, unfortunately; there is a file in there that is needed to build libstd. (And anyway there's just 190 KiB uncompressed left in that folder.)

In principle, we could try excluding the Rust test suite directories (that would be `libcore/tests` and `libcollection/tests`). I don't know enough about how this component is used to judge whether that would cause any problems. Anyway this is just 600 KiB uncompressed.

Fixes rust-lang#41952
bors added a commit that referenced this pull request May 29, 2017
rust-src: include everything needed to compile libstd with jemalloc

I am not very happy about all this `Path::new`, but did not find a nice way to avoid it. Also, this shouldn't be very performance-critical.

With this patch, rust-src-1.19.0-dev.tar.gz grows from 1.4 to 3.1 MiB (new uncompressed size: 15.5 MiB). Not great, but shipping incomplete sources is also not great, and this is still much smaller than pre-#41546. Excluding the entire `src/jemalloc/test` does not work, unfortunately; there is a file in there that is needed to build libstd. (And anyway there's just 190 KiB uncompressed left in that folder.)

In principle, we could try excluding the Rust test suite directories (that would be `libcore/tests` and `libcollection/tests`). I don't know enough about how this component is used to judge whether that would cause any problems. Anyway this is just 600 KiB uncompressed.

Fixes #41952
@cuviper cuviper deleted the reduced-rust-src branch September 26, 2017 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants