ensure compiler existance of tools on the dist step #140006

onur-ozkan · 2025-04-18T09:05:02Z

Fixes #138778 with a coverage on #138123 and #138004.

try-job: dist-powerpc64le-linux

Signed-off-by: onur-ozkan <work@onurozkan.dev>

rustbot · 2025-04-18T09:05:07Z

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

Kobzol · 2025-04-18T09:19:31Z

src/bootstrap/src/core/build_steps/dist.rs

@@ -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));


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.

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

Ok, fair enough.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

Kobzol · 2025-04-20T07:19:26Z

@bors try

Kobzol · 2025-04-20T07:20:35Z

@bors try

ensure compiler existance of tools on the dist step Fixes rust-lang#138778 with a coverage on rust-lang#138123 and rust-lang#138004. try-job: dist-powerpc64le-linux

bors · 2025-04-20T07:21:47Z

⌛ Trying commit 4ba9fff with merge 927bf00...

bors · 2025-04-20T10:07:59Z

☀️ Try build successful - checks-actions
Build commit: 927bf00 (927bf00b29ad8261c60384c24fcac128268adfcb)

Kobzol · 2025-04-22T08:36:28Z

It seems like the exact same bootstrap steps were executed in dist-powerpc64le-linux as before (https://github.com/rust-lang-ci/rust/actions/runs/14557218149#summary-40835733463). Is that expected? I thought that this would reduce some executed steps.

onur-ozkan · 2025-04-22T12:01:59Z

Why do you expect this PR to reduce executed steps? It's not the goal here.

Kobzol · 2025-04-22T12:50:04Z

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?

onur-ozkan · 2025-04-22T12:55:33Z

Yeah.

Kobzol · 2025-04-23T08:11:26Z

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?

onur-ozkan · 2025-04-23T17:36:33Z

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.

Kobzol · 2025-04-24T13:19:42Z

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.

onur-ozkan · 2025-04-24T14:38:12Z

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

Kobzol · 2025-04-24T15:02:37Z

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.

onur-ozkan · 2025-04-24T18:13:44Z

@bors r+

bors · 2025-04-24T18:13:47Z

📌 Commit 4ba9fff has been approved by onur-ozkan

It is now in the queue for this repository.

…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::Env` into `sys::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

Rollup merge of rust-lang#140006 - onur-ozkan:138778, r=onur-ozkan ensure compiler existance of tools on the dist step Fixes rust-lang#138778 with a coverage on rust-lang#138123 and rust-lang#138004. try-job: dist-powerpc64le-linux

ensure compiler existance of tools on the dist step

edfef42

Signed-off-by: onur-ozkan <work@onurozkan.dev>

rustbot assigned clubby789 Apr 18, 2025

rustbot added 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) labels Apr 18, 2025

Kobzol reviewed Apr 18, 2025

View reviewed changes

add regression test for dist compilation

4ba9fff

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the 138778 branch from b377ae9 to 4ba9fff Compare April 18, 2025 18:05

bors added S-waiting-on-bors 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 Apr 24, 2025

matthiaskrgr mentioned this pull request Apr 25, 2025

Rollup of 8 pull requests #140282

Merged

bors merged commit 8868163 into rust-lang:master Apr 25, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone Apr 25, 2025

onur-ozkan deleted the 138778 branch April 25, 2025 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensure compiler existance of tools on the dist step #140006

ensure compiler existance of tools on the dist step #140006

onur-ozkan commented Apr 18, 2025 •

edited by Kobzol

Loading

rustbot commented Apr 18, 2025

Kobzol Apr 18, 2025

onur-ozkan Apr 18, 2025

Kobzol Apr 18, 2025

Kobzol commented Apr 20, 2025 •

edited

Loading

Kobzol commented Apr 20, 2025

bors commented Apr 20, 2025

bors commented Apr 20, 2025

Kobzol commented Apr 22, 2025

onur-ozkan commented Apr 22, 2025

Kobzol commented Apr 22, 2025

onur-ozkan commented Apr 22, 2025

Kobzol commented Apr 23, 2025

onur-ozkan commented Apr 23, 2025 •

edited

Loading

Kobzol commented Apr 24, 2025

onur-ozkan commented Apr 24, 2025

Kobzol commented Apr 24, 2025

onur-ozkan commented Apr 24, 2025

bors commented Apr 24, 2025

ensure compiler existance of tools on the dist step #140006

ensure compiler existance of tools on the dist step #140006

Conversation

onur-ozkan commented Apr 18, 2025 • edited by Kobzol Loading

rustbot commented Apr 18, 2025

Kobzol Apr 18, 2025

Choose a reason for hiding this comment

onur-ozkan Apr 18, 2025

Choose a reason for hiding this comment

Kobzol Apr 18, 2025

Choose a reason for hiding this comment

Kobzol commented Apr 20, 2025 • edited Loading

Kobzol commented Apr 20, 2025

bors commented Apr 20, 2025

bors commented Apr 20, 2025

Kobzol commented Apr 22, 2025

onur-ozkan commented Apr 22, 2025

Kobzol commented Apr 22, 2025

onur-ozkan commented Apr 22, 2025

Kobzol commented Apr 23, 2025

onur-ozkan commented Apr 23, 2025 • edited Loading

Kobzol commented Apr 24, 2025

onur-ozkan commented Apr 24, 2025

Kobzol commented Apr 24, 2025

onur-ozkan commented Apr 24, 2025

bors commented Apr 24, 2025

onur-ozkan commented Apr 18, 2025 •

edited by Kobzol

Loading

Kobzol commented Apr 20, 2025 •

edited

Loading

onur-ozkan commented Apr 23, 2025 •

edited

Loading