change download-ci-llvm default from if-unchanged to true

[onur-ozkan]

Since #129473 and #130202, using download-ci-llvm=true is now the better default and it also fixes #130515.

[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

[rustbot]

This PR modifies config.example.toml.

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

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

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

[Kobzol]

Why does the default behave differently than setting true explicitly?

[onur-ozkan]

Why does the default behave differently than setting true explicitly?

The default is "if-unchanged" which fetches the LLVM submodule, but setting it to true does not.

[Kobzol]

I meant after this PR. I thought that when you don't set download-ci-llvm, it will behave as if you set it to true. But the code paths for "unset" and "true" look different.

[onur-ozkan]

Hmm. Yeah, that makes sense. Should we set it true unconditionally if it's not provided from config.toml?

[Kobzol]

Making it behave differently would introduce essentially another mode and would be unintuitive. When it's not set, it should IMO behave exactly the same as if the user used "true".

We can just unify the logic by restructuring the pattern matching, I suppose?

[Kobzol]

I expressed myself badly 😆 What I meant was that the code executed if (self.channel == "dev" || self.download_rustc_commit.is_some()) holds should be the same as the code executed if the user sets true. Before, that was not the case, because the llvm::is_ci_llvm_available(self, asserts) call was missing.

That being said, just using true as the actual default value simplifies things, so if it's working, that's even better. I don't suppose that beta or stable contains local modifications to LLVM, so it should be fine, hopefully.

You can r=me once CI is green.

[onur-ozkan]

That being said, just using true as the actual default value simplifies things, so if it's working, that's even better. I don't suppose that beta or stable contains local modifications to LLVM, so it should be fine, hopefully.

AFAIK building it from scratch is truly necessary only for distribution runners and the "dist" profile already overrides the default with false.

[onur-ozkan]

@bors r=Kobzol

[bors]

📌 Commit 05f10f4 has been approved by Kobzol

It is now in the queue for this repository.

[bors]

⌛ Testing commit 05f10f4 with merge 13a5097...

[bors]

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

[rust-timer]

Finished benchmarking commit (13a5097): 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)

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

Cycles

Results (secondary -5.1%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.1%	[-5.1%, -5.1%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 769.526s -> 769.055s (-0.06%)
Artifact size: 341.30 MiB -> 341.32 MiB (0.01%)