Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Sep 15, 2024
Merged

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

merged 2 commits into from
Sep 15, 2024

Conversation

onur-ozkan
Copy link
Member

It's very rare for developers to need to modify LLVM, so "if-unchanged" isn't a good default for "tools" and "library" profiles since it fetches the LLVM submodule to track changes.

@onur-ozkan
force ci-llvm by default on library and tools profiles
ed89d9b 
It's very rare for developers to need to modify LLVM,
so "if-unchanged" isn't a good default since it fetches
the LLVM submodule to track changes.

Signed-off-by: onur-ozkan <[email protected]>
@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2024

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 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 Sep 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2024

This PR modifies src/bootstrap/defaults.

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

@Kobzol
Copy link
Contributor

Kobzol commented Sep 10, 2024

Not sure if this needs wider consensus from the team, but I think that it makes sense, especially since it's now also the default for the compiler profile.

@ChrisDenton
Copy link
Member

For whatever it's worth, I primarily work on libs stuff and long ago set it to true manually.

@Mark-Simulacrum
Copy link
Member

Huh, I guess I'm surprised we need to checkout the module to track changes... that seems like it could be fixed.

But I agree that this seems fairly reasonable for these two profiles.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2024

📌 Commit b5d69ba has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors 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 Sep 14, 2024
@Zalathar
Copy link
Contributor

Huh, I guess I'm surprised we need to checkout the module to track changes... that seems like it could be fixed.

There was a proposed fix in (the original version of) #129473, but that approach was eventually rejected in favour of making download-ci-llvm = true behave more like the old if-available, and making that the default instead.

This was referenced Sep 15, 2024
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2024
@bors
Auto merge of rust-lang#130379 - Zalathar:rollup-wpmcnql, r=Zalathar
584cf23 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#130042 (properly handle EOF in BufReader::peek)
 - rust-lang#130061 (Add `NonNull` convenience methods to `Box` and `Vec`)
 - rust-lang#130202 (set `download-ci-llvm = true` by default on "library" and "tools" profiles)
 - rust-lang#130214 (MaybeUninit::zeroed: mention that padding is not zeroed)
 - rust-lang#130353 (Make some lint doctests compatible with `--stage=0`)
 - rust-lang#130370 (unstable-book: `trait_upcasting` example should not have `#![allow(incomplete_features)]`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2024
@bors
Auto merge of rust-lang#130379 - Zalathar:rollup-wpmcnql, r=Zalathar
bc486f3 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#130042 (properly handle EOF in BufReader::peek)
 - rust-lang#130061 (Add `NonNull` convenience methods to `Box` and `Vec`)
 - rust-lang#130202 (set `download-ci-llvm = true` by default on "library" and "tools" profiles)
 - rust-lang#130214 (MaybeUninit::zeroed: mention that padding is not zeroed)
 - rust-lang#130353 (Make some lint doctests compatible with `--stage=0`)
 - rust-lang#130370 (unstable-book: `trait_upcasting` example should not have `#![allow(incomplete_features)]`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 36ee852 into rust-lang:master Sep 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2024
@rust-timer
Unrolled build for rust-lang#130202
64be942 
Rollup merge of rust-lang#130202 - onur-ozkan:force-ci-llvm-on-default-profiles, r=Mark-Simulacrum

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

It's very rare for developers to need to modify LLVM, so "if-unchanged" isn't a good default for "tools" and "library" profiles since it fetches the LLVM submodule to track changes.
@onur-ozkan onur-ozkan deleted the force-ci-llvm-on-default-profiles branch September 15, 2024 09:20
@RalfJung
Copy link
Member

since it fetches the LLVM submodule to track changes.

That doesn't seem right? I have download-ci-llvm = "if-unchanged" and I do not have the LLVM submodule checked out, and that seems to work just fine. I guess that's because I also have submodules = false, but this seems to indicate that even with submodules = true we don't have to force the LLVM submodule?

@RalfJung
Copy link
Member

Or does that just mean that true and if-unchanged are equivalent in my setup?

In that case I think even most compiler contributors want true. We shouldn't force people to checkout the LLVM submodule as that is huge, and only needed if you're working on the LLVM bindings / bumping LLVM versions, which is very rare.

@onur-ozkan onur-ozkan mentioned this pull request Sep 18, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2024
@bors
Auto merge of rust-lang#130529 - onur-ozkan:better-ci-llvm-default, r…
13a5097 
…=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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
8 participants
@onur-ozkan @rustbot @Kobzol @ChrisDenton @Mark-Simulacrum @bors @Zalathar @RalfJung