ship LLVM tools with the toolchain #50336
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
Could you add Otherwise I can do it in a followup myself I guess. Thanks for working on this! |
|
@emilio added llvm-profdata |
5 tasks
|
@bors: try I'm curious to poke around these from the bots as well |
bors
added a commit
that referenced
this pull request
Apr 30, 2018
ship LLVM tools with the toolchain
this PR adds llvm-{nm,objcopy,objdump,size} to the rustc sysroot (right next to LLD)
this slightly increases the size of the rustc component. I measured these numbers on x86_64 Linux:
- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.gz 180M -> 193M (+7%)
- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.xz 129M -> 137M (+6%)
r? @alexcrichton
cc #49584
src/bootstrap/lib.rs
Outdated
| @@ -199,6 +199,10 @@ use flags::Subcommand; | |||
| use cache::{Interned, INTERNER}; | |||
| use toolstate::ToolState; | |||
|
|
|||
| const LLVM_TOOLS: &[&str] = &[ | |||
| "llvm-nm", "llvm-objcopy", "llvm-objdump", "llvm-profdata", "llvm-size", | |||
There was a problem hiding this comment.
Could you add some comments here for what each tool is and why it's included?
|
The code changes here look good to me, thanks @japaric! I'd like to poke around the output and then I think we can FCP for inclusion with... some team (?) |
|
I agree with concerns raised on the issue about including this by-default in the rustc component; primarily because I worry we're effectively stabilizing their (default) presence in the sysroot. I think that a temporary resolution that I would be okay with is making them nightly-only components of the sysroot (that is, next beta will not have them) until we figure out how to make them into a component. It's also not completely clear to me why they cannot be in a component -- the concerns raised on the issue spoke of rustup not understanding host-specific components, but this seems like a non-issue: we can provide this component for all targets, it's just on some it would be empty (sure, confusing, but unlikely to matter in practice I think). |
|
☀️ Test successful - status-travis |
|
It looks like the compiled tools have a dynamic dependency on |
|
@Mark-Simulacrum a downside to making this a component, however, is that it's "yet another step" to get up and running with embedded development. This could perhaps be folded into an existing step if it already existed, but @japaric it sounds like there's not a great place for this to get folded into? I previously hadn't been able to articulate a great downside to shipping this (and I agree that it's defacto stabilizing), but thinking now I think I may have come up with one (although seriousness of it TBD). This runs the risk of bifurcating "rust through rustup" and "rust through system installers". It seems likely that distros would delete these tools and/or would not ship them by default. That means that the experience in the embedded world would be that you'd have to deal with either the absence of the tools or perhaps a variety of versions (not great). Now this may not actually matter too much. AFAIK the distro "cross compile" experience is definitely not as good as it is through rustup, so it may be perhaps a safe assumption that everyone using these tools are already using rustup and are cross compiling, meaning it's fine to ship/stabilize. One aspect I particularly like about this is that we're not shipping these tools into users' PATH, but rather it's buried in a rustc-specific location that you have to go out of your way to access (which tools I believe @japaric is envisioning for embedded will do) |
Fedora LLVM used to do this (for Steam's sake) using: But I think you'll need
In Fedora they're built and shipped as part of the We don't generally support cross compilation at all though, so as far as I'm concerned, you can assume such users have rustup toolchains. Just don't break my native compilation, please! 😄 |
|
I don't really object to installing it by default; that way it's not a roadblock. I just don't really feel like it fits well into the If I understand correctly that these tools are primarily intended for embedded development, perhaps there's a way we can have them auto-install as part of the "cargo template" solution we're looking for? In theory there's no particular reason a build script or template script (if these exist?) couldn't go fetch them from static.r-l.o... |
|
@cuviper Thanks for the hint. Setting CMAKE_EXE_LINKER_FLAGS seems to have done the trick. I'm seeing 1MB increase in both the tar.gz and the tar.xz tarballs after statically linking to libstdc++ and adding llvm-profdata. |
|
@bors: try |
|
⌛ Trying commit 7d2e24f7f54a7f4bfc61f9c522e8bf9e49b38d41 with merge e65039eccd58896b04fae96a79e9df3b9870aee7... |
|
@japaric is there perhaps a good location in the "embedded setup workflow" to automatically inject installation of these tools? |
|
☀️ Test successful - status-travis |
|
Ok the binary artifacts themselves look good to me! I think we just need to figure out "component or not" now |
|
@alexcrichton the setup looks like this:
(a) Distro specific installation until someone Rewrites It In Rust We could do one of these to not increase the number of setup steps:
|
|
I'll make the argument that these tools belong with rustc because they are used for introspecting and manipulating the output of the compiler itself, and it's an oversight that they haven't been included in the past. This is different than If these tools were much bigger (tens or hundreds of megabytes), I would be sympathetic with the argument that we should create a new component for them, but I don't think it's a great user experience to have a component that we advertise to the user but that turns out in some cases to do nothing once they install it. |
|
Ok thanks for the info! With all that in mind I'm personally still tempted to ship these by default and starting to leverage the in the ecosystem. I'd want to also cc @rust-lang/dev-tools, however, as this is likely most directly impacting y'all! |
|
With these arguments in mind I also agree that at least for now we can probably ship these in the rustc component and re-evaluate if necessary later down the line. I'd like for there to be documentation somewhere semi-official (e.g., the forge, maybe an RFC) that states the APIs of these tools and their existence is not part of Rust's stability promises. I imagine that in practice we'll do our best to avoid breakage but I would like to have that stated somewhere. (Of course, this is presuming we agree with such a guarantee.) |
|
Would this help with #50000, where I'd like |
TimNN
added
the
S-waiting-on-team
|
Looking at how r? latest changes @alexcrichton |
kennytm
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit 9a96876 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 9a96876 with merge debd38f92e45eb55b4d124852b411ae1432c723b... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@ 3 hour timeout. LLVM build takes ~50 minutes because the cache has been invalidated. Need time to re-cache the LLVM artifacts on |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jun 21, 2018
ship LLVM tools with the toolchain
this PR adds llvm-{nm,objcopy,objdump,size} to the rustc sysroot (right next to LLD)
this slightly increases the size of the rustc component. I measured these numbers on x86_64 Linux:
- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.gz 180M -> 193M (+7%)
- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.xz 129M -> 137M (+6%)
r? @alexcrichton
cc #49584
|
☀️ Test successful - status-appveyor, status-travis |
|
Does it take into account theoretical future non-LLVM rustc? |
|
@vi the tools are packaged in their own rustup component. rustc doesn't depend on these tools (or rustup). We are free to leave the llvm-tools component empty for some target platforms if we can't compile the tools for that platform (for any reason). We are also free to drop tools from the component if LLVM stops supporting them in the future. I would be more concerned about lld which is always shipped with the toolchain and it's a hard requirement for cross compiling to wasm. |
andre-richter
added a commit
to rust-embedded/rust-raspberrypi-OS-tutorials
that referenced
this pull request
Jun 29, 2018
Its the last missing util we needed to be a rust-toolchain-only project. rust-lang/rust#50336 https://github.com/japaric/cargo-binutils
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this PR adds llvm-{nm,objcopy,objdump,size} to the rustc sysroot (right next to LLD)
this slightly increases the size of the rustc component. I measured these numbers on x86_64 Linux:
r? @alexcrichton
cc #49584