From 860f38a161268bde5a79d9799b4d2ee8ddde2860 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 9 Jun 2016 04:14:41 -0700 Subject: [PATCH] Don't print extra info on -q + `cargo run` If a process fails then it's probably printing out why and Cargo doesn't need to do it all over again. Closes #2735 --- src/bin/bench.rs | 6 +++--- src/bin/cargo.rs | 4 ++-- src/bin/git_checkout.rs | 4 ++-- src/bin/help.rs | 4 ++-- src/bin/locate_project.rs | 2 +- src/bin/run.rs | 18 +++++++++++++++--- src/bin/rustc.rs | 7 ++++--- src/bin/test.rs | 6 +++--- src/cargo/lib.rs | 32 +++++++++++++++++--------------- src/cargo/util/errors.rs | 36 ++++++++++++++++++++---------------- tests/run.rs | 21 +++++++++++++++++++++ 11 files changed, 90 insertions(+), 50 deletions(-) diff --git a/src/bin/bench.rs b/src/bin/bench.rs index ca8653442b5..3aff7fb1d81 100644 --- a/src/bin/bench.rs +++ b/src/bin/bench.rs @@ -1,5 +1,5 @@ use cargo::ops; -use cargo::util::{CliResult, CliError, Human, Config}; +use cargo::util::{CliResult, CliError, Human, Config, human}; use cargo::util::important_paths::{find_root_manifest_for_wd}; #[derive(RustcDecodable)] @@ -96,8 +96,8 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { None => Ok(None), Some(err) => { Err(match err.exit.as_ref().and_then(|e| e.code()) { - Some(i) => CliError::new("bench failed", i), - None => CliError::from_error(Human(err), 101) + Some(i) => CliError::new(human("bench failed"), i), + None => CliError::new(Box::new(Human(err)), 101) }) } } diff --git a/src/bin/cargo.rs b/src/bin/cargo.rs index ad9416d9884..d7bd169cd98 100644 --- a/src/bin/cargo.rs +++ b/src/bin/cargo.rs @@ -221,9 +221,9 @@ fn execute_subcommand(config: &Config, }; if let Some(code) = err.exit.as_ref().and_then(|c| c.code()) { - Err(CliError::new("", code)) + Err(CliError::code(code)) } else { - Err(CliError::from_error(err, 101)) + Err(CliError::new(Box::new(err), 101)) } } diff --git a/src/bin/git_checkout.rs b/src/bin/git_checkout.rs index a453055147c..8d3e29ac2eb 100644 --- a/src/bin/git_checkout.rs +++ b/src/bin/git_checkout.rs @@ -35,7 +35,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { human(format!("The URL `{}` you passed was \ not a valid URL: {}", url, e)) }) - .map_err(|e| CliError::from_boxed(e, 1))); + .map_err(|e| CliError::new(e, 1))); let reference = GitReference::Branch(reference.clone()); let source_id = SourceId::for_git(&url, reference); @@ -43,7 +43,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { let mut source = GitSource::new(&source_id, config); try!(source.update().map_err(|e| { - CliError::new(&format!("Couldn't update {:?}: {:?}", source, e), 1) + CliError::new(human(format!("Couldn't update {:?}: {:?}", source, e)), 1) })); Ok(None) diff --git a/src/bin/help.rs b/src/bin/help.rs index 8f104fcbcf2..8597bd59439 100644 --- a/src/bin/help.rs +++ b/src/bin/help.rs @@ -1,4 +1,4 @@ -use cargo::util::{CliResult, CliError, Config}; +use cargo::util::{CliResult, CliError, Config, human}; #[derive(RustcDecodable)] pub struct Options; @@ -18,5 +18,5 @@ pub fn execute(_: Options, _: &Config) -> CliResult> { // This is a dummy command just so that `cargo help help` works. // The actual delegation of help flag to subcommands is handled by the // cargo command. - Err(CliError::new("Help command should not be executed directly.", 101)) + Err(CliError::new(human("help command should not be executed directly"), 101)) } diff --git a/src/bin/locate_project.rs b/src/bin/locate_project.rs index b6c7aa075ee..f162788fcbd 100644 --- a/src/bin/locate_project.rs +++ b/src/bin/locate_project.rs @@ -30,7 +30,7 @@ pub fn execute(flags: LocateProjectFlags, .chain_error(|| human("Your project path contains \ characters not representable in \ Unicode")) - .map_err(|e| CliError::from_boxed(e, 1))); + .map_err(|e| CliError::new(e, 1))); Ok(Some(ProjectLocation { root: string.to_string() })) } diff --git a/src/bin/run.rs b/src/bin/run.rs index 34f83c66941..789c90e4c89 100644 --- a/src/bin/run.rs +++ b/src/bin/run.rs @@ -88,9 +88,21 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { match try!(ops::run(&root, &compile_opts, &options.arg_args)) { None => Ok(None), Some(err) => { - Err(match err.exit.as_ref().and_then(|e| e.code()) { - Some(code) => CliError::from_error(Human(err), code), - None => CliError::from_error(err, 101), + // If we never actually spawned the process then that sounds pretty + // bad and we always want to forward that up. + let exit = match err.exit.clone() { + Some(exit) => exit, + None => return Err(CliError::new(Box::new(Human(err)), 101)), + }; + + // If `-q` was passed then we suppress extra error information about + // a failed process, we assume the process itself printed out enough + // information about why it failed so we don't do so as well + let exit_code = exit.code().unwrap_or(101); + Err(if options.flag_quiet == Some(true) { + CliError::code(exit_code) + } else { + CliError::new(Box::new(Human(err)), exit_code) }) } } diff --git a/src/bin/rustc.rs b/src/bin/rustc.rs index aae58dfb81e..51fec6d8680 100644 --- a/src/bin/rustc.rs +++ b/src/bin/rustc.rs @@ -3,7 +3,7 @@ use std::env; use cargo::ops::{CompileOptions, CompileMode}; use cargo::ops; use cargo::util::important_paths::{find_root_manifest_for_wd}; -use cargo::util::{CliResult, CliError, Config}; +use cargo::util::{CliResult, CliError, Config, human}; #[derive(RustcDecodable)] pub struct Options { @@ -79,8 +79,9 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { Some("test") => CompileMode::Test, Some("bench") => CompileMode::Bench, Some(mode) => { - return Err(CliError::new(&format!("unknown profile: `{}`, use dev, - test, or bench", mode), 101)) + let err = human(format!("unknown profile: `{}`, use dev, + test, or bench", mode)); + return Err(CliError::new(err, 101)) } }; diff --git a/src/bin/test.rs b/src/bin/test.rs index e2602b1a2c1..8a21ee3fa51 100644 --- a/src/bin/test.rs +++ b/src/bin/test.rs @@ -1,5 +1,5 @@ use cargo::ops; -use cargo::util::{CliResult, CliError, Human, Config}; +use cargo::util::{CliResult, CliError, Human, human, Config}; use cargo::util::important_paths::{find_root_manifest_for_wd}; #[derive(RustcDecodable)] @@ -120,8 +120,8 @@ pub fn execute(options: Options, config: &Config) -> CliResult> { None => Ok(None), Some(err) => { Err(match err.exit.as_ref().and_then(|e| e.code()) { - Some(i) => CliError::new("test failed", i), - None => CliError::from_error(Human(err), 101) + Some(i) => CliError::new(human("test failed"), i), + None => CliError::new(Box::new(Human(err)), 101) }) } } diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 45676e2ef99..2db31fe7658 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -186,17 +186,19 @@ pub fn handle_error(err: CliError, shell: &mut MultiShell) { let hide = unknown && shell.get_verbose() != Verbose; - let _ignored_result = if hide { - shell.error("An unknown error occurred") - } else if fatal { - shell.error(&error) - } else { - shell.say(&error, BLACK) - }; - - if !handle_cause(&error, shell) || hide { - let _ = shell.err().say("\nTo learn more, run the command again \ - with --verbose.".to_string(), BLACK); + if let Some(error) = error { + let _ignored_result = if hide { + shell.error("An unknown error occurred") + } else if fatal { + shell.error(&error) + } else { + shell.say(&error, BLACK) + }; + + if !handle_cause(&error, shell) || hide { + let _ = shell.err().say("\nTo learn more, run the command again \ + with --verbose.".to_string(), BLACK); + } } std::process::exit(exit_code); @@ -247,7 +249,7 @@ fn flags_from_args(usage: &str, args: &[String], .help(true); docopt.decode().map_err(|e| { let code = if e.fatal() {1} else {0}; - CliError::from_error(human(e.to_string()), code) + CliError::new(human(e.to_string()), code) }) } @@ -255,15 +257,15 @@ fn json_from_stdin() -> CliResult { let mut reader = io::stdin(); let mut input = String::new(); try!(reader.read_to_string(&mut input).map_err(|_| { - CliError::new("Standard in did not exist or was not UTF-8", 1) + CliError::new(human("Standard in did not exist or was not UTF-8"), 1) })); let json = try!(Json::from_str(&input).map_err(|_| { - CliError::new("Could not parse standard in as JSON", 1) + CliError::new(human("Could not parse standard in as JSON"), 1) })); let mut decoder = json::Decoder::new(json); Decodable::decode(&mut decoder).map_err(|_| { - CliError::new("Could not process standard in as input", 1) + CliError::new(human("Could not process standard in as input"), 1) }) } diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index a31fffc04c1..3a84be560f6 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -246,42 +246,46 @@ pub type CliResult = Result; #[derive(Debug)] pub struct CliError { - pub error: Box, + pub error: Option>, pub unknown: bool, pub exit_code: i32 } impl Error for CliError { - fn description(&self) -> &str { self.error.description() } - fn cause(&self) -> Option<&Error> { self.error.cause() } + fn description(&self) -> &str { + self.error.as_ref().map(|e| e.description()) + .unwrap_or("unknown cli error") + } + + fn cause(&self) -> Option<&Error> { + self.error.as_ref().and_then(|e| e.cause()) + } } impl fmt::Display for CliError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - fmt::Display::fmt(&self.error, f) + if let Some(ref error) = self.error { + error.fmt(f) + } else { + self.description().fmt(f) + } } } impl CliError { - pub fn new(error: &str, code: i32) -> CliError { - let error = human(error.to_string()); - CliError::from_boxed(error, code) - } - - pub fn from_error(error: E, code: i32) -> CliError { - let error = Box::new(error); - CliError::from_boxed(error, code) + pub fn new(error: Box, code: i32) -> CliError { + let human = error.is_human(); + CliError { error: Some(error), exit_code: code, unknown: !human } } - pub fn from_boxed(error: Box, code: i32) -> CliError { - let human = error.is_human(); - CliError { error: error, exit_code: code, unknown: !human } + pub fn code(code: i32) -> CliError { + CliError { error: None, exit_code: code, unknown: false } } } impl From> for CliError { fn from(err: Box) -> CliError { - CliError::from_boxed(err, 101) + CliError::new(err, 101) } } diff --git a/tests/run.rs b/tests/run.rs index 7f821eef96b..15be9bfd9e4 100644 --- a/tests/run.rs +++ b/tests/run.rs @@ -610,3 +610,24 @@ fn run_with_library_paths() { assert_that(p.cargo_process("run"), execs().with_status(0)); } + +#[test] +fn fail_no_extra_verbose() { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + "#) + .file("src/main.rs", r#" + fn main() { + std::process::exit(1); + } + "#); + + assert_that(p.cargo_process("run").arg("-q"), + execs().with_status(1) + .with_stdout("") + .with_stderr("")); +}