ci: fix explanation why LLVM download is disabled for windows-gnu

[mati865]

Continuation of #132781

[rustbot]

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

[mati865]

Basically my plan is to do try build with dist so the artifacts get uploaded and then do another try but with test jobs.
If that fails, I think we should merge dist builders change only and then in later PR do the same for test jobs.

[Kobzol]

I would personally prefer to disable LLVM download on the dist runners, to exercise the LLVM build path regularly.

[mati865]

I think dist runners always build LLVM artifacts for download (at least that appears to be the case in #132781) despite using the same environment variable.
Even if that's not the case, changing it would be a bigger change that affects all platforms.

[Kobzol]

Yeah, just to clarify, what I meant was that we shouldn't change the behavior on dist runners - they should always build.

[Mark-Simulacrum]

@bors r+ rollup=never

[bors]

📌 Commit 81f25bb has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[Kobzol]

FWIW, this PR currently still enables LLVM download for the dist- builders. Not sure if that works, or if we even want to enable that.

[Mark-Simulacrum]

Does it actually do that? AFAICT, the NO_DOWNLOAD_CI_LLVM there was just a no-op -- we don't ever read that outside of non-dist builds afaict (

rust/src/ci/run.sh

Lines 165 to 185 in 481b5fa

	# We enable this for non-dist builders, since those aren't trying to produce
	# fresh binaries. We currently don't entirely support distributing a fresh
	# copy of the compiler (including llvm tools, etc.) if we haven't actually
	# built LLVM, since not everything necessary is copied into the
	# local-usage-only LLVM artifacts. If that changes, this could maybe be made
	# true for all builds. In practice it's probably a good idea to keep building
	# LLVM continuously on at least some builders to ensure it works, though.
	# (And PGO is its own can of worms).
	if [ "$NO_DOWNLOAD_CI_LLVM" = "" ]; then
	RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set llvm.download-ci-llvm=if-unchanged"
	else
	# CI rustc requires CI LLVM to be enabled (see https://github.com/rust-lang/rust/issues/123586).
	NO_DOWNLOAD_CI_RUSTC=1
	# When building for CI we want to use the static C++ Standard library
	# included with LLVM, since a dynamic libstdcpp may not be available.
	RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set llvm.static-libstdcpp"
	fi
	if [ "$NO_DOWNLOAD_CI_RUSTC" = "" ]; then
	RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.download-rustc=if-unchanged"
	fi

is all outside DEPLOY=1)

@bors r- until we settle this

[Kobzol]

Sorry, you're right, I missed this, the bash scripts are a bit confusing. Then this should be fine.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 81f25bb has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 81f25bb with merge 4fde74c...

[bors]

💔 Test failed - checks-actions

[Mark-Simulacrum]

@rustbot author

Seems probably real?

[mati865]

Yeah, looks valid. I'll look into it on the weekend.

[bors]

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

[mati865]

The problem stems from the fact that dist builders have LLVM assertions disabled while test runners have assertions enabled.
On platforms like Linux this is solved by alt dist builders:

rust/src/ci/run.sh

Lines 127 to 128 in 6c76ed5

	elif [ "$DEPLOY_ALT" != "" ]; then
	RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-llvm-assertions"

Do we want to change it somehow, or shall I just update the comments to describe what really happens?

[bors]

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

[alex-semenyuk]

@mati865
Thanks for your contribution
From wg-triage. Do we have questions to proceed with this?

[mati865]

@alex-semenyuk yes, the question I had asked in the previous comment remains unanswered.

[alex-semenyuk]

@Mark-Simulacrum @Kobzol Could you please help with

The problem stems from the fact that dist builders have LLVM assertions disabled while test runners have assertions enabled. On platforms like Linux this is solved by alt dist builders:

rust/src/ci/run.sh

Lines 127 to 128 in 6c76ed5

	elif [ "$DEPLOY_ALT" != "" ]; then
	RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-llvm-assertions"

Do we want to change it somehow, or shall I just update the comments to describe what really happens?

[Mark-Simulacrum]

My guess is that implies we should close this; I don't think it merits spinning up alt builders for having this enabled (the cost-benefit isn't really there I suspect).

[mati865]

I will repurpose the PR to only change the comment layer today.

[mati865]

Some time has passed since I investigated this issue, but I think dist runners should work fine with downloaded LLVM.

Updated comments regarding test runners and enabled download on dist runners. I'm relatively confident it should pass now.

[Kobzol]

As Mark correctly said, the environment variable is simply ignored on dist builders, so this should be fine - dist builders will not download LLVM. But still, to be sure:

@bors try

[bors]

⌛ Trying commit 4691415 with merge 17692a3...

[mati865]

As Mark correctly said, the environment variable is simply ignored on non-dist builders, so this should be fine - dist builders will not download LLVM. But still, to be sure:

Ah, I forgot about that. Sorry for not going through the whole topic.
In that case I'll retitle PR and commit after try build confirms that. Still, removing it from dist builders is a net win for readability if it's a noop.
BTW I think you meant "ignored on dist builders" 😉

[bors]

☀️ Try build successful - checks-actions
Build commit: 17692a3 (17692a36107aaf21135e0dbe1f46e8bcb940b359)

[Kobzol]

@bors r+ rollup

[bors]

📌 Commit 4691415 has been approved by Kobzol

It is now in the queue for this repository.

[mati865]

Retitled PR and commit to make it reflect the true change. The previous title would give a false hope for CI time improvements.