Skip to content

Add ToolTarget to bootstrap #143641

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add ToolTarget to bootstrap #143641

wants to merge 7 commits into from

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Jul 8, 2025

Oh, you thought I'm done with refactoring bootstrap tools? Na-ah, think again! After the failure of #143581, ToolTarget is back with a vengeance. This time, I implemented the test changes and tool cleanups without forcing these tools to be built with the stage0 compiler.

There are still some small wins though, LlvmBitcodeLinker now starts at stage 1, and not stage 2. Cargo should also be ported to this new mode, but I'm leaving that for a follow-up PR.

Hopefully X-th time's the charm 🤞

r? @jieyouxu

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 8, 2025
@Kobzol Kobzol marked this pull request as ready for review July 9, 2025 09:46
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2025
@Kobzol
Copy link
Member Author

Kobzol commented Jul 9, 2025

#142357 was merged, so undrafting.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

I need to sit on this scheme a bit, I still find it a bit confusing.

(I was reviewing this commit-by-commit, but now I realize I should've looked at the overall diff)

@@ -365,6 +365,17 @@ pub(crate) fn get_tool_rustc_compiler(
builder.compiler(target_compiler.stage.saturating_sub(1), builder.config.host_target)
}

/// Returns a compiler that is able to compile a `ToolTarget` tool for the given `target`.
pub(crate) fn get_tool_target_compiler(builder: &Builder<'_>, target: TargetSelection) -> Compiler {
todo!("FIX");
Copy link
Member

Choose a reason for hiding this comment

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

Problem [STAGING 0/2]: leftover todo!()? 🤔

Comment on lines 374 to 375
// FIXME: should this be builder.top_stage to avoid rebuilds?
builder.compiler(1, target)
Copy link
Member

Choose a reason for hiding this comment

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

Discussion: hm, for dist tools (cross-compiled for target platform), does the final distributed tool need to be built by the stage 2 compiler? Though I imagine the differences of tools produced by $\mathrm{rustc}_{1}$ and $\mathrm{rustc}_{2}$ are ABI-wise, but since they are not intended to be linked against nor depend on unstable features, that should be fine?

Comment on lines 371 to 372
if builder.host_target == target {
builder.compiler(0, builder.host_target)
Copy link
Member

Choose a reason for hiding this comment

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

Question [STAGING 1/2]: wait, does this actually agree with the comment

While we could compile them using the stage0 compiler when not cross-compiling, we instead
use the in-tree compiler (and std) to build them, so that we can ship e.g. std security
fixes and avoid depending fully on stage0 for the artifacts that we ship.

are we not using the stage 0 compiler (not in-tree!) when not cross-compiling?

Comment on lines 368 to 376
/// Returns the smallest stage compiler that is able to compile code for the given `target`.
pub(crate) fn get_compiler_for_target(builder: &Builder<'_>, target: TargetSelection) -> Compiler {
let compiler = if builder.host_target == target {
builder.compiler(0, builder.host_target)
} else {
// FIXME: should this be builder.top_stage to avoid rebuilds?
builder.compiler(1, target)
}
builder.compiler(1, builder.host_target)
};
builder.std(compiler, target);
compiler
Copy link
Member

Choose a reason for hiding this comment

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

Question [STAGING 2/2]: oh, it's changed in this commit. But wait, doesn't this still disagree with the comment re. building ToolTarget tools with the in-tree compiler and std, even when not cross-compiling?

Comment on lines +368 to +377
/// Determines how to build a `ToolTarget`, i.e. which compiler should be used to compile it.
/// The compiler stage is automatically auto-bumped if we need to cross-compile a stage 1 tool.
pub enum ToolTargetBuildMode {
/// Build the tool using rustc that corresponds to the selected CLI stage.
Build(TargetSelection),
/// Build the tool so that it can be attached to the sysroot of the passed compiler.
/// Since we always dist stage 2+, the compiler that builds the tool in this case has to be
/// stage 1+.
Dist(Compiler),
}
Copy link
Member

Choose a reason for hiding this comment

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

Discussion [STAGING 3/2]: oh I see, this is exactly what I was curious about.

Comment on lines 384 to 397
let (target, min_build_compiler_stage) = match mode {
ToolTargetBuildMode::Build(target) => {
assert!(builder.top_stage > 0);
(target, builder.top_stage - 1)
}
ToolTargetBuildMode::Dist(target_compiler) => {
assert!(target_compiler.stage > 0);
(target_compiler.host, target_compiler.stage - 1)
}
};
let compiler = if builder.host_target == target {
builder.compiler(0, builder.host_target)
builder.compiler(min_build_compiler_stage, builder.host_target)
} else {
builder.compiler(1, builder.host_target)
builder.compiler(min_build_compiler_stage.max(1), builder.host_target)
Copy link
Member

Choose a reason for hiding this comment

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

Question [STAGING 4/2]: I have to admit this logic is a bit confusing to me, especially the stage adjustments and also the stage cap. I need to think about this a bit.

path: "src/tools/wasm-component-ld",
source_type: SourceType::InTree,
extra_features: vec![],
allow_features: "min-specialization",
Copy link
Member

Choose a reason for hiding this comment

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

Question: does wasm-component-ld actually need min-specialization? At least, I'm not seeing it used at all.

Comment on lines +1167 to +1168
/// Returns `LlvmBitcodeLinker` that should be **used** by the passed compiler.
pub fn for_compiler(builder: &Builder<'_>, target_compiler: Compiler) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: for_use_by_compiler?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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
Development

Successfully merging this pull request may close these issues.

3 participants