deprecate `if-available` value of `download-ci-llvm` #117813

onur-ozkan · 2023-11-11T12:22:49Z

This PR deprecates the use of the if-available value for download-ci-llvm since if-unchanged serves the same purpose when no changes are detected. In cases where changes are present, it is assumed that compiling LLVM is acceptable (otherwise, why make changes there?).

This was probably missing in the #110087 issue before.

cc @RalfJung

rustbot · 2023-11-11T12:22:57Z

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-11-11T12:23:01Z

This PR modifies src/bootstrap/defaults. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

This PR changes config.example.toml. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

This PR modifies src/bootstrap/src/core/config. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

This PR changes src/bootstrap/defaults/config.compiler.toml. If appropriate, please also update config.codegen.toml so the defaults are in sync.

rustbot · 2023-11-11T12:25:25Z

This PR modifies src/bootstrap/src/core/config. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

This PR changes config.example.toml. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

This PR changes src/bootstrap/defaults/config.compiler.toml. If appropriate, please also update config.codegen.toml so the defaults are in sync.

This PR modifies src/bootstrap/defaults. If appropriate, please also update CONFIG_CHANGE_HISTORY in src/bootstrap/src/lib.rs and change-id in config.example.toml.

RalfJung · 2023-11-11T13:08:35Z

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

@@ -2104,9 +2104,6 @@ impl Config {
        match download_ci_llvm {
            None => self.channel == "dev" && llvm::is_ci_llvm_available(&self, asserts),
            Some(StringOrBool::Bool(b)) => b,
-            Some(StringOrBool::String(s)) if s == "if-available" => {


This needs a transition period, otherwise switching between branches before this and after this will be painful (since config.toml can't be compatible with both).

if you just set it to if-unchanged, it should work just fine

Signed-off-by: onur-ozkan <work@onurozkan.dev>

Mark-Simulacrum · 2023-11-18T17:04:52Z

If you have come here due to the bootstrap change detection alert, if-available for download-ci-llvm is no longer available in the build configuration, as explained above.

Per #117813 (comment), is it accurate that any current users should just switch to if-unchanged? Can we adjust the PR description to give that guidance?

Mark-Simulacrum · 2023-11-18T17:06:16Z

Although maybe we're no longer actually removing the older option if I'm reading the code right - in which case bumping the change-id seems like needless churn?

onur-ozkan · 2023-11-18T17:14:50Z

if-available for download-ci-llvm is no longer available in the build configuration, as explained above.

This has been changed with #117813 (comment), I forgot to update PR description.

if-available is still available but deprecated and it will be removed later in 2024. In the meantime, I think we can print some logs like if-available is deprecated; please use if-unchanged instead. Alternatively, we can wait until #117815 gets merged, as it introduces a way of informing users about the change-id bumps.

Mark-Simulacrum · 2023-11-18T17:18:39Z

I'd rather avoid an ad-hoc warning; we should use the existing functionality (change-id) or do nothing. I think doing nothing for now and then following up with #117815-based active deprecation (i.e., using the changelog entries suggested there) makes sense. So, that would mean:

Remove the change-id bump from this PR
Update description to coincide with state of this PR
r=me on the PR

onur-ozkan · 2023-11-18T17:26:55Z

@bors r=Mark-Simulacrum

bors · 2023-11-18T17:26:57Z

📌 Commit 2344642 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

…vm-option, r=Mark-Simulacrum deprecate `if-available` value of `download-ci-llvm` This PR deprecates the use of the `if-available` value for `download-ci-llvm` since `if-unchanged` serves the same purpose when no changes are detected. In cases where changes are present, it is assumed that compiling LLVM is acceptable (otherwise, why make changes there?). This was probably missing in the rust-lang#110087 issue before. cc `@RalfJung`

bors · 2023-11-18T23:02:16Z

⌛ Testing commit 2344642 with merge ea6b131...

bors · 2023-11-19T00:59:09Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing ea6b131 to master...

rust-timer · 2023-11-19T02:21:29Z

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

Results

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

Cycles

Results

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

Binary size

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

Bootstrap: 673.122s -> 674.495s (0.20%)
Artifact size: 313.81 MiB -> 313.82 MiB (0.00%)

…acrum general improvements/fixes on bootstrap - adds rust-lang#117813 into change tracker rust-lang@6d9b92f - fixes a bug in change tracker rust-lang@63a4410 - relocates `CONFIG_CHANGE_HISTORY` rust-lang@a7dcb98

Rollup merge of rust-lang#118220 - onur-ozkan:followups, r=Mark-Simulacrum general improvements/fixes on bootstrap - adds rust-lang#117813 into change tracker rust-lang@6d9b92f - fixes a bug in change tracker rust-lang@63a4410 - relocates `CONFIG_CHANGE_HISTORY` rust-lang@a7dcb98

rustbot assigned Mark-Simulacrum Nov 11, 2023

RalfJung reviewed Nov 11, 2023

View reviewed changes

merge if-available and if-unchanged for download-ci-llvm

2344642

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the simplify-download-ci-llvm-option branch from 9c3cab0 to c849eb9 Compare November 11, 2023 15:29

onur-ozkan changed the title ~~replace and remove if-available with if-unchanged for download-ci-llvm~~ deprecate if-available value of download-ci-llvm Nov 18, 2023

onur-ozkan force-pushed the simplify-download-ci-llvm-option branch from c849eb9 to 2344642 Compare November 18, 2023 17:25

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 Nov 18, 2023

compiler-errors mentioned this pull request Nov 18, 2023

Rollup of 9 pull requests #118049

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2023

bors merged commit ea6b131 into rust-lang:master Nov 19, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 19, 2023

onur-ozkan mentioned this pull request Nov 23, 2023

general improvements/fixes on bootstrap #118220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate `if-available` value of `download-ci-llvm` #117813

deprecate `if-available` value of `download-ci-llvm` #117813

onur-ozkan commented Nov 11, 2023 •

edited

Loading

rustbot commented Nov 11, 2023

rustbot commented Nov 11, 2023

rustbot commented Nov 11, 2023

RalfJung Nov 11, 2023

Noratrieb Nov 13, 2023

Mark-Simulacrum commented Nov 18, 2023

Mark-Simulacrum commented Nov 18, 2023

onur-ozkan commented Nov 18, 2023

Mark-Simulacrum commented Nov 18, 2023

onur-ozkan commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 19, 2023

rust-timer commented Nov 19, 2023

deprecate if-available value of download-ci-llvm #117813

deprecate if-available value of download-ci-llvm #117813

Conversation

onur-ozkan commented Nov 11, 2023 • edited Loading

rustbot commented Nov 11, 2023

rustbot commented Nov 11, 2023

rustbot commented Nov 11, 2023

RalfJung Nov 11, 2023

Choose a reason for hiding this comment

Noratrieb Nov 13, 2023

Choose a reason for hiding this comment

Mark-Simulacrum commented Nov 18, 2023

Mark-Simulacrum commented Nov 18, 2023

onur-ozkan commented Nov 18, 2023

Mark-Simulacrum commented Nov 18, 2023

onur-ozkan commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 19, 2023

rust-timer commented Nov 19, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

deprecate `if-available` value of `download-ci-llvm` #117813

deprecate `if-available` value of `download-ci-llvm` #117813

onur-ozkan commented Nov 11, 2023 •

edited

Loading