-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Reduce usage of compiler_for
in bootstrap
#145310
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
Conversation
This PR modifies If appropriate, please update |
This comment has been minimized.
This comment has been minimized.
b4c6c8e
to
39ff9a3
Compare
☔ The latest upstream changes (presumably #145295) made this pull request unmergeable. Please resolve the merge conflicts. |
39ff9a3
to
4d9cbfd
Compare
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.
Thanks. The staging reasoning looks right to me. You can r=me once the tracing PR merges then rebase, as I think there might be a merge conflict there.
@@ -143,7 +144,7 @@ impl Step for Std { | |||
skip_all, | |||
fields( | |||
target = ?self.target, | |||
compiler = ?self.compiler, | |||
build_compiler = ?self.build_compiler, |
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: I think this will merge-conflict with #145340
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.
Actually, why should it? That only modified check and this modifies compile 🤔
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.
Ah, you probably meant the #[instrument]
removal PR.
@rustbot author |
☔ The latest upstream changes (presumably #145407) made this pull request unmergeable. Please resolve the merge conflicts. |
4d9cbfd
to
f2c2d3e
Compare
Rebased. @bors r=jieyouxu |
Rollup of 11 pull requests Successful merges: - #144210 (std: thread: Return error if setting thread stack size fails) - #145310 (Reduce usage of `compiler_for` in bootstrap) - #145311 (ci: clean windows disk space in background) - #145340 (Split codegen backend check step into two and don't run it with `x check compiler`) - #145408 (Deduplicate -L search paths) - #145412 (Windows: Replace `GetThreadId`+`GetCurrentThread` with `GetCurrentThreadId`) - #145413 (bootstrap: Reduce dependencies) - #145426 (Fix typos in bootstrap.example.toml) - #145430 (Fix wrong spans with external macros in the `dropping_copy_types` lint) - #145431 (Enhance UI test output handling for runtime errors) - #145448 (Autolabel `src/tools/{rustfmt,rust-analyzer}` changes with `T-{rustfmt,rust-analyzer}`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145310 - Kobzol:compiler-for-revamp, r=jieyouxu Reduce usage of `compiler_for` in bootstrap While working on refactoring/fixing `dist` steps, I realized that `build.full-bootstrap` does much more than it should, and that it its documentation is wrong. It seems that the main purpose of this option should be to enable/disable stdlib/compiler uplifting (https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Purpose.20of.20.60build.2Efull-bootstrap.60/with/533985624), but currently it also affects staging, or more precisely which compiler will be used to build selected steps, because this option is used in the cursed `compiler_for` function. I would like to change the option it so that it *only* affects uplifting, and doesn't affect stage selection, which I (partially) did in this PR. I removed the usage of `compiler_for` from the `Std` and `Rustc` steps, and explicitly implemented uplifting, without going through `compiler_for`. The only remaining usages of `compiler_for` are in dist steps (which I'm currently refactoring, will send a PR later) and test steps (which I will take a look at after dist). After that we can finally remove the function. I tried to document the case when uplifting was happening during cross-compilation, which was very implicit before. I also did a slight change in the uplifting logic for rustc when cross-compiling. Before, we would attempt to uplift a stage1 rustc, but that is not really a thing when cross-compiling. r? `@jieyouxu`
While working on refactoring/fixing
dist
steps, I realized thatbuild.full-bootstrap
does much more than it should, and that it its documentation is wrong. It seems that the main purpose of this option should be to enable/disable stdlib/compiler uplifting (https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Purpose.20of.20.60build.2Efull-bootstrap.60/with/533985624), but currently it also affects staging, or more precisely which compiler will be used to build selected steps, because this option is used in the cursedcompiler_for
function.I would like to change the option it so that it only affects uplifting, and doesn't affect stage selection, which I (partially) did in this PR. I removed the usage of
compiler_for
from theStd
andRustc
steps, and explicitly implemented uplifting, without going throughcompiler_for
.The only remaining usages of
compiler_for
are in dist steps (which I'm currently refactoring, will send a PR later) and test steps (which I will take a look at after dist). After that we can finally remove the function.I tried to document the case when uplifting was happening during cross-compilation, which was very implicit before. I also did a slight change in the uplifting logic for rustc when cross-compiling. Before, we would attempt to uplift a stage1 rustc, but that is not really a thing when cross-compiling.
r? @jieyouxu