Assorted bootstrap LLVM refactors (part 4/N) by Kobzol · Pull Request #161290 · rust-lang/rust

Kobzol · 2026-08-18T11:34:42Z

Continuation on #161247.

This PR completely removes handling of git changes or LLVM downloads from config parsing, and moves it into the LlvmFromCi step. Thanks to that, we now also allow downloading LLVM for non-host targets.

There is one annoyance related to that, and that is that download-ci-llvm now applies to all targets for which you try to build LLVM (d'uh), but that also means that if (for whatever reason) LLVM fails to be downloaded from CI, the build will fail. So if you build for target T2 from target T1:

If you want to download T1, but build T2, that's not possible to express.
If T2 fails to be downloaded, the build fails, even if it could be built locally.

I think that we mostly have four options how to deal with this:

Just ignore it and wait to see if someone complains.
Revert the change and always download only for the host target. Worked so far. However, downloading LLVM for non-host targets would be quite useful for further bootstrap improvements and refactorings, because the current logic around sysroots and libdirs is.. convoluted, to say the last, and making cross-compilation easier would help with that a lot.
Allow specifying download-ci-llvm per target in the target config section. So that you can say that you want to download for T1, but build for T2.
Make download failures non-fatal, and cause them to trigger a local build. This would also help with removing the hacky is_ci_llvm_available_for_target logic, which hard-codes a bunch of targets to "know" which ones offer LLVM and which don't. We could just try to download, and if the result is 404, then we print a warning and continue with building (but this is slightly orthogonal, we can do this even if we don't make LLVM build failures non-fatal).

I think that 3 or 4 would be the best solution, perhaps slightly opting for 4. If we get a 404, there's no way we can download, so we build instead. If we get a different error, we still make the failed download fail the build. And only if someone has a use-case for 3, we'd add the new config.

Already before this PR, we did this:

// If download-ci-llvm=true we also want to check that CI llvm is available
b && llvm::is_ci_llvm_available_for_target(&dwn_ctx.host_target, asserts)

so if LLVM wasn't available, we just silently reverted from download-ci-llvm=true to download-ci-llvm=false. The 4. proposal would just generalize that, to actually check whether the LLVM files are present on the CDN or not.

The problem with 4. is that you can't really set any custom build options for LLVM though, because if you also enable download-ci-llvm, the config sanity check will tell you to GTFO :( So we would probably need to make some changes there.

r? jieyouxu

rustbot · 2026-08-24T19:23:12Z

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

Kobzol · 2026-08-24T19:23:13Z

Ok, should be ready for a review now.

jieyouxu

Some thoughts
@rustbot author

View changes since this review

jieyouxu · 2026-08-26T08:59:23Z

There is one annoyance related to that, and that is that download-ci-llvm now applies to all targets for which you try to build LLVM (d'uh), but that also means that if (for whatever reason) LLVM fails to be downloaded from CI, the build will fail. So if you build for target T2 from target T1:

If you want to download T1, but build T2, that's not possible to express.

If T2 fails to be downloaded, the build fails, even if it could be built locally.

I think that we mostly have four options how to deal with this:

Just ignore it and wait to see if someone complains.

Revert the change and always download only for the host target. Worked so far. However, downloading LLVM for non-host targets would be quite useful for further bootstrap improvements and refactorings, because the current logic around sysroots and libdirs is.. convoluted, to say the last, and making cross-compilation easier would help with that a lot.

Allow specifying download-ci-llvm per target in the target config section. So that you can say that you want to download for T1, but build for T2.

Make download failures non-fatal, and cause them to trigger a local build. This would also help with removing the hacky is_ci_llvm_available_for_target logic, which hard-codes a bunch of targets to "know" which ones offer LLVM and which don't. We could just try to download, and if the result is 404, then we print a warning and continue with building (but this is slightly orthogonal, we can do this even if we don't make LLVM build failures non-fatal).

I think that 3 or 4 would be the best solution, perhaps slightly opting for 4. If we get a 404, there's no way we can download, so we build instead. If we get a different error, we still make the failed download fail the build. And only if someone has a use-case for 3, we'd add the new config.

I'm on board with option (4) but with a caveat:

If user did not specify their preference (so getting a cascading default to download-ci-llvm = "if-unchanged") or if they explicitly specified download-ci-llvm = "if-unchanged", then "try to download, build if missing" fallback feels reasonable.

If user specified download-ci-llvm = true, we should fail the build if no CI LLVM is available.

I think this matches what currently download-ci-llvm = "if-unchanged" for host targets do?

To clarify, I think what you mean here is for download-ci-llvm = "if-unchanged"?

I think this matches what currently download-ci-llvm = "if-unchanged" for host targets do?

Actualy, it doesn't :) true and if-unchanged are not different in this behavior currently. If you use either of them, and the given target is unavailable on CI, bootstrap will just build LLVM locally. It would seem surprising to me if those two modes had a different behavior in this specific aspect.

if-unchanged:

rust/src/bootstrap/src/core/config/config.rs

Line 2489 in 287084b

llvm::is_ci_llvm_available_for_target(&dwn_ctx.host_target, asserts)

true:

rust/src/bootstrap/src/core/config/config.rs

Line 2513 in 287084b

b && llvm::is_ci_llvm_available_for_target(&dwn_ctx.host_target, asserts)

(By the way, I'm considering merging those two modes together, same as we do for gcc. So that you only say true, and if there are local changes, you would build. But if the LLVM submodule is checked out, you wouldn't check git changes, ofc.)

Hm... I guess that sometimes I do want to download-ci-llvm to fail the build if CI LLVM cannot be used for whatever reason and never checkout the llvm submodule. (In that, for specific cases like testing in-tree tools I prefer to fail the build rather than trying to recover.)

But yeah if this matches existing behavior then we can consider revisiting this behavior later.

jieyouxu · 2026-08-26T09:36:46Z

+                    // CI-rustc can't be used without CI-LLVM. If LLVM Ci is requested, but the
+                    // LLVM submodule has changes, it is an error.


"LLVM Ci" => "CI LLVM"

jieyouxu · 2026-08-26T09:40:15Z

        "setting dictionary value"
    );
-    assert!(!config.llvm_ci_mode.download_from_ci());
+    assert!(matches!(config.llvm_ci_mode, LlvmCiMode::BuildLocally));


assert_matches!

Oh, I always forget it's already stable :)

jieyouxu · 2026-08-27T02:39:55Z

        // Sanity check
-        check_llvm_version(builder, llvm_ci.output.llvm_config());
+        if builder.host_target == self.target {
+            check_llvm_version(builder, llvm_ci.output.llvm_config());
+        }


Do we only check host LLVM version via host llvm-config here? Since target LLVM's llvm-config on cross-compile isn't guaranteed to be runnable on host? Maybe worth a comment.

Yeah, we can't execute the binary if it's not built for the proper target. I'll add a comment.

jieyouxu · 2026-08-27T02:40:49Z

-        // Set a flag for `check`/`clippy`/`fix`, so that certain build
-        // scripts can do less work (i.e. not building/requiring LLVM).
-        if matches!(cmd_kind, Kind::Check | Kind::Clippy | Kind::Fix) {
+        // Set a flag for `check`/`clippy`/`fix`, so that te rustc_llvm build


"te" => "the"

…uests_download_from_ci`

…FromCi` step

rustbot · 2026-08-29T07:25:30Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Kobzol · 2026-08-29T07:26:36Z

Rebased.

@rustbot ready

jieyouxu

I have a question re. external LLVM vs CI LLVM on host != target scenario

View changes since this review

jieyouxu · 2026-08-29T08:26:26Z

+                        assert!(self.is_running_on_ci());
+                        panic!("ERROR: LLVM submodule has changes, `download-rustc` can't be used.");


If we are local, won't the assertion swallow the actual useful panic message here?

jieyouxu · 2026-08-29T08:29:19Z

+                    // CI-rustc can't be used without CI-LLVM. If LLVM Ci is requested, but the
+                    // LLVM submodule has changes, it is an error.


jieyouxu · 2026-08-29T08:29:22Z

        "setting dictionary value"
    );
-    assert!(!config.llvm_ci_mode.download_from_ci());
+    assert!(matches!(config.llvm_ci_mode, LlvmCiMode::BuildLocally));


jieyouxu · 2026-08-29T08:34:00Z

+    if builder.config.llvm_ci_mode.requests_download_from_ci()
+        && let Some(config) = builder.config.target_config.get(&target)
+    {
+        if config.llvm_config.is_some() {
+            panic!(
+                "Cannot configure `llvm-config` for {target} when using `llvm.download-ci-llvm`",
+            );
+        }
+        if config.llvm_filecheck.is_some() {
+            panic!(
+                "Cannot configure `llvm-filecheck` for {target} when using `llvm.download-ci-llvm`"
+            );
+        }
+    }


Without per-target download-ci-llvm, with this logic here, we can't mix:

I want to download CI LLVM for host, where it is available

I want to provide an external LLVM for the target, where CI LLVM is unavailable

Previously, did we just use external LLVM for the target when host != target, when CI LLVM was available for host and requested? Or did this just never work?

This logic seems a bit fishy, won't in-tree default profiles (compiler/tools/libs) also hit this with if-changed if they try to cross-compile?

rustbot assigned jieyouxu Aug 18, 2026

This comment has been minimized.

Sign in to view

Kobzol force-pushed the bootstrap-llvm-4 branch from e33910a to c2469f0 Compare August 24, 2026 14:52

This comment has been minimized.

Sign in to view

Kobzol force-pushed the bootstrap-llvm-4 branch from c2469f0 to 8a5b940 Compare August 24, 2026 17:21

Kobzol marked this pull request as ready for review August 24, 2026 19:23

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2026

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 24, 2026

jieyouxu reviewed Aug 27, 2026

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2026

jieyouxu mentioned this pull request Aug 27, 2026

Consolidate LLVM skip in check builds in bootstrap #161853

Merged

This comment has been minimized.

Sign in to view

Kobzol added 3 commits August 29, 2026 09:23

Reduce usage of the download_from_ci function and rename it to `req…

794790d

…uests_download_from_ci`

Move download-ci-llvm if-unchanged detection logic into the `Llvm…

23ed00f

…FromCi` step

Allow downloading LLVM for arbitrary targets

9cc2384

Kobzol force-pushed the bootstrap-llvm-4 branch from 8a5b940 to 9cc2384 Compare August 29, 2026 07:25

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 Aug 29, 2026

jieyouxu reviewed Aug 29, 2026

View reviewed changes

		// CI-rustc can't be used without CI-LLVM. If LLVM Ci is requested, but the
		// LLVM submodule has changes, it is an error.

		assert!(self.is_running_on_ci());
		panic!("ERROR: LLVM submodule has changes, `download-rustc` can't be used.");

Uh oh!

Uh oh!

Conversation

Kobzol commented Aug 18, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 24, 2026

Uh oh!

Kobzol commented Aug 24, 2026

Uh oh!

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

rustbot commented Aug 29, 2026

Uh oh!

Kobzol commented Aug 29, 2026

Uh oh!

jieyouxu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Kobzol commented Aug 18, 2026 •

edited by rustbot

Loading

jieyouxu left a comment •

edited

Loading

jieyouxu left a comment •

edited by rustbot

Loading

jieyouxu Aug 29, 2026 •

edited

Loading