From 54e704437b3968c3cb14bf8ceb08ca34610425ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Sun, 9 Jun 2024 14:21:04 +0000 Subject: [PATCH] run-make-support: arm command with drop bombs - Update all command wrappers and command construction helpers with `#[track_caller]` where suitable to help the drop bomb panic message. - Remove `Deref`/`DerefMut` for `Command` because it was causing issues with resolving to `std::process::Command` in a method call chain. --- src/tools/run-make-support/src/cc.rs | 7 +- src/tools/run-make-support/src/clang.rs | 2 + src/tools/run-make-support/src/command.rs | 115 +++++++++++++----- src/tools/run-make-support/src/diff/mod.rs | 10 +- src/tools/run-make-support/src/lib.rs | 25 ++-- .../run-make-support/src/llvm_readobj.rs | 2 + src/tools/run-make-support/src/run.rs | 30 ++--- src/tools/run-make-support/src/rustc.rs | 5 + src/tools/run-make-support/src/rustdoc.rs | 5 + 9 files changed, 137 insertions(+), 64 deletions(-) diff --git a/src/tools/run-make-support/src/cc.rs b/src/tools/run-make-support/src/cc.rs index 5c0158d7547..8694740cfc3 100644 --- a/src/tools/run-make-support/src/cc.rs +++ b/src/tools/run-make-support/src/cc.rs @@ -7,6 +7,7 @@ /// /// WARNING: This means that what flags are accepted by the underlying C compiler is /// platform- AND compiler-specific. Consult the relevant docs for `gcc`, `clang` and `mvsc`. +#[track_caller] pub fn cc() -> Cc { Cc::new() } @@ -25,6 +26,7 @@ impl Cc { /// /// WARNING: This means that what flags are accepted by the underlying C compile is /// platform- AND compiler-specific. Consult the relevant docs for `gcc`, `clang` and `mvsc`. + #[track_caller] pub fn new() -> Self { let compiler = env_var("CC"); @@ -84,11 +86,6 @@ pub fn output>(&mut self, path: P) -> &mut Self { self.cmd.arg(path.as_ref()); self } - - /// Get the [`Output`][::std::process::Output] of the finished process. - pub fn command_output(&mut self) -> ::std::process::Output { - self.cmd.output().expect("failed to get output of finished process") - } } /// `EXTRACFLAGS` diff --git a/src/tools/run-make-support/src/clang.rs b/src/tools/run-make-support/src/clang.rs index d2ebed7ab06..a31004659c1 100644 --- a/src/tools/run-make-support/src/clang.rs +++ b/src/tools/run-make-support/src/clang.rs @@ -4,6 +4,7 @@ use crate::{bin_name, env_var}; /// Construct a new `clang` invocation. `clang` is not always available for all targets. +#[track_caller] pub fn clang() -> Clang { Clang::new() } @@ -18,6 +19,7 @@ pub struct Clang { impl Clang { /// Construct a new `clang` invocation. `clang` is not always available for all targets. + #[track_caller] pub fn new() -> Self { let clang = env_var("CLANG"); let cmd = Command::new(clang); diff --git a/src/tools/run-make-support/src/command.rs b/src/tools/run-make-support/src/command.rs index b9e56ab632a..f427e50906f 100644 --- a/src/tools/run-make-support/src/command.rs +++ b/src/tools/run-make-support/src/command.rs @@ -1,36 +1,107 @@ -use crate::{assert_not_contains, handle_failed_output}; +use std::ffi; use std::ffi::OsStr; use std::io::Write; -use std::ops::{Deref, DerefMut}; +use std::panic; +use std::path::Path; use std::process::{Command as StdCommand, ExitStatus, Output, Stdio}; -/// This is a custom command wrapper that simplifies working with commands -/// and makes it easier to ensure that we check the exit status of executed -/// processes. +use crate::drop_bomb::DropBomb; +use crate::{assert_not_contains, handle_failed_output}; + +/// This is a custom command wrapper that simplifies working with commands and makes it easier to +/// ensure that we check the exit status of executed processes. +/// +/// # A [`Command`] must be executed +/// +/// A [`Command`] is armed by a [`DropBomb`] on construction to enforce that it will be executed. If +/// a [`Command`] is constructed but never executed, the drop bomb will explode and cause the test +/// to panic. Execution methods [`run`] and [`run_fail`] will defuse the drop bomb. A test +/// containing constructed but never executed commands is dangerous because it can give a false +/// sense of confidence. +/// +/// [`run`]: Self::run +/// [`run_fail`]: Self::run_fail #[derive(Debug)] pub struct Command { cmd: StdCommand, stdin: Option>, + drop_bomb: DropBomb, } impl Command { - pub fn new>(program: S) -> Self { - Self { cmd: StdCommand::new(program), stdin: None } + #[track_caller] + pub fn new>(program: P) -> Self { + let program = program.as_ref(); + Self { cmd: StdCommand::new(program), stdin: None, drop_bomb: DropBomb::arm(program) } } pub fn set_stdin(&mut self, stdin: Box<[u8]>) { self.stdin = Some(stdin); } + /// Specify an environment variable. + pub fn env(&mut self, key: K, value: V) -> &mut Self + where + K: AsRef, + V: AsRef, + { + self.cmd.env(key, value); + self + } + + /// Remove an environmental variable. + pub fn env_remove(&mut self, key: K) -> &mut Self + where + K: AsRef, + { + self.cmd.env_remove(key); + self + } + + /// Generic command argument provider. Prefer specific helper methods if possible. + /// Note that for some executables, arguments might be platform specific. For C/C++ + /// compilers, arguments might be platform *and* compiler specific. + pub fn arg(&mut self, arg: S) -> &mut Self + where + S: AsRef, + { + self.cmd.arg(arg); + self + } + + /// Generic command arguments provider. Prefer specific helper methods if possible. + /// Note that for some executables, arguments might be platform specific. For C/C++ + /// compilers, arguments might be platform *and* compiler specific. + pub fn args(&mut self, args: &[S]) -> &mut Self + where + S: AsRef, + { + self.cmd.args(args); + self + } + + /// Inspect what the underlying [`std::process::Command`] is up to the + /// current construction. + pub fn inspect(&mut self, inspector: I) -> &mut Self + where + I: FnOnce(&StdCommand), + { + inspector(&self.cmd); + self + } + + /// Set the path where the command will be run. + pub fn current_dir>(&mut self, path: P) -> &mut Self { + self.cmd.current_dir(path); + self + } + /// Run the constructed command and assert that it is successfully run. #[track_caller] pub fn run(&mut self) -> CompletedProcess { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - let output = self.command_output(); if !output.status().success() { - handle_failed_output(&self, output, caller_line_number); + handle_failed_output(&self, output, panic::Location::caller().line()); } output } @@ -38,18 +109,16 @@ pub fn run(&mut self) -> CompletedProcess { /// Run the constructed command and assert that it does not successfully run. #[track_caller] pub fn run_fail(&mut self) -> CompletedProcess { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - let output = self.command_output(); if output.status().success() { - handle_failed_output(&self, output, caller_line_number); + handle_failed_output(&self, output, panic::Location::caller().line()); } output } #[track_caller] - pub(crate) fn command_output(&mut self) -> CompletedProcess { + fn command_output(&mut self) -> CompletedProcess { + self.drop_bomb.defuse(); // let's make sure we piped all the input and outputs self.cmd.stdin(Stdio::piped()); self.cmd.stdout(Stdio::piped()); @@ -71,20 +140,6 @@ pub(crate) fn command_output(&mut self) -> CompletedProcess { } } -impl Deref for Command { - type Target = StdCommand; - - fn deref(&self) -> &Self::Target { - &self.cmd - } -} - -impl DerefMut for Command { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.cmd - } -} - /// Represents the result of an executed process. /// The various `assert_` helper methods should preferably be used for /// checking the contents of stdout/stderr. diff --git a/src/tools/run-make-support/src/diff/mod.rs b/src/tools/run-make-support/src/diff/mod.rs index d864ddf4eb1..0fb6fec9d58 100644 --- a/src/tools/run-make-support/src/diff/mod.rs +++ b/src/tools/run-make-support/src/diff/mod.rs @@ -2,9 +2,12 @@ use similar::TextDiff; use std::path::Path; +use crate::drop_bomb::DropBomb; + #[cfg(test)] mod tests; +#[track_caller] pub fn diff() -> Diff { Diff::new() } @@ -16,10 +19,12 @@ pub struct Diff { actual: Option, actual_name: Option, normalizers: Vec<(String, String)>, + drop_bomb: DropBomb, } impl Diff { /// Construct a bare `diff` invocation. + #[track_caller] pub fn new() -> Self { Self { expected: None, @@ -27,6 +32,7 @@ pub fn new() -> Self { actual: None, actual_name: None, normalizers: Vec::new(), + drop_bomb: DropBomb::arm("diff"), } } @@ -79,9 +85,9 @@ pub fn normalize, I: Into>( self } - /// Executes the diff process, prints any differences to the standard error. #[track_caller] - pub fn run(&self) { + pub fn run(&mut self) { + self.drop_bomb.defuse(); let expected = self.expected.as_ref().expect("expected text not set"); let mut actual = self.actual.as_ref().expect("actual text not set").to_string(); let expected_name = self.expected_name.as_ref().unwrap(); diff --git a/src/tools/run-make-support/src/lib.rs b/src/tools/run-make-support/src/lib.rs index 20ad25efaf0..bf74d94f911 100644 --- a/src/tools/run-make-support/src/lib.rs +++ b/src/tools/run-make-support/src/lib.rs @@ -17,6 +17,7 @@ use std::ffi::OsString; use std::fs; use std::io; +use std::panic; use std::path::{Path, PathBuf}; pub use gimli; @@ -32,6 +33,7 @@ pub use rustc::{aux_build, rustc, Rustc}; pub use rustdoc::{bare_rustdoc, rustdoc, Rustdoc}; +#[track_caller] pub fn env_var(name: &str) -> String { match env::var(name) { Ok(v) => v, @@ -39,6 +41,7 @@ pub fn env_var(name: &str) -> String { } } +#[track_caller] pub fn env_var_os(name: &str) -> OsString { match env::var_os(name) { Some(v) => v, @@ -66,11 +69,13 @@ pub fn is_darwin() -> bool { target().contains("darwin") } +#[track_caller] pub fn python_command() -> Command { let python_path = env_var("PYTHON"); Command::new(python_path) } +#[track_caller] pub fn htmldocck() -> Command { let mut python = python_command(); python.arg(source_root().join("src/etc/htmldocck.py")); @@ -162,15 +167,13 @@ pub fn cwd() -> PathBuf { /// available on the platform! #[track_caller] pub fn cygpath_windows>(path: P) -> String { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - + let caller = panic::Location::caller(); let mut cygpath = Command::new("cygpath"); cygpath.arg("-w"); cygpath.arg(path.as_ref()); - let output = cygpath.command_output(); + let output = cygpath.run(); if !output.status().success() { - handle_failed_output(&cygpath, output, caller_line_number); + handle_failed_output(&cygpath, output, caller.line()); } // cygpath -w can attach a newline output.stdout_utf8().trim().to_string() @@ -179,13 +182,11 @@ pub fn cygpath_windows>(path: P) -> String { /// Run `uname`. This assumes that `uname` is available on the platform! #[track_caller] pub fn uname() -> String { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - + let caller = panic::Location::caller(); let mut uname = Command::new("uname"); - let output = uname.command_output(); + let output = uname.run(); if !output.status().success() { - handle_failed_output(&uname, output, caller_line_number); + handle_failed_output(&uname, output, caller.line()); } output.stdout_utf8() } @@ -393,7 +394,7 @@ pub fn inspect(&mut self, inspector: I) -> &mut Self where I: FnOnce(&::std::process::Command), { - inspector(&self.cmd); + self.cmd.inspect(inspector); self } @@ -410,7 +411,7 @@ pub fn run_fail(&mut self) -> crate::command::CompletedProcess { } /// Set the path where the command will be run. - pub fn current_dir>(&mut self, path: P) -> &mut Self { + pub fn current_dir>(&mut self, path: P) -> &mut Self { self.cmd.current_dir(path); self } diff --git a/src/tools/run-make-support/src/llvm_readobj.rs b/src/tools/run-make-support/src/llvm_readobj.rs index db2f9db6e41..57ddfc205e6 100644 --- a/src/tools/run-make-support/src/llvm_readobj.rs +++ b/src/tools/run-make-support/src/llvm_readobj.rs @@ -5,6 +5,7 @@ /// Construct a new `llvm-readobj` invocation. This assumes that `llvm-readobj` is available /// at `$LLVM_BIN_DIR/llvm-readobj`. +#[track_caller] pub fn llvm_readobj() -> LlvmReadobj { LlvmReadobj::new() } @@ -20,6 +21,7 @@ pub struct LlvmReadobj { impl LlvmReadobj { /// Construct a new `llvm-readobj` invocation. This assumes that `llvm-readobj` is available /// at `$LLVM_BIN_DIR/llvm-readobj`. + #[track_caller] pub fn new() -> Self { let llvm_bin_dir = env_var("LLVM_BIN_DIR"); let llvm_bin_dir = PathBuf::from(llvm_bin_dir); diff --git a/src/tools/run-make-support/src/run.rs b/src/tools/run-make-support/src/run.rs index 4a6fd7c432e..6fa1a75363c 100644 --- a/src/tools/run-make-support/src/run.rs +++ b/src/tools/run-make-support/src/run.rs @@ -1,5 +1,6 @@ use std::env; use std::ffi::OsStr; +use std::panic; use std::path::{Path, PathBuf}; use crate::command::{Command, CompletedProcess}; @@ -7,7 +8,8 @@ use super::handle_failed_output; -fn run_common(name: &str) -> (Command, CompletedProcess) { +#[track_caller] +fn run_common(name: &str) -> Command { let mut bin_path = PathBuf::new(); bin_path.push(cwd()); bin_path.push(name); @@ -34,19 +36,17 @@ fn run_common(name: &str) -> (Command, CompletedProcess) { cmd.env("PATH", env::join_paths(paths.iter()).unwrap()); } - let output = cmd.command_output(); - (cmd, output) + cmd } /// Run a built binary and make sure it succeeds. #[track_caller] pub fn run(name: &str) -> CompletedProcess { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - - let (cmd, output) = run_common(name); + let caller = panic::Location::caller(); + let mut cmd = run_common(name); + let output = cmd.run(); if !output.status().success() { - handle_failed_output(&cmd, output, caller_line_number); + handle_failed_output(&cmd, output, caller.line()); } output } @@ -54,18 +54,18 @@ pub fn run(name: &str) -> CompletedProcess { /// Run a built binary and make sure it fails. #[track_caller] pub fn run_fail(name: &str) -> CompletedProcess { - let caller_location = std::panic::Location::caller(); - let caller_line_number = caller_location.line(); - - let (cmd, output) = run_common(name); + let caller = panic::Location::caller(); + let mut cmd = run_common(name); + let output = cmd.run_fail(); if output.status().success() { - handle_failed_output(&cmd, output, caller_line_number); + handle_failed_output(&cmd, output, caller.line()); } output } -/// Create a new custom Command. -/// This should be preferred to creating `std::process::Command` directly. +/// Create a new custom [`Command`]. This should be preferred to creating [`std::process::Command`] +/// directly. +#[track_caller] pub fn cmd>(program: S) -> Command { let mut command = Command::new(program); set_host_rpath(&mut command); diff --git a/src/tools/run-make-support/src/rustc.rs b/src/tools/run-make-support/src/rustc.rs index 5ea231442bc..32fa5018d80 100644 --- a/src/tools/run-make-support/src/rustc.rs +++ b/src/tools/run-make-support/src/rustc.rs @@ -5,11 +5,13 @@ use crate::{command, cwd, env_var, set_host_rpath}; /// Construct a new `rustc` invocation. +#[track_caller] pub fn rustc() -> Rustc { Rustc::new() } /// Construct a new `rustc` aux-build invocation. +#[track_caller] pub fn aux_build() -> Rustc { Rustc::new_aux_build() } @@ -22,6 +24,7 @@ pub struct Rustc { crate::impl_common_helpers!(Rustc); +#[track_caller] fn setup_common() -> Command { let rustc = env_var("RUSTC"); let mut cmd = Command::new(rustc); @@ -34,12 +37,14 @@ impl Rustc { // `rustc` invocation constructor methods /// Construct a new `rustc` invocation. + #[track_caller] pub fn new() -> Self { let cmd = setup_common(); Self { cmd } } /// Construct a new `rustc` invocation with `aux_build` preset (setting `--crate-type=lib`). + #[track_caller] pub fn new_aux_build() -> Self { let mut cmd = setup_common(); cmd.arg("--crate-type=lib"); diff --git a/src/tools/run-make-support/src/rustdoc.rs b/src/tools/run-make-support/src/rustdoc.rs index 39a698a47b4..332906f739a 100644 --- a/src/tools/run-make-support/src/rustdoc.rs +++ b/src/tools/run-make-support/src/rustdoc.rs @@ -5,11 +5,13 @@ use crate::{env_var, env_var_os, set_host_rpath}; /// Construct a plain `rustdoc` invocation with no flags set. +#[track_caller] pub fn bare_rustdoc() -> Rustdoc { Rustdoc::bare() } /// Construct a new `rustdoc` invocation with `-L $(TARGET_RPATH_DIR)` set. +#[track_caller] pub fn rustdoc() -> Rustdoc { Rustdoc::new() } @@ -21,6 +23,7 @@ pub struct Rustdoc { crate::impl_common_helpers!(Rustdoc); +#[track_caller] fn setup_common() -> Command { let rustdoc = env_var("RUSTDOC"); let mut cmd = Command::new(rustdoc); @@ -30,12 +33,14 @@ fn setup_common() -> Command { impl Rustdoc { /// Construct a bare `rustdoc` invocation. + #[track_caller] pub fn bare() -> Self { let cmd = setup_common(); Self { cmd } } /// Construct a `rustdoc` invocation with `-L $(TARGET_RPATH_DIR)` set. + #[track_caller] pub fn new() -> Self { let mut cmd = setup_common(); let target_rpath_dir = env_var_os("TARGET_RPATH_DIR");