From bed2cbd2cef6080ba35fe1554f6ba82634590134 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 22 Jun 2024 11:33:11 +0200 Subject: [PATCH] Get rid of `Deref/DerefMut` impl for `BootstrapCmd` --- src/bootstrap/src/core/build_steps/suggest.rs | 1 + src/bootstrap/src/core/build_steps/test.rs | 21 +++++++----- src/bootstrap/src/core/builder.rs | 2 +- src/bootstrap/src/lib.rs | 2 +- src/bootstrap/src/utils/exec.rs | 34 ++++++++----------- src/bootstrap/src/utils/helpers.rs | 4 +-- src/bootstrap/src/utils/render_tests.rs | 17 +++++++--- 7 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/suggest.rs b/src/bootstrap/src/core/build_steps/suggest.rs index 754d1e61da8..7c5e0d4e13e 100644 --- a/src/bootstrap/src/core/build_steps/suggest.rs +++ b/src/bootstrap/src/core/build_steps/suggest.rs @@ -16,6 +16,7 @@ pub fn suggest(builder: &Builder<'_>, run: bool) { .tool_cmd(Tool::SuggestTests) .env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository) .env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch) + .command .output() .expect("failed to run `suggest-tests` tool"); diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index db6f54a49ac..e5243e26f2b 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -482,8 +482,10 @@ pub fn build_miri_sysroot( String::new() } else { builder.verbose(|| println!("running: {cargo:?}")); - let out = - cargo.output().expect("We already ran `cargo miri setup` before and that worked"); + let out = cargo + .command + .output() + .expect("We already ran `cargo miri setup` before and that worked"); assert!(out.status.success(), "`cargo miri setup` returned with non-0 exit code"); // Output is "\n". let stdout = String::from_utf8(out.stdout) @@ -2065,7 +2067,8 @@ fn run(self, builder: &Builder<'_>) { cmd.arg("--git-repository").arg(git_config.git_repository); cmd.arg("--nightly-branch").arg(git_config.nightly_branch); - builder.ci_env.force_coloring_in_ci(&mut cmd); + // FIXME: Move CiEnv back to bootstrap, it is only used here anyway + builder.ci_env.force_coloring_in_ci(&mut cmd.command); #[cfg(feature = "build-metrics")] builder.metrics.begin_test_suite( @@ -2424,7 +2427,7 @@ fn run(self, builder: &Builder<'_>) { /// Returns whether the test succeeded. #[allow(clippy::too_many_arguments)] // FIXME: reduce the number of args and remove this. fn run_cargo_test<'a>( - cargo: impl Into, + cargo: impl Into, libtest_args: &[&str], crates: &[String], primary_crate: &str, @@ -2455,14 +2458,14 @@ fn run_cargo_test<'a>( /// Given a `cargo test` subcommand, pass it the appropriate test flags given a `builder`. fn prepare_cargo_test( - cargo: impl Into, + cargo: impl Into, libtest_args: &[&str], crates: &[String], primary_crate: &str, compiler: Compiler, target: TargetSelection, builder: &Builder<'_>, -) -> Command { +) -> BootstrapCommand { let mut cargo = cargo.into(); // Propegate `--bless` if it has not already been set/unset @@ -2978,7 +2981,7 @@ fn run(self, builder: &Builder<'_>) { // Some tests require cargo submodule to be present. builder.build.update_submodule(Path::new("src/tools/cargo")); - let mut check_bootstrap = Command::new(builder.python()); + let mut check_bootstrap = BootstrapCommand::new(builder.python()); check_bootstrap .args(["-m", "unittest", "bootstrap_test.py"]) .env("BUILD_DIR", &builder.out) @@ -2986,9 +2989,9 @@ fn run(self, builder: &Builder<'_>) { .current_dir(builder.src.join("src/bootstrap/")); // NOTE: we intentionally don't pass test_args here because the args for unittest and cargo test are mutually incompatible. // Use `python -m unittest` manually if you want to pass arguments. - builder.run(BootstrapCommand::from(&mut check_bootstrap).delay_failure()); + builder.run(check_bootstrap.delay_failure()); - let mut cmd = Command::new(&builder.initial_cargo); + let mut cmd = BootstrapCommand::new(&builder.initial_cargo); cmd.arg("test") .args(["--features", "bootstrap-self-test"]) .current_dir(builder.src.join("src/bootstrap")) diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 08866d1a97d..58d6e7a58e3 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -2105,7 +2105,7 @@ fn cargo( // Try to use a sysroot-relative bindir, in case it was configured absolutely. cargo.env("RUSTC_INSTALL_BINDIR", self.config.bindir_relative()); - self.ci_env.force_coloring_in_ci(&mut cargo); + self.ci_env.force_coloring_in_ci(&mut cargo.command); // When we build Rust dylibs they're all intended for intermediate // usage, so make sure we pass the -Cprefer-dynamic flag instead of diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index cee6b344902..8361e01014b 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -937,7 +937,7 @@ fn test_helpers_out(&self, target: TargetSelection) -> PathBuf { } /// Adds the `RUST_TEST_THREADS` env var if necessary - fn add_rust_test_threads(&self, cmd: &mut Command) { + fn add_rust_test_threads(&self, cmd: &mut BootstrapCommand) { if env::var_os("RUST_TEST_THREADS").is_none() { cmd.env("RUST_TEST_THREADS", self.jobs().to_string()); } diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 4dbcc2b54f1..8bcb2301f1a 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -1,7 +1,6 @@ use std::ffi::OsStr; -use std::ops::{Deref, DerefMut}; use std::path::Path; -use std::process::{Command, ExitStatus, Output}; +use std::process::{Command, CommandArgs, CommandEnvs, ExitStatus, Output}; /// What should be done when the command fails. #[derive(Debug, Copy, Clone)] @@ -74,6 +73,19 @@ pub fn env(&mut self, key: K, val: V) -> &mut Self self } + pub fn get_envs(&self) -> CommandEnvs<'_> { + self.command.get_envs() + } + + pub fn get_args(&self) -> CommandArgs<'_> { + self.command.get_args() + } + + pub fn env_remove>(&mut self, key: K) -> &mut Self { + self.command.env_remove(key); + self + } + pub fn current_dir>(&mut self, dir: P) -> &mut Self { self.command.current_dir(dir); self @@ -134,24 +146,6 @@ fn from(command: &'a mut BootstrapCommand) -> Self { } } -/// This implementation is temporary, until all `Command` invocations are migrated to -/// `BootstrapCommand`. -impl Deref for BootstrapCommand { - type Target = Command; - - fn deref(&self) -> &Self::Target { - &self.command - } -} - -/// This implementation is temporary, until all `Command` invocations are migrated to -/// `BootstrapCommand`. -impl DerefMut for BootstrapCommand { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.command - } -} - impl From for BootstrapCommand { fn from(command: Command) -> Self { Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: None } diff --git a/src/bootstrap/src/utils/helpers.rs b/src/bootstrap/src/utils/helpers.rs index ee2caef4684..ab53c08ba21 100644 --- a/src/bootstrap/src/utils/helpers.rs +++ b/src/bootstrap/src/utils/helpers.rs @@ -82,7 +82,7 @@ pub fn add_dylib_path(path: Vec, cmd: &mut BootstrapCommand) { } /// Adds a list of lookup paths to `cmd`'s link library lookup path. -pub fn add_link_lib_path(path: Vec, cmd: &mut Command) { +pub fn add_link_lib_path(path: Vec, cmd: &mut BootstrapCommand) { let mut list = link_lib_path(); for path in path { list.insert(0, path); @@ -439,7 +439,7 @@ pub fn linker_flags( } pub fn add_rustdoc_cargo_linker_args( - cmd: &mut Command, + cmd: &mut BootstrapCommand, builder: &Builder<'_>, target: TargetSelection, lld_threads: LldThreads, diff --git a/src/bootstrap/src/utils/render_tests.rs b/src/bootstrap/src/utils/render_tests.rs index 5c9918bce32..2e99bc68a8b 100644 --- a/src/bootstrap/src/utils/render_tests.rs +++ b/src/bootstrap/src/utils/render_tests.rs @@ -7,14 +7,18 @@ //! to reimplement all the rendering logic in this module because of that. use crate::core::builder::Builder; +use crate::utils::exec::BootstrapCommand; use std::io::{BufRead, BufReader, Read, Write}; -use std::process::{ChildStdout, Command, Stdio}; +use std::process::{ChildStdout, Stdio}; use std::time::Duration; use termcolor::{Color, ColorSpec, WriteColor}; const TERSE_TESTS_PER_LINE: usize = 88; -pub(crate) fn add_flags_and_try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { +pub(crate) fn add_flags_and_try_run_tests( + builder: &Builder<'_>, + cmd: &mut BootstrapCommand, +) -> bool { if !cmd.get_args().any(|arg| arg == "--") { cmd.arg("--"); } @@ -23,7 +27,11 @@ pub(crate) fn add_flags_and_try_run_tests(builder: &Builder<'_>, cmd: &mut Comma try_run_tests(builder, cmd, false) } -pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command, stream: bool) -> bool { +pub(crate) fn try_run_tests( + builder: &Builder<'_>, + cmd: &mut BootstrapCommand, + stream: bool, +) -> bool { if builder.config.dry_run() { return true; } @@ -41,7 +49,8 @@ pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command, stream: bo } } -fn run_tests(builder: &Builder<'_>, cmd: &mut Command, stream: bool) -> bool { +fn run_tests(builder: &Builder<'_>, cmd: &mut BootstrapCommand, stream: bool) -> bool { + let cmd = &mut cmd.command; cmd.stdout(Stdio::piped()); builder.verbose(|| println!("running: {cmd:?}"));