-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Enforce in bootstrap that build must have stage at least 1 #142581
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
base: master
Are you sure you want to change the base?
Conversation
|
This actually interacts with cross-compilation, as I mentioned here:
You need to pass |
This comment has been minimized.
This comment has been minimized.
This PR modifies If appropriate, please update This PR modifies If appropriate, please update |
…ompiler` path It was confusing that the `Rustc` step was invoked twice, once with all crates and once with no crates (even though both of these mean the same thing).
49ed3f9
to
6bf0ab7
Compare
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
// Now check that the selected stage makes sense, and if not, print a warning and end | ||
match (config.stage, &config.cmd) { | ||
(0, Subcommand::Build) => { | ||
eprintln!("WARNING: cannot build anything on stage 0. Use at least stage 1."); | ||
exit!(1) | ||
} | ||
_ => {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: if we warn on the no-op that is ./x build --stage=0
, what about ./x check --stage=0
? I guess for the check case, we can't do that yet without untying bootstrap tool checking from staging?
Also, does this reject e.g. ./x build src/tools/opt-dist --stage=0
?
EDIT: indeed,
#[test]
#[should_panic]
fn build_bootstrap_tool_stage_0() {
insta::assert_snapshot!(get_steps("opt-dist", "build", Some(0)), @"");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check is a whole another beast, I'm working on a PR for it, but the individual bootstrap kinds (build/check/etc.) are mostly separate in the stage handling, so I will do it piece by piece.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I'm planning for check --stage 0
to also be an error.
// If only `compiler` was passed, do not run this step. | ||
// Instead the `Assemble` step will take care of compiling Rustc. | ||
if run.builder.paths == vec![PathBuf::from("compiler")] { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: this kinda feels iffy, in that I feel like the path filtering/handling logic that lead to
Rustc
step was invoked twice, once with all crates and once with no crates (even though both of these mean the same thing).
makes this kinda weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love Rustc
and Assemble
to be just one step (to avoid things like this), but I guess that it's sort of an optimizatoin, Rustc
allows building only selected crates, and doesn't involve building a ton of other things that you might not want for a quick build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that does make sense.
I'm generally in favor of imposing more discipline (or more order, for that matter) on build staging as is done in this PR. Not super sure about rejecting Maybe r? @Mark-Simulacrum |
That depends on how we decide #t-infra/bootstrap > Proposal to cleanup stages and steps after the redesign. The bootstrap tools don't really participate in staging (unless you're cross-compiling, but we don't even support cross-compiling these tools, as they are host tools, essentially). So in theory we could allow them to be built with whatever |
☔ The latest upstream changes (presumably #142685) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR is a step towards https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Proposal.20to.20cleanup.20stages.20and.20steps.20after.20the.20redesign/with/523586917. It's very hard or me to make self-contained changes to bootstrap at this moment, so this PR kind of does several things:
Std::new
in bootstrap, and replace it with aBuilder::std
method (similar toBuilder::compiler
). This is mostly to remove executions of theStep
step for stage 0, which doesn't make a lot of sense; I'd like to ideally have the invariant that when a step is invoked, it actually builds or does something. Eventually, I'd like for everything to go throughBuilder::std
. (Note: I'm not totally married to this idea, if you don't like it, we can remove it from this PR. I mostly did it right now to remove stage 0 std steps from snapshot tests, which shouldn't be there, but we can also filter them out in a different way)x build compiler
, only theAssemble
root level step will be invoked, and not theRustc
step. Before, both were invoked, which actually ranRustc
twice, once with allcrates
filled, and once with no crates (but both actually represent the same situation). Since theRustc::make_run
step actually requests a compile that is one stage below it, this actually madebuild compiler --stage 0
work, which we don't want to have anymore.build
commands are always on stage>=1
. If you try tobuild
anything on stage 0, it will print a warning and exit bootstrap. This follows the intuition from the new staging rules after the stage redesign; artifacts that are "stage 0" come outside of bootstrap, and we can't really build something for which we don't have source (although we can still test it, but that's for another PR).Now the logic for build should be quite simple. For pretty much everything except for
Std
, you first use the stage0 compiler to build stage 1. Then you can build a stage 2 using the previously built stage 1 (and then you can continue to stage 3 etc.). And that's it. The default build stage for everything is 1 (modulo download-ci-rustc, but that's a separate can of worms).The snapshot test infra isn't super nice at the moment, as one of next steps I want to create some simple Builder pattern that will allow us to configure the bootstrap invocations in a more "forward-compatible" way (e.g. now it's not possible to modify the config passed to
configure_with_args
).There are some things not yet fully resolved for build stage 0:
ModeRustc
tool, even though it doesn't really have to be, it is buildable with the stage0 compileropt-dist
,build-manifest
etc.) are still called stage0 tools, and in the bootstrap output it says something like "stage 0 rustc builds stage 0 opt-dist". Which is a bit weird, but functionally there's no difference, it's just a slightly inconsistent output. We still haven't decided if we should make these tools ignore staging altogether (which is IMO the right choice) or if we want to allow building stage 1/2/3/... bootstrap tools.r? @jieyouxu