-
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)
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.
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 might have over-engineered this code. The logic is: if you want to build a stage N tool, or attach a tool to a disted stage N rustc, you need to build the tool with rustc stage N - 1. However, if you're cross-compiling a stage 1 tool, we auto-bump it to a stage 2 tool. That's the logic.
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.
Tried to clarify it a bit in the latest commit.
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.
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.
It compiled without it. @alexcrichton Do you know if this feature is required for wasm-component-ld
?
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.
No need can definitely be removed. IIRC this was required historically due to the various alignment of other issues but if things work without it then definitely jettison it
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, the historical reason is at tkaitchuck/aHash#238
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.
Thank you for the context! I removed it, let's see what CI says.
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.
Wait hold on, that means this is necessary because ahash will try to enable it because of fragile can-enable-feature detection (I think?)
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 was able to build it locally without the feature, IIRC. So maybe the lockfile has been updated to a newer ahash version in the meantime? I remember that ahash eventually gave up with these detections. We'll see what happens on CI.
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 still see min_spec feature detection in ahash master's build.rs, hm
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 did cargo tree
on wasm-component-ld
and it doesn't return ahash
anymore, so maybe that's the reason =D
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 yes hashbrown updated to switch away from ahash in 0.15.0 and rustc is on 0.15.4 so the ahash dependency no longer shows up here.
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