-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
base: master
Are you sure you want to change the base?
Add ToolTarget
to bootstrap
#143641
Conversation
#142357 was merged, so undrafting. |
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 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"); |
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.
Problem [STAGING 0/2]: leftover todo!()
? 🤔
// FIXME: should this be builder.top_stage to avoid rebuilds? | ||
builder.compiler(1, target) |
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.
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
if builder.host_target == target { | ||
builder.compiler(0, builder.host_target) |
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 [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?
/// 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 |
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 [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?
/// 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), | ||
} |
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.
Discussion [STAGING 3/2]: oh I see, this is exactly what I was curious about.
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) |
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 [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", |
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: does wasm-component-ld
actually need min-specialization
? At least, I'm not seeing it used at all.
/// Returns `LlvmBitcodeLinker` that should be **used** by the passed compiler. | ||
pub fn for_compiler(builder: &Builder<'_>, target_compiler: Compiler) -> Self { |
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.
Suggestion: for_use_by_compiler
?
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