From 65aacd083a1a8a5e2ae1e5ffab4bb1b032854327 Mon Sep 17 00:00:00 2001 From: Tim Chevalier Date: Thu, 7 Nov 2013 15:34:31 -0800 Subject: [PATCH] rustpkg: Eliminate the spurious `os::path_exists(&pkg_src.start_dir.join(p))` assertion failure This addresses the problem reported in #10253 and possibly elsewhere. Closes #10253 --- src/librustpkg/package_source.rs | 3 +- src/librustpkg/path_util.rs | 2 + src/librustpkg/tests.rs | 64 ++++++++++++++++++++++++-------- src/librustpkg/util.rs | 24 +++++++++--- 4 files changed, 71 insertions(+), 22 deletions(-) diff --git a/src/librustpkg/package_source.rs b/src/librustpkg/package_source.rs index f27ca9e0a1818..b6edab3d65ad0 100644 --- a/src/librustpkg/package_source.rs +++ b/src/librustpkg/package_source.rs @@ -118,7 +118,8 @@ impl PkgSrc { debug!("Checking dirs: {:?}", to_try.map(|p| p.display().to_str()).connect(":")); - let path = to_try.iter().find(|&d| d.exists()); + let path = to_try.iter().find(|&d| d.is_dir() + && dir_has_crate_file(d)); // See the comments on the definition of PkgSrc let mut build_in_destination = use_rust_path_hack; diff --git a/src/librustpkg/path_util.rs b/src/librustpkg/path_util.rs index bce41e5a49fe0..921005fdaab57 100644 --- a/src/librustpkg/path_util.rs +++ b/src/librustpkg/path_util.rs @@ -461,6 +461,7 @@ pub fn versionize(p: &Path, v: &Version) -> Path { } #[cfg(target_os = "win32")] +#[fixed_stack_segment] pub fn chmod_read_only(p: &Path) -> bool { unsafe { do p.with_c_str |src_buf| { @@ -470,6 +471,7 @@ pub fn chmod_read_only(p: &Path) -> bool { } #[cfg(not(target_os = "win32"))] +#[fixed_stack_segment] pub fn chmod_read_only(p: &Path) -> bool { unsafe { do p.with_c_str |src_buf| { diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs index 02d2f9095090a..bf62e7068f325 100644 --- a/src/librustpkg/tests.rs +++ b/src/librustpkg/tests.rs @@ -239,7 +239,8 @@ 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, - Fail(error) => fail!("Command line test failed with error {}", error) + Fail(error) => fail!("Command line test failed with error {}", + error.status) } } @@ -253,15 +254,15 @@ fn command_line_test_expect_fail(args: &[~str], expected_exitcode: int) { match command_line_test_with_env(args, cwd, env) { Success(_) => fail!("Should have failed with {}, but it succeeded", expected_exitcode), - Fail(error) if error.matches_exit_status(expected_exitcode) => (), // ok + Fail(ref error) if error.status.matches_exit_status(expected_exitcode) => (), // ok Fail(other) => fail!("Expected to fail with {}, but failed with {} instead", - expected_exitcode, other) + expected_exitcode, other.status) } } enum ProcessResult { Success(ProcessOutput), - Fail(ProcessExit) + Fail(ProcessOutput) } /// Runs `rustpkg` (based on the directory that this executable was @@ -295,7 +296,7 @@ fn command_line_test_with_env(args: &[~str], cwd: &Path, env: Option<~[(~str, ~s debug!("Command {} {:?} failed with exit code {:?}; its output was --- {} ---", cmd, args, output.status, str::from_utf8(output.output) + str::from_utf8(output.error)); - Fail(output.status) + Fail(output) } else { Success(output) @@ -1093,7 +1094,7 @@ fn no_rebuilding() { match command_line_test_partial([~"build", ~"foo"], workspace) { Success(*) => (), // ok - Fail(status) if status.matches_exit_status(65) => + Fail(ref status) if status.status.matches_exit_status(65) => fail!("no_rebuilding failed: it tried to rebuild bar"), Fail(_) => fail!("no_rebuilding failed for some other reason") } @@ -1112,7 +1113,8 @@ fn no_recopying() { match command_line_test_partial([~"install", ~"foo"], workspace) { Success(*) => (), // ok - Fail(process::ExitStatus(65)) => fail!("no_recopying failed: it tried to re-copy foo"), + Fail(ref status) if status.status.matches_exit_status(65) => + fail!("no_recopying failed: it tried to re-copy foo"), Fail(_) => fail!("no_copying failed for some other reason") } } @@ -1130,7 +1132,7 @@ fn no_rebuilding_dep() { assert!(chmod_read_only(&bar_lib)); match command_line_test_partial([~"build", ~"foo"], workspace) { Success(*) => (), // ok - Fail(status) if status.matches_exit_status(65) => + Fail(ref r) if r.status.matches_exit_status(65) => fail!("no_rebuilding_dep failed: it tried to rebuild bar"), Fail(_) => fail!("no_rebuilding_dep failed for some other reason") } @@ -1151,7 +1153,7 @@ fn do_rebuild_dep_dates_change() { match command_line_test_partial([~"build", ~"foo"], workspace) { Success(*) => fail!("do_rebuild_dep_dates_change failed: it didn't rebuild bar"), - Fail(status) if status.matches_exit_status(65) => (), // ok + Fail(ref r) if r.status.matches_exit_status(65) => (), // ok Fail(_) => fail!("do_rebuild_dep_dates_change failed for some other reason") } } @@ -1172,7 +1174,7 @@ fn do_rebuild_dep_only_contents_change() { // should adjust the datestamp match command_line_test_partial([~"build", ~"foo"], workspace) { Success(*) => fail!("do_rebuild_dep_only_contents_change failed: it didn't rebuild bar"), - Fail(status) if status.matches_exit_status(65) => (), // ok + Fail(ref r) if r.status.matches_exit_status(65) => (), // ok Fail(_) => fail!("do_rebuild_dep_only_contents_change failed for some other reason") } } @@ -2148,7 +2150,7 @@ fn test_rebuild_when_needed() { chmod_read_only(&test_executable); match command_line_test_partial([~"test", ~"foo"], foo_workspace) { Success(*) => fail!("test_rebuild_when_needed didn't rebuild"), - Fail(status) if status.matches_exit_status(65) => (), // ok + Fail(ref r) if r.status.matches_exit_status(65) => (), // ok Fail(_) => fail!("test_rebuild_when_needed failed for some other reason") } } @@ -2168,7 +2170,7 @@ fn test_no_rebuilding() { chmod_read_only(&test_executable); match command_line_test_partial([~"test", ~"foo"], foo_workspace) { Success(*) => (), // ok - Fail(status) if status.matches_exit_status(65) => + Fail(ref r) if r.status.matches_exit_status(65) => fail!("test_no_rebuilding failed: it rebuilt the tests"), Fail(_) => fail!("test_no_rebuilding failed for some other reason") } @@ -2296,7 +2298,7 @@ fn test_compile_error() { let result = command_line_test_partial([~"build", ~"foo"], foo_workspace); match result { Success(*) => fail!("Failed by succeeding!"), // should be a compile error - Fail(status) => { + Fail(ref status) => { debug!("Failed with status {:?}... that's good, right?", status); } } @@ -2364,7 +2366,7 @@ fn test_c_dependency_no_rebuilding() { match command_line_test_partial([~"build", ~"cdep"], dir) { Success(*) => (), // ok - Fail(status) if status.matches_exit_status(65) => + Fail(ref r) if r.status.matches_exit_status(65) => fail!("test_c_dependency_no_rebuilding failed: \ it tried to rebuild foo.c"), Fail(_) => @@ -2403,11 +2405,43 @@ fn test_c_dependency_yes_rebuilding() { match command_line_test_partial([~"build", ~"cdep"], dir) { Success(*) => fail!("test_c_dependency_yes_rebuilding failed: \ it didn't rebuild and should have"), - Fail(status) if status.matches_exit_status(65) => (), + Fail(ref r) if r.status.matches_exit_status(65) => (), Fail(_) => fail!("test_c_dependency_yes_rebuilding failed for some other reason") } } +// n.b. This might help with #10253, or at least the error will be different. +#[test] +fn correct_error_dependency() { + // Supposing a package we're trying to install via a dependency doesn't + // exist, we should throw a condition, and not ICE + let dir = create_local_package(&PkgId::new("badpkg")); + + let dir = dir.path(); + writeFile(&dir.join_many(["src", "badpkg-0.1", "main.rs"]), + "extern mod p = \"some_package_that_doesnt_exist\"; + fn main() {}"); + + match command_line_test_partial([~"build", ~"badpkg"], dir) { + Fail(ProcessOutput{ error: error, output: output, _ }) => { + assert!(str::is_utf8(error)); + assert!(str::is_utf8(output)); + let error_str = str::from_utf8(error); + let out_str = str::from_utf8(output); + debug!("ss = {}", error_str); + debug!("out_str = {}", out_str); + if out_str.contains("Package badpkg depends on some_package_that_doesnt_exist") && + !error_str.contains("nonexistent_package") { + // Ok + () + } else { + fail!("Wrong error"); + } + } + Success(*) => fail!("Test passed when it should have failed") + } +} + /// Returns true if p exists and is executable fn is_executable(p: &Path) -> bool { p.exists() && p.stat().perm & io::UserExecute == io::UserExecute diff --git a/src/librustpkg/util.rs b/src/librustpkg/util.rs index df94e166dc211..6156cc0838ab7 100644 --- a/src/librustpkg/util.rs +++ b/src/librustpkg/util.rs @@ -36,6 +36,7 @@ pub use target::{Target, Build, Install}; use extra::treemap::TreeMap; pub use target::{lib_name_of, lib_crate_filename, WhatToBuild, MaybeCustom, Inferred}; use workcache_support::{digest_file_with_date, digest_only_date}; +use messages::error; // It would be nice to have the list of commands in just one place -- for example, // you could update the match in rustpkg.rc but forget to update this list. I think @@ -430,6 +431,8 @@ struct ViewItemVisitor<'self> { impl<'self> Visitor<()> for ViewItemVisitor<'self> { fn visit_view_item(&mut self, vi: &ast::view_item, env: ()) { + use conditions::nonexistent_package::cond; + match vi.node { // ignore metadata, I guess ast::view_item_extern_mod(lib_ident, path_opt, _, _) => { @@ -490,12 +493,21 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> { // and the `PkgSrc` constructor will detect that; // or else it's already in a workspace and we'll build into that // workspace - let pkg_src = PkgSrc::new(source_workspace, - dest_workspace, - // Use the rust_path_hack to search for dependencies iff - // we were already using it - self.context.context.use_rust_path_hack, - pkg_id.clone()); + let pkg_src = do cond.trap(|_| { + // Nonexistent package? Then print a better error + error(format!("Package {} depends on {}, but I don't know \ + how to find it", + self.parent.path.display(), + pkg_id.path.display())); + fail!() + }).inside { + PkgSrc::new(source_workspace.clone(), + dest_workspace.clone(), + // Use the rust_path_hack to search for dependencies iff + // we were already using it + self.context.context.use_rust_path_hack, + pkg_id.clone()) + }; let (outputs_disc, inputs_disc) = self.context.install( pkg_src,