Tracking Issue for consolidating command invocations in bootstrap #126819

Kobzol · 2024-06-22T07:26:50Z

This is a tracking issue for monitoring the work of improving how we deal with command invocations in bootstrap (proposed here).

Currently, bootstrap executes commands on many places, often in arbitrary ways. This makes it harder to debug, profile and test the command invocations executed by it. The goal of this refactoring is to make sure that all external command invocations use the same API (a custom command wrapper) and that they go through a centralized location.

Intermediate steps:

The text was updated successfully, but these errors were encountered:

…=onur-ozkan Bootstrap command refactoring: port more `Command` usages to `BootstrapCmd` (step 2) This PR moves more of bootstrap to use `BooststrapCmd`, and also refactors the struct to allow it to serve as a proper command wrapper. Tracking issue: rust-lang#126819 Best reviewed commit-by-commit, I have been adding some helper impls along the way to ease the migration, and then later I remove some of them since they were no longer needed. r? `@onur-ozkan`

Rollup merge of rust-lang#126822 - Kobzol:bootstrap-cmd-refactor-2, r=onur-ozkan Bootstrap command refactoring: port more `Command` usages to `BootstrapCmd` (step 2) This PR moves more of bootstrap to use `BooststrapCmd`, and also refactors the struct to allow it to serve as a proper command wrapper. Tracking issue: rust-lang#126819 Best reviewed commit-by-commit, I have been adding some helper impls along the way to ease the migration, and then later I remove some of them since they were no longer needed. r? `@onur-ozkan`

…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`

…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

…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` try-job: aarch64-apple

…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`

…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`

…=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`

…=onur-ozkan Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of rust-lang#127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: rust-lang#126819 r? `@onur-ozkan`

…=onur-ozkan Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of rust-lang#127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: rust-lang#126819 r? ``@onur-ozkan``

…=onur-ozkan Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of rust-lang#127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: rust-lang#126819 r? ```@onur-ozkan```

…nur-ozkan Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of rust-lang#127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: rust-lang#126819 r? `@onur-ozkan`

…try> Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of rust-lang#127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: rust-lang#126819 r? `@onur-ozkan` try-job: x86_64-msvc

…nur-ozkan Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of rust-lang#127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: rust-lang#126819 r? `@onur-ozkan` try-job: x86_64-msvc

Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of rust-lang/rust#127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: rust-lang/rust#126819 r? `@onur-ozkan` try-job: x86_64-msvc

Slightly refactor `Flags` in bootstrap The next step for rust-lang#126819 is to track commands executed inside `Config::parse`. This is quite challenging, because (tracked) command execution needs to access some state that is stored inside `Config`, which creates a sort of a chicken-and-egg problem. I would like to first untangle `Config::parse` a little bit, which is what this PR starts with. Tracking issue: rust-lang#126819 r? `@onur-ozkan`

Slightly refactor `Flags` in bootstrap The next step for rust-lang#126819 is to track commands executed inside `Config::parse`. This is quite challenging, because (tracked) command execution needs to access some state that is stored inside `Config`, which creates a sort of a chicken-and-egg problem. I would like to first untangle `Config::parse` a little bit, which is what this PR starts with. Tracking issue: rust-lang#126819 r? ``@onur-ozkan``

Slightly refactor `Flags` in bootstrap The next step for rust-lang#126819 is to track commands executed inside `Config::parse`. This is quite challenging, because (tracked) command execution needs to access some state that is stored inside `Config`, which creates a sort of a chicken-and-egg problem. I would like to first untangle `Config::parse` a little bit, which is what this PR starts with. Tracking issue: rust-lang#126819 r? `@onur-ozkan`

Rollup merge of rust-lang#128878 - Kobzol:refactor-flags, r=onur-ozkan Slightly refactor `Flags` in bootstrap The next step for rust-lang#126819 is to track commands executed inside `Config::parse`. This is quite challenging, because (tracked) command execution needs to access some state that is stored inside `Config`, which creates a sort of a chicken-and-egg problem. I would like to first untangle `Config::parse` a little bit, which is what this PR starts with. Tracking issue: rust-lang#126819 r? `@onur-ozkan`

Kobzol added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jun 22, 2024

Kobzol mentioned this issue Jun 22, 2024

Bootstrap command refactoring: port more Command usages to BootstrapCmd (step 2) #126822

Merged

Kobzol mentioned this issue Jun 29, 2024

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

Merged

Kobzol mentioned this issue Jul 4, 2024

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

Merged

Kobzol mentioned this issue Jul 7, 2024

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

Merged

Kobzol mentioned this issue Jul 13, 2024

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

Merged

Kobzol mentioned this issue Jul 16, 2024

Bootstrap command refactoring: make command output API more bulletproof (step 7) #127799

Merged

Kobzol mentioned this issue Aug 9, 2024

Slightly refactor Flags in bootstrap #128878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking Issue for consolidating command invocations in bootstrap #126819

Tracking Issue for consolidating command invocations in bootstrap #126819

Kobzol commented Jun 22, 2024 •

edited

Loading

Tracking Issue for consolidating command invocations in bootstrap #126819

Tracking Issue for consolidating command invocations in bootstrap #126819

Comments

Kobzol commented Jun 22, 2024 • edited Loading

Kobzol commented Jun 22, 2024 •

edited

Loading