Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/bootstrap/src/core/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,7 +977,7 @@ impl<'a> Builder<'a> {

/// Similar to `compiler`, except handles the full-bootstrap option to
/// silently use the stage1 compiler instead of a stage2 compiler if one is
/// requested.
/// requested. This behaviour is ignored if `--stage` is explicitly set.
///
/// Note that this does *not* have the side effect of creating
/// `compiler(stage, host)`, unlike `compiler` above which does have such
Expand All @@ -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
Contributor Author

@onur-ozkan onur-ozkan Jan 1, 2024 •
edited
Loading

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
Contributor 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.

self.compiler(stage, host)
} else if self.build.force_use_stage2(stage) {
self.compiler(2, self.config.build)
} else if self.build.force_use_stage1(stage, target) {
self.compiler(1, self.config.build)
Expand Down
3 changes: 3 additions & 0 deletions src/bootstrap/src/core/config/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ pub struct Config {

pub on_fail: Option<String>,
pub stage: u32,
/// Indicates whether `--stage` was explicitly used or not.
pub explicit_stage: bool,
pub keep_stage: Vec<u32>,
pub keep_stage_std: Vec<u32>,
pub src: PathBuf,
Expand Down Expand Up @@ -1216,6 +1218,7 @@ impl Config {
config.incremental = flags.incremental;
config.dry_run = if flags.dry_run { DryRun::UserSelected } else { DryRun::Disabled };
config.dump_bootstrap_shims = flags.dump_bootstrap_shims;
config.explicit_stage = flags.stage.is_some();
config.keep_stage = flags.keep_stage;
config.keep_stage_std = flags.keep_stage_std;
config.color = flags.color;
Expand Down