use `download-ci-llvm=true` in the default compiler config #129473

Urgau · 2024-08-23T15:07:07Z

1ca2708 made it so that the src/llvm-project submodule has to be checkout for download-ci-llvm = "if-unchanged" to know if the submodule has been changed, but that is not required, if the submodule hasn't been checkout it cannot have been modified.

~~This PR restore the previous behavior by only updating the submodule if it has already been checkout.~~

This PR makes download-ci-llvm = true check if CI llvm is available and make it the default for the compiler profile, as to prevent unnecessarily checking out src/llvm-project with "if-unchanged".

r? @onur-ozkan

rustbot · 2024-08-23T15:07:19Z

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

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

Urgau · 2024-08-23T15:15:56Z

For context I have been using download-ci-llvm = "if-unchanged" with src/llvm-project not checkout for quite some time now and I would like to keep it that way as it allows me to not have 2Gio of space used for nothing, while at the same being able to manually init it if I need to without changing my config.toml.

onur-ozkan · 2024-08-23T18:00:25Z

1ca2708 made it so that the src/llvm-project submodule has to be checkout for download-ci-llvm = "if-unchanged" to know if the submodule has been changed, but that is not required, if the submodule hasn't been checkout it cannot have been modified.

I think this will regress #122787 ?

For context I have been using download-ci-llvm = "if-unchanged" with src/llvm-project not checkout for quite some time now and I would like to keep it that way as it allows me to not have 2Gio of space used for nothing

You could set "download-ci-llvm" to true if you don't want to fetch llvm tree.

Urgau · 2024-08-23T19:01:36Z

I think this will regress #122787 ?

I don't see why it would, the only thing that is changing with this PR is the detection of git changes under src/llvm-project, which would no longer require the submodule to be initialized. There is no change in behavior, other than not unnecessarily cloning the repo anymore.

Maybe I'm missing something. Idk.

You could set "download-ci-llvm" to true if you don't want to fetch llvm tree.

Sure, and that's what I'm doing in the mean time, but it sometimes happens that my src/llvm-project is initialized or that I'm modifying it in which case having in to true would silence any modifications (wanted or not).

onur-ozkan · 2024-08-24T04:52:54Z

If llvm-project isn't initialized, we could consider has_changes as false without needing to call update_submodule and last_modified_call, which simplifies the logic and doesn't require an additional testing (with the current approach, we would need to add tests for last_modified_commit to ensure it doesn't regress again).

onur-ozkan · 2024-08-24T11:00:13Z

In addition, I believe fetching the LLVM submodule for if-unchanged makes more sense than not fetching it. If we don't fetch the submodule, how are people supposed to modify it? 🤔 IMO people who don't want to allocate disk space for LLVM should set this option to true rather than if-unchanged.

Urgau · 2024-08-25T10:27:14Z

I've changed the code so that if src/llvm-project isn't initialized we don't call update_submodule and last_modified_call.

If we don't fetch the submodule, how are people supposed to modify it?

git submodule init "src/llvm-project" && git submodule update (I think)

or when they clone the repo with git clone --recurse "https://github.com/rust-lang/rust/"

It's also worth nothing that when download-ci-llvm isn't set we still end-up taking the if_unchanged closure, which I find even more confusing as it might clone the repo but not use it.

Another interesting think is that src/bootstrap/defaults/compiler.config.toml defines it-self as:

These defaults are meant for contributors to the compiler who do not modify codegen or LLVM

And yet it uses download-ci-llvm = "if-unchanged" which will clone the repo for nothing, despite the profile being for contributors that will not modify it.

This last point is especially interesting as the "compiler" profile claims to be exactly what I want and yet in practice isn't anymore.

onur-ozkan · 2024-08-25T12:34:01Z

Another interesting think is that src/bootstrap/defaults/compiler.config.toml defines it-self as:

These defaults are meant for contributors to the compiler who do not modify codegen or LLVM

That's a good point, but since Remove the "codegen" profile from bootstrap, I believe some people have switched from the codegen profile to the compiler profile (@Zalathar is one of them iirc).

Other than that, I believe the current approach makes more sense than what this PR proposes (we wouldn't really want to force people to handle llvm submodule manually).

Zalathar · 2024-08-27T10:41:20Z

I think it's fine and even desirable for the default compiler profile to avoid checking out LLVM whenever possible, as long as there's a reasonable way to get it checked out for the minority of contributors who actively want it, and things continue to work seamlessly after it is checked out.

IMO:

Telling contributors to run git submodule init by hand is not reasonable, but
Telling contributors to set download-ci-llvm = false (temporarily or permanently) is reasonable, and works today.

The comments in config.compiler.toml do need to be updated to reflect the fact that the codegen profile no longer exists. But I think it would be fine to just add a comment that says “if you do want to modify LLVM, here are some extra recommended settings that you should add to your own config.toml by hand”.

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

Urgau · 2024-08-29T09:59:08Z

Telling contributors to set download-ci-llvm = false (temporarily or permanently) is reasonable, and works today.

@onur-ozkan Do you also see this as a reasonable (enough) workflow for contributors ?

onur-ozkan · 2024-08-29T10:28:47Z

Telling contributors to set download-ci-llvm = false (temporarily or permanently) is reasonable, and works today.

@onur-ozkan Do you also see this as a reasonable (enough) workflow for contributors ?

It seems fine to me, but I think this should be discussed in the t-compiler Zulip channel, as this is their default profile. I wouldn't recommend changing defaults on this profile without their knowledge.

Urgau · 2024-08-29T10:35:07Z

Well I would argue that this PR restore the previous behavior and that #129365 was the one that change it and broke the previously working workflow that I used.

But very well, I will start a T-compiler topic.

onur-ozkan · 2024-08-29T10:39:02Z

Well I would argue that this PR restore the previous behavior and that #129365 was the one that change it and broke the previously working workflow that I used.

I think there was a miscommunication. This PR doesn’t really make sense to me (see #129473 (comment)). I was referring to setting download-ci-llvm=true for the default compiler profile and adding a comment that says "set this to false/if-unchanged to fetch the LLVM submodule.".

Urgau · 2024-08-29T12:36:21Z

Opened https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Interaction.20between.20config.2Ecompiler.2Etoml.20.26.20download-ci-llvm

rustbot · 2024-09-05T20:49:07Z

This PR modifies src/bootstrap/defaults.

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

This PR modifies config.example.toml.

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

Urgau · 2024-09-05T21:08:05Z

As discussed in the Zulip thread I've updated this PR to instead make download-ci-llvm = true check if CI llvm is available and made it the default for the compiler profile, as to prevent
unnecessarily checking out src/llvm-project with "if-unchanged".

download-ci-llvm = "if-unchanged" remains unchanged.

Zalathar · 2024-09-06T01:25:08Z

Summary of my thoughts:

Not checking out LLVM by default is the correct behaviour, and should not have been changed by improve submodule updates #129231
Using true to restore that behaviour (and making it the default) would be a huge mistake

The whole premise of wanting to break true instead of fixing if-unchanged is bizarre to me.

onur-ozkan

LGTM, thanks! Feel free to r=me with/without the suggestions.

config.example.toml

src/bootstrap/defaults/config.compiler.toml

onur-ozkan · 2024-09-06T04:01:10Z

Not checking out LLVM by default is the correct behaviour, and should not have been changed by #129231

Just to be clear, #129231 did not change anything on that behaviour. It just made bootstrap to checkout llvm earlier during config parse so we get the correct result on "did the llvm-project changed?" logic. Just tested it on 37d56da, it fetches the submodule when it's not synced. So the "avoid cloning if not changed" thing did not really work all the time.

Using true to restore that behaviour (and making it the default) would be a huge mistake

I don't really think ending up with a huge mistake is possible with this PR. We have two options, and neither of them would lead to a a huge mistake.

With the other option (using if-unchanged to check out only if the submodule is initialized), there will initially be no llvm submodule to modify. To modify LLVM, developers would need to either temporarily set download-ci-llvm to false to fetch the submodule and then switch back to if-unchanged, or run a git command. Given this perspective, setting download-ci-llvm=true as the default seems to be the best option here. Since needing to modify LLVM is a rare case (as you mentioned in the Zulip thread), people can generally use download-ci-llvm=if-unchanged for their own build configurations. In fact, we may consider creating a specific default profile for working on the LLVM submodule similar to the codegen profile we had before.

Zalathar · 2024-09-06T04:51:02Z

As far as I’m aware, if-unchanged had always avoided checking out LLVM if possible, until #129231 had the side effect of changing it to always check out the LLVM submodule instead.

bors · 2024-09-06T12:56:49Z

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

and make it the default for the compiler profile, as to prevent unnecessarily checking out `src/llvm-project` with `"if-unchanged"`.

Urgau · 2024-09-06T16:24:53Z

Rebased to fix the conflicts.

@bors r=onur-ozkan

bors · 2024-09-06T16:24:55Z

📌 Commit 5f367bb has been approved by onur-ozkan

It is now in the queue for this repository.

…r-ozkan use `download-ci-llvm=true` in the default compiler config rust-lang@1ca2708 made it so that the `src/llvm-project` submodule has to be checkout for `download-ci-llvm = "if-unchanged"` to know if the submodule has been changed, but that is not required, if the submodule hasn't been checkout it cannot have been modified. ~~This PR restore the previous behavior by only updating the submodule if it has already been checkout.~~ This PR makes `download-ci-llvm = true` check if CI llvm is available and make it the default for the compiler profile, as to prevent unnecessarily checking out `src/llvm-project` with `"if-unchanged"`. r? `@onur-ozkan`

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#129337 (rustdoc rfc#3662 changes under unstable flags) - rust-lang#129473 (use `download-ci-llvm=true` in the default compiler config) - rust-lang#129529 (Add test to build crates used by r-a on stable) - rust-lang#130025 (Also emit `missing_docs` lint with `--test` to fulfil expectations) - rust-lang#130070 (Rename variant `AddrOfRegion` of `RegionVariableOrigin` to `BorrowRegion`) - rust-lang#130087 (remove 'const' from 'Option::iter') - rust-lang#130092 (Fixes typo in wasm32-wasip2 doc comment) r? `@ghost` `@rustbot` modify labels: rollup

…r-ozkan use `download-ci-llvm=true` in the default compiler config rust-lang@1ca2708 made it so that the `src/llvm-project` submodule has to be checkout for `download-ci-llvm = "if-unchanged"` to know if the submodule has been changed, but that is not required, if the submodule hasn't been checkout it cannot have been modified. ~~This PR restore the previous behavior by only updating the submodule if it has already been checkout.~~ This PR makes `download-ci-llvm = true` check if CI llvm is available and make it the default for the compiler profile, as to prevent unnecessarily checking out `src/llvm-project` with `"if-unchanged"`. r? ``@onur-ozkan``

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#129473 (use `download-ci-llvm=true` in the default compiler config) - rust-lang#129529 (Add test to build crates used by r-a on stable) - rust-lang#130025 (Also emit `missing_docs` lint with `--test` to fulfil expectations) - rust-lang#130048 (run-make-support: Add llvm-pdbutil) - rust-lang#130070 (Rename variant `AddrOfRegion` of `RegionVariableOrigin` to `BorrowRegion`) - rust-lang#130087 (remove 'const' from 'Option::iter') - rust-lang#130092 (Fixes typo in wasm32-wasip2 doc comment) r? `@ghost` `@rustbot` modify labels: rollup

…r-ozkan use `download-ci-llvm=true` in the default compiler config rust-lang@1ca2708 made it so that the `src/llvm-project` submodule has to be checkout for `download-ci-llvm = "if-unchanged"` to know if the submodule has been changed, but that is not required, if the submodule hasn't been checkout it cannot have been modified. ~~This PR restore the previous behavior by only updating the submodule if it has already been checkout.~~ This PR makes `download-ci-llvm = true` check if CI llvm is available and make it the default for the compiler profile, as to prevent unnecessarily checking out `src/llvm-project` with `"if-unchanged"`. r? ```@onur-ozkan```

…r-ozkan use `download-ci-llvm=true` in the default compiler config rust-lang@1ca2708 made it so that the `src/llvm-project` submodule has to be checkout for `download-ci-llvm = "if-unchanged"` to know if the submodule has been changed, but that is not required, if the submodule hasn't been checkout it cannot have been modified. ~~This PR restore the previous behavior by only updating the submodule if it has already been checkout.~~ This PR makes `download-ci-llvm = true` check if CI llvm is available and make it the default for the compiler profile, as to prevent unnecessarily checking out `src/llvm-project` with `"if-unchanged"`. r? ````@onur-ozkan````

…kingjubilee Rollup of 14 pull requests Successful merges: - rust-lang#128316 (Stabilize most of `io_error_more`) - rust-lang#129473 (use `download-ci-llvm=true` in the default compiler config) - rust-lang#129529 (Add test to build crates used by r-a on stable) - rust-lang#129778 (interpret: make typed copies lossy wrt provenance and padding) - rust-lang#129981 (Remove `serialized_bitcode` from `LtoModuleCodegen`.) - rust-lang#130025 (Also emit `missing_docs` lint with `--test` to fulfil expectations) - rust-lang#130040 (unify `llvm-bitcode-linker`, `wasm-component-ld` and llvm-tools logics) - rust-lang#130094 (Inform the solver if evaluation is concurrent) - rust-lang#130132 ([illumos] enable SIGSEGV handler to detect stack overflows) - rust-lang#130146 (bootstrap `naked_asm!` for `compiler-builtins`) - rust-lang#130149 (Helper function for formatting with `LifetimeSuggestionPosition`) - rust-lang#130152 (adapt a test for llvm 20) - rust-lang#130162 (bump download-ci-llvm-stamp) - rust-lang#130164 (move some const fn out of the const_ptr_as_ref feature) r? `@ghost` `@rustbot` modify labels: rollup

…kingjubilee Rollup of 11 pull requests Successful merges: - rust-lang#128316 (Stabilize most of `io_error_more`) - rust-lang#129473 (use `download-ci-llvm=true` in the default compiler config) - rust-lang#129529 (Add test to build crates used by r-a on stable) - rust-lang#129981 (Remove `serialized_bitcode` from `LtoModuleCodegen`.) - rust-lang#130094 (Inform the solver if evaluation is concurrent) - rust-lang#130132 ([illumos] enable SIGSEGV handler to detect stack overflows) - rust-lang#130146 (bootstrap `naked_asm!` for `compiler-builtins`) - rust-lang#130149 (Helper function for formatting with `LifetimeSuggestionPosition`) - rust-lang#130152 (adapt a test for llvm 20) - rust-lang#130162 (bump download-ci-llvm-stamp) - rust-lang#130164 (move some const fn out of the const_ptr_as_ref feature) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129473 - Urgau:fix-llvm-if-unchanged, r=onur-ozkan use `download-ci-llvm=true` in the default compiler config rust-lang@1ca2708 made it so that the `src/llvm-project` submodule has to be checkout for `download-ci-llvm = "if-unchanged"` to know if the submodule has been changed, but that is not required, if the submodule hasn't been checkout it cannot have been modified. ~~This PR restore the previous behavior by only updating the submodule if it has already been checkout.~~ This PR makes `download-ci-llvm = true` check if CI llvm is available and make it the default for the compiler profile, as to prevent unnecessarily checking out `src/llvm-project` with `"if-unchanged"`. r? `````@onur-ozkan`````

…=Kobzol change `download-ci-llvm` default from `if-unchanged` to `true` Since rust-lang#129473 and rust-lang#130202, using `download-ci-llvm=true` is now the better default and it also fixes rust-lang#130515.

rustbot assigned onur-ozkan Aug 23, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 23, 2024

Urgau changed the title ~~Un-required checkout src/llvm-project submodule with download-ci-llvm = "if-unchanged"~~ Avoid requiring checkout src/llvm-project with download-ci-llvm = "if-unchanged" Aug 23, 2024

Urgau force-pushed the fix-llvm-if-unchanged branch from 427f039 to 29886a3 Compare August 25, 2024 10:04

Zalathar reviewed Aug 27, 2024

View reviewed changes

src/bootstrap/src/core/config/config.rs Outdated Show resolved Hide resolved

Urgau force-pushed the fix-llvm-if-unchanged branch from 29886a3 to 9fed172 Compare September 5, 2024 20:49

Urgau force-pushed the fix-llvm-if-unchanged branch from 9fed172 to 634a2c2 Compare September 5, 2024 20:54

Urgau changed the title ~~Avoid requiring checkout src/llvm-project with download-ci-llvm = "if-unchanged"~~ Avoid requiring checkout of src/llvm-project with default compiler config Sep 5, 2024

onur-ozkan approved these changes Sep 6, 2024

View reviewed changes

config.example.toml Outdated Show resolved Hide resolved

src/bootstrap/defaults/config.compiler.toml Outdated Show resolved Hide resolved

onur-ozkan changed the title ~~Avoid requiring checkout of src/llvm-project with default compiler config~~ use download-ci-llvm=true in the default compiler config Sep 6, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 6, 2024

Make download-ci-llvm = true check if CI llvm is available

5f367bb

and make it the default for the compiler profile, as to prevent unnecessarily checking out `src/llvm-project` with `"if-unchanged"`.

Urgau force-pushed the fix-llvm-if-unchanged branch from 13522e7 to 5f367bb Compare September 6, 2024 15:50

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 6, 2024

matthiaskrgr mentioned this pull request Sep 8, 2024

Rollup of 7 pull requests #130100

Closed

matthiaskrgr mentioned this pull request Sep 8, 2024

Rollup of 7 pull requests #130109

Closed

workingjubilee mentioned this pull request Sep 9, 2024

Rollup of 19 pull requests #130127

Closed

workingjubilee mentioned this pull request Sep 10, 2024

Rollup of 14 pull requests #130177

Closed

workingjubilee mentioned this pull request Sep 10, 2024

Rollup of 11 pull requests #130179

Merged

bors merged commit 5c91cc5 into rust-lang:master Sep 10, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 10, 2024

Urgau deleted the fix-llvm-if-unchanged branch September 10, 2024 10:22

Zalathar mentioned this pull request Sep 15, 2024

set download-ci-llvm = true by default on "library" and "tools" profiles #130202

Merged

onur-ozkan mentioned this pull request Sep 18, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `download-ci-llvm=true` in the default compiler config #129473

use `download-ci-llvm=true` in the default compiler config #129473

Urgau commented Aug 23, 2024 •

edited

Loading

rustbot commented Aug 23, 2024

Urgau commented Aug 23, 2024

onur-ozkan commented Aug 23, 2024

Urgau commented Aug 23, 2024

onur-ozkan commented Aug 24, 2024 •

edited

Loading

onur-ozkan commented Aug 24, 2024

Urgau commented Aug 25, 2024 •

edited

Loading

onur-ozkan commented Aug 25, 2024

Zalathar commented Aug 27, 2024 •

edited

Loading

Urgau commented Aug 29, 2024

onur-ozkan commented Aug 29, 2024

Urgau commented Aug 29, 2024

onur-ozkan commented Aug 29, 2024

Urgau commented Aug 29, 2024

rustbot commented Sep 5, 2024

Urgau commented Sep 5, 2024

Zalathar commented Sep 6, 2024

onur-ozkan left a comment

onur-ozkan commented Sep 6, 2024 •

edited

Loading

Zalathar commented Sep 6, 2024 •

edited

Loading

bors commented Sep 6, 2024

Urgau commented Sep 6, 2024

bors commented Sep 6, 2024

use download-ci-llvm=true in the default compiler config #129473

use download-ci-llvm=true in the default compiler config #129473

Conversation

Urgau commented Aug 23, 2024 • edited Loading

rustbot commented Aug 23, 2024

Urgau commented Aug 23, 2024

onur-ozkan commented Aug 23, 2024

Urgau commented Aug 23, 2024

onur-ozkan commented Aug 24, 2024 • edited Loading

onur-ozkan commented Aug 24, 2024

Urgau commented Aug 25, 2024 • edited Loading

onur-ozkan commented Aug 25, 2024

Zalathar commented Aug 27, 2024 • edited Loading

Urgau commented Aug 29, 2024

onur-ozkan commented Aug 29, 2024

Urgau commented Aug 29, 2024

onur-ozkan commented Aug 29, 2024

Urgau commented Aug 29, 2024

rustbot commented Sep 5, 2024

Urgau commented Sep 5, 2024

Zalathar commented Sep 6, 2024

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan commented Sep 6, 2024 • edited Loading

Zalathar commented Sep 6, 2024 • edited Loading

bors commented Sep 6, 2024

Urgau commented Sep 6, 2024

bors commented Sep 6, 2024

use `download-ci-llvm=true` in the default compiler config #129473

use `download-ci-llvm=true` in the default compiler config #129473

Urgau commented Aug 23, 2024 •

edited

Loading

onur-ozkan commented Aug 24, 2024 •

edited

Loading

Urgau commented Aug 25, 2024 •

edited

Loading

Zalathar commented Aug 27, 2024 •

edited

Loading

onur-ozkan commented Sep 6, 2024 •

edited

Loading

Zalathar commented Sep 6, 2024 •

edited

Loading