Bootstrap command refactoring: consolidate output modes (step 3) #127120

Kobzol · 2024-06-29T12:02:58Z

This PR is a continuation to #126731. It consolidates the output modes of bootstrap (Print vs CaptureAll vs CaptureStdout) and simplifies the logic around error printing (now a command error is always printed if the failure is not ignored). It also ports even more usages of Command to BootstrapCommand, most notably the git helpers and many usages of the output function.

The last commit was added because the third commit made two variants of the Tool enum unused (no idea why, but it seems to have been a false positive that they were used before).

It can be reviewed now, but I would wait with merging until at least a few days after #126731, just to catch any potential issues from that PR before we move further.

As a next step, I want to clean up the API of the command a little bit to make usage easier (currently it's a bit verbose), and then continue with the rest of the tasks from the tracking issue.

As always, best reviewed commit by commit.

Tracking issue: #126819

r? @onur-ozkan

try-job: aarch64-apple

rustbot · 2024-06-29T12:03:07Z

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

src/bootstrap/src/utils/helpers.rs

src/bootstrap/src/utils/exec.rs

onur-ozkan · 2024-07-02T16:27:47Z

src/bootstrap/src/utils/exec.rs

@@ -32,17 +35,20 @@ pub enum OutputMode {
 /// If you want to delay failures until the end of bootstrap, use [delay_failure].
 ///
 /// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap


Keeping ([OutputMode::Print]). part alone on the next line seems annoying

Suggested change

/// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap

/// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap ([OutputMode::Print]).

Huh, I thought that doc comments need to be within 100 characters on a line, but tidy doesn't complain. Ok.

Yeah tidy doesn't check line lengths in source files as it can be quite annoying to wrap lines that contain code.

onur-ozkan · 2024-07-02T16:31:55Z

src/bootstrap/src/utils/exec.rs

+ /// Captures the stdout of the command into memory, inherits stderr.
+ /// Corresponds to calling `cmd.output()`.
+ CaptureStdout,


CaptureStdout explicitly mentions stdout in its name, why does it also inherit stderr?

Well, if you capture (only) stdout, you need to do something with stderr (instead of capturing it). You can either ignore it or inherit it (so that it will be printed to the stderr of bootstrap). Ignoring output seems bad, and all previous usages of this mode inherited stderr, so I suppose that it makes sense as a default?

CaptureStdout - capture stdout, print stderr

CaptureAll - capture both stdout and stderr

so I suppose that it makes sense as a default?

I have no objections to making it the default behaviour. My point was it's unclear to me that a type explicitly mentions stdout and inherits stderr, which I'm pretty sure most people will not expect until they read the type's doc-comment. It's just about the name-description clarity, not about whether this behavior should be the default or not.

I see :) Any suggestions? CaptureStdoutPrintStderr?

Yeah, that seems more clear.

Tried to refactor this in a more general way, as the name reminded me of Java too much.

onur-ozkan · 2024-07-02T16:35:09Z

src/bootstrap/src/core/build_steps/tool.rs

@@ -246,6 +244,7 @@ macro_rules! bootstrap_tool {
 ;
 )+) => {
 #[derive(PartialEq, Eq, Clone)]
+ #[allow(dead_code)]


What's becoming unused in this macro? Worth to add a FIXME here.

Specifically it is Tool::JsonDocCk and Tool::JsonDocLint. For some reason, the compiler thought that they were used before this PR, probably because of some false positive (?). I haven't found anything in this PR that would remove their usage.

In general, I don't think that this should be a FIXME. The Tool enum is generated by a macro, and not all tools need to be built using tool_cmd, so it may be unused. The macro uses these two enum variants in the tool_exe function, but it is probably ignored by the compiler since the usage is inside a macro.

onur-ozkan · 2024-07-02T16:38:13Z

src/bootstrap/src/core/build_steps/test.rs

- builder.run(BootstrapCommand::from(&mut tool).output_mode(OutputMode::OnlyOnFailure));
+ builder.run(tool.capture());


Is CaptureAll the new equivalent for OnlyOnFailure?

Also, I think we changed the variant names of this enum like 3 times already.. 😄

Is CaptureAll the new equivalent for OnlyOnFailure?

CaptureAll is essentially OnlyOnFailure, yes. I made the decision whether to print something during a command failure fully derived from other options, rather than configurable by users, because it was introducing unnecessary complexity to the command and it was only used in two places of bootstrap anyway. We should IMO always print failures, unless the failure is explicitly ignored.

Also, I think we changed the variant names of this enum like 3 times already.. 😄

Well, refactoring is a continuous process, at least for me :) First I was trying to make the names as similar to the previous names used ad hoc in bootstrap code, to make it easier fo review that the migration is doing the same thing as before. Now that the usages of BootstrapCmd have been migrated, it was time to make the names better.

Kobzol · 2024-07-03T09:34:02Z

@rustbot ready

bors · 2024-07-03T20:14:24Z

📌 Commit 54952b4 has been approved by onur-ozkan

It is now in the queue for this repository.

onur-ozkan · 2024-07-03T20:20:19Z

Let's make it easier to revert for worst-case scenarios.

@bors rollup=never

bors · 2024-07-04T01:44:49Z

⌛ Testing commit 54952b4 with merge f439646...

…nur-ozkan Bootstrap command refactoring: consolidate output modes (step 3) This PR is a continuation to rust-lang#126731. It consolidates the output modes of bootstrap (`Print` vs `CaptureAll` vs `CaptureStdout`) and simplifies the logic around error printing (now a command error is always printed if the failure is not ignored). It also ports even more usages of `Command` to `BootstrapCommand`, most notably the git helpers and many usages of the `output` function. The last commit was added because the third commit made two variants of the `Tool` enum unused (no idea why, but it seems to have been a false positive that they were used before). It can be reviewed now, but I would wait with merging until at least a few days after rust-lang#126731, just to catch any potential issues from that PR before we move further. As a next step, I want to clean up the API of the command a little bit to make usage easier (currently it's a bit verbose), and then continue with the rest of the tasks from the tracking issue. As always, best reviewed commit by commit. Tracking issue: rust-lang#126819 r? `@onur-ozkan`

bors · 2024-07-04T01:49:24Z

💔 Test failed - checks-actions

When dry run is enabled, the command for finding LLDB version would succeed, but return an empty string. This was inadvertently enabling a code path that should only be executed when the LLDB is actually present and its version is valid. This commit makes sure that if the version is empty, LLDB will be considered not found.

Kobzol · 2024-07-04T08:10:55Z

@bors try

bors · 2024-07-04T08:12:05Z

⌛ Trying commit c198c0e with merge e15709d...

…try> Bootstrap command refactoring: consolidate output modes (step 3) This PR is a continuation to rust-lang#126731. It consolidates the output modes of bootstrap (`Print` vs `CaptureAll` vs `CaptureStdout`) and simplifies the logic around error printing (now a command error is always printed if the failure is not ignored). It also ports even more usages of `Command` to `BootstrapCommand`, most notably the git helpers and many usages of the `output` function. The last commit was added because the third commit made two variants of the `Tool` enum unused (no idea why, but it seems to have been a false positive that they were used before). It can be reviewed now, but I would wait with merging until at least a few days after rust-lang#126731, just to catch any potential issues from that PR before we move further. As a next step, I want to clean up the API of the command a little bit to make usage easier (currently it's a bit verbose), and then continue with the rest of the tasks from the tracking issue. As always, best reviewed commit by commit. Tracking issue: rust-lang#126819 r? `@onur-ozkan` try-job: aarch64-apple

bors · 2024-07-04T09:24:35Z

☀️ Try build successful - checks-actions
Build commit: e15709d (e15709dec2e7237899641da79d49bbf6e185cdf2)

Kobzol · 2024-07-04T09:26:06Z

Fixed problem with dry run in Apple job.

@bors r=onur-ozkan

bors · 2024-07-04T09:26:08Z

📌 Commit c198c0e has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2024-07-04T10:54:26Z

⌛ Testing commit c198c0e with merge e2cf31a...

bors · 2024-07-04T13:12:39Z

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

rust-timer · 2024-07-04T16:27:11Z

Finished benchmarking commit (e2cf31a): 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 (secondary 0.2%)

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)	1.1%	[0.8%, 1.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.3%, -1.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -1.7%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.6%, -0.8%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 718.904s -> 734.204s (2.13%)
Artifact size: 328.18 MiB -> 328.34 MiB (0.05%)

…nur-ozkan Bootstrap command refactoring: quality-of-life improvements (step 4) Continuation of rust-lang#127120. This PR simply introduce two new functions (`BootstrapCommand:run` and `command`) that make it a bit easier to use commands in bootstrap. It also adds several `#[must_use]` annotations. This shouldn't (hopefully) have any effect on behavior. Especially the first commit IMO makes any code that runs commands more readable, and allows using the API in a fluent way, without needing to jump back and forth between the command and the `Build(er)`. Tracking issue: rust-lang#126819 r? `@onur-ozkan`

Bootstrap command refactoring: quality-of-life improvements (step 4) Continuation of rust-lang/rust#127120. This PR simply introduce two new functions (`BootstrapCommand:run` and `command`) that make it a bit easier to use commands in bootstrap. It also adds several `#[must_use]` annotations. This shouldn't (hopefully) have any effect on behavior. Especially the first commit IMO makes any code that runs commands more readable, and allows using the API in a fluent way, without needing to jump back and forth between the command and the `Build(er)`. Tracking issue: rust-lang/rust#126819 r? `@onur-ozkan`

rustbot assigned onur-ozkan Jun 29, 2024

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 29, 2024

Kobzol mentioned this pull request Jun 29, 2024

Tracking Issue for consolidating command invocations in bootstrap #126819

Open

19 tasks

This comment has been minimized.

Sign in to view

Kobzol force-pushed the bootstrap-cmd-refactor-3 branch from 463bc8d to 31c3983 Compare June 29, 2024 12:36

This comment has been minimized.

Sign in to view

Kobzol mentioned this pull request Jun 29, 2024

Implement x perf as a separate tool #127002

Merged

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

Kobzol force-pushed the bootstrap-cmd-refactor-3 branch from 740f4e0 to b92344e Compare June 29, 2024 18:55

This comment has been minimized.

Sign in to view

Kobzol force-pushed the bootstrap-cmd-refactor-3 branch from b92344e to 67d6eb1 Compare June 29, 2024 20:08

This comment was marked as outdated.

Sign in to view

Kobzol force-pushed the bootstrap-cmd-refactor-3 branch from 67d6eb1 to e631949 Compare June 30, 2024 12:38

This comment has been minimized.

Sign in to view

Kobzol force-pushed the bootstrap-cmd-refactor-3 branch from e631949 to 7a0fa5a Compare July 1, 2024 08:38

onur-ozkan reviewed Jul 2, 2024

View reviewed changes

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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2024

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 Jul 3, 2024

Kobzol force-pushed the bootstrap-cmd-refactor-3 branch from a47933f to 8dc4485 Compare July 3, 2024 19:05

This comment was marked as outdated.

Sign in to view

Kobzol and others added 3 commits July 3, 2024 21:13

Improve documentation

9192479

Make git helper return BootstrapCmd

a34d0a8

Remove temporary BootstrapCommand trait impls

b14ff77

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 Jul 3, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 4, 2024

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 Jul 4, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 4, 2024

bors merged commit e2cf31a into rust-lang:master Jul 4, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jul 4, 2024

Kobzol deleted the bootstrap-cmd-refactor-3 branch July 4, 2024 13:13

Kobzol mentioned this pull request Jul 4, 2024

Bootstrap command refactoring: quality-of-life improvements (step 4) #127321

Merged

	/// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap
	/// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap ([OutputMode::Print]).

		builder.run(BootstrapCommand::from(&mut tool).output_mode(OutputMode::OnlyOnFailure));
		builder.run(tool.capture());

Bootstrap command refactoring: consolidate output modes (step 3) #127120

Bootstrap command refactoring: consolidate output modes (step 3) #127120

Conversation

Kobzol commented Jun 29, 2024 • edited Loading

rustbot commented Jun 29, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kobzol commented Jul 3, 2024

This comment was marked as outdated.

bors commented Jul 3, 2024

onur-ozkan commented Jul 3, 2024

bors commented Jul 4, 2024

This comment has been minimized.

bors commented Jul 4, 2024

Kobzol commented Jul 4, 2024

bors commented Jul 4, 2024

bors commented Jul 4, 2024

Kobzol commented Jul 4, 2024

bors commented Jul 4, 2024

bors commented Jul 4, 2024

bors commented Jul 4, 2024

rust-timer commented Jul 4, 2024

Overall result: no relevant changes - no action needed

Kobzol commented Jun 29, 2024 •

edited

Loading