From 7472bae5e76918bad5904c31e43b4237c015d6a5 Mon Sep 17 00:00:00 2001 From: Tim Chevalier Date: Thu, 10 Oct 2013 13:48:11 -0700 Subject: [PATCH] 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) --- src/librustpkg/context.rs | 4 +- src/librustpkg/exit_codes.rs | 3 ++ src/librustpkg/rustpkg.rs | 18 ++++++--- src/librustpkg/tests.rs | 74 ++++++++++++++++++++++++++---------- 4 files changed, 72 insertions(+), 27 deletions(-) diff --git a/src/librustpkg/context.rs b/src/librustpkg/context.rs index b7a295f0070..3b3cfe45b7a 100644 --- a/src/librustpkg/context.rs +++ b/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); }; diff --git a/src/librustpkg/exit_codes.rs b/src/librustpkg/exit_codes.rs index 56ee2033d14..daa5eee62d2 100644 --- 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 e6ce5ba9d49..63195112747 100644 --- a/src/librustpkg/rustpkg.rs +++ b/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 } diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs index 25e5617f78a..d03243599ef 100644 --- a/src/librustpkg/tests.rs +++ b/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,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};