From d14abb6ec327e1e402a138046070ad5ab44a0181 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 16 Jan 2015 09:43:28 -0800 Subject: [PATCH] Print human errors in traces by default This helps get nice pretty stack traces which terminate when we get into the weeds of errors but still present relevant contextual information when Cargo isn't run with --verbose --- src/cargo/lib.rs | 63 +++++++++++++----------- src/cargo/util/errors.rs | 14 ++++-- src/cargo/util/toml.rs | 6 ++- tests/test_cargo_compile.rs | 20 +++++--- tests/test_cargo_compile_custom_build.rs | 4 +- tests/test_cargo_compile_git_deps.rs | 4 +- tests/test_cargo_features.rs | 24 ++++----- 7 files changed, 77 insertions(+), 58 deletions(-) diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index cd3dc898a..2341af8b5 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -151,7 +151,7 @@ pub fn shell(verbose: bool) -> MultiShell { // `output` print variant error strings to either stderr or stdout. // For fatal errors, print to stderr; // and for others, e.g. docopt version info, print to stdout. -fn output(caption: Option, detail: Option, +fn output(caption: Option<&str>, detail: Option, shell: &mut MultiShell, fatal: bool) { let std_shell = if fatal {shell.err()} else {shell.out()}; if let Some(caption) = caption { @@ -167,46 +167,51 @@ pub fn handle_error(err: CliError, shell: &mut MultiShell) { log!(4, "handle_error; err={:?}", err); let CliError { error, exit_code, unknown } = err; - let verbose = shell.get_verbose(); let fatal = exit_code != 0; // exit_code == 0 is non-fatal error + + let desc = error.description(); if unknown { - output(Some("An unknown error occurred".to_string()), None, shell, fatal); - } else if error.to_string().len() > 0 { - output(Some(error.to_string()), None, shell, fatal); + output(Some("An unknown error occurred"), None, shell, fatal); + } else if desc.len() > 0 { + output(Some(desc), None, shell, fatal); } - - if error.cause().is_some() || unknown { - if !verbose { - output(None, - Some("\nTo learn more, run the command again with --verbose.".to_string()), - shell, fatal); - } + if shell.get_verbose() { + output(None, error.detail(), shell, fatal); } - - if verbose { - if unknown { - output(Some(error.to_string()), None, shell, fatal); - } - if let Some(detail) = error.detail() { - output(None, Some(detail), shell, fatal); - } - if let Some(err) = error.cause() { - let _ = handle_cause(err, shell); - } + if !handle_cause(&*error, shell) { + let _ = shell.err().say("\nTo learn more, run the command again \ + with --verbose.".to_string(), BLACK); } std::os::set_exit_status(exit_code as isize); } -fn handle_cause(mut err: &Error, shell: &mut MultiShell) { +fn handle_cause(mut cargo_err: &CargoError, shell: &mut MultiShell) -> bool { + let verbose = shell.get_verbose(); + let mut err; loop { - let _ = shell.err().say("\nCaused by:", BLACK); - let _ = shell.err().say(format!(" {}", err.description()), BLACK); + cargo_err = match cargo_err.cargo_cause() { + Some(cause) => cause, + None => { err = cargo_err.cause(); break } + }; + if !verbose && !cargo_err.is_human() { return false } + print(cargo_err.description(), cargo_err.detail(), shell); + } + loop { + let cause = match err { Some(err) => err, None => return true }; + if !verbose { return false } + print(cause.description(), cause.detail(), shell); + err = cause.cause(); + } - match err.cause() { - Some(e) => err = e, - None => break, + fn print(desc: &str, detail: Option, shell: &mut MultiShell) { + let _ = shell.err().say("\nCaused by:", BLACK); + let _ = shell.err().say(format!(" {}", desc), BLACK); + if shell.get_verbose() { + if let Some(detail) = detail { + let _ = shell.err().say(detail, BLACK); + } } } } diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index 6a906aab2..8466a3bd0 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -19,6 +19,7 @@ pub type CargoResult = Result>; pub trait CargoError: Error { fn is_human(&self) -> bool { false } + fn cargo_cause(&self) -> Option<&CargoError>{ None } } impl fmt::String for Box { @@ -41,6 +42,7 @@ impl Error for Box { impl CargoError for Box { fn is_human(&self) -> bool { (**self).is_human() } + fn cargo_cause(&self) -> Option<&CargoError> { (**self).cargo_cause() } } // ============================================================================= @@ -53,7 +55,7 @@ pub trait ChainError { struct ChainedError { error: E, - cause: Box, + cause: Box, } impl<'a, T, F> ChainError for F where F: FnOnce() -> CargoResult { @@ -63,7 +65,7 @@ impl<'a, T, F> ChainError for F where F: FnOnce() -> CargoResult { } } -impl ChainError for Result { +impl ChainError for Result { fn chain_error(self, callback: C) -> CargoResult where E2: CargoError, C: FnOnce() -> E2 { self.map_err(move |err| { @@ -88,11 +90,11 @@ impl ChainError for Option { impl Error for ChainedError { fn description(&self) -> &str { self.error.description() } fn detail(&self) -> Option { self.error.detail() } - fn cause(&self) -> Option<&Error> { Some(&*self.cause) } } impl CargoError for ChainedError { fn is_human(&self) -> bool { self.error.is_human() } + fn cargo_cause(&self) -> Option<&CargoError> { Some(&*self.cause) } } // ============================================================================= @@ -170,8 +172,9 @@ impl Error for Human { fn cause(&self) -> Option<&Error> { self.0.cause() } } -impl CargoError for Human { +impl CargoError for Human { fn is_human(&self) -> bool { true } + fn cargo_cause(&self) -> Option<&CargoError> { self.0.cargo_cause() } } // ============================================================================= @@ -233,7 +236,7 @@ from_error! { toml::DecodeError, } -impl FromError> for Box { +impl FromError> for Box { fn from_error(t: Human) -> Box { Box::new(t) } } @@ -247,6 +250,7 @@ impl CargoError for CliError {} impl CargoError for toml::Error {} impl CargoError for toml::DecodeError {} impl CargoError for url::ParseError {} +impl CargoError for str::Utf8Error {} // ============================================================================= // Construction helpers diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs index 7addd794c..ca072252b 100644 --- a/src/cargo/util/toml.rs +++ b/src/cargo/util/toml.rs @@ -104,9 +104,11 @@ pub fn to_manifest(contents: &[u8], })); let root = try!(parse(contents, &manifest)); let mut d = toml::Decoder::new(toml::Value::Table(root)); - let toml_manifest: TomlManifest = try!(Decodable::decode(&mut d)); + let manifest: TomlManifest = try!(Decodable::decode(&mut d).map_err(|e| { + human(e.to_string()) + })); - let pair = try!(toml_manifest.to_manifest(source_id, &layout, config)); + let pair = try!(manifest.to_manifest(source_id, &layout, config)); let (mut manifest, paths) = pair; match d.toml { Some(ref toml) => add_unused_keys(&mut manifest, toml, "".to_string()), diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs index 162ccb36e..6b08da08f 100644 --- a/tests/test_cargo_compile.rs +++ b/tests/test_cargo_compile.rs @@ -45,9 +45,9 @@ test!(cargo_compile_with_invalid_manifest { .with_status(101) .with_stderr("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -No `package` or `project` section found. +Caused by: + No `package` or `project` section found. ")) }); @@ -63,7 +63,9 @@ test!(cargo_compile_with_invalid_manifest2 { .with_status(101) .with_stderr("\ failed to parse manifest at `[..]` -could not parse input as TOML + +Caused by: + could not parse input as TOML Cargo.toml:3:19-3:20 expected a value ")) @@ -85,7 +87,9 @@ test!(cargo_compile_with_invalid_manifest3 { .with_status(101) .with_stderr("\ failed to parse manifest at `[..]` -could not parse input as TOML\n\ + +Caused by: + could not parse input as TOML\n\ src[..]Cargo.toml:1:5-1:6 expected a value\n\n")) }); @@ -103,9 +107,9 @@ test!(cargo_compile_with_invalid_version { .with_status(101) .with_stderr("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -cannot parse '1.0' as a semver for the key `project.version` +Caused by: + cannot parse '1.0' as a semver for the key `project.version` ")) }); @@ -784,7 +788,9 @@ test!(missing_lib_and_bin { execs().with_status(101) .with_stderr("\ failed to parse manifest at `[..]Cargo.toml` -either a [lib] or [[bin]] section must be present\n")); + +Caused by: + either a [lib] or [[bin]] section must be present\n")); }); test!(lto_build { diff --git a/tests/test_cargo_compile_custom_build.rs b/tests/test_cargo_compile_custom_build.rs index 32ede9525..6c6a7dd5d 100644 --- a/tests/test_cargo_compile_custom_build.rs +++ b/tests/test_cargo_compile_custom_build.rs @@ -859,7 +859,9 @@ test!(build_script_only { execs().with_status(101) .with_stderr("\ failed to parse manifest at `[..]` -either a [lib] or [[bin]] section must be present")); + +Caused by: + either a [lib] or [[bin]] section must be present")); }); test!(shared_dep_with_a_build_script { diff --git a/tests/test_cargo_compile_git_deps.rs b/tests/test_cargo_compile_git_deps.rs index 018f45b39..f9bfedeba 100644 --- a/tests/test_cargo_compile_git_deps.rs +++ b/tests/test_cargo_compile_git_deps.rs @@ -447,9 +447,9 @@ test!(cargo_compile_with_short_ssh_git { .with_stdout("") .with_stderr(format!("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -invalid url `{}`: relative URL without a base +Caused by: + invalid url `{}`: relative URL without a base ", url))); }); diff --git a/tests/test_cargo_features.rs b/tests/test_cargo_features.rs index 55482b4ec..7536c7cf3 100644 --- a/tests/test_cargo_features.rs +++ b/tests/test_cargo_features.rs @@ -24,9 +24,9 @@ test!(invalid1 { assert_that(p.cargo_process("build"), execs().with_status(101).with_stderr(format!("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -Feature `bar` includes `baz` which is neither a dependency nor another feature +Caused by: + Feature `bar` includes `baz` which is neither a dependency nor another feature ").as_slice())); }); @@ -49,9 +49,9 @@ test!(invalid2 { assert_that(p.cargo_process("build"), execs().with_status(101).with_stderr(format!("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -Features and dependencies cannot have the same name: `bar` +Caused by: + Features and dependencies cannot have the same name: `bar` ").as_slice())); }); @@ -74,9 +74,9 @@ test!(invalid3 { assert_that(p.cargo_process("build"), execs().with_status(101).with_stderr(format!("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -Feature `bar` depends on `baz` which is not an optional dependency. +Caused by: + Feature `bar` depends on `baz` which is not an optional dependency. Consider adding `optional = true` to the dependency ").as_slice())); }); @@ -137,9 +137,9 @@ test!(invalid5 { assert_that(p.cargo_process("build"), execs().with_status(101).with_stderr(format!("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -Dev-dependencies are not allowed to be optional: `bar` +Caused by: + Dev-dependencies are not allowed to be optional: `bar` ").as_slice())); }); @@ -159,9 +159,9 @@ test!(invalid6 { assert_that(p.cargo_process("build").arg("--features").arg("foo"), execs().with_status(101).with_stderr(format!("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -Feature `foo` requires `bar` which is not an optional dependency +Caused by: + Feature `foo` requires `bar` which is not an optional dependency ").as_slice())); }); @@ -182,9 +182,9 @@ test!(invalid7 { assert_that(p.cargo_process("build").arg("--features").arg("foo"), execs().with_status(101).with_stderr(format!("\ failed to parse manifest at `[..]` -Cargo.toml is not a valid manifest -Feature `foo` requires `bar` which is not an optional dependency +Caused by: + Feature `foo` requires `bar` which is not an optional dependency ").as_slice())); });