Skip to content

Remove start_process helper method from bootstrap #142486

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

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

Conversation

Shourya742
Copy link
Contributor

This PR removes start_process utility method.

r? @Kobzol

@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 Jun 14, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Jun 14, 2025

Thanks! Could you please do a small benchmark to check how long it takes to run all the gitinfos now? Before, these three commands executed in parallel, while now they run sequentially. I don't expect a lot of changes, but we do invoke a bunch of gitinfos, so it would be nice to check.

@bors
Copy link
Collaborator

bors commented Jun 14, 2025

☔ The latest upstream changes (presumably #142483) made this pull request unmergeable. Please resolve the merge conflicts.

@the8472
Copy link
Member

the8472 commented Jun 14, 2025

I itentionally added this as a perf optimization in #131954, is there a problem with this method existing?

@Kobzol
Copy link
Contributor

Kobzol commented Jun 14, 2025

We're trying to get rid of various adhoc ways of executing commands in bootstrap, and instead use a unified API so that we can get caching/tracing/mocking.

We already noticed that this helps perf. a bit, yeah. We'll add some API to allow batching commands to keep the current behavior. In fact, I think we could parallelize across all GitInfo invocations, we do a lot of them.

@the8472
Copy link
Member

the8472 commented Jun 14, 2025

Ok, the PR didn't provide any context, so it looked like a change without much benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants