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 10 commits into
base: master
Choose a base branch
from
Open

Add ToolTarget to bootstrap #143641

wants to merge 10 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)

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.

Copy link
Member Author

@Kobzol Kobzol Jul 10, 2025

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.

Copy link
Member Author

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",
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.

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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?)

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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.

@rust-log-analyzer

This comment has been minimized.

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.

5 participants