Bootstrap command refactoring: port more Command usages to BootstrapCmd (step 2)
#126822
Conversation
rustbot
added
S-waiting-on-review
| @@ -1024,18 +1022,7 @@ impl Build { | |||
|
|
|||
| /// Runs a command, printing out nice contextual information if it fails. | |||
| fn run(&self, cmd: &mut Command) { | |||
| self.run_cmd(BootstrapCommand::from(cmd).fail_fast().output_mode( | |||
There was a problem hiding this comment.
Note: fail_fast and the given output_mode are default in BootstrapCommand.
onur-ozkan
added
S-blocked
Kobzol
force-pushed
the
bootstrap-cmd-refactor-2
branch
from
7554ba6
to
bee136f
Compare
rustbot
added
S-waiting-on-review
| @@ -2070,7 +2071,7 @@ pub fn stream_cargo( | |||
| tail_args: Vec<String>, | |||
| cb: &mut dyn FnMut(CargoMessage<'_>), | |||
| ) -> bool { | |||
| let mut cargo = Command::from(cargo); | |||
| let mut cargo = BootstrapCommand::from(cargo).command; | |||
There was a problem hiding this comment.
It seems like this usage makes it like building commands from std Command, so it will not be possible to profile/debug/log (pretty much any reason that this implementation was based on) the execution. The inner command should never be exposed to avoid this problem.
There was a problem hiding this comment.
Indeed, I plan to address this later. I'm doing the refactoring step by step, by introducing compatibility layers in the meantime, otherwise, I'd have to change all usages at once and that would be very hard to review and implement for me. One of those compatibility layers is making the Command pub, so that I can access it when needed.
As one of the next N steps, I will make the command private, which will force me to deal with the more complex cases, like output streaming. But I don't want to do that yet, first I want to port all/most of the simple use-cases and iterate on the API.
onur-ozkan
added
S-waiting-on-author
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☔ The latest upstream changes (presumably #127076) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
This will make it easier to migrate existing commands to bootstrap command.
By allowing `run` to receive all of `BootstrapCmd`, `&mut BootstrapCmd`, `Command` and `&mut Command`.
Kobzol
force-pushed
the
bootstrap-cmd-refactor-2
branch
from
0c599cb
to
2ebfcce
Compare
|
Rebased. @bors r=onur-ozkan |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#126822 (Bootstrap command refactoring: port more `Command` usages to `BootstrapCmd` (step 2)) - rust-lang#126835 (Simplifications in match lowering) - rust-lang#126953 (std: separate TLS key creation from TLS access) - rust-lang#127045 (Rename `super_predicates_of` and similar queries to `explicit_*` to note that they're not elaborated) - rust-lang#127075 (rustc_data_structures: Explicitly check for 64-bit atomics support) - rust-lang#127101 (remove redundant match statement from dataflow const prop) - rust-lang#127102 (Rename fuchsia builder and bump Fuchsia) - rust-lang#127103 (Move binder and polarity parsing into `parse_generic_ty_bound`) - rust-lang#127108 (unify `dylib` and `bin_helpers` and create `shared_helpers::parse_value_from_args`) r? `@ghost` `@rustbot` modify labels: rollup
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`
|
BootstrapCommand looks similar to rmake's Command wrapper, can it be potentially shared? |
|
I thought about it, but I don't think it's a good idea. They seem similar, but they are actually quite different. In rmake, we want to make sure that all errors fail as soon and as loudly as possible, and we test the commands, so the API is accustomed to asserts. In bootstrap, we want to handle failures in various different ways, and we don't really need asserts. Trying to shoehorn both into a single abstraction would IMO lead to more complexity and a worse API. It's also not a lot of code, so I don't think it's needed. |
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: #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