Skip to content

Commit

Permalink
Get rid of Deref/DerefMut impl for BootstrapCmd
Browse files Browse the repository at this point in the history
  • Loading branch information
Kobzol committed Jun 22, 2024
1 parent ab0756d commit b906027
Show file tree
Hide file tree
Showing 7 changed files with 44 additions and 37 deletions.
1 change: 1 addition & 0 deletions src/bootstrap/src/core/build_steps/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down
21 changes: 12 additions & 9 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,10 @@ impl Miri {
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 "<sysroot>\n".
let stdout = String::from_utf8(out.stdout)
Expand Down Expand Up @@ -2067,7 +2069,8 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
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(
Expand Down Expand Up @@ -2426,7 +2429,7 @@ impl Step for CrateLibrustc {
/// 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<Command>,
cargo: impl Into<BootstrapCommand>,
libtest_args: &[&str],
crates: &[String],
primary_crate: &str,
Expand Down Expand Up @@ -2457,14 +2460,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<Command>,
cargo: impl Into<BootstrapCommand>,
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
Expand Down Expand Up @@ -2977,17 +2980,17 @@ impl Step for Bootstrap {
let compiler = builder.compiler(0, host);
let _guard = builder.msg(Kind::Test, 0, "bootstrap", host, host);

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)
.env("BUILD_PLATFORM", builder.build.build.triple)
.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"))
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/core/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2085,7 +2085,7 @@ impl<'a> Builder<'a> {
// 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
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -935,7 +935,7 @@ impl Build {
}

/// 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());
}
Expand Down
34 changes: 14 additions & 20 deletions src/bootstrap/src/utils/exec.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down Expand Up @@ -74,6 +73,19 @@ impl BootstrapCommand {
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<K: AsRef<OsStr>>(&mut self, key: K) -> &mut Self {
self.command.env_remove(key);
self
}

pub fn current_dir<P: AsRef<Path>>(&mut self, dir: P) -> &mut Self {
self.command.current_dir(dir);
self
Expand Down Expand Up @@ -134,24 +146,6 @@ impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand {
}
}

/// 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<Command> for BootstrapCommand {
fn from(command: Command) -> Self {
Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: None }
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/src/utils/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ pub fn add_dylib_path(path: Vec<PathBuf>, cmd: &mut BootstrapCommand) {
}

/// Adds a list of lookup paths to `cmd`'s link library lookup path.
pub fn add_link_lib_path(path: Vec<PathBuf>, cmd: &mut Command) {
pub fn add_link_lib_path(path: Vec<PathBuf>, cmd: &mut BootstrapCommand) {
let mut list = link_lib_path();
for path in path {
list.insert(0, path);
Expand Down Expand Up @@ -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,
Expand Down
17 changes: 13 additions & 4 deletions src/bootstrap/src/utils/render_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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("--");
}
Expand All @@ -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;
}
Expand All @@ -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:?}"));
Expand Down

0 comments on commit b906027

Please sign in to comment.