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

unify llvm-bitcode-linker, wasm-component-ld and llvm-tools logics #130040

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Sep 6, 2024

To use the precompiled ci-rustc in CI, we need to install llvm-bitcode-linker and LLVM tools into ci-rustc's sysroot. Without them some CI pipelines may fail, as shown here.

Blocker for #122709

@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 6, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Sep 7, 2024

Hmm. This seems a bit brittle. Why are we copying LLVM tools and the LLVM bitcode linker, but not wasm-component-ld, lld and other codegen backends? It feels like either the downloaded Rustc sysroot should already contain these things or we should add all of them to its sysroot unconditionally (if they are supposed to be built). Currently it looks like this is done rather arbitrarily.

@onur-ozkan
Copy link
Member Author

Hmm. This seems a bit brittle. Why are we copying LLVM tools and the LLVM bitcode linker, but not wasm-component-ld, lld and other codegen backends? It feels like either the downloaded Rustc sysroot should already contain these things or we should add all of them to its sysroot unconditionally (if they are supposed to be built). Currently it looks like this is done rather arbitrarily.

wasm-component-ld and rust-lld are already included in the CI rustc and we can do the same for llvm-bitcode-linker too. But I am uncertain about adding LLVM tools since developers might prefer using custom LLVM builds.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 7, 2024

I suppose that someone might want a custom/locally built LLD or WASM component build with CI rustc? 😆 And for working on custom codegen backends, it would also be useful to apply a custom backend to the downloaded sysroot, to avoid having to build the compiler.

Just to make sure that I understand the issue, I'll try to describe it: We build the rustc toolchain in CI with some options. Then, if you want to use the CI toolchain locally, your config.toml options should match what has been configured on CI. So if you set option rust.foo = true, then if it wasn't used on CI, you're out of luck, and the build fails. If it was configured on CI, we currently have the assumption that the option will be taken from the downloaded sysroot. On top of that, we have detection logic that tries to rebuild the sysroot if you have local modifications to compiler or library, thus again using the local version/build of that option.

This sort of seems to work for rust options, however there is an issue for llvm options. We currently don't check LLVM options at all in the download-ci-rustc machinery (AFAIK). We do check it for CI LLVM, and CI LLVM is required for CI rustc, so we kind of should know about the options present on CI. However, it's unclear whether the user wants to say "I want to use this (LLVM) option from CI" or "I want to build this (LLVM) option locally". I think that it would be great if we could let users configure thits (see below), but currently if they configure things that are not on LLVM CI, it will error the build out.

Currently, this decision was kind of arbitrary (we just copied the primary LLVM libs to the downloaded sysroots, because they are always needed, and just assumed that LLD will be there as a bonus). And this PR just changes that arbitrary logic to include a few more things.

I'm fine with doing this to unblock #122709, but it seems like just another hack, and we should think of some more general solution.

I would propose something like the following to fix the situation more generally:

  1. Unify the logic of including stuff into local and downloaded sysroots (modulo some special requirements). So always include LLVM, LLD, tools, codegen backends, wasm-component-lld etc. into the built sysroot, regardless of where it comes from (of course, it will still depend on whether these options are configured in config.toml).
  2. Since we require CI LLVM, these components will be taken from the CI LLVM sysroot by default. If they are not present there OR if there are local modifications to the source code of said tool (e.g. lld or wasm-component-ld), it should be rebuilt locally instead of being taken from CI LLVM.

What do you think?

EDIT: I realized that 2) is a bit more complex, because currently we just reject options incompatible with LLVM CI, rather than detecting local changes. But 1) seems like an improvement to the Assemble step.

@onur-ozkan onur-ozkan force-pushed the llvm-tools-with-ci-rustc branch 2 times, most recently from 4ace433 to b6fdb74 Compare September 7, 2024 10:35
@onur-ozkan onur-ozkan changed the title install llvm-bitcode-linker and llvm tools properly unify llvm-bitcode-linker, wasm-component-ld and llvm-tools logics Sep 7, 2024
@onur-ozkan
Copy link
Member Author

This sort of seems to work for rust options, however there is an issue for llvm options. We currently don't check LLVM options at all in the download-ci-rustc machinery (AFAIK).

Yeah, we don't check [llvm] section options in the ci-rustc logic. As download-ci-llvm is required with download-rustc, we should never need that.

EDIT: I realized that 2) is a bit more complex, because currently we just reject options incompatible with LLVM CI, rather than detecting local changes. But 1) seems like an improvement to the Assemble step.

Agreed. Even the first one alone needs a lot of refactoring. For now, I've unified the three parts: llvm tools, llvm-bitcode-linker and wasm-component-ld. Once this PR is shipped, we can check the next day (with the new ci-rustc) to see if it’s enough (at least for now) for #122709.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 8, 2024

Let's see what CI says.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 8, 2024

📌 Commit b6fdb74 has been approved by Kobzol

It is now in the queue for this repository.

@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 Sep 8, 2024
@bors
Copy link
Contributor

bors commented Sep 9, 2024

☔ The latest upstream changes (presumably #130133) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 9, 2024
@onur-ozkan
Copy link
Member Author

Rebased.

@bors r=Kobzol

@bors
Copy link
Contributor

bors commented Sep 9, 2024

📌 Commit 0e59075 has been approved by Kobzol

It is now in the queue for this repository.

@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 9, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 10, 2024
…c, r=Kobzol

unify `llvm-bitcode-linker`, `wasm-component-ld` and llvm-tools logics

To use the precompiled `ci-rustc` in CI, we need to install `llvm-bitcode-linker` and LLVM tools into ci-rustc's sysroot. Without them some CI pipelines may fail, as shown [here](rust-lang#122709 (comment)).

Blocker for rust-lang#122709
@rust-log-analyzer

This comment has been minimized.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
… r=<try>

unify `llvm-bitcode-linker`, `wasm-component-ld` and llvm-tools logics

To use the precompiled `ci-rustc` in CI, we need to install `llvm-bitcode-linker` and LLVM tools into ci-rustc's sysroot. Without them some CI pipelines may fail, as shown [here](rust-lang#122709 (comment)).

Blocker for rust-lang#122709

try-job: dist-arm-linux
@bors
Copy link
Contributor

bors commented Sep 11, 2024

⌛ Trying commit 4967093 with merge e837dec...

@bors
Copy link
Contributor

bors commented Sep 11, 2024

☀️ Try build successful - checks-actions
Build commit: e837dec (e837decd8a8a0e3ab905aa38e17e865656e96ac3)

@onur-ozkan
Copy link
Member Author

Fixed the problem of using incorrect build compiler in dist step.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Sep 12, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Sep 12, 2024

📌 Commit 4967093 has been approved by Kobzol

It is now in the queue for this repository.

@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 Sep 12, 2024
@bors
Copy link
Contributor

bors commented Sep 12, 2024

⌛ Testing commit 4967093 with merge adaff53...

@bors
Copy link
Contributor

bors commented Sep 12, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing adaff53 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2024
@bors bors merged commit adaff53 into rust-lang:master Sep 12, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 12, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (adaff53): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 0.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.5% [1.1%, 1.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 755.803s -> 756.349s (0.07%)
Artifact size: 341.20 MiB -> 341.21 MiB (0.00%)

ehuss added a commit to ehuss/rust that referenced this pull request Sep 13, 2024
…i-rustc, r=Kobzol"

This reverts commit adaff53, reversing
changes made to 2e8db5e.
@onur-ozkan onur-ozkan deleted the llvm-tools-with-ci-rustc branch September 13, 2024 06:16
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2024
Revert  rust-lang#130040 - unify llvm-bitcode-linker, wasm-component-ld and llvm-tools logics

This is a revert of rust-lang#130040 to fix rust-lang#130291 which is preventing installing nightly with the llvm-tools component due to a conflict with the `bin/llc` file which exists in both the rustc and llvm-tools components.
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 14, 2024
…bzol

add llvm-bitcode-linker and llvm-tools bins to ci-rustc's sysroot

rust-lang#130040 is [reverted](rust-lang#130292) because adding component binaries directly to the dist tarball of the compiler caused conflicts (see rust-lang#130291 and rust-lang/rustup#4019). This PR solves the original problem without touching the dist tarball.

r? Kobzol
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2024
Rollup merge of rust-lang#130302 - onur-ozkan:130040-with-fixes, r=Kobzol

add llvm-bitcode-linker and llvm-tools bins to ci-rustc's sysroot

rust-lang#130040 is [reverted](rust-lang#130292) because adding component binaries directly to the dist tarball of the compiler caused conflicts (see rust-lang#130291 and rust-lang/rustup#4019). This PR solves the original problem without touching the dist tarball.

r? Kobzol
shrirambalaji pushed a commit to shrirambalaji/rust that referenced this pull request Oct 6, 2024
…i-rustc, r=Kobzol"

This reverts commit adaff53, reversing
changes made to 2e8db5e.
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants