rustpkg: Set exit codes properly and make tests take advantage of that

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)

diff --git a/src/librustpkg/context.rs b/src/librustpkg/context.rs
index b7a295f00707ae15075fa244439916c7af9d71a7..3b3cfe45b7a36b51f6097503b75ea606988c6c8b 100644 (file)
--- a/src/librustpkg/context.rs
+++ b/src/librustpkg/context.rs
@@ -210,11 +210,11 @@ pub fn default() -> 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);
     };
 

diff --git a/src/librustpkg/exit_codes.rs b/src/librustpkg/exit_codes.rs
index 56ee2033d1438ea9e962d95648ae2df0d56ce8f3..daa5eee62d205d29ba996685e1928f3edf612b20 100644 (file)
--- a/src/librustpkg/exit_codes.rs
+++ b/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;
+

diff --git a/src/librustpkg/rustpkg.rs b/src/librustpkg/rustpkg.rs
index e6ce5ba9d499481b444ec2a4bfb078ea042ae3a3..63195112747ac862a78cb8ce141a5990dbe22dbe 100644 (file)
--- a/src/librustpkg/rustpkg.rs
+++ b/src/librustpkg/rustpkg.rs
@@ -50,7 +50,7 @@
 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
             }

diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs
index 25e5617f78a9f7fbf8e5ac9c31f42775e8f582ad..d03243599ef5984abb48565a186fb3f7165e3864 100644 (file)
--- a/src/librustpkg/tests.rs
+++ b/src/librustpkg/tests.rs
@@ -36,6 +36,7 @@
 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,7 +245,7 @@ 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)
     }
 }
 
@@ -252,6 +253,18 @@ 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};