rustpkg: Eliminate the spurious `os::path_exists(&pkg_src.start_dir.join(p))` asserti...

This addresses the problem reported in #10253 and possibly elsewhere.

Closes #10253

diff --git a/src/librustpkg/package_source.rs b/src/librustpkg/package_source.rs
index f27ca9e0a18180af8b14abb9158a8d6a50738438..b6edab3d65ad037fc1200b4f5b93d8056f9c3b89 100644 (file)
--- a/src/librustpkg/package_source.rs
+++ b/src/librustpkg/package_source.rs
@@ -118,7 +118,8 @@ pub fn new(mut source_workspace: Path,
 
         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 bce41e5a49fe0560fd04103a87402cf574a79b75..921005fdaab57014e71f4be37583112c25b1e6a8 100644 (file)
--- 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 02d2f9095090ab0b8c7815b2fe5f4db9b68cb84e..bf62e7068f325f7d865398335975045ab46ce544 100644 (file)
--- 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 df94e166dc211de2f4d8605f66731574775640dc..6156cc0838ab7f3407c9bf0d22492940ca60f0a7 100644 (file)
--- a/src/librustpkg/util.rs
+++ b/src/librustpkg/util.rs
@@ -36,6 +36,7 @@
 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 @@ fn visit_view_item(&mut self, vi: &ast::view_item, env: ()) {
                         // 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,