auto merge of #9799 : catamorphism/rust/rustpkg-exitcodes, r=catamorphism,metajack

r? @metajack When I started writing the rustpkg tests, task failure didn't set the
exit code properly. But bblum's work from July fixed that. Hooray! I
just didn't know about it till now.

So, now rustpkg uses exit codes in a more conventional way, and some of
the tests are simpler.

The bigger issue will be to make task failure propagate the error message.
Right now, rustpkg does most of the work in separate tasks, which means if
a task fails, rustpkg can't distinguish between different types of failure
(see #3408)

Author: bors

src/librustpkg/context.rs

@@ -210,11 +210,11 @@ impl RustcFlags {
 }
 
 /// Returns true if any of the flags given are incompatible with the cmd
-pub fn flags_ok_for_cmd(flags: &RustcFlags,
+pub fn flags_forbidden_for_cmd(flags: &RustcFlags,
                         cfgs: &[~str],
                         cmd: &str, user_supplied_opt_level: bool) -> bool {
     let complain = |s| {
-        println!("The {} option can only be used with the build command:
+        println!("The {} option can only be used with the `build` command:
                   rustpkg [options..] build {} [package-ID]", s, s);
     };
 

src/librustpkg/exit_codes.rs

@@ -9,3 +9,6 @@
 // except according to those terms.
 
 pub static COPY_FAILED_CODE: int = 65;
+pub static BAD_FLAG_CODE: int    = 67;
+pub static NONEXISTENT_PACKAGE_CODE: int = 68;
+

src/librustpkg/rustpkg.rs

@@ -50,7 +50,7 @@ use package_source::PkgSrc;
 use target::{WhatToBuild, Everything, is_lib, is_main, is_test, is_bench, Tests};
 // use workcache_support::{discover_outputs, digest_only_date};
 use workcache_support::digest_only_date;
-use exit_codes::COPY_FAILED_CODE;
+use exit_codes::{COPY_FAILED_CODE, BAD_FLAG_CODE};
 
 pub mod api;
 mod conditions;
@@ -701,7 +701,7 @@ pub fn main_args(args: &[~str]) -> int {
             return 1;
         }
     };
-    let mut help = matches.opt_present("h") ||
+    let help = matches.opt_present("h") ||
                    matches.opt_present("help");
     let no_link = matches.opt_present("no-link");
     let no_trans = matches.opt_present("no-trans");
@@ -798,8 +798,11 @@ pub fn main_args(args: &[~str]) -> int {
             return 0;
         }
         Some(cmd) => {
-            help |= context::flags_ok_for_cmd(&rustc_flags, cfgs, *cmd, user_supplied_opt_level);
-            if help {
+            let bad_option = context::flags_forbidden_for_cmd(&rustc_flags,
+                                                              cfgs,
+                                                              *cmd,
+                                                              user_supplied_opt_level);
+            if help || bad_option {
                 match *cmd {
                     ~"build" => usage::build(),
                     ~"clean" => usage::clean(),
@@ -814,7 +817,12 @@ pub fn main_args(args: &[~str]) -> int {
                     ~"unprefer" => usage::unprefer(),
                     _ => usage::general()
                 };
-                return 0;
+                if bad_option {
+                    return BAD_FLAG_CODE;
+                }
+                else {
+                    return 0;
+                }
             } else {
                 cmd
             }

src/librustpkg/tests.rs

@@ -36,6 +36,7 @@ use syntax::diagnostic;
 use target::*;
 use package_source::PkgSrc;
 use source_control::{CheckedOutSources, DirToUse, safe_git_clone};
+use exit_codes::{BAD_FLAG_CODE, COPY_FAILED_CODE};
 use util::datestamp;
 
 fn fake_ctxt(sysroot: Path, workspace: &Path) -> BuildContext {
@@ -244,14 +245,26 @@ fn rustpkg_exec() -> Path {
 fn command_line_test(args: &[~str], cwd: &Path) -> ProcessOutput {
     match command_line_test_with_env(args, cwd, None) {
         Success(r) => r,
-        _ => fail2!("Command line test failed")
+        Fail(error) => fail2!("Command line test failed with error {}", error)
     }
 }
 
 fn command_line_test_partial(args: &[~str], cwd: &Path) -> ProcessResult {
     command_line_test_with_env(args, cwd, None)
 }
 
+fn command_line_test_expect_fail(args: &[~str],
+                                 cwd: &Path,
+                                 env: Option<~[(~str, ~str)]>,
+                                 expected_exitcode: int) {
+    match command_line_test_with_env(args, cwd, env) {
+        Success(_) => fail2!("Should have failed with {}, but it succeeded", expected_exitcode),
+        Fail(error) if error == expected_exitcode => (), // ok
+        Fail(other) => fail2!("Expected to fail with {}, but failed with {} instead",
+                              expected_exitcode, other)
+    }
+}
+
 enum ProcessResult {
     Success(ProcessOutput),
     Fail(int) // exit code
@@ -1448,11 +1461,11 @@ fn compile_flag_fail() {
     let p_id = PkgId::new("foo");
     let workspace = create_local_package(&p_id);
     let workspace = workspace.path();
-    command_line_test([test_sysroot().to_str(),
+    command_line_test_expect_fail([test_sysroot().to_str(),
                        ~"install",
                        ~"--no-link",
                        ~"foo"],
-                      workspace);
+                      workspace, None, BAD_FLAG_CODE);
     assert!(!built_executable_exists(workspace, "foo"));
     assert!(!object_file_exists(workspace, "foo"));
 }
@@ -1488,14 +1501,11 @@ fn notrans_flag_fail() {
     let flags_to_test = [~"--no-trans", ~"--parse-only",
                          ~"--pretty", ~"-S"];
     for flag in flags_to_test.iter() {
-        command_line_test([test_sysroot().to_str(),
+        command_line_test_expect_fail([test_sysroot().to_str(),
                            ~"install",
                            flag.clone(),
                            ~"foo"],
-                          workspace);
-        // Ideally we'd test that rustpkg actually fails, but
-        // since task failure doesn't set the exit code properly,
-        // we can't tell
+                          workspace, None, BAD_FLAG_CODE);
         assert!(!built_executable_exists(workspace, "foo"));
         assert!(!object_file_exists(workspace, "foo"));
         assert!(!lib_exists(workspace, &Path("foo"), NoVersion));
@@ -1522,11 +1532,11 @@ fn dash_S_fail() {
     let p_id = PkgId::new("foo");
     let workspace = create_local_package(&p_id);
     let workspace = workspace.path();
-    command_line_test([test_sysroot().to_str(),
+    command_line_test_expect_fail([test_sysroot().to_str(),
                        ~"install",
                        ~"-S",
                        ~"foo"],
-                      workspace);
+                       workspace, None, BAD_FLAG_CODE);
     assert!(!built_executable_exists(workspace, "foo"));
     assert!(!object_file_exists(workspace, "foo"));
     assert!(!assembly_file_exists(workspace, "foo"));
@@ -1587,11 +1597,13 @@ fn test_emit_llvm_S_fail() {
     let p_id = PkgId::new("foo");
     let workspace = create_local_package(&p_id);
     let workspace = workspace.path();
-    command_line_test([test_sysroot().to_str(),
+    command_line_test_expect_fail([test_sysroot().to_str(),
                        ~"install",
                        ~"-S", ~"--emit-llvm",
                        ~"foo"],
-                      workspace);
+                       workspace,
+                       None,
+                       BAD_FLAG_CODE);
     assert!(!built_executable_exists(workspace, "foo"));
     assert!(!object_file_exists(workspace, "foo"));
     assert!(!llvm_assembly_file_exists(workspace, "foo"));
@@ -1620,11 +1632,13 @@ fn test_emit_llvm_fail() {
     let p_id = PkgId::new("foo");
     let workspace = create_local_package(&p_id);
     let workspace = workspace.path();
-    command_line_test([test_sysroot().to_str(),
+    command_line_test_expect_fail([test_sysroot().to_str(),
                        ~"install",
                        ~"--emit-llvm",
                        ~"foo"],
-                      workspace);
+                                  workspace,
+                                  None,
+                                  BAD_FLAG_CODE);
     assert!(!built_executable_exists(workspace, "foo"));
     assert!(!object_file_exists(workspace, "foo"));
     assert!(!llvm_bitcode_file_exists(workspace, "foo"));
@@ -1665,11 +1679,10 @@ fn test_build_install_flags_fail() {
                      ~[~"--target", host_triple()],
                      ~[~"--target-cpu", ~"generic"],
                      ~[~"-Z", ~"--time-passes"]];
+    let cwd = os::getcwd();
     for flag in forbidden.iter() {
-        let output = command_line_test_output([test_sysroot().to_str(),
-                           ~"list"] + *flag);
-        assert!(output.len() > 1);
-        assert!(output[1].find_str("can only be used with").is_some());
+        command_line_test_expect_fail([test_sysroot().to_str(),
+                           ~"list"] + *flag, &cwd, None, BAD_FLAG_CODE);
     }
 }
 
@@ -1686,6 +1699,7 @@ fn test_optimized_build() {
     assert!(built_executable_exists(workspace, "foo"));
 }
 
+#[test]
 fn pkgid_pointing_to_subdir() {
     // The actual repo is mockgithub.com/mozilla/some_repo
     // rustpkg should recognize that and treat the part after some_repo/ as a subdir
@@ -1717,6 +1731,7 @@ fn pkgid_pointing_to_subdir() {
     assert_executable_exists(workspace, "testpkg");
 }
 
+#[test]
 fn test_recursive_deps() {
     let a_id = PkgId::new("a");
     let b_id = PkgId::new("b");
@@ -1762,6 +1777,7 @@ fn test_install_to_rust_path() {
     assert!(!executable_exists(second_workspace, "foo"));
 }
 
+#[test]
 fn test_target_specific_build_dir() {
     let p_id = PkgId::new("foo");
     let workspace = create_local_package(&p_id);
@@ -1870,8 +1886,9 @@ fn correct_package_name_with_rust_path_hack() {
     let rust_path = Some(~[(~"RUST_PATH", format!("{}:{}", dest_workspace.to_str(),
                         foo_workspace.push_many(["src", "foo-0.1"]).to_str()))]);
     // bar doesn't exist, but we want to make sure rustpkg doesn't think foo is bar
-    command_line_test_with_env([~"install", ~"--rust-path-hack", ~"bar"],
-                               dest_workspace, rust_path);
+    command_line_test_expect_fail([~"install", ~"--rust-path-hack", ~"bar"],
+                                  // FIXME #3408: Should be NONEXISTENT_PACKAGE_CODE
+                               dest_workspace, rust_path, COPY_FAILED_CODE);
     assert!(!executable_exists(dest_workspace, "bar"));
     assert!(!lib_exists(dest_workspace, &bar_id.path.clone(), bar_id.version.clone()));
     assert!(!executable_exists(dest_workspace, "foo"));
@@ -2050,6 +2067,23 @@ fn test_7402() {
     assert_executable_exists(dest_workspace, "foo");
 }
 
+#[test]
+fn test_compile_error() {
+    let foo_id = PkgId::new("foo");
+    let foo_workspace = create_local_package(&foo_id);
+    let foo_workspace = foo_workspace.path();
+    let main_crate = foo_workspace.push_many(["src", "foo-0.1", "main.rs"]);
+    // Write something bogus
+    writeFile(&main_crate, "pub fn main() { if 42 != ~\"the answer\" { fail!(); } }");
+    let result = command_line_test_partial([~"build", ~"foo"], foo_workspace);
+    match result {
+        Success(*) => fail2!("Failed by succeeding!"), // should be a compile error
+        Fail(status) => {
+            debug2!("Failed with status {:?}... that's good, right?", status);
+        }
+    }
+}
+
 /// Returns true if p exists and is executable
 fn is_executable(p: &Path) -> bool {
     use std::libc::consts::os::posix88::{S_IXUSR};