-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
redesign stage 0 std #119899
base: master
Are you sure you want to change the base?
redesign stage 0 std #119899
Conversation
This comment has been minimized.
This comment has been minimized.
9fcee5c
to
066ce06
Compare
This comment has been minimized.
This comment has been minimized.
066ce06
to
472c50c
Compare
This comment has been minimized.
This comment has been minimized.
ce81474
to
b688ffa
Compare
This comment has been minimized.
This comment has been minimized.
5f1747d
to
00e59f0
Compare
This PR modifies If appropriate, please update |
@rustbot ready r? bootstrap |
00e59f0
to
8ac271c
Compare
@rustbot author (currently stage 2 std is copied from stage 1 and this behaviour should change with the beta std change) |
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
b49ea3f
to
16f1d09
Compare
Rebased. |
Sorry, to clarify, it shouldn't be removed altogether. The modifications that you made are good, but we should still keep the information about I played with this PR locally a bit and it seems to work how I expected it to. It's great not having to rebuild the library as a first thing in the build! It should also make rebases much less costly, as modifications to stdlib will no longer invalidate rustc (but this is harder to test without the PR being merged). |
When I want to repeatedly rebuild the compiler locally without rebuilding stdlib, ./x build --keep-stage 1 is still incredibly useful, and neither this redesign nor download-ci-rustc can replace it
Isn't that what keep-stage-std is for?
|
I use |
@@ -390,4 +390,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[ | |||
severity: ChangeSeverity::Warning, | |||
summary: "The default configuration filename has changed from `config.toml` to `bootstrap.toml`. `config.toml` is deprecated but remains supported for backward compatibility.", | |||
}, | |||
ChangeInfo { |
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.
This is a huge change, which IMO requires a much more thorough explanation to rustc contributors. We should provide a link to some resource that will clearly describe what has changed and what implications it has. The current description of this PR contains almost no information, and links to an old MCP that describes changes that were already made, but also some things that weren't made yet, and mentions things like --target-sysroot
that don't exist in bootstrap currently.
I think that expanding the description of this PR to include a more comprehensive explanation of what changes would be enough.
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.
FWIW, I think we should have a dedicated Inside Rust blog post to announce this and explain the changes to not just rustc contributors but also libs contributors (I'm happy to help write one, just haven't quite caught up to the changes here yet).
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.
Yeah, or that :)
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 can update the PR description with more details.
Is this still needed after this PR? |
Yes, it's needed even more. |
I am not really sure if that's even possible. When |
After this PR |
Before this PR library would be on the exclusion list only for local builds, where it should stay. CI should not exclude anything that could affect the contents of rust-dev I think (which includes library). |
I don't see why CI would use different settings than local builds. The |
Doesn't download-rustc use the downloaded rustc from rust-dev as stage 1 compiler and thus use the associated std for stage 1 builds whereas disabling download-rustc would use the local library folder for stage 1 builds? |
If you only modify the library, |
Uh, what? It skips a stage? That's very odd. Why? |
(That's pre-existing behavior, so I'm not sure if this PR is relevant to discuss that, tbh). I suppose that it is stage 2 because you download the whole stage1 compiler from CI (including its stdlib), so since you already have stage1 downloaded, you then built stage 2 locally. But Onur can most likely explain it better. |
With this PR, the stage 2 library should be built by the stage 2 compiler, just like stage 1 works. So I think it is relevant for this PR, as this PR exactly changes how the staging is chained:
So the dependency chain now is: Even if you only modify the library, the stage 1 compiler will be different, and therefore everything past this point as well. The stage 2 library is quite far down the chain and nothing there can be cached. But it also shouldn't be needed as the stage 0 compiler + library should be enough for almost everything. |
Ah, I meant the CI vs local difference, not the staging itself. Yeah, you're right that the staging with download-rustc should be diferent here, it seems a bit suspicious. If I unconditionally enable
While it should instead build stage1 stdlib.
What shouldn't be needed? |
A stage 2 library build. |
I misunderstood #119899 (comment)... I thought Jakub was asking whether we should still keep the library in the exclusion list. We can definitely keep the library path unconditionally in the exclusion list and remove the CI check! I will do that in couple of minutes. |
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Summary
This PR changes how bootstrap builds the stage 1 compiler by switching to precompiled stage 0 standard library instead of building the in-tree one. The goal was to update bootstrap to use the beta standard library at stage 0 rather than compiling it from source (see the motivation at rust-lang/compiler-team#619).
Previously, to build a stage 1 compiler bootstrap followed this path:
With this PR, the new path is:
This also means that
cfg(bootstrap)
/cfg(not(bootstrap))
is no longer needed for library development.Building "library"
Since stage0
std
is no longer in-treex build/test/check library --stage 0
is now no-op. The minimum supported stage to buildstd
is now 1. For the same reason, default stage values in the library profile is no longer 0.Because building the in-tree library now requires a stage1 compiler, I highly recommend library developers to enable
download-rustc
to speed up compilation time.If you encounter a bug or unexpected results please open a topic in the #t-infra/bootstrap Zulip channel or create a bootstrap issue.
Blocked on #122709