ensure compiler existance of tools on the dist step by onur-ozkan · Pull Request #140006 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Signed-off-by: onur-ozkan work@onurozkan.dev
r? @clubby789
rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
| @@ -421,13 +421,13 @@ impl Step for Rustc { |
|---|
| builder.install(&rustdoc, &image.join("bin"), FileType::Executable); |
| } |
| let ra_proc_macro_srv_compiler = |
| builder.compiler_for(compiler.stage, builder.config.build, compiler.host); |
| builder.ensure(compile::Rustc::new(ra_proc_macro_srv_compiler, compiler.host)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this now build the compiler for the proc macro server even if tool::RustAnalyzerProcMacroSrv should not actually be built? 🤔 It's not always enabled.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler is passed before the condition here so I'm not sure how to guard against that without applying ugly hacks. Also, in practice, I don't think we ever hit the case of "trying to build RustAnalyzerProcMacroSrv while the compiler isn't compiled/ready for use".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fair enough.
Signed-off-by: onur-ozkan work@onurozkan.dev
1 similar comment
bors added a commit to rust-lang-ci/rust that referenced this pull request
☀️ Try build successful - checks-actions
Build commit: 927bf00 (927bf00b29ad8261c60384c24fcac128268adfcb)
Why do you expect this PR to reduce executed steps? It's not the goal here.
I thought that this was also a fix for #138123. So this PR adds a test to make sure that the fix for #138778 doesn't regress #138123?
I see. Hmm, I don't like this approach though, it's super brittle. The fact that we have to sprinkle these ensure calls sucks. Could we instead move the Rustc build into ToolBuild::run to solve this at a single place?
There is no simple solution we can do in ToolBuild::run. Steps with ensure(Rustc...) diffs rely on compiler_for, which sets forced_compiler to true. When forced_compiler is true, we expect that compiler to be available. We could try something to handle this in ToolBuild::run but it would be a magical hack/workaround instead of a solution. The reason I didn't go to that path was because this change makes it obvious and we do a lot of ensure stuff almost every part of bootstrap.
magical hack/workaround instead of a solution
Well, I would argue that adding ensure at a bunch of places in bootstrap is a hack, rather than solving this at a single location (it's fine as a hotfix though, ofc). Maybe I'm missing something, but it seems like the ensure is always called with the same compiler and target parameters that are then passed to the tool being built. Why can't the tool build step invoke ensure(Rustc) with these same parameters? It already does ensure Rustc/Std in various ways based on the tool mode.
So why are these specific tools for which you added ensure in this PR special and we cannot handle them through the usual code in ToolBuild? Requiring ensure for only some tools or under some conditions, without at least documenting why is it needed, is brittle, and is likely to break in the future, IMO.
Seems like we have completely different understanding of what hack really means. Current diff doesn't do anything other than preparing the compiler for the compiler tool and it's obvious for why we doing it. It can look unpleasant but IMO it's definitely not a hack for sure.
Requiring ensure for only some tools or under some conditions, without at least documenting why is it needed, is brittle, and is likely to break in the future, IMO.
You can use as many ensure calls as you want and it won't break anything, we already do this at almost every part of bootstrap as I said before.
If you want to fix this in a different way, you would either re-write/remove compiler_for functions, or change the forced_compiler conditions, and to me both have potential of breaking things in the future or even now (like doing extra calls unnecessarily and stuff).
If you want to fix this in a different way, you would either re-write/remove compiler_for functions, or change the forced_compiler conditions, and to me both have potential of breaking things in the future or even now (like doing extra calls unnecessarily and stuff).
Yeah, I have that in my TODO list, because the current system makes it too easy to make a mistake. But it will be quite hard to modify. And it shouldn't happen before the stage 0 redesign lands anyway :)
Anyway, you can r=me, there's no need to block this fix PR on a larger refactoring.
📌 Commit 4ba9fff has been approved by onur-ozkan
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 8 pull requests
Successful merges:
- rust-lang#137653 (Deprecate the unstable
concat_idents!) - rust-lang#138957 (Update the index of Option to make the summary more comprehensive)
- rust-lang#140006 (ensure compiler existance of tools on the dist step)
- rust-lang#140143 (Move
sys::pal::os::Envintosys::env) - rust-lang#140202 (Make #![feature(let_chains)] bootstrap conditional in compiler/)
- rust-lang#140236 (norm nested aliases before evaluating the parent goal)
- rust-lang#140257 (Some drive-by housecleaning in
rustc_borrowck) - rust-lang#140278 (Don't use item name to look up associated item from trait item)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)