Skip to content

ensure compiler existance of tools on the dist step #140006

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ensure compiler existance of tools on the dist step #140006

wants to merge 2 commits into from
+50 −5

Conversation

onur-ozkan
Copy link
Member

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

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

try-job: dist-powerpc64le-linux

@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2025

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

@rustbot 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
Kobzol reviewed Apr 18, 2025
View reviewed changes
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));
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fair enough.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 20, 2025 •
edited
Loading

@bors try

1 similar comment
@Kobzol
Copy link
Contributor

Kobzol commented Apr 20, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2025
@bors
Auto merge of rust-lang#140006 - onur-ozkan:138778, r=<try>
927bf00 
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
Copy link
Collaborator

bors commented Apr 20, 2025

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

@bors
Copy link
Collaborator

bors commented Apr 20, 2025

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

@Kobzol
Copy link
Contributor

Kobzol commented Apr 22, 2025

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
Copy link
Member Author

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

@Kobzol
Copy link
Contributor

Kobzol commented Apr 22, 2025

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
Copy link
Member Author

Yeah.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 23, 2025

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
Copy link
Member Author

onur-ozkan commented Apr 23, 2025 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

Assignees

@clubby789 clubby789

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

./x dist --stage 2 $tool broken after #138224
5 participants
@onur-ozkan @rustbot @Kobzol @bors @clubby789