Remove temporary BootstrapCommand trait impls

Author: Kobzol

src/bootstrap/src/core/build_steps/compile.rs

@@ -2077,7 +2077,7 @@ pub fn stream_cargo(
     tail_args: Vec<String>,
     cb: &mut dyn FnMut(CargoMessage<'_>),
 ) -> bool {
-    let mut cargo = BootstrapCommand::from(cargo).command;
+    let mut cargo = cargo.into_cmd().command;
     // Instruct Cargo to give us json messages on stdout, critically leaving
     // stderr as piped so we can get those pretty colors.
     let mut message_format = if builder.config.json_output {

src/bootstrap/src/core/build_steps/doc.rs

@@ -738,7 +738,7 @@ fn doc_std(
         format!("library{} in {} format", crate_description(requested_crates), format.as_str());
     let _guard = builder.msg_doc(compiler, description, target);
 
-    builder.run(cargo);
+    builder.run(cargo.into_cmd());
     builder.cp_link_r(&out_dir, out);
 }
 
@@ -863,7 +863,7 @@ impl Step for Rustc {
         let proc_macro_out_dir = builder.stage_out(compiler, Mode::Rustc).join("doc");
         symlink_dir_force(&builder.config, &out, &proc_macro_out_dir);
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         if !builder.config.dry_run() {
             // Sanity check on linked compiler crates
@@ -996,7 +996,7 @@ macro_rules! tool_doc {
                 symlink_dir_force(&builder.config, &out, &proc_macro_out_dir);
 
                 let _guard = builder.msg_doc(compiler, stringify!($tool).to_lowercase(), target);
-                builder.run(cargo);
+                builder.run(cargo.into_cmd());
 
                 if !builder.config.dry_run() {
                     // Sanity check on linked doc directories

src/bootstrap/src/core/build_steps/run.rs

@@ -158,7 +158,7 @@ impl Step for Miri {
         // after another --, so this must be at the end.
         miri.args(builder.config.args());
 
-        builder.run(miri);
+        builder.run(miri.into_cmd());
     }
 }
 

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

@@ -150,16 +150,13 @@ You can skip linkcheck with --skip src/tools/linkchecker"
         builder.default_doc(&[]);
 
         // Build the linkchecker before calling `msg`, since GHA doesn't support nested groups.
-        let mut linkchecker = builder.tool_cmd(Tool::Linkchecker);
+        let linkchecker = builder.tool_cmd(Tool::Linkchecker);
 
         // Run the linkchecker.
         let _guard =
             builder.msg(Kind::Test, compiler.stage, "Linkcheck", bootstrap_host, bootstrap_host);
         let _time = helpers::timeit(builder);
-        builder.run(
-            BootstrapCommand::from(linkchecker.arg(builder.out.join(host.triple).join("doc")))
-                .delay_failure(),
-        );
+        builder.run(linkchecker.delay_failure().arg(builder.out.join(host.triple).join("doc")));
     }
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
@@ -217,10 +214,7 @@ impl Step for HtmlCheck {
         ));
 
         builder.run(
-            BootstrapCommand::from(
-                builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)),
-            )
-            .delay_failure(),
+            builder.tool_cmd(Tool::HtmlChecker).delay_failure().arg(builder.doc_out(self.target)),
         );
     }
 }
@@ -260,14 +254,13 @@ impl Step for Cargotest {
 
         let _time = helpers::timeit(builder);
         let mut cmd = builder.tool_cmd(Tool::CargoTest);
-        let cmd = cmd
-            .arg(&cargo)
+        cmd.arg(&cargo)
             .arg(&out_dir)
             .args(builder.config.test_args())
             .env("RUSTC", builder.rustc(compiler))
             .env("RUSTDOC", builder.rustdoc(compiler));
-        add_rustdoc_cargo_linker_args(cmd, builder, compiler.host, LldThreads::No);
-        builder.run(BootstrapCommand::from(cmd).delay_failure());
+        add_rustdoc_cargo_linker_args(&mut cmd, builder, compiler.host, LldThreads::No);
+        builder.run(cmd.delay_failure());
     }
 }
 
@@ -763,12 +756,12 @@ impl Step for Clippy {
         cargo.env("HOST_LIBS", host_libs);
 
         cargo.add_rustc_lib_path(builder);
-        let mut cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder);
+        let cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder);
 
         let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
 
         // Clippy reports errors if it blessed the outputs
-        if builder.run(BootstrapCommand::from(&mut cargo).allow_failure()).is_success() {
+        if builder.run(cargo.allow_failure()).is_success() {
             // The tests succeeded; nothing to do.
             return;
         }
@@ -821,7 +814,7 @@ impl Step for RustdocTheme {
             .env("RUSTC_BOOTSTRAP", "1");
         cmd.args(linker_args(builder, self.compiler.host, LldThreads::No));
 
-        builder.run(BootstrapCommand::from(&mut cmd).delay_failure());
+        builder.run(cmd.delay_failure());
     }
 }
 
@@ -1099,7 +1092,7 @@ HELP: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to
         }
 
         builder.info("tidy check");
-        builder.run(BootstrapCommand::from(&mut cmd).delay_failure());
+        builder.run(cmd.delay_failure());
 
         builder.info("x.py completions check");
         let [bash, zsh, fish, powershell] = ["x.py.sh", "x.py.zsh", "x.py.fish", "x.py.ps1"]
@@ -1306,7 +1299,7 @@ impl Step for RunMakeSupport {
             &[],
         );
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         let lib_name = "librun_make_support.rlib";
         let lib = builder.tools_dir(self.compiler).join(lib_name);
@@ -2185,7 +2178,7 @@ impl BookTest {
             compiler.host,
         );
         let _time = helpers::timeit(builder);
-        let cmd = BootstrapCommand::from(&mut rustbook_cmd).delay_failure();
+        let cmd = rustbook_cmd.delay_failure();
         let toolstate =
             if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail };
         builder.save_toolstate(self.name, toolstate);
@@ -2316,7 +2309,7 @@ impl Step for ErrorIndex {
         let guard =
             builder.msg(Kind::Test, compiler.stage, "error-index", compiler.host, compiler.host);
         let _time = helpers::timeit(builder);
-        builder.run(BootstrapCommand::from(&mut tool).output_mode(OutputMode::OnlyOnFailure));
+        builder.run(tool.output_mode(OutputMode::OnlyOnFailure));
         drop(guard);
         // The tests themselves need to link to std, so make sure it is
         // available.
@@ -2373,10 +2366,13 @@ impl Step for RustcGuide {
         builder.update_submodule(&relative_path);
 
         let src = builder.src.join(relative_path);
-        let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
-        let cmd = BootstrapCommand::from(rustbook_cmd.arg("linkcheck").arg(&src)).delay_failure();
-        let toolstate =
-            if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail };
+        let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook).delay_failure();
+        rustbook_cmd.arg("linkcheck").arg(&src);
+        let toolstate = if builder.run(rustbook_cmd).is_success() {
+            ToolState::TestPass
+        } else {
+            ToolState::TestFail
+        };
         builder.save_toolstate("rustc-dev-guide", toolstate);
     }
 }
@@ -3345,7 +3341,7 @@ impl Step for CodegenCranelift {
             .arg("testsuite.extended_sysroot");
         cargo.args(builder.config.test_args());
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
     }
 }
 
@@ -3470,6 +3466,6 @@ impl Step for CodegenGCC {
             .arg("--std-tests");
         cargo.args(builder.config.test_args());
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
     }
 }

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

@@ -602,7 +602,7 @@ impl Step for Rustdoc {
             &self.compiler.host,
             &target,
         );
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         // Cargo adds a number of paths to the dylib search path on windows, which results in
         // the wrong rustdoc being executed. To avoid the conflicting rustdocs, we name the "tool"
@@ -857,7 +857,7 @@ impl Step for LlvmBitcodeLinker {
             &self.extra_features,
         );
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         let tool_out = builder
             .cargo_out(self.compiler, Mode::ToolRustc, self.target)

src/bootstrap/src/core/builder.rs

@@ -2398,6 +2398,10 @@ impl Cargo {
         cargo
     }
 
+    pub fn into_cmd(self) -> BootstrapCommand {
+        self.into()
+    }
+
     /// Same as `Cargo::new` except this one doesn't configure the linker with `Cargo::configure_linker`
     pub fn new_for_mir_opt_tests(
         builder: &Builder<'_>,
@@ -2622,9 +2626,3 @@ impl From<Cargo> for BootstrapCommand {
         cargo.command
     }
 }
-
-impl From<Cargo> for Command {
-    fn from(cargo: Cargo) -> Command {
-        BootstrapCommand::from(cargo).command
-    }
-}

src/bootstrap/src/lib.rs

@@ -583,10 +583,7 @@ impl Build {
         // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error).
         // diff-index reports the modifications through the exit status
         let has_local_modifications = self
-            .run(
-                BootstrapCommand::from(submodule_git().args(["diff-index", "--quiet", "HEAD"]))
-                    .allow_failure(),
-            )
+            .run(submodule_git().allow_failure().args(["diff-index", "--quiet", "HEAD"]))
             .is_failure();
         if has_local_modifications {
             self.run(submodule_git().args(["stash", "push"]));
@@ -965,12 +962,12 @@ impl Build {
 
     /// Execute a command and return its output.
     /// This method should be used for all command executions in bootstrap.
-    fn run<C: Into<BootstrapCommand>>(&self, command: C) -> CommandOutput {
+    fn run<C: AsMut<BootstrapCommand>>(&self, mut command: C) -> CommandOutput {
         if self.config.dry_run() {
             return CommandOutput::default();
         }
 
-        let mut command = command.into();
+        let command = command.as_mut();
 
         self.verbose(|| println!("running: {command:?}"));
 

src/bootstrap/src/utils/exec.rs

@@ -50,7 +50,11 @@ pub struct BootstrapCommand {
 
 impl BootstrapCommand {
     pub fn new<S: AsRef<OsStr>>(program: S) -> Self {
-        Command::new(program).into()
+        Self {
+            command: Command::new(program),
+            failure_behavior: BehaviorOnFailure::Exit,
+            output_mode: None,
+        }
     }
 
     pub fn arg<S: AsRef<OsStr>>(&mut self, arg: S) -> &mut Self {
@@ -121,33 +125,11 @@ impl BootstrapCommand {
     }
 }
 
-/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to
-/// `BootstrapCommand`.
-impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand {
-    fn from(command: &'a mut BootstrapCommand) -> Self {
-        // This is essentially a manual `Command::clone`
-        let mut cmd = Command::new(command.command.get_program());
-        if let Some(dir) = command.command.get_current_dir() {
-            cmd.current_dir(dir);
-        }
-        cmd.args(command.command.get_args());
-        for (key, value) in command.command.get_envs() {
-            match value {
-                Some(value) => {
-                    cmd.env(key, value);
-                }
-                None => {
-                    cmd.env_remove(key);
-                }
-            }
-        }
-        cmd.into()
-    }
-}
-
-impl From<Command> for BootstrapCommand {
-    fn from(command: Command) -> Self {
-        Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: None }
+/// This implementation exists to make it possible to pass both [BootstrapCommand] and
+/// `&mut BootstrapCommand` to `Build.run()`.
+impl AsMut<BootstrapCommand> for BootstrapCommand {
+    fn as_mut(&mut self) -> &mut BootstrapCommand {
+        self
     }
 }