Address review comments

This commit is contained in:
Tavian Barnes authored and David Peter committed 2022-05-17 22:30:45 +02:00
1 parent efdf735723
commit 3df63ba2bb
3 files changed
+28 -31

No files matched your search

+1 -1
View File
@@ -49,6 +49,6 @@ pub enum OptionsError<'a> {
EmptyShell, EmptyShell,
#[error("Failed to parse '--shell <command>' expression as command line: {0}")] #[error("Failed to parse '--shell <command>' expression as command line: {0}")]
ShellParseError(shell_words::ParseError), ShellParseError(shell_words::ParseError),
#[error("Unknown output policy '{0}'. Use ./{0} to output to a file named {0}.")] #[error("Unknown output policy '{0}'. Use './{0}' to output to a file named '{0}'.")]
UnknownOutputPolicy(String), UnknownOutputPolicy(String),
} }
+15 -15
View File
@@ -113,8 +113,8 @@ impl Default for RunBounds {
/// How to handle the output of benchmarked commands /// How to handle the output of benchmarked commands
#[derive(Debug, Clone, PartialEq)] #[derive(Debug, Clone, PartialEq)]
pub enum CommandOutputPolicy { pub enum CommandOutputPolicy {
/// Discard all output /// Redirect output to the null device
Discard, Null,
/// Feed output through a pipe before discarding it /// Feed output through a pipe before discarding it
Pipe, Pipe,
@@ -123,19 +123,19 @@ pub enum CommandOutputPolicy {
File(PathBuf), File(PathBuf),
/// Show command output on the terminal /// Show command output on the terminal
Forward, Inherit,
} }
impl Default for CommandOutputPolicy { impl Default for CommandOutputPolicy {
fn default() -> Self { fn default() -> Self {
CommandOutputPolicy::Discard CommandOutputPolicy::Null
} }
} }
impl CommandOutputPolicy { impl CommandOutputPolicy {
pub fn get_stdout_stderr(&self) -> io::Result<(Stdio, Stdio)> { pub fn get_stdout_stderr(&self) -> io::Result<(Stdio, Stdio)> {
let streams = match self { let streams = match self {
CommandOutputPolicy::Discard => (Stdio::null(), Stdio::null()), CommandOutputPolicy::Null => (Stdio::null(), Stdio::null()),
// Typically only stdout is performance-relevant, so just pipe that // Typically only stdout is performance-relevant, so just pipe that
CommandOutputPolicy::Pipe => (Stdio::piped(), Stdio::null()), CommandOutputPolicy::Pipe => (Stdio::piped(), Stdio::null()),
@@ -145,7 +145,7 @@ impl CommandOutputPolicy {
(file.into(), Stdio::null()) (file.into(), Stdio::null())
} }
CommandOutputPolicy::Forward => (Stdio::inherit(), Stdio::inherit()), CommandOutputPolicy::Inherit => (Stdio::inherit(), Stdio::inherit()),
}; };
Ok(streams) Ok(streams)
@@ -212,7 +212,7 @@ impl Default for Options {
cleanup_command: None, cleanup_command: None,
output_style: OutputStyleOption::Full, output_style: OutputStyleOption::Full,
executor_kind: ExecutorKind::default(), executor_kind: ExecutorKind::default(),
command_output_policy: CommandOutputPolicy::Discard, command_output_policy: CommandOutputPolicy::Null,
time_unit: None, time_unit: None,
} }
} }
@@ -269,12 +269,12 @@ impl Options {
options.cleanup_command = matches.value_of("cleanup").map(String::from); options.cleanup_command = matches.value_of("cleanup").map(String::from);
options.command_output_policy = if matches.is_present("show-output") { options.command_output_policy = if matches.is_present("show-output") {
CommandOutputPolicy::Forward CommandOutputPolicy::Inherit
} else if let Some(output) = matches.value_of("output") { } else if let Some(output) = matches.value_of("output") {
match output { match output {
"null" => CommandOutputPolicy::Discard, "null" => CommandOutputPolicy::Null,
"pipe" => CommandOutputPolicy::Pipe, "pipe" => CommandOutputPolicy::Pipe,
"inherit" => CommandOutputPolicy::Forward, "inherit" => CommandOutputPolicy::Inherit,
arg => { arg => {
let path = PathBuf::from(arg); let path = PathBuf::from(arg);
if path.components().count() <= 1 { if path.components().count() <= 1 {
@@ -284,7 +284,7 @@ impl Options {
} }
} }
} else { } else {
CommandOutputPolicy::Discard CommandOutputPolicy::Null
}; };
options.output_style = match matches.value_of("style") { options.output_style = match matches.value_of("style") {
@@ -294,12 +294,12 @@ impl Options {
Some("color") => OutputStyleOption::Color, Some("color") => OutputStyleOption::Color,
Some("none") => OutputStyleOption::Disabled, Some("none") => OutputStyleOption::Disabled,
_ => { _ => {
if options.command_output_policy != CommandOutputPolicy::Forward if options.command_output_policy == CommandOutputPolicy::Inherit
&& atty::is(Stream::Stdout) || !atty::is(Stream::Stdout)
{ {
OutputStyleOption::Full
} else {
OutputStyleOption::Basic OutputStyleOption::Basic
} else {
OutputStyleOption::Full
} }
} }
}; };
+12 -15
View File
@@ -13,12 +13,10 @@ use std::fs::File;
#[cfg(target_os = "linux")] #[cfg(target_os = "linux")]
use std::os::unix::io::AsRawFd; use std::os::unix::io::AsRawFd;
#[cfg(not(target_os = "linux"))]
use std::io::Read;
use crate::util::units::Second; use crate::util::units::Second;
use wall_clock_timer::WallClockTimer; use wall_clock_timer::WallClockTimer;
use std::io::Read;
use std::process::{ChildStdout, Command, ExitStatus}; use std::process::{ChildStdout, Command, ExitStatus};
use anyhow::Result; use anyhow::Result;
@@ -65,14 +63,11 @@ fn discard(output: ChildStdout) {
} }
} }
#[cfg(not(target_os = "linux"))] let mut output = output;
{ let mut buf = [0; CHUNK_SIZE];
let mut output = output; while let Ok(bytes) = output.read(&mut buf) {
let mut buf = [0; CHUNK_SIZE]; if bytes == 0 {
while let Ok(bytes) = output.read(&mut buf) { break;
if bytes == 0 {
break;
}
} }
} }
} }
@@ -81,17 +76,19 @@ fn discard(output: ChildStdout) {
pub fn execute_and_measure(mut command: Command) -> Result<TimerResult> { pub fn execute_and_measure(mut command: Command) -> Result<TimerResult> {
let wallclock_timer = WallClockTimer::start(); let wallclock_timer = WallClockTimer::start();
let mut child = command.spawn()?;
let output = child.stdout.take();
#[cfg(not(windows))] #[cfg(not(windows))]
let cpu_timer = self::unix_timer::CPUTimer::start(); let cpu_timer = self::unix_timer::CPUTimer::start();
let mut child = command.spawn()?;
#[cfg(windows)] #[cfg(windows)]
let cpu_timer = self::windows_timer::CPUTimer::start_for_process(&child); let cpu_timer = self::windows_timer::CPUTimer::start_for_process(&child);
if let Some(output) = output { if let Some(output) = child.stdout.take() {
// Handle CommandOutputPolicy::Pipe
discard(output); discard(output);
} }
let status = child.wait()?; let status = child.wait()?;
let (time_user, time_system) = cpu_timer.stop(); let (time_user, time_system) = cpu_timer.stop();