Ensure `lld` is supported with `download-ci-llvm` by lqd · Pull Request #104748 · rust-lang/rust

lqd · 2022-11-22T22:37:22Z

This PR:

ensures LLD's step in bootstrap's dist, but it's not strictly necessary since dist will already package it when it's present.
makes bootstrap's native::LLD step support using the packaged ci-llvm/bin/lld, instead of building it from source (which would most likely not be available today, nor in the future where download-ci-llvm = if-available is the default).

If I understand correctly, --enable-full-tools will also enable rust.lld, and this is why LLD is already packaged today in the rust-dev component on the main targets (and why -Zgcc-ld=lld does work there).

That means it's likely that this PR will not be able to land before I've reworked and landed #101792: if LLD is available in download-ci-llvm, the needs-rust-lld tests should start being executed on the x64 macOS test builders, and CI would fail today.

I've tested locally that building with download-ci-llvm = true and lld = true with the LLVM submodule unregistered was successful, and that rust-lld and the various lld-wrappers are present and -Zgcc-ld=lld works as well, on a few different platforms:

x86_64-unknown-linux-gnu
aarch64-apple-darwin
x86_64-pc-windows-msvc (with -Clinker=rust-lld rather than -Zgcc-ld=lld)
x86_64-apple-darwin, with the MACOSX_DEPLOYMENT_TARGET workaround for -Z gcc-ld=lld fails again #101653

I don't think we really need to bump the download-ci-llvm-stamp in this case, since ./build/$triple/ci-llvm/bin/lld is present on all the above targets already, but have added it mechanically, and it should probably be removed to avoid unnecessary downloads/churn.

Fixes #98340
Supersedes #100010

lqd · 2022-11-22T22:43:48Z

@bors try

bors · 2022-11-22T22:43:56Z

⌛ Trying commit fdf2238424e42ef67cd1826723a792c86735ded7 with merge 2b02e73d38e5508c9eeda99c5c321b11cd0667f6...

bors · 2022-11-23T02:14:15Z

☀️ Try build successful - checks-actions
Build commit: 2b02e73d38e5508c9eeda99c5c321b11cd0667f6 (2b02e73d38e5508c9eeda99c5c321b11cd0667f6)

jyn514

If I understand correctly, --enable-full-tools will also enable rust.lld, and this is why LLD is already packaged today in the rust-dev component on the main targets (and why -Zgcc-ld=lld does work there).

Can you expand on "main targets"? Is it available on all tier 1 targets, or only targets where we build LLVM from source? and if it's on all tier 1 targets, what is this change doing?

src/bootstrap/dist.rs

src/bootstrap/native.rs

lqd · 2022-11-30T10:12:29Z

Can you expand on "main targets"? Is it available on all tier 1 targets, or only targets where we build LLVM from source?

Ah I only meant the targets listed in the OP (minus aarch64 darwin which is tier 2) where I manually tested that lld was already available. Looking at dist::RustDev's code, I'd expect it to be available where we build LLVM from source and full tools are enabled (or rust.lld of course). I wouldn't know the full target list but this PR doesn't change anything there as it's not about packaging lld.

what is this change doing?

The current situation is a bit subtle and I think in #100010 it was expected that we don't package lld in download-ci-llvm today: we do, but never use it even if it's available. So this change makes bootstrap's lld step check whether we can use the possibly already present $llvm/bin/lld (where dist packages it; this is not where the lld executable is when we build it from source), instead of unconditionally building it from source.

jyn514

Thanks for explaining. This looks good to me if you change LLD to be included in RustDev unconditionally.

src/bootstrap/dist.rs

src/bootstrap/native.rs

jyn514 · 2022-11-30T15:49:19Z

That means it's likely that this PR will not be able to land before I've reworked and landed #101792: if LLD is available in download-ci-llvm, the needs-rust-lld tests should start being executed on the x64 macOS test builders, and CI would fail today.

I am not quite sure what the relation is here, but FWIW I don't have any problem with speculatively approving this and seeing if CI fails or not.

lqd · 2022-11-30T17:35:42Z

To summarize the intent I described on zulip: our rust-lld support (e.g. -Zgcc-ld=lld) has been broken during refactoring a couple times and CI didn't catch it: my understanding is that no test builders are running the existing needs-rust-lld tests (they don't build llvm/lld from source), and lld is only enabled on the dist builders.

This PR is ultimately to fix CI so that this doesn't regress anymore: the test builders will have access to lld with download-ci-llvm. But since things are still currently broken on x64 apple, I'll have to fix that before this PR lands fixing CI: the needs-rust-lld tests would automatically be enabled, and fail because of the breakage.

bors · 2022-12-22T17:36:20Z

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

Makes the lld step avoid building it from source when possible: when dist has packaged it along the other LLVM binaries for the rust-dev component.

lqd · 2023-01-03T20:40:51Z

@jyn514 #101792 has just landed, so #101653 is fixed: that means the various run-make tests depending on rust-lld's availability should now pass on the mac builders.

I've pushed a commit making the LLD step ensure unconditional for the RustDev component as you requested. I don't know if all our dist builders already enable rust.lld— I'd think/hope they do, via enabling full tools — but this will be a fun way to find out 😅 .

I'm not sure we really need to bump the download-ci-llvm-stamp here (since the lld binary should already be in RustDev if lld is enabled on the builder), let me know if you want me to remove that ?

@rustbot ready

jyn514 · 2023-01-04T00:11:28Z

Thanks, this looks great!

I'm not sure we really need to bump the download-ci-llvm-stamp here (since the lld binary should already be in RustDev if lld is enabled on the builder), let me know if you want me to remove that ?

I think it's not strictly necessary, but there's no harm bumping it, the download isn't too big.

@bors r+

bors · 2023-01-04T00:11:29Z

📌 Commit e0f5c6d has been approved by jyn514

It is now in the queue for this repository.

…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#104748 (Ensure `lld` is supported with `download-ci-llvm`) - rust-lang#105541 (Simplify some iterator combinators) - rust-lang#106045 (default OOM handler: use non-unwinding panic, to match std handler) - rust-lang#106157 (Don't trim path for `unsafe_op_in_unsafe_fn` lints) - rust-lang#106353 (Reduce spans for `unsafe impl` errors) - rust-lang#106381 (Jsondoclint: Add `--verbose` and `--json-output` options) - rust-lang#106411 (rustdoc: remove legacy font-feature-settings CSS) - rust-lang#106414 (Add cuviper to the review rotation for libs) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

… r=jyn514 Use CI LLVM in `test-various` builder It was disabled because it needs `lld`, but since rust-lang#104748 was merged it is no longer needed. This will speed this test, since it no longer needs to build LLVM.

lqd force-pushed the download_lld branch 2 times, most recently from cba3d17 to 573b2ba Compare November 23, 2022 18:21

lqd changed the title ~~Ensure lld is packaged in download-ci-llvm~~ Ensure lld is supported with download-ci-llvm Nov 23, 2022

jyn514 mentioned this pull request Nov 25, 2022

Package lld in download-ci-llvm #100010

Closed

jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2022

jyn514 self-assigned this Nov 28, 2022

jyn514 reviewed Nov 28, 2022

View reviewed changes

src/bootstrap/dist.rs Outdated Show resolved Hide resolved

src/bootstrap/native.rs Outdated Show resolved Hide resolved

jyn514 approved these changes Nov 30, 2022

View reviewed changes

src/bootstrap/dist.rs Outdated Show resolved Hide resolved

src/bootstrap/native.rs Outdated Show resolved Hide resolved

lqd added 6 commits January 3, 2023 20:06

bootstrap dist: ensure LLD's step

6eb205d

make the native::LLD step able to use an already built lld

835b58d

Makes the lld step avoid building it from source when possible: when dist has packaged it along the other LLVM binaries for the rust-dev component.

slight cleanup

4e4658f

turn a comment into an actual doc comment

f635f9d

bump download-ci-llvm-stamp

c73a46c

ensure lld's step unconditionally for RustDev component

e0f5c6d

lqd force-pushed the download_lld branch from 573b2ba to e0f5c6d Compare January 3, 2023 20:32

lqd removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jan 3, 2023

lqd marked this pull request as ready for review January 3, 2023 20:33

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 3, 2023

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 Jan 4, 2023

compiler-errors mentioned this pull request Jan 4, 2023

Rollup of 8 pull requests #106432

Merged

bors merged commit 752c0f5 into rust-lang:master Jan 4, 2023

rustbot added this to the 1.68.0 milestone Jan 4, 2023

lqd deleted the download_lld branch January 4, 2023 07:57

albertlarsan68 mentioned this pull request Jan 11, 2023

Use CI LLVM in test-various builder #106711

Merged

Uh oh!

Conversation

lqd commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lqd commented Nov 22, 2022

Uh oh!

bors commented Nov 22, 2022

Uh oh!

bors commented Nov 23, 2022

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lqd commented Nov 30, 2022

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jyn514 commented Nov 30, 2022

Uh oh!

lqd commented Nov 30, 2022

Uh oh!

bors commented Dec 22, 2022

Uh oh!

lqd commented Jan 3, 2023

Uh oh!

jyn514 commented Jan 4, 2023

Uh oh!

bors commented Jan 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lqd commented Nov 22, 2022 •

edited

Loading