More tests for error cases in compile

Also some refactoring of the error structure. More cleanup work around
human-readable and CLI errors is still required.

src/bin/cargo-compile.rs

@@ -5,11 +5,11 @@ extern crate cargo;
 extern crate hammer;
 extern crate serialize;
 
+use cargo::{execute_main_without_stdin,CLIResult,CLIError,ToResult};
 use cargo::ops::cargo_compile::compile;
-use cargo::core::errors::{CLIResult,CLIError,ToResult};
 use cargo::util::important_paths::find_project;
-use hammer::{FlagDecoder,FlagConfig,HammerError};
-use serialize::Decodable;
+use cargo::util::ToCLI;
+use hammer::FlagConfig;
 use std::os;
 
 #[deriving(Eq,Clone,Decodable,Encodable)]
@@ -19,29 +19,18 @@ pub struct Options {
 
 impl FlagConfig for Options {}
 
-fn flags<T: FlagConfig + Decodable<FlagDecoder, HammerError>>() -> CLIResult<T> {
-    let mut decoder = FlagDecoder::new::<T>(std::os::args().tail());
-    Decodable::decode(&mut decoder).to_result(|e: HammerError| CLIError::new(e.message, None, 1))
+fn main() {
+    execute_main_without_stdin(execute);
 }
 
-fn execute() -> CLIResult<()> {
-    let options = try!(flags::<Options>());
-
+fn execute(options: Options) -> CLIResult<Option<()>> {
     let root = match options.manifest_path {
         Some(path) => Path::new(path),
         None => try!(find_project(os::getcwd(), "Cargo.toml".to_owned())
                     .map(|path| path.join("Cargo.toml"))
                     .to_result(|err|
-                        CLIError::new("Could not find Cargo.toml in this directory or any parent directory", Some(err.to_str()), 1)))
+                        CLIError::new("Could not find Cargo.toml in this directory or any parent directory", Some(err.to_str()), 102)))
     };
 
-    compile(root.as_str().unwrap().as_slice()).to_result(|err|
-        CLIError::new(format!("Compilation failed: {}", err), None, 1))
-}
-
-fn main() {
-    match execute() {
-        Err(err) => fail!("{}", err),
-        Ok(_) => return
-    }
+    compile(root.as_str().unwrap().as_slice()).map(|v| Some(v)).to_cli(101)
 }

src/cargo/core/manifest.rs

@@ -8,7 +8,6 @@ use core::{
     Package,
     Summary
 };
-use util::result::CargoResult;
 
 #[deriving(Eq,Clone)]
 pub struct Manifest {
@@ -177,7 +176,7 @@ pub struct TomlManifest {
 }
 
 impl TomlManifest {
-    pub fn to_package(&self, path: &str) -> CargoResult<Package> {
+    pub fn to_package(&self, path: &str) -> Package {
         // TODO: Convert hte argument to take a Path
         let path = Path::new(path);
 
@@ -195,12 +194,12 @@ impl TomlManifest {
         // TODO: https://github.com/mozilla/rust/issues/14049
         let root = Path::new(path.dirname());
 
-        Ok(Package::new(
+        Package::new(
             &Manifest::new(
                 &Summary::new(&self.project.to_name_ver(), deps.as_slice()),
                 targets.as_slice(),
                 &Path::new("target")),
-            &root))
+            &root)
     }
 }
 

src/cargo/ops/cargo_read_manifest.rs

@@ -2,23 +2,26 @@ use toml;
 use toml::from_toml;
 use core::Package;
 use core::manifest::{TomlManifest};
-use util::{other_error,CargoResult,CargoError};
+use util::{toml_error,human_error,CargoResult,CargoError};
 
 pub fn read_manifest(path: &str) -> CargoResult<Package> {
-    let root = try!(parse_from_file(path));
-    let toml = try!(load_toml(path, root));
-    toml.to_package(path)
+    let root = try!(parse_from_file(path).map_err(|err: CargoError|
+        human_error(format!("Cargo.toml is not valid Toml"), format!("path={}", path), err)));
+
+    let toml = try!(load_toml(root).map_err(|err: CargoError|
+        human_error(format!("Cargo.toml is not a valid Cargo manifest"), format!("path={}", path), err)));
+
+    Ok(toml.to_package(path))
 }
 
 fn parse_from_file(path: &str) -> CargoResult<toml::Value> {
-    toml::parse_from_file(path.clone()).map_err(|err| to_cargo_err(path, err))
+    toml::parse_from_file(path.clone()).map_err(to_cargo_err)
 }
 
-fn load_toml(path: &str, root: toml::Value) -> CargoResult<TomlManifest> {
-    from_toml::<TomlManifest>(root).map_err(|err| to_cargo_err(path, err))
+fn load_toml(root: toml::Value) -> CargoResult<TomlManifest> {
+    from_toml::<TomlManifest>(root).map_err(to_cargo_err)
 }
 
-fn to_cargo_err(path: &str, err: toml::Error) -> CargoError {
-    other_error("Cargo.toml is not valid Toml")
-        .with_detail(format!("path={}; err={}", path, err.to_str()))
+fn to_cargo_err(err: toml::Error) -> CargoError {
+    toml_error("Problem loading manifest", err)
 }

src/cargo/util/mod.rs

@@ -1,5 +1,5 @@
 pub use self::process_builder::{process,ProcessBuilder};
-pub use self::result::{CargoError,CargoResult,Wrap,Require,other_error};
+pub use self::result::{CargoError,CargoResult,Wrap,Require,ToCLI,other_error,human_error,toml_error};
 
 pub mod graph;
 pub mod process_builder;

src/cargo/util/result.rs

@@ -1,4 +1,6 @@
 use std::io;
+use core::errors::{CLIError,CLIResult};
+use toml;
 
 /*
  * CargoResult should be used in libcargo. CargoCliResult should be used in the
@@ -10,7 +12,25 @@ pub type CargoResult<T> = Result<T, CargoError>;
 pub fn other_error(desc: &'static str) -> CargoError {
     CargoError {
         kind: OtherCargoError,
-        desc: desc,
+        desc: StaticDescription(desc),
+        detail: None,
+        cause: None
+    }
+}
+
+pub fn human_error(desc: ~str, detail: ~str, cause: CargoError) -> CargoError {
+    CargoError {
+        kind: HumanReadableError,
+        desc: BoxedDescription(desc),
+        detail: Some(detail),
+        cause: Some(box cause)
+    }
+}
+
+pub fn toml_error(desc: &'static str, error: toml::Error) -> CargoError {
+    CargoError {
+        kind: TomlError(error),
+        desc: StaticDescription(desc),
         detail: None,
         cause: None
     }
@@ -19,14 +39,23 @@ pub fn other_error(desc: &'static str) -> CargoError {
 #[deriving(Show,Clone)]
 pub struct CargoError {
     kind: CargoErrorKind,
-    desc: &'static str,
+    desc: CargoErrorDescription,
     detail: Option<~str>,
     cause: Option<Box<CargoError>>
 }
 
+#[deriving(Show,Clone)]
+enum CargoErrorDescription {
+    StaticDescription(&'static str),
+    BoxedDescription(~str)
+}
+
 impl CargoError {
-    pub fn get_desc(&self) -> &'static str {
-        self.desc
+    pub fn get_desc<'a>(&'a self) -> &'a str {
+        match self.desc {
+            StaticDescription(desc) => desc,
+            BoxedDescription(ref desc) => desc.as_slice()
+        }
     }
 
     pub fn get_detail<'a>(&'a self) -> Option<&'a str> {
@@ -37,12 +66,31 @@ impl CargoError {
         self.detail = Some(detail);
         self
     }
+
+    pub fn to_cli(self, exit_code: uint) -> CLIError {
+        match self {
+            CargoError { kind: HumanReadableError, desc: BoxedDescription(desc), detail: detail, .. } => {
+                CLIError::new(desc, detail, exit_code)
+            },
+            CargoError { kind: InternalError, desc: StaticDescription(desc), detail: None, .. } => {
+                CLIError::new("An unexpected error occurred", Some(desc.to_owned()), exit_code)
+            },
+            CargoError { kind: InternalError, desc: StaticDescription(desc), detail: Some(detail), .. } => {
+                CLIError::new("An unexpected error occurred", Some(format!("{}\n{}", desc, detail)), exit_code)
+            },
+            _ => {
+                CLIError::new("An unexpected error occurred", None, exit_code)
+            }
+        }
+    }
 }
 
 #[deriving(Show,Clone)]
 pub enum CargoErrorKind {
+    HumanReadableError,
     InternalError,
     IoError(io::IoError),
+    TomlError(toml::Error),
     OtherCargoError
 }
 
@@ -73,7 +121,7 @@ impl<T> Wrap for Result<T, CargoError> {
             Err(e) => {
                 Err(CargoError {
                     kind: e.kind.clone(),
-                    desc: desc,
+                    desc: StaticDescription(desc),
                     detail: None,
                     cause: Some(box e)
                 })
@@ -94,3 +142,16 @@ impl<T> Require<T> for Option<T> {
         }
     }
 }
+
+pub trait ToCLI<T> {
+    fn to_cli(self, exit_code: uint) -> CLIResult<T>;
+}
+
+impl<T> ToCLI<T> for Result<T, CargoError> {
+    fn to_cli(self, exit_code: uint) -> CLIResult<T> {
+        match self {
+            Ok(val) => Ok(val),
+            Err(err) => Err(err.to_cli(exit_code))
+        }
+    }
+}

tests/support.rs

@@ -155,9 +155,10 @@ pub fn cargo_dir() -> Path {
 
 #[deriving(Clone,Eq)]
 struct Execs {
-  expect_stdout: Option<~str>,
-  expect_stdin: Option<~str>,
-  expect_exit_code: Option<int>
+    expect_stdout: Option<~str>,
+    expect_stdin: Option<~str>,
+    expect_stderr: Option<~str>,
+    expect_exit_code: Option<int>
 }
 
 impl Execs {
@@ -167,9 +168,20 @@ impl Execs {
     self
   }
 
+  pub fn with_stderr(mut ~self, expected: &str) -> Box<Execs> {
+      self.expect_stderr = Some(expected.to_owned());
+      self
+  }
+
+  pub fn with_status(mut ~self, expected: int) -> Box<Execs> {
+       self.expect_exit_code = Some(expected);
+       self
+  }
+
   fn match_output(&self, actual: &ProcessOutput) -> ham::MatchResult {
     self.match_status(actual.status)
       .and(self.match_stdout(&actual.output))
+      .and(self.match_stderr(&actual.error))
   }
 
   fn match_status(&self, actual: ProcessExit) -> ham::MatchResult {
@@ -184,13 +196,21 @@ impl Execs {
   }
 
   fn match_stdout(&self, actual: &Vec<u8>) -> ham::MatchResult {
-    match self.expect_stdout.as_ref().map(|s| s.as_slice()) {
+      self.match_std(&self.expect_stdout, actual, "stdout")
+  }
+
+  fn match_stderr(&self, actual: &Vec<u8>) -> ham::MatchResult {
+      self.match_std(&self.expect_stderr, actual, "stderr")
+  }
+
+  fn match_std(&self, expected: &Option<~str>, actual: &Vec<u8>, description: &str) -> ham::MatchResult {
+    match expected.as_ref().map(|s| s.as_slice()) {
       None => ham::success(),
       Some(out) => {
         match str::from_utf8(actual.as_slice()) {
-          None => Err("stdout was not utf8 encoded".to_owned()),
+          None => Err(format!("{} was not utf8 encoded", description)),
           Some(actual) => {
-            ham::expect(actual == out, format!("stdout was `{}`", actual))
+            ham::expect(actual == out, format!("{} was `{}`", description, actual))
           }
         }
       }
@@ -216,11 +236,12 @@ impl ham::Matcher<ProcessBuilder> for Execs {
 }
 
 pub fn execs() -> Box<Execs> {
-  box Execs {
-    expect_stdout: None,
-    expect_stdin: None,
-    expect_exit_code: None
-  }
+    box Execs {
+        expect_stdout: None,
+        expect_stderr: None,
+        expect_stdin: None,
+        expect_exit_code: None
+    }
 }
 
 pub trait ResultTest<T,E> {

tests/test_cargo_compile.rs

@@ -31,19 +31,24 @@ test!(cargo_compile {
       execs().with_stdout("i am foo\n"));
 })
 
-fn main_file(println: &str, deps: &[&str]) -> ~str {
-    let mut buf = StrBuf::new();
+test!(cargo_compile_with_invalid_manifest {
+    let p = project("foo")
+        .file("Cargo.toml", "");
 
-    for dep in deps.iter() {
-        buf.push_str(format!("extern crate {};\n", dep));
-    }
+    assert_that(p.cargo_process("cargo-compile"),
+        execs()
+        .with_status(101)
+        .with_stderr("Cargo.toml is not a valid Cargo manifest"));
+})
 
-    buf.push_str("fn main() { println!(");
-    buf.push_str(println);
-    buf.push_str("); }\n");
+test!(cargo_compile_without_manifest {
+    let p = project("foo");
 
-    buf.to_owned()
-}
+    assert_that(p.cargo_process("cargo-compile"),
+        execs()
+        .with_status(102)
+        .with_stderr("Could not find Cargo.toml in this directory or any parent directory"));
+})
 
 test!(cargo_compile_with_nested_deps {
     let mut p = project("foo");
@@ -120,4 +125,18 @@ test!(cargo_compile_with_nested_deps {
       execs().with_stdout("test passed\n"));
 })
 
+fn main_file(println: &str, deps: &[&str]) -> ~str {
+    let mut buf = StrBuf::new();
+
+    for dep in deps.iter() {
+        buf.push_str(format!("extern crate {};\n", dep));
+    }
+
+    buf.push_str("fn main() { println!(");
+    buf.push_str(println);
+    buf.push_str("); }\n");
+
+    buf.to_owned()
+}
+
 // test!(compiling_project_with_invalid_manifest)