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

Use download-rustc=false global default, if-unchanged for tools and library profiles, and make rust.debug-assertions=true inhibit downloading CI rustc #133068

Merged
merged 4 commits into from
Nov 24, 2024
Merged

Use download-rustc=false global default, if-unchanged for tools and library profiles, and make rust.debug-assertions=true inhibit downloading CI rustc #133068

merged 4 commits into from
Nov 24, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Nov 15, 2024 •
edited
Loading

  • Use download-rustc = false as global default.
    • Use download-rustc = 'if-unchanged' for tools and library profiles.
  • Make rust.debug-assertions = true inhibit downloading CI rustc because alt rustc builds do not yet have rustc debug assertions enabled.

Fixes #133132.

cc discussions: https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Bootstrap.20breakage
compiler contributors poll: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/.60download-rustc.20.3D.20'if-unchanged'.60.20for.20.60compiler.60.20profile.3F/near/481877253
library contributors poll: https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/.60download-rustc.20.3D.20.22if-unchanged.22.60.20default.20for.20libs.20profile.3F/near/482607011
cc https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/When.20is.20rustc.20built.20with.20debug.20assertions.3F

cc @marcoieni since you're working on improving CI job times, sorry, this will definitely regress some CI job times because we're probably lying to ourselves that CI rustc had debug assertions for some time 😅

cc @onur-ozkan for FYI, but since you're on vacation (sorry for the ping),

r? @Kobzol (I think you have a bit more context than other bootstrap reviewers?)

@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 Nov 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 15, 2024

This PR modifies src/bootstrap/defaults.

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.

This PR modifies config.example.toml.

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

@jieyouxu
Copy link
Member Author

I think the motivation for download-rustc = 'if-unchanged' for library profile is to prepare for the staging redesign, not 100% certain.

@jieyouxu
Copy link
Member Author

Waiting for libs contributors poll to determine if we want to set the library profile to use if-unchanged.
@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2024
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from 2e477ae to 92c6f0f Compare November 15, 2024 14:04
@jieyouxu jieyouxu changed the title Revert global default for download-rustc to false and only use if-unchanged for tools profile default Revert global default for download-rustc to false and only use if-unchanged for tools and library profile defaults Nov 15, 2024
@jieyouxu
Copy link
Member Author

This seems mostly uncontroversial to use if-unchanged for library profile after improved detection logic in #131831.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 16, 2024
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from 92c6f0f to 727183f Compare November 17, 2024 14:38
@jieyouxu jieyouxu changed the title Revert global default for download-rustc to false and only use if-unchanged for tools and library profile defaults Use download-rustc=false global default, if-unchanged for tools and library profiles, and make download-rustc = true | if-unchanged incompatible with rust.debug-assertions=true Nov 17, 2024
@jieyouxu jieyouxu 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2024
@jieyouxu jieyouxu mentioned this pull request Nov 17, 2024
Merged
@jieyouxu
Copy link
Member Author

Maybe r? @clubby789 (Onur is on vacation and Jakub/Mark/Albert is busy AFAIK)

@rustbot rustbot assigned clubby789 and unassigned Kobzol Nov 17, 2024
@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2024
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from 727183f to b05d37e Compare November 17, 2024 14:52
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from b05d37e to 9f10635 Compare November 17, 2024 14:54
clubby789
clubby789 approved these changes Nov 17, 2024
View reviewed changes
Copy link
Contributor

@clubby789 clubby789 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, r=me with typo fix

EDIT: There's also a leftover 'for' a couple of lines above

src/bootstrap/src/core/config/config.rs Outdated Show resolved Hide resolved
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from 9f10635 to 1c4940e Compare November 17, 2024 15:24
@jieyouxu
Copy link
Member Author

Only change is typo fix (diff).

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu
Copy link
Member Author

Review comments addressed, this has no other significant changes since last review, so
@bors r=@clubby789 rollup=never (significant bootstrap/CI changes)

@bors
Copy link
Contributor

bors commented Nov 23, 2024

📌 Commit f7f8ad9 has been approved by clubby789

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 Nov 23, 2024
@jieyouxu
Copy link
Member Author

Actually I should wait for PR CI.
@bors r-

@bors 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 Nov 23, 2024
@jieyouxu
Copy link
Member Author

@bors r=@clubby789 rollup=never

@bors
Copy link
Contributor

bors commented Nov 24, 2024

📌 Commit f7f8ad9 has been approved by clubby789

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 24, 2024
@bors
Auto merge of rust-lang#133068 - jieyouxu:download-rustc-default-only…
c16c0f6 
…-for-tools, r=clubby789

Use `download-rustc=false` global default, `if-unchanged` for tools and library profiles, and make `rust.debug-assertions=true` inhibit downloading CI rustc

- Use `download-rustc = false` as global default.
    - Use `download-rustc = 'if-unchanged'` for tools and library profiles.
- Make `rust.debug-assertions = true` inhibit downloading CI rustc because alt rustc builds do not yet have rustc debug assertions enabled.

Fixes rust-lang#133132.

cc discussions: https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Bootstrap.20breakage
compiler contributors poll: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/.60download-rustc.20.3D.20'if-unchanged'.60.20for.20.60compiler.60.20profile.3F/near/481877253
library contributors poll: https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/.60download-rustc.20.3D.20.22if-unchanged.22.60.20default.20for.20libs.20profile.3F/near/482607011
cc https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/When.20is.20rustc.20built.20with.20debug.20assertions.3F

cc `@MarcoIeni` since you're working on improving CI job times, sorry, this will definitely regress some CI job times because we're probably lying to ourselves that CI rustc had debug assertions for some time 😅

cc `@onur-ozkan` for FYI, but since you're on vacation (sorry for the ping),

r? `@Kobzol` (I *think* you have a bit more context than other bootstrap reviewers?)
@bors
Copy link
Contributor

bors commented Nov 24, 2024

⌛ Testing commit f7f8ad9 with merge c16c0f6...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Nov 24, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 24, 2024
@jieyouxu
Copy link
Member Author

@bors retry (spurious miri failure)

@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 Nov 24, 2024
@jieyouxu jieyouxu mentioned this pull request Nov 24, 2024
Closed
@bors
Copy link
Contributor

bors commented Nov 24, 2024

⌛ Testing commit f7f8ad9 with merge 481b5fa...

@jieyouxu jieyouxu added the CI-ABA-ptr-provenance-lockless-queue-fail CI spurious failure: related to #121950 label Nov 24, 2024
@bors
Copy link
Contributor

bors commented Nov 24, 2024

☀️ Test successful - checks-actions
Approved by: clubby789
Pushing 481b5fa to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 24, 2024
@bors bors merged commit 481b5fa into rust-lang:master Nov 24, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 24, 2024
@jieyouxu jieyouxu deleted the download-rustc-default-only-for-tools branch November 24, 2024 19:16
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (481b5fa): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.2% [1.2%, 1.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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

Cycles

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

Binary size

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

Bootstrap: 797.067s -> 798.184s (0.14%)
Artifact size: 336.23 MiB -> 336.25 MiB (0.01%)

@jieyouxu jieyouxu mentioned this pull request Dec 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clubby789 clubby789 clubby789 approved these changes

Assignees

@clubby789 clubby789

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc CI-ABA-ptr-provenance-lockless-queue-fail CI spurious failure: related to #121950 merged-by-bors This PR was explicitly merged by bors. 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.

rust.debug-assertions = true should be incompatible with rust.download-rustc = true | 'if-unchanged' atm
9 participants
@jieyouxu @rustbot @rust-log-analyzer @bors @marcoieni @clubby789 @rust-timer @Kobzol @Mark-Simulacrum