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

…uests_download_from_ci`

…FromCi` step

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"

jieyouxu · 2026-08-27T02:43:38Z

+        // script can do less work (i.e. not building C/C++ code and requiring LLVM).
+        if matches!(cmd_kind, Kind::Check | Kind::Clippy | Kind::Fix) && mode == Mode::Rustc {


Hold on, can this cache bust actually be saved in the general case? Consider a check mode requested at stage 2, don't we still have to fully build out stage 1 compiler/std? Absent a prebuilt LLVM, or if built LLVM is stale, don't we still have to bust rustc_llvm cache even for check/clippy/fix?

I'm not sure if I follow 😅 And to be honest, I don't much understand the comment about "cache busting" either. All that RUST_CHECK does is that it prevents the rustc_llvm's build script from running, essentially. There is no other usage of RUST_CHECK across the whole codebase (which is why I modified the comment, because it sounded more general).

If you do a check in stage 2 (e.g. of the compiler), then you will first build the stage 1 compiler, which will build or download LLVM anyway. After that point, this condition will never be triggered, so the stage 2 compiler's rustc_llvm build script will run even in check mode. But even if it didn't run, it wouldn't really bust any caches? Because stage 1 and stage 2 doesn't share a cache.

The only thing that this is supposed to do, AFAIK, is that if you do x check compiler, and LLVM is not built, bootstrap will not attempt to build it, nor to check its submodule out. That behavior should be kept after my changes.

There is one interesting question, and that is what should happen if you do x check compiler, you don't have LLVM locally, but you have download-ci-llvm enabled. Before my changes, bootstrap would still eagerly download LLVM from CI, because it happened during config parsing (lol). After my changes, we still download LLVM eagerly, so the behavior is unchanged, but in theory now we can relatively easily change that decision, and just completely ignore LLVM when we do a check. Though I don't think this is necessarily a good idea, because if you do a check, likely you will follow that with a build soon anyway, and downloading LLVM and running the build script likely isn't that much additional work, especially since it only happens once, and doesn't get repeated in follow-up rebuilds.

I think that we should actually move this whole code block from here and move it to the part that checks whether we should pass environment variables to rustc_llvm. And then move that part out of rustc_cargo_env and put it into rustc_cargo, because I don't understand why should these environment variables be also passed to cg_clif and cg_gcc, which currently use rustc_cargo_env (but they surely don't run the rustc_llvm build script :) ).

Hm yeah, I can't say I understand this cache busting comment either.

I think that we should actually move this whole code block from here and move it to the part that checks whether we should pass environment variables to rustc_llvm. And then move that part out of rustc_cargo_env and put it into rustc_cargo, because I don't understand why should these environment variables be also passed to cg_clif and cg_gcc, which currently use rustc_cargo_env (but they surely don't run the rustc_llvm build script :) ).

Seems like a solid plan to me 👍

Implemented separately here: #161853 to make it easier to review and separate the blast radius.

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 added 4 commits August 24, 2026 19:20

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

3a65929

…uests_download_from_ci`

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

71a91ad

…FromCi` step

Allow downloading LLVM for arbitrary targets

8413511

Only attempt to download LLVM from CI when building rustc

8a5b940

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

Open

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

		// script can do less work (i.e. not building C/C++ code and requiring LLVM).
		if matches!(cmd_kind, Kind::Check \| Kind::Clippy \| Kind::Fix) && mode == Mode::Rustc {

Uh oh!

Uh oh!

Conversation

Kobzol commented Aug 18, 2026 • edited 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!

Choose a reason for hiding this comment

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

Loading

jieyouxu left a comment •

edited

Loading