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

Download LLVM from CI to bootstrap (linux-only to start) #76349

Merged
merged 2 commits into from
Sep 13, 2020

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Sep 4, 2020

This follows #76332, adding support for using CI-built LLVM rather than building it locally. This should essentially "just work," but is left off by default in this PR.

While we can support downloading LLVM for multiple host triples, this currently only downloads it for the build triple. That said, it should be possible to expand this relatively easily should multiple host triples be desired. Most people shouldn't be adjusting host/target triples though, so this should cover most use cases.

Currently this downloads LLVM for the last bors-authored commit in the git log. This is a bit suboptimal -- we want the last bors-authored commit that touched the llvm-project submodule in basically all cases. But for now this just adds an extra ~20 MB download when rebasing atop latest master. Once we have a submodule bump landing after #76332, we can fix this behavior to reduce downloads further.

src/bootstrap/lib.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Sep 5, 2020
@Mark-Simulacrum Mark-Simulacrum changed the title [WIP] Download LLVM from CI to bootstrap Download LLVM from CI to bootstrap Sep 5, 2020
@Mark-Simulacrum
Copy link
Member Author

Mark-Simulacrum commented Sep 5, 2020

I still want to test this with alt builds and on macOS (I don't have a Windows computer to test on right now, and suspect that this won't work as-is there due to LLVM being statically linked rather than shared on Windows in CI).

Since the option is off by default we can technically land it before #76332 -- it just won't work to enable it. Probably best to wait after that lands, though.

r? @pietroalbini but anyone else should feel free to approve too

Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes mostly look good, but I'm not familiar with rustbuild enough to know if they're correct :)

src/bootstrap/bootstrap.py Show resolved Hide resolved
src/bootstrap/bootstrap.py Show resolved Hide resolved
@mati865
Copy link
Contributor

mati865 commented Sep 7, 2020

suspect that this won't work as-is there due to LLVM being statically linked rather than shared on Windows in CI

At least for MinGW, linking LLVM dynamically should work fine when using libc++ instead of libstdc++ (because of the hangs mentioned in 3rd item at Upgrading MinGW on the CI).

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2020
…troalbini

Add rust-dev component to support rustc development

This is preparatory work for permitting rustc developers to use CI-built LLVM rather than building it locally. Unlike distro-built LLVM, CI built LLVM is essentially guaranteed to behave perfectly for local development -- it is fully up to date, and carries all necessary patches.

This is a separate PR from rust-lang#76349 because it needs to land before that one, since we want a master build with the full CI LLVM to be available for easier testing.
@Mark-Simulacrum Mark-Simulacrum changed the title Download LLVM from CI to bootstrap Download LLVM from CI to bootstrap (linux-only to start) Sep 9, 2020
@Mark-Simulacrum
Copy link
Member Author

I've confirmed this works on linux. I plan to spend some time trying to make it work on macOS, but I would like to get this in contributor's hands to the extent possible soon (since it's a major help for people who can use it and are first-time contributors). It is still off by default until we can make it work on all tier-1 platforms, though.

r? @alexcrichton

@jyn514 jyn514 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2020
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise, would it be possible to minimize the impact of this new option? For example could there be one option that recognizes the boolean flag and then sets llvm-config based on the convention that x.py downloads to? That way there'd just be one place in rustbuild (ideally) that the flag is checked.

src/bootstrap/bootstrap.py Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member Author

@bors r=alexcrichton

We weren't uplifting properly because while ThinLTO enabled shared linking, it did not do so properly by setting the link-shared configuration option.

@bors
Copy link
Contributor

bors commented Sep 12, 2020

📌 Commit 2e87a6e has been approved by alexcrichton

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2020
@bors
Copy link
Contributor

bors commented Sep 12, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: alexcrichton
Pushing a53f449516f23486d2dfd4e5685d4e869e8591d9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2020
@bors
Copy link
Contributor

bors commented Sep 12, 2020

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@Mark-Simulacrum
Copy link
Member Author

Gah I forgot about that bors bug, but looks like it didn't actually push to master...

@bors r=alexcrichton retry

@bors
Copy link
Contributor

bors commented Sep 12, 2020

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

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: Align rustc_metadata Table to word boundary #76620

@bors
Copy link
Contributor

bors commented Sep 12, 2020

📌 Commit 2e87a6e has been approved by alexcrichton

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 12, 2020
…hton

Download LLVM from CI to bootstrap (linux-only to start)

This follows rust-lang#76332, adding support for using CI-built LLVM rather than building it locally. This should essentially "just work," but is left off by default in this PR.

While we can support downloading LLVM for multiple host triples, this currently only downloads it for the build triple. That said, it should be possible to expand this relatively easily should multiple host triples be desired. Most people shouldn't be adjusting host/target triples though, so this should cover most use cases.

Currently this downloads LLVM for the last bors-authored commit in the `git log`. This is a bit suboptimal -- we want the last bors-authored commit that touched the llvm-project submodule in basically all cases. But for now this just adds an extra ~20 MB download when rebasing atop latest master. Once we have a submodule bump landing after rust-lang#76332, we can fix this behavior to reduce downloads further.
@bors
Copy link
Contributor

bors commented Sep 13, 2020

⌛ Testing commit 2e87a6e with merge 04b72b4...

@bors
Copy link
Contributor

bors commented Sep 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: alexcrichton
Pushing 04b72b4 to master...

@bors bors merged commit 04b72b4 into rust-lang:master Sep 13, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 13, 2020
matklad added a commit to matklad/rustc-dev-guide that referenced this pull request Sep 13, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2020
…hton

Always try to promote shared LLVM to the sysroot

Even when LLVM is not generally participating in a shared link with rustc, we
will likely still link to the shared dylib from rust-lld, so we still need to
promote it.

This reverts part of rust-lang#76349; my expectation that the link-shared rule was sufficient was likely wrong.

Hopefully fixes rust-lang#76698.

r? `@alexcrichton`
jyn514 pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Oct 4, 2020
@Mark-Simulacrum Mark-Simulacrum deleted the dl-llvm branch August 7, 2023 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants