Skip to content

Use the bootstrap compiler for x check on bootstrap tools #139170

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use the bootstrap compiler for x check on bootstrap tools #139170

wants to merge 1 commit into from
+93 −25

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Mar 31, 2025 •
edited
Loading

The existing implementation of tool_check_step! uses the specified --stage compiler, assumes Mode::ToolRustc, and unconditionally performs a check build of the compiler before checking the tool.

That behaviour makes sense for tools like rustdoc, but is inappropriate for checking things like bootstrap itself, or compiletest. Bootstrap tools are built the bootstrap compiler, and should be checked with that same compiler. And they have no reason to check the compiler crates.

This PR therefore adds support for specifying a mode in tool_check_step!, and using that mode to determine which compiler to use.

@rustbot
Copy link
Collaborator

rustbot commented Mar 31, 2025

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Mar 31, 2025
Kobzol
Kobzol reviewed Mar 31, 2025
View reviewed changes
src/bootstrap/src/core/build_steps/check.rs Outdated
}
Mode::ToolStd => {
compiler = builder.compiler(0, host);
builder.ensure(Std::new(target).with_compiler(compiler));
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have similar macros for building tools. Why can these work without with_compiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compile::Std step takes an explicit compiler argument, so the equivalent of with_compiler is built into the constructor.

The check::Std step does not, and is hardcoded to use the top_stage compiler unless overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I added with_compiler, instead of reworking the check::Std::new constructor, was that this PR had already grown in scope beyond my original intentions (I just wanted to be able to register non-wasteful check builds for other bootstrap tools), and I didn't want to have to go down the extra rabbit hole of redesigning the check::Std step.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Yeah, that's a common situation in bootstrap, when you touch something, it's often immediately time for refactoring of the previous state :D

@onur-ozkan
Copy link
Member

Can't we skip ensuring Std for bootstrap tools?

@Zalathar
Copy link
Contributor Author

Zalathar commented Apr 1, 2025

Added comments to the ToolBootstrap and ToolStd arms, and clarified that we are ensuring check::Std and check::Rustc (diff).

@Zalathar
Copy link
Contributor Author

Zalathar commented Apr 1, 2025

Can't we skip ensuring Std for bootstrap tools?

With this PR, most bootstrap tools use Mode::ToolBootstrap, so the call to .ensure(check::Std) does not occur.

We only do a check build of std for tools explicitly marked with Mode::ToolStd. This is needed so that a check build of compiletest can find check-artifacts for libtest, for example.

@bors
Copy link
Collaborator

bors commented Apr 8, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

Assignees

@onur-ozkan onur-ozkan

Labels
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)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Zalathar @rustbot @onur-ozkan @bors @Kobzol