From fe9c89ccbaaa91a443b4207c9b24d58a27ec312f Mon Sep 17 00:00:00 2001 From: rhysd Date: Mon, 4 Oct 2021 22:34:19 +0900 Subject: [PATCH 1/3] Parse --shell option value as shell command --- Cargo.lock | 7 ++++++ Cargo.toml | 1 + src/benchmark.rs | 12 ++++----- src/error.rs | 6 +++++ src/main.rs | 9 +++---- src/options.rs | 64 ++++++++++++++++++++++++++++++++++++++++++++++-- src/shell.rs | 17 +++++++------ 7 files changed, 96 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 670836201..bb05cde17 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -243,6 +243,7 @@ dependencies = [ "rust_decimal", "serde", "serde_json", + "shell-words", "statistical", "tempfile", "winapi", @@ -680,6 +681,12 @@ dependencies = [ "serde", ] +[[package]] +name = "shell-words" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6fa3938c99da4914afedd13bf3d79bcb6c277d1b2c398d23257a304d9e1b074" + [[package]] name = "statistical" version = "1.0.0" diff --git a/Cargo.toml b/Cargo.toml index c6c825a1e..00690eba3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" rust_decimal = "1.15" rand = "0.8" +shell-words = "1.0" [target.'cfg(not(windows))'.dependencies] libc = "0.2" diff --git a/src/benchmark.rs b/src/benchmark.rs index 47870ca33..5a0526976 100644 --- a/src/benchmark.rs +++ b/src/benchmark.rs @@ -9,7 +9,7 @@ use crate::benchmark_result::BenchmarkResult; use crate::command::Command; use crate::format::{format_duration, format_duration_unit}; use crate::min_max::{max, min}; -use crate::options::{CmdFailureAction, HyperfineOptions, OutputStyleOption}; +use crate::options::{CmdFailureAction, HyperfineOptions, OutputStyleOption, Shell}; use crate::outlier_detection::{modified_zscores, OUTLIER_THRESHOLD}; use crate::progress_bar::get_progress_bar; use crate::shell::execute_and_time; @@ -45,7 +45,7 @@ fn subtract_shell_spawning_time(time: Second, shell_spawning_time: Second) -> Se /// Run the given shell command and measure the execution time pub fn time_shell_command( - shell: &str, + shell: &Shell, command: &Command<'_>, show_output: bool, failure_action: CmdFailureAction, @@ -98,7 +98,7 @@ pub fn time_shell_command( /// Measure the average shell spawning time pub fn mean_shell_spawning_time( - shell: &str, + shell: &Shell, style: OutputStyleOption, show_output: bool, ) -> io::Result { @@ -168,7 +168,7 @@ pub fn mean_shell_spawning_time( } fn run_intermediate_command( - shell: &str, + shell: &Shell, command: &Option>, show_output: bool, error_output: &'static str, @@ -187,7 +187,7 @@ fn run_intermediate_command( /// Run the command specified by `--prepare`. fn run_preparation_command( - shell: &str, + shell: &Shell, command: &Option>, show_output: bool, ) -> io::Result { @@ -199,7 +199,7 @@ fn run_preparation_command( /// Run the command specified by `--cleanup`. fn run_cleanup_command( - shell: &str, + shell: &Shell, command: &Option>, show_output: bool, ) -> io::Result { diff --git a/src/error.rs b/src/error.rs index 925779048..298b310dc 100644 --- a/src/error.rs +++ b/src/error.rs @@ -62,6 +62,8 @@ pub enum OptionsError<'a> { TooManyCommandNames(usize), UnexpectedCommandNameCount(usize, usize), NumericParsingError(&'a str, ParseIntError), + EmptyShell, + ShellParseError(shell_words::ParseError), } impl<'a> fmt::Display for OptionsError<'a> { @@ -85,6 +87,10 @@ impl<'a> fmt::Display for OptionsError<'a> { cmd = cmd, err = err ), + OptionsError::EmptyShell => write!(f, "Empty command at --shell option"), + OptionsError::ShellParseError(ref err) => { + write!(f, "Could not parse --shell value as command line: {}", err) + } } } } diff --git a/src/main.rs b/src/main.rs index 7b10e6982..a5c048254 100644 --- a/src/main.rs +++ b/src/main.rs @@ -33,7 +33,7 @@ use benchmark_result::BenchmarkResult; use command::Command; use error::OptionsError; use export::{ExportManager, ExportType}; -use options::{CmdFailureAction, HyperfineOptions, OutputStyleOption}; +use options::{CmdFailureAction, HyperfineOptions, OutputStyleOption, Shell}; use parameter_range::get_parameterized_commands; use tokenize::tokenize; use types::ParameterValue; @@ -229,10 +229,9 @@ fn build_hyperfine_options<'a>( OutputStyleOption::Disabled => {} }; - options.shell = matches - .value_of("shell") - .unwrap_or(&options.shell) - .to_string(); + if let Some(shell) = matches.value_of("shell") { + options.shell = Shell::parse(shell)?; + } if matches.is_present("ignore-failure") { options.failure_action = CmdFailureAction::Ignore; diff --git a/src/options.rs b/src/options.rs index 2ffaccd49..228d3c296 100644 --- a/src/options.rs +++ b/src/options.rs @@ -1,3 +1,7 @@ +use std::fmt; +use std::process::Command; + +use crate::error::OptionsError; use crate::units::{Second, Unit}; #[cfg(not(windows))] @@ -6,6 +10,62 @@ pub const DEFAULT_SHELL: &str = "sh"; #[cfg(windows)] pub const DEFAULT_SHELL: &str = "cmd.exe"; +/// Shell to use for executing benchmarked commands +pub enum Shell { + /// Default shell command + Default(&'static str), + /// Custom shell command specified via --shell + Custom(Vec), +} + +impl Default for Shell { + fn default() -> Self { + Shell::Default(DEFAULT_SHELL) + } +} + +impl fmt::Display for Shell { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Shell::Default(cmd) => write!(f, "{}", cmd), + Shell::Custom(cmdline) => { + let mut first = true; + for s in cmdline.iter() { + if first { + first = false; + write!(f, "{}", s)?; + } else { + write!(f, " {}", s)?; + } + } + Ok(()) + } + } + } +} + +impl Shell { + /// Parse given string as shell command line + pub fn parse<'a>(s: &str) -> Result> { + let v = shell_words::split(s.as_ref()).map_err(OptionsError::ShellParseError)?; + if v.is_empty() { + return Err(OptionsError::EmptyShell); + } + Ok(Shell::Custom(v)) + } + + pub fn command(&self) -> Command { + match self { + Shell::Default(cmd) => Command::new(cmd), + Shell::Custom(cmdline) => { + let mut c = Command::new(&cmdline[0]); + c.args(&cmdline[1..]); + c + } + } + } +} + /// Action to take when an executed command fails. #[derive(Debug, Clone, Copy, PartialEq)] pub enum CmdFailureAction { @@ -74,7 +134,7 @@ pub struct HyperfineOptions { pub output_style: OutputStyleOption, /// The shell to use for executing commands. - pub shell: String, + pub shell: Shell, /// Forward benchmark's stdout to hyperfine's stdout pub show_output: bool, @@ -99,7 +159,7 @@ impl Default for HyperfineOptions { preparation_command: None, cleanup_command: None, output_style: OutputStyleOption::Full, - shell: DEFAULT_SHELL.to_string(), + shell: Shell::default(), show_output: false, time_unit: None, } diff --git a/src/shell.rs b/src/shell.rs index c47ee59ae..22f34f872 100644 --- a/src/shell.rs +++ b/src/shell.rs @@ -1,6 +1,7 @@ use std::io; -use std::process::{Command, ExitStatus, Stdio}; +use std::process::{ExitStatus, Stdio}; +use crate::options::Shell; use crate::timer::get_cpu_timer; /// Used to indicate the result of running a command @@ -22,7 +23,7 @@ pub fn execute_and_time( stdout: Stdio, stderr: Stdio, command: &str, - shell: &str, + shell: &Shell, ) -> io::Result { let mut child = run_shell_command(stdout, stderr, command, shell)?; let cpu_timer = get_cpu_timer(&child); @@ -42,7 +43,7 @@ pub fn execute_and_time( stdout: Stdio, stderr: Stdio, command: &str, - shell: &str, + shell: &Shell, ) -> io::Result { let cpu_timer = get_cpu_timer(); @@ -63,9 +64,10 @@ fn run_shell_command( stdout: Stdio, stderr: Stdio, command: &str, - shell: &str, + shell: &Shell, ) -> io::Result { - Command::new(shell) + shell + .command() .arg("-c") .arg(command) .env( @@ -84,9 +86,10 @@ fn run_shell_command( stdout: Stdio, stderr: Stdio, command: &str, - shell: &str, + shell: &Shell, ) -> io::Result { - Command::new(shell) + shell + .command() .arg("/C") .arg(command) .stdin(Stdio::null()) From a6a85d985a02e988dbc96546d2299322ffec6bd4 Mon Sep 17 00:00:00 2001 From: rhysd Date: Mon, 4 Oct 2021 22:49:37 +0900 Subject: [PATCH 2/3] Add tests for options::Shell --- src/options.rs | 61 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/src/options.rs b/src/options.rs index 228d3c296..f3eafc3e9 100644 --- a/src/options.rs +++ b/src/options.rs @@ -11,6 +11,7 @@ pub const DEFAULT_SHELL: &str = "sh"; pub const DEFAULT_SHELL: &str = "cmd.exe"; /// Shell to use for executing benchmarked commands +#[derive(Debug)] pub enum Shell { /// Default shell command Default(&'static str), @@ -28,18 +29,7 @@ impl fmt::Display for Shell { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Shell::Default(cmd) => write!(f, "{}", cmd), - Shell::Custom(cmdline) => { - let mut first = true; - for s in cmdline.iter() { - if first { - first = false; - write!(f, "{}", s)?; - } else { - write!(f, " {}", s)?; - } - } - Ok(()) - } + Shell::Custom(cmdline) => write!(f, "{}", shell_words::join(cmdline)), } } } @@ -48,7 +38,7 @@ impl Shell { /// Parse given string as shell command line pub fn parse<'a>(s: &str) -> Result> { let v = shell_words::split(s.as_ref()).map_err(OptionsError::ShellParseError)?; - if v.is_empty() { + if v.is_empty() || v[0].is_empty() { return Err(OptionsError::EmptyShell); } Ok(Shell::Custom(v)) @@ -165,3 +155,48 @@ impl Default for HyperfineOptions { } } } + +#[test] +fn test_shell_default_command() { + let shell = Shell::default(); + + let s = format!("{}", shell); + assert_eq!(&s, DEFAULT_SHELL); + + let cmd = shell.command(); + // Command::get_program is not yet available in stable channel. + // https://doc.rust-lang.org/std/process/struct.Command.html#method.get_program + let s = format!("{:?}", cmd); + assert_eq!(s, format!("\"{}\"", DEFAULT_SHELL)); +} + +#[test] +fn test_shell_parse_command() { + let shell = Shell::parse("shell -x 'aaa bbb'").unwrap(); + + let s = format!("{}", shell); + assert_eq!(&s, "shell -x 'aaa bbb'"); + + let cmd = shell.command(); + // Command::get_program and Command::args are not yet available in stable channel. + // https://doc.rust-lang.org/std/process/struct.Command.html#method.get_program + let s = format!("{:?}", cmd); + assert_eq!(&s, r#""shell" "-x" "aaa bbb""#); + + // Error cases + + match Shell::parse("shell 'foo").unwrap_err() { + OptionsError::ShellParseError(_) => { /* ok */ } + e => assert!(false, "Unexpected error: {}", e), + } + + match Shell::parse("").unwrap_err() { + OptionsError::EmptyShell => { /* ok */ } + e => assert!(false, "Unexpected error: {}", e), + } + + match Shell::parse("''").unwrap_err() { + OptionsError::EmptyShell => { /* ok */ } + e => assert!(false, "Unexpected error: {}", e), + } +} From 230dd6b64ed19931a756910c7ba55924209be7e5 Mon Sep 17 00:00:00 2001 From: rhysd Date: Mon, 4 Oct 2021 23:07:38 +0900 Subject: [PATCH 3/3] Fix clippy warnings --- src/export/mod.rs | 6 ++++-- src/main.rs | 2 +- src/options.rs | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/export/mod.rs b/src/export/mod.rs index aa2718556..c4dc88716 100644 --- a/src/export/mod.rs +++ b/src/export/mod.rs @@ -46,14 +46,16 @@ pub struct ExportManager { exporters: Vec, } -impl ExportManager { +impl Default for ExportManager { /// Create a new ExportManager - pub fn new() -> ExportManager { + fn default() -> ExportManager { ExportManager { exporters: Vec::new(), } } +} +impl ExportManager { /// Add an additional exporter to the ExportManager pub fn add_exporter(&mut self, export_type: ExportType, filename: &str) -> Result<()> { let _ = File::create(filename)?; diff --git a/src/main.rs b/src/main.rs index a5c048254..cfd933a20 100644 --- a/src/main.rs +++ b/src/main.rs @@ -249,7 +249,7 @@ fn build_hyperfine_options<'a>( /// Build the ExportManager that will export the results specified /// in the given ArgMatches fn build_export_manager(matches: &ArgMatches<'_>) -> io::Result { - let mut export_manager = ExportManager::new(); + let mut export_manager = ExportManager::default(); { let mut add_exporter = |flag, exporttype| -> io::Result<()> { if let Some(filename) = matches.value_of(flag) { diff --git a/src/options.rs b/src/options.rs index f3eafc3e9..20706b72b 100644 --- a/src/options.rs +++ b/src/options.rs @@ -37,7 +37,7 @@ impl fmt::Display for Shell { impl Shell { /// Parse given string as shell command line pub fn parse<'a>(s: &str) -> Result> { - let v = shell_words::split(s.as_ref()).map_err(OptionsError::ShellParseError)?; + let v = shell_words::split(s).map_err(OptionsError::ShellParseError)?; if v.is_empty() || v[0].is_empty() { return Err(OptionsError::EmptyShell); }