-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Enforce in bootstrap that dist and install must have stage at least 1 #145472
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?
Conversation
This PR modifies If appropriate, please update |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@bors try |
This comment has been minimized.
This comment has been minimized.
Enforce in bootstrap that dist and install must have stage at least 1 try-job: dist-i686-linux try-job: dist-armhf-linux try-job: dist-x86_64-linux try-job: dist-x86_64-msvc try-job: dist-apple-various try-job: dist-x86_64-apple
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.
Thanks, the other changes I'm on board with. The only bit I'm not sure is the rust-std build optimization, IMO it's not worth the hack to introduce a special case. WDYT?
let sysroot = self.sysroot; | ||
let dst = sysroot.join("lib/rustlib/etc"); | ||
t!(fs::create_dir_all(&dst)); | ||
let cp_debugger_script = |file: &str| { | ||
builder.install(&builder.src.join("src/etc/").join(file), &dst, FileType::Regular); | ||
}; | ||
if host.contains("windows-msvc") { | ||
if target.contains("windows-msvc") { |
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.
Remark: I really hope in the future we have some way to share an in-tree structured target spec with rustc... Target tuple string matching feels bad every time I see it 😔
"Note: stage {top_stage} library for `{}` would be uplifted from stage 1, so stage was downgraded from {top_stage} to 1 to avoid needless compiler build(s)", | ||
run.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.
Suggestion: maybe say
so build compiler stage was downgraded from {top_stage} to 1 to avoid needless compiler build(s)
?
// This is a build time optimization for running just `x dist rust-std` (without | ||
// `x dist rustc`). | ||
// If we know that we will be uplifting a stage2+ library from stage 1 anyway, | ||
// there is no point in building a stage2 rustc, which will then not do anything (because | ||
// the stdlib will be uplifted). |
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:
- Who's actually using
./x dist rust-std
specifically? Usually, we would be distributing a toolchain together, right? Or at least both compiler and std? - I feel uneasy about doing this build optimization, it feels rather "special" (hacky) especially when this is the dist flow. Could we get away with just doing the straightforward ™️ build? I feel like we should just do the "useless" stage 2 rustc build, and then use the same stage 2 std uplifting logic, instead of doing this special casing.
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.
There are some CI jobs that do ./x dist --host='' --target i686-unknown-linux-gnu,aarch64-unknown-linux-gnu
. But I confirmed it produced the same steps with/without this "optimization".
I agree, let's get rid of it.
@rustbot author |
Agreed, removed the special case, and fixed one |
@rustbot ready |
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.
Thanks!
@@ -994,7 +994,7 @@ macro_rules! tool_doc { | |||
(compilers.build_compiler(), Mode::ToolRustc) | |||
} else { | |||
// bootstrap/host tools have to be documented with the stage 0 compiler | |||
(prepare_doc_compiler(run.builder, target, 1), Mode::ToolBootstrap) |
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.
Remark: hecc...
@bors r+ rollup=never |
This is a continuation of my efforts to fix staging in bootstrap after the stage0 redesign. This PR gets rid of all
compiler_for
usages in thedist
steps 🎉 The only remaining usages are in thetest
steps, which are my next goal, and which will be the final boss.. both because they are quite tricky, and because most of the the rest of the steps have been already fixed.The changes are relatively straightforward,
x dist
defaults to stage 2, so in that case we want to use a stage 1 build compiler for everything (except stdlib, but we usually use it even there because of uplifting).What I didn't fix yet are the docs steps, because they are very implicit right now, and fixing them deserves its own PR.
The first commit reverts the tests that I accidentally removed in #145340 🤦♂️ I did it in this PR to avoid further git conflicts..
Best reviewed commit-by-commit.
r? @jieyouxu
try-job: dist-i686-linux
try-job: dist-armhf-linux
try-job: dist-x86_64-linux
try-job: dist-x86_64-msvc
try-job: dist-apple-various
try-job: dist-x86_64-apple