Centralize command running in boostrap (part one) #116581

Kobzol · 2023-10-09T21:48:29Z

This PR tries to consolidate the various run, try_run, run_quiet, run_quiet_delaying_failure, run_delaying_failure etc. methods on Builder. This PR only touches command execution which doesn't produce output that would be later read by bootstrap, and it also only refactors spawning of commands that happens after a builder is created (commands executed during download & git submodule checkout are left as-is, for now).

The run_cmd method is quite meaty, but I expect that it will be changing rapidly soon, so I considered it easy to kept everything in a single method, and only after things settle down a bit, then maybe again split it up a bit.

I still kept the original shortcut methods like run_quiet_delaying_failure, but they now only delegate to run_cmd. I tried to keep the original behavior (or as close to it as possible) for all the various commands, but it is a giant mess, so there may be some deviations. Notably, cmd.output() is now always called, instead of just status(), which was called previously in some situations.

Apart from the refactored methods, there is also Config::try_run, check_run, methods that run commands that produce output, oh my… that's left for follow-up PRs :)

The driving goal of this (and following) refactors is to centralize command execution in bootstrap on a single place, to make command mocking feasible.

r? @onur-ozkan

onur-ozkan · 2023-10-10T08:53:11Z

Fails on CI currently @rustbot author

onur-ozkan · 2023-10-10T08:54:37Z

src/bootstrap/lib.rs

+    /// A centralized function for running commands that do not return output.
+    pub(crate) fn run_cmd<'a, C: Into<BootstrapCommand<'a>>>(&self, cmd: C) -> bool {


Shouldn't we handle this in exec module?

That's not so easy yet - we need to access self.fail_fast, self.delayed_failures, self.verbose etc. It would need more refactoring to introduce some shared command context or something like that.

Kobzol · 2023-10-10T19:16:46Z

I refactored the commits a bit, because I realized that we can't just always run output instead of status to replicate the old behavior. The old behavior is also quite messy, basically it was doing one of these three things:

Suppress all output, but print it on failure
Print all output, don't print anything on failure
Print all output, print an error on failure

I tried to replicate this behavior, but eventually we might want to e.g. consolidate 2) and 3) to make this less complicated.

Kobzol · 2023-10-10T19:33:44Z

@rustbot ready

src/bootstrap/lib.rs

src/bootstrap/exec.rs

onur-ozkan · 2023-10-11T10:53:45Z

@rustbot author

Kobzol · 2023-10-12T09:19:47Z

@rustbot ready

src/bootstrap/lib.rs

onur-ozkan · 2023-10-12T11:23:46Z

@bors r+ rollup=iffy

bors · 2023-10-12T11:23:48Z

📌 Commit 3bb226f has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2023-10-12T12:32:38Z

⌛ Testing commit 3bb226f with merge 14f0550...

Centralize command running in boostrap (part one) This PR tries to consolidate the various `run, try_run, run_quiet, run_quiet_delaying_failure, run_delaying_failure` etc. methods on `Builder`. This PR only touches command execution which doesn't produce output that would be later read by bootstrap, and it also only refactors spawning of commands that happens after a builder is created (commands executed during download & git submodule checkout are left as-is, for now). The `run_cmd` method is quite meaty, but I expect that it will be changing rapidly soon, so I considered it easy to kept everything in a single method, and only after things settle down a bit, then maybe again split it up a bit. I still kept the original shortcut methods like `run_quiet_delaying_failure`, but they now only delegate to `run_cmd`. I tried to keep the original behavior (or as close to it as possible) for all the various commands, but it is a giant mess, so there may be some deviations. Notably, `cmd.output()` is now always called, instead of just `status()`, which was called previously in some situations. Apart from the refactored methods, there is also `Config::try_run`, `check_run`, methods that run commands that produce output, oh my… that's left for follow-up PRs :) The driving goal of this (and following) refactors is to centralize command execution in bootstrap on a single place, to make command mocking feasible. r? `@onur-ozkan`

bors · 2023-10-12T13:09:07Z

💔 Test failed - checks-actions

Kobzol · 2023-10-23T11:20:47Z

Ok, the specific problem with powerpc seems to have been resolved.

I'm not sure if it's a good thing that bootstrap now behaves in a different way though (although probably we want to know about such build failures, unless we explicitly silence them?). Ideally, this PR wouldn't change behaviour.

Should we keep the behaviour or should I go debug why has it changed from before? @onur-ozkan

RalfJung · 2023-10-23T12:05:41Z

From a Miri perspective, we shouldn't ignore build failures. This is probably a leftover from the toolstate days. We shod either build and then stop CI if that fails, or not even attempt to build. So this behavior change seems fine. But I can understand that you might want to figure out where the behavior change comes from.

onur-ozkan · 2023-10-23T16:21:34Z

Finding the reason of this behaviour and applying it in a seperated commit would be great IMO. That way we can revert that specific commit easily instead of reverting the whole implementation if needed.

Miri subtree update This should unblock rust-lang/rust#116581

Kobzol · 2023-10-26T08:56:17Z

Ok I found out the issue. Previously, there were three places (finding local git modifications, blessing tests and building a tool) where a failure of the command didn't "crash" bootstrap immediately, nor did it delay the failure until the end of bootstrap's executino. This mode wasn't possible in the new command structure, so I have added it, and used it in these three places, to keep the original behavior. I still kept "fail fast" as the default behavior, I think that it is better to see errors early and opt-in to ignoring a failure, rather than vice versa.

Regarding the tools, most tools built using the ToolBuild step are optional, which means that if their build fails, bootstrap will not crash.

Kobzol · 2023-10-26T08:56:24Z

@rustbot ready

onur-ozkan

Thanks! This LGTM.

r=me with or without the suggested minor change

src/bootstrap/src/utils/exec.rs

Kobzol · 2023-10-26T19:53:06Z

@bors r=onur-ozkan

bors · 2023-10-26T19:53:08Z

📌 Commit 5d7fca1 has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2023-10-26T20:15:19Z

⌛ Testing commit 5d7fca1 with merge aa1a71e...

bors · 2023-10-26T22:09:10Z

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing aa1a71e to master...

rust-timer · 2023-10-27T00:39:50Z

Finished benchmarking commit (aa1a71e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	10.2%	[10.1%, 10.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.153s -> 634.754s (-0.06%)
Artifact size: 304.47 MiB -> 304.49 MiB (0.01%)

Miri subtree update This should unblock rust-lang/rust#116581

rustbot assigned onur-ozkan Oct 9, 2023

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 Oct 9, 2023

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2023

onur-ozkan reviewed Oct 10, 2023

View reviewed changes

Kobzol force-pushed the bootstrap-cmd-run branch from 1c2a157 to b2f121c Compare October 10, 2023 19:06

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2023

onur-ozkan requested changes Oct 11, 2023

View reviewed changes

src/bootstrap/lib.rs Outdated Show resolved Hide resolved

src/bootstrap/lib.rs Outdated Show resolved Hide resolved

src/bootstrap/exec.rs Outdated Show resolved Hide resolved

src/bootstrap/exec.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2023

onur-ozkan reviewed Oct 12, 2023

View reviewed changes

src/bootstrap/lib.rs Outdated Show resolved Hide resolved

onur-ozkan approved these changes Oct 12, 2023

View reviewed changes

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 Oct 12, 2023

This comment has been minimized.

Sign in to view

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 24, 2023

Rollup merge of #117044 - RalfJung:miri, r=RalfJung

a7827c2

Miri subtree update This should unblock rust-lang/rust#116581

onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 24, 2023

Kobzol force-pushed the bootstrap-cmd-run branch from 8c5874b to ca6db0f Compare October 26, 2023 08:54

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2023

onur-ozkan approved these changes Oct 26, 2023

View reviewed changes

src/bootstrap/src/utils/exec.rs Outdated Show resolved Hide resolved

Allow ignoring the failure of command execution

5d7fca1

Kobzol force-pushed the bootstrap-cmd-run branch from ca6db0f to 5d7fca1 Compare October 26, 2023 19:52

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 Oct 26, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 26, 2023

bors merged commit aa1a71e into rust-lang:master Oct 26, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 26, 2023

Kobzol deleted the bootstrap-cmd-run branch October 27, 2023 06:08

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

Kobzol mentioned this pull request Jun 22, 2024

Tracking Issue for consolidating command invocations in bootstrap #126819

Open

19 tasks

spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024

Rollup merge of #117044 - RalfJung:miri, r=RalfJung

01a7f13

Miri subtree update This should unblock rust-lang/rust#116581

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize command running in boostrap (part one) #116581

Centralize command running in boostrap (part one) #116581

Kobzol commented Oct 9, 2023

This comment has been minimized.

onur-ozkan commented Oct 10, 2023

onur-ozkan Oct 10, 2023

Kobzol Oct 10, 2023

Kobzol commented Oct 10, 2023

Kobzol commented Oct 10, 2023

onur-ozkan commented Oct 11, 2023

Kobzol commented Oct 12, 2023

onur-ozkan commented Oct 12, 2023

bors commented Oct 12, 2023

bors commented Oct 12, 2023

This comment has been minimized.

bors commented Oct 12, 2023

Kobzol commented Oct 23, 2023

RalfJung commented Oct 23, 2023 via email

onur-ozkan commented Oct 23, 2023 •

edited

Loading

Kobzol commented Oct 26, 2023 •

edited

Loading

Kobzol commented Oct 26, 2023

onur-ozkan left a comment

Kobzol commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 27, 2023

		/// A centralized function for running commands that do not return output.
		pub(crate) fn run_cmd<'a, C: Into<BootstrapCommand<'a>>>(&self, cmd: C) -> bool {

Centralize command running in boostrap (part one) #116581

Centralize command running in boostrap (part one) #116581

Conversation

Kobzol commented Oct 9, 2023

This comment has been minimized.

onur-ozkan commented Oct 10, 2023

onur-ozkan Oct 10, 2023

Choose a reason for hiding this comment

Kobzol Oct 10, 2023

Choose a reason for hiding this comment

Kobzol commented Oct 10, 2023

Kobzol commented Oct 10, 2023

onur-ozkan commented Oct 11, 2023

Kobzol commented Oct 12, 2023

onur-ozkan commented Oct 12, 2023

bors commented Oct 12, 2023

bors commented Oct 12, 2023

This comment has been minimized.

bors commented Oct 12, 2023

Kobzol commented Oct 23, 2023

RalfJung commented Oct 23, 2023 via email

onur-ozkan commented Oct 23, 2023 • edited Loading

Kobzol commented Oct 26, 2023 • edited Loading

Kobzol commented Oct 26, 2023

onur-ozkan left a comment

Choose a reason for hiding this comment

Kobzol commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 27, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

onur-ozkan commented Oct 23, 2023 •

edited

Loading

Kobzol commented Oct 26, 2023 •

edited

Loading