Bootstrap command refactoring: improve debuggability (step 5) #127450

Kobzol · 2024-07-07T09:58:29Z

Continuation of #127321.

This PR improves the debuggability of command execution, by improving the output logged when a command fails (it now includes the exact location where the command was created and where it was executed), and also by adding a "drop bomb", which will panic if a command was created, but not executed (which is probably a bug).

Here is how the output of a failed command looks like:

Command "git" "foo" "[bar]" (failure_mode=Exit, stdout_mode=Capture, stderr_mode=Capture) did not execute successfully.
Expected success, got exit status: 1
Created at: src/core/build_steps/compile.rs:1699:9
Executed at: src/core/build_steps/compile.rs:1699:58

STDOUT ----


STDERR ----
git: 'foo' is not a git command. See 'git --help'.

And this is what is printed if you forget to execute a command:

thread 'main' panicked at /projects/personal/rust/rust/src/tools/build_helper/src/drop_bomb/mod.rs:42:13:
command constructed at `src/core/build_steps/compile.rs:1699:9` was dropped without being executed: `git`

Best reviewed commit by commit.

Tracking issue: #126819

r? @onur-ozkan

rustbot · 2024-07-07T09:58:38Z

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

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

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

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The run-make-support library was changed

cc @jieyouxu

Kobzol · 2024-07-07T11:30:01Z

The drop bomb is already hitting some cases where a command wasn't being executed.

Kobzol · 2024-07-07T19:56:53Z

Ok, PR CI seems to have finally stabilized.

@rustbot ready

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

onur-ozkan · 2024-07-12T14:26:49Z

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

+        let host = self.host;
+        let compiler = builder.compiler(builder.top_stage, host);
+
+        builder.ensure(compile::Std::new(compiler, host));


Any reason for this?

You mean this specific line or the whole step? I copy-pasted the whole step from the RunMakeSupport step.

This specific line. RunMakeSupport is using specific compiler in its type so it makes sense to ensure std for it. But we don't need to build the top stage compiler and std for testing build helper crate, using stage 0 compiler should be enough (and faster as we don't need to build entire tree for testing it). Same for CrateRunMakeSupport step too, it should use stage 0 compiler without needing to build compiler and std.

Ok, changed it to stage 0.

src/bootstrap/src/utils/exec.rs

bors · 2024-07-12T18:10:00Z

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

So that it can be also used in bootstrap.

To enable the previously moved `DropBomb` tests.

Before, only the line was stored. This was enough for run-make tests, since these mostly only contain a single `rmake.rs` file, but not for bootstrap.

… it through the `as_command_mut` method This will be useful for disarming drop bombs when the inner command is accessed.

This makes it harder to accidentally forget to execute a created command in bootstrap.

This should make it quicker to debug command failures.

Avoid printing useless information in the `Debug` output.

We can move the command creation to a block where it is clear that the command will be executed.

The code for running tests uses a custom command machinery because it streams the output of the command. We thus need to mark the command as executed in a dry run, to avoid a drop bomb panic.

If we're in dry run mode, the command will return an empty string, so we can just execute it.

There is no need to build a stage N toolchain for testing it.

Kobzol · 2024-07-12T18:15:41Z

Rebased to fix conflicts.

onur-ozkan · 2024-07-13T05:10:02Z

Thanks!

@bors r+

bors · 2024-07-13T05:10:05Z

📌 Commit 72c3540 has been approved by onur-ozkan

It is now in the queue for this repository.

…nur-ozkan Bootstrap command refactoring: improve debuggability (step 5) Continuation of rust-lang#127321. This PR improves the debuggability of command execution, by improving the output logged when a command fails (it now includes the exact location where the command was created and where it was executed), and also by adding a "drop bomb", which will panic if a command was created, but not executed (which is probably a bug). Here is how the output of a failed command looks like: ``` Command "git" "foo" "[bar]" (failure_mode=Exit, stdout_mode=Capture, stderr_mode=Capture) did not execute successfully. Expected success, got exit status: 1 Created at: src/core/build_steps/compile.rs:1699:9 Executed at: src/core/build_steps/compile.rs:1699:58 STDOUT ---- STDERR ---- git: 'foo' is not a git command. See 'git --help'. ``` And this is what is printed if you forget to execute a command: ``` thread 'main' panicked at /projects/personal/rust/rust/src/tools/build_helper/src/drop_bomb/mod.rs:42:13: command constructed at `src/core/build_steps/compile.rs:1699:9` was dropped without being executed: `git` ``` Best reviewed commit by commit. Tracking issue: rust-lang#126819 r? `@onur-ozkan`

bors · 2024-07-13T08:25:24Z

⌛ Testing commit 72c3540 with merge 679b9c5...

bors · 2024-07-13T08:54:07Z

💔 Test failed - checks-actions

Kobzol · 2024-07-13T09:42:56Z

Found two more cases of unused command executions.

@bors r=onur-ozkan rollup=never

bors · 2024-07-13T09:42:58Z

📌 Commit 0cce0bb has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2024-07-13T09:44:09Z

⌛ Testing commit 0cce0bb with merge c1e3f03...

bors · 2024-07-13T12:05:04Z

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

rust-timer · 2024-07-13T13:20:54Z

Finished benchmarking commit (c1e3f03): 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)

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

Cycles

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

Binary size

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

Bootstrap: 705.609s -> 704.365s (-0.18%)
Artifact size: 328.61 MiB -> 328.69 MiB (0.02%)

…=onur-ozkan Bootstrap command refactoring: port remaining commands with access to `Build` (step 6) Continuation of rust-lang#127450. This PR ports commands in bootstrap that can easily get access to `Build(er)` to `BootstrapCommand`. After this PR, everything that can access `Build(er)` should be using the new API. Statistics of `bootstrap` code (ignoring `src/bin/<shims>`) after this PR: ``` 7 usages of `Command::new` 69 usages of `command()` (new API) - out of that: 16 usages of `as_command_mut()` (new API, but accesses the inner command) ``` Tracking issue: rust-lang#126819 r? `@onur-ozkan`

…nur-ozkan Bootstrap command refactoring: port remaining commands with access to `Build` (step 6) Continuation of rust-lang#127450. This PR ports commands in bootstrap that can easily get access to `Build(er)` to `BootstrapCommand`. After this PR, everything that can access `Build(er)` should be using the new API. Statistics of `bootstrap` code (ignoring `src/bin/<shims>`) after this PR: ``` 7 usages of `Command::new` 69 usages of `command()` (new API) - out of that: 16 usages of `as_command_mut()` (new API, but accesses the inner command) ``` Tracking issue: rust-lang#126819 r? `@onur-ozkan`

Bootstrap command refactoring: port remaining commands with access to `Build` (step 6) Continuation of rust-lang/rust#127450. This PR ports commands in bootstrap that can easily get access to `Build(er)` to `BootstrapCommand`. After this PR, everything that can access `Build(er)` should be using the new API. Statistics of `bootstrap` code (ignoring `src/bin/<shims>`) after this PR: ``` 7 usages of `Command::new` 69 usages of `command()` (new API) - out of that: 16 usages of `as_command_mut()` (new API, but accesses the inner command) ``` Tracking issue: rust-lang/rust#126819 r? `@onur-ozkan`

rustbot assigned onur-ozkan Jul 7, 2024

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs 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 Jul 7, 2024

Kobzol mentioned this pull request Jul 7, 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-5 branch from 62b5127 to 62cd815 Compare July 7, 2024 15:24

This comment has been minimized.

Sign in to view

Kobzol force-pushed the bootstrap-cmd-refactor-5 branch from 0d9e7a5 to 6729f67 Compare July 7, 2024 17:22

onur-ozkan reviewed Jul 12, 2024

View reviewed changes

Kobzol added 12 commits July 12, 2024 20:14

Move DropBomb from run-make-support to build_helper

97990a4

So that it can be also used in bootstrap.

Configure test execution for the build_helper crate in bootstrap

49f54b8

To enable the previously moved `DropBomb` tests.

Store full arm location in DropBomb

042473f

Before, only the line was stored. This was enough for run-make tests, since these mostly only contain a single `rmake.rs` file, but not for bootstrap.

Make command field of BootstrapCommand private to force access to…

a1626d7

… it through the `as_command_mut` method This will be useful for disarming drop bombs when the inner command is accessed.

Add DropBomb to BootstrapCommand

cefd5b3

This makes it harder to accidentally forget to execute a created command in bootstrap.

Print command creation and execution location when it fails

542344f

This should make it quicker to debug command failures.

Improve the Debug representation of BootstrapCommand

fdf1477

Avoid printing useless information in the `Debug` output.

Improve error message of drop bombs

9634633

Fix a case where a RustBook command is potentially not executed

8ee18d6

We can move the command creation to a block where it is clear that the command will be executed.

Remove unused rustdoc command

ebb3089

Disarm drop bombs for unexecuted test Cargo commands

0cab962

The code for running tests uses a custom command machinery because it streams the output of the command. We thus need to mark the command as executed in a dry run, to avoid a drop bomb panic.

Simplify command executions for plain source tarballs

1089de4

If we're in dry run mode, the command will return an empty string, so we can just execute it.

Kobzol added 2 commits July 12, 2024 20:15

Modify FIXME comment

823ea0e

Test build_helper with the stage 0 compiler

72c3540

There is no need to build a stage N toolchain for testing it.

Kobzol force-pushed the bootstrap-cmd-refactor-5 branch from b3ead2c to 72c3540 Compare July 12, 2024 18:15

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

Avoid more instances of unused command execution in dry run

0cce0bb

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

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

bors merged commit c1e3f03 into rust-lang:master Jul 13, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jul 13, 2024

Kobzol deleted the bootstrap-cmd-refactor-5 branch July 13, 2024 12:05

Kobzol mentioned this pull request Jul 13, 2024

Bootstrap command refactoring: port remaining commands with access to Build (step 6) #127680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrap command refactoring: improve debuggability (step 5) #127450

Bootstrap command refactoring: improve debuggability (step 5) #127450

Kobzol commented Jul 7, 2024

rustbot commented Jul 7, 2024

This comment has been minimized.

Kobzol commented Jul 7, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Kobzol commented Jul 7, 2024

onur-ozkan Jul 12, 2024

Kobzol Jul 12, 2024

onur-ozkan Jul 12, 2024

Kobzol Jul 12, 2024

bors commented Jul 12, 2024

Kobzol commented Jul 12, 2024

onur-ozkan commented Jul 13, 2024

bors commented Jul 13, 2024

bors commented Jul 13, 2024

This comment has been minimized.

bors commented Jul 13, 2024

Kobzol commented Jul 13, 2024

bors commented Jul 13, 2024

bors commented Jul 13, 2024

bors commented Jul 13, 2024

rust-timer commented Jul 13, 2024

Bootstrap command refactoring: improve debuggability (step 5) #127450

Bootstrap command refactoring: improve debuggability (step 5) #127450

Conversation

Kobzol commented Jul 7, 2024

rustbot commented Jul 7, 2024

This comment has been minimized.

Kobzol commented Jul 7, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Kobzol commented Jul 7, 2024

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

bors commented Jul 12, 2024

Kobzol commented Jul 12, 2024

onur-ozkan commented Jul 13, 2024

bors commented Jul 13, 2024

bors commented Jul 13, 2024

This comment has been minimized.

bors commented Jul 13, 2024

Kobzol commented Jul 13, 2024

bors commented Jul 13, 2024

bors commented Jul 13, 2024

bors commented Jul 13, 2024

rust-timer commented Jul 13, 2024

Overall result: no relevant changes - no action needed

Kobzol commented Jul 7, 2024 •

edited

Loading