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.

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

force current stage when --stage is expilicitly used #118999

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

onur-ozkan
Copy link
Member

Allows users to build std with stage2 compiler without using full-bootstrap.

Fixes #118233
Fixes #118918

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2023

r? @clubby789

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

@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 Dec 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2023

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

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs and change-id in config.example.toml.

Allows users to build std with stage2 compiler without using `full-bootstrap`.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

Do you have any concerns on this? @Mark-Simulacrum

@onur-ozkan
Copy link
Member Author

r? Mark-Simulacrum

@rustbot rustbot assigned Mark-Simulacrum and unassigned clubby789 Dec 31, 2023
@@ -992,7 +992,9 @@ impl<'a> Builder<'a> {
host: TargetSelection,
target: TargetSelection,
) -> Compiler {
if self.build.force_use_stage2(stage) {
if self.config.explicit_stage {
Copy link
Member

Choose a reason for hiding this comment

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

This condition feels iffy to me as being first -- if rustbuild wants to force some stage, shouldn't we respect that? Or should this condition be made part of force_use_stage1/2?

It seems like perhaps those functions should just be no-ops with explicit stage set?

Or perhaps more/different work is warranted; it seems odd to me that --stage opts-in to behavior different than what the default is. If we think the right thing to do is to build stage 2 when it's requested on the CLI, shouldn't we do the same when it's the implicit default? (IIRC, we made a change to make stage 1 the default in most modes, so presumably this wouldn't be a change in default behavior for most people?)

Copy link
Member Author

@onur-ozkan onur-ozkan Jan 1, 2024

Choose a reason for hiding this comment

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

This condition feels iffy to me as being first -- if rustbuild wants to force some stage, shouldn't we respect that?

Not if the stage was explicitly requested by the user, in my opinion.

From #118233 (comment)

I don't want to compile the compiler three times. I just want the standard library built with the stage that I explicitly specified.

This seems like a very reasonable request for adding an interface that allows users to use their desired stage when they want. I am aware of that this use case is quite rare, but we should still provide a way for users to enforce their desired stage.

If we think the right thing to do is to build stage 2 when it's requested on the CLI, shouldn't we do the same when it's the implicit default?

This is very true. Maybe we should add another flag to bootstrap something like --force-stage with explaining how its different than --stage one?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the question I'm asking is why do we need force-stage separate from just stage? I.e., can we just make the behavior of adjusting the stage (--stage N or default stage N) the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are several cases where force_use_stage1/2 make sense. By default they should be enabled as they can significantly reduce the build time. But for certain cases we should also allow developers to disable/ignore them.

@Mark-Simulacrum Mark-Simulacrum 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 Dec 31, 2023
@Dylan-DPC Dylan-DPC 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 Aug 2, 2024
@Mark-Simulacrum
Copy link
Member

@onur-ozkan is this still relevant, or has subsequent work replaced it? I'll need to page it back into cache but I think the open comment thread is still something I haven't had my mind changed on yet...

@onur-ozkan
Copy link
Member Author

I need to check this again and see if we can improve the stage handling logic without adding explicit_stage field.

@rustbot author

@rustbot rustbot 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 Aug 4, 2024
@alex-semenyuk alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2024
@bors
Copy link
Contributor

bors commented Oct 21, 2024

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

@onur-ozkan onur-ozkan marked this pull request as draft October 21, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
7 participants