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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 3 commits into from
+74 −13

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: Marked as 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: Marked as 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
Open
@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 commented Nov 17, 2024
View reviewed changes
src/bootstrap/src/core/config/config.rs Outdated
Comment on lines 1779 to 1782
if matches!(rustc_debug_assertions, Some(true))
|| (matches!(debug, Some(true)) && rustc_debug_assertions.is_none())
{
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like this but this is the simplest thing I could think of without fixing our config layering

@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
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Nov 17, 2024
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from 5269c84 to 4fd0d92 Compare November 17, 2024 18:54
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu
Copy link
Member Author

Yeah, like I suspected, this is going to be problematic.

@rustbot blocked (https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/CI.20jobs.20with.20.60--enable-debug-assertions.60.20.2B.20CI.20rustc)

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. 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 4fd0d92 to c1e065a Compare November 18, 2024 11:54
@jieyouxu
Copy link
Member Author

jieyouxu commented Nov 18, 2024 •
edited
Loading

@bors rollup=never (modifies CI and bootstrap in a significant way, likely to fail)

@jieyouxu
Copy link
Member Author

This will also need someone from infra to double-check... so r? @Mark-Simulacrum (or @Kobzol or infra)

@rustbot rustbot assigned Mark-Simulacrum and unassigned clubby789 Nov 18, 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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Nov 18, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from c1e065a to ef5d82e Compare November 18, 2024 12:12
@jieyouxu
Copy link
Member Author

For mingw-check, AFAICT this is intended for use as a fast-rejection job, so for this specifically I set NO_DEBUG_ASSERTIONS=1, while for x86_64-gnu-tools I removed FORCE_CI_RUSTC.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu
Copy link
Member Author

Hm this approach is problematic. I need to change the logic such that rust.debug-assertions=true inhibits download-rustc and force a fresh build.

@bors
Copy link
Contributor

bors commented Nov 20, 2024

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

@jieyouxu
Revert global default for download-rustc to false
da09d24 
And only use `download-rustc = "if-unchanged"` for the `library` and
`tools` profile defaults.
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from ef5d82e to de92e3e Compare November 21, 2024 11:19
@jieyouxu jieyouxu changed the title 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 Use download-rustc=false global default, if-unchanged for tools and library profiles, and make rust.debug-assertions=true inhibit downloading CI rustc Nov 21, 2024
@MarcoIeni
Copy link
Member

cc @MarcoIeni since you're working on improving CI job times, sorry, this will definitely regress some CI job times

Thanks for letting me know! I will have a look at the job timings after this one is merged to make sure the auto build has reasonable timing 👍

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from de92e3e to 4999c0f Compare November 21, 2024 12:22
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu
Skip downloading CI rustc if rust.debug-assertions = true
5563bf5 
Warn if `download-rustc` used with `rust.debug-assertions` as alt rustc
builds currently do not have rustc debug assertions enabled.
@jieyouxu
CI: don't force CI rustc for x86_64-gnu-tools
46ba361 
CI alt rustc builds do not have rustc debug assertions currently.
@jieyouxu jieyouxu force-pushed the download-rustc-default-only-for-tools branch from 4999c0f to 46ba361 Compare November 21, 2024 12:34
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

@Mark-Simulacrum Mark-Simulacrum

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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
No milestone
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
8 participants
@jieyouxu @rustbot @rust-log-analyzer @bors @MarcoIeni @clubby789 @Kobzol @Mark-Simulacrum