rustpkg: Make rustpkg tests stop comparing dates

Instead of scrutinizing modification times in rustpkg tests,
change output files to be read-only and detect attempts to write
to them (hack suggested by Jack). This avoids time granularity problems.

As part of this change, I discovered that some dependencies weren't
getting written correctly (involving built executables and library
files), so this patch fixes that too.

This partly addresses #9441, but one test (test_rebuild_when_needed)
is still ignored on Linux.

Author: catamorphism

src/libextra/workcache.rs

@@ -219,7 +219,7 @@ impl Logger {
     }
 
     pub fn info(&self, i: &str) {
-        io::println(~"workcache: " + i);
+        info2!("workcache: {}", i);
     }
 }
 

src/librustpkg/conditions.rs

@@ -23,6 +23,10 @@ condition! {
     pub bad_stat: (Path, ~str) -> stat;
 }
 
+condition! {
+    pub bad_kind: (~str) -> ();
+}
+
 condition! {
     pub nonexistent_package: (PkgId, ~str) -> Path;
 }

src/librustpkg/package_source.rs

@@ -19,11 +19,12 @@ use crate::Crate;
 use messages::*;
 use source_control::{git_clone, git_clone_general};
 use path_util::{find_dir_using_rust_path_hack, default_workspace, make_dir_rwx_recursive};
-use util::compile_crate;
+use util::{compile_crate, DepMap};
 use workspace::is_workspace;
 use workcache_support;
 use workcache_support::crate_tag;
 use extra::workcache;
+use extra::treemap::TreeMap;
 
 // An enumeration of the unpacked source of a package workspace.
 // This contains a list of files found in the source workspace.
@@ -293,6 +294,7 @@ impl PkgSrc {
     fn build_crates(&self,
                     ctx: &BuildContext,
                     destination_dir: &Path,
+                    deps: &mut DepMap,
                     crates: &[Crate],
                     cfgs: &[~str],
                     what: OutputType) {
@@ -313,12 +315,14 @@ impl PkgSrc {
                 let id = self.id.clone();
                 let sub_dir = destination_dir.clone();
                 let sub_flags = crate.flags.clone();
+                let sub_deps = deps.clone();
                 do prep.exec |exec| {
                     let result = compile_crate(&subcx,
                                                exec,
                                                &id,
                                                &subpath,
                                                &sub_dir,
+                                               &mut (sub_deps.clone()),
                                                sub_flags,
                                                subcfgs,
                                                false,
@@ -347,13 +351,17 @@ impl PkgSrc {
         }
     }
 
-    // It would be better if build returned a Path, but then Path would have to derive
+    // It would be better if build returned a (Path, DepMap), but then Path would have to derive
     // Encodable.
     pub fn build(&self,
                  build_context: &BuildContext,
-                 cfgs: ~[~str]) -> ~str {
+                 // DepMap is a map from str (crate name) to (kind, name) --
+                 // it tracks discovered dependencies per-crate
+                 cfgs: ~[~str]) -> (~str, DepMap) {
         use conditions::not_a_workspace::cond;
 
+        let mut deps = TreeMap::new();
+
         // Determine the destination workspace (which depends on whether
         // we're using the rust_path_hack)
         let destination_workspace = if is_workspace(&self.workspace) {
@@ -379,14 +387,14 @@ impl PkgSrc {
         let benchs = self.benchs.clone();
         debug2!("Building libs in {}, destination = {}",
                destination_workspace.to_str(), destination_workspace.to_str());
-        self.build_crates(build_context, &destination_workspace, libs, cfgs, Lib);
+        self.build_crates(build_context, &destination_workspace, &mut deps, libs, cfgs, Lib);
         debug2!("Building mains");
-        self.build_crates(build_context, &destination_workspace, mains, cfgs, Main);
+        self.build_crates(build_context, &destination_workspace, &mut deps, mains, cfgs, Main);
         debug2!("Building tests");
-        self.build_crates(build_context, &destination_workspace, tests, cfgs, Test);
+        self.build_crates(build_context, &destination_workspace, &mut deps, tests, cfgs, Test);
         debug2!("Building benches");
-        self.build_crates(build_context, &destination_workspace, benchs, cfgs, Bench);
-        destination_workspace.to_str()
+        self.build_crates(build_context, &destination_workspace, &mut deps, benchs, cfgs, Bench);
+        (destination_workspace.to_str(), deps)
     }
 
     /// Debugging

src/librustpkg/rustpkg.rs

@@ -189,6 +189,7 @@ pub trait CtxMethods {
     fn install(&self, src: PkgSrc, what: &WhatToBuild) -> (~[Path], ~[(~str, ~str)]);
     /// Returns a list of installed files
     fn install_no_build(&self,
+                        declared_inputs: &[Path],
                         source_workspace: &Path,
                         target_workspace: &Path,
                         id: &PkgId) -> ~[~str];
@@ -385,6 +386,9 @@ impl CtxMethods for BuildContext {
     /// what_to_build says: "Just build the lib.rs file in one subdirectory,
     /// don't walk anything recursively." Or else, everything.
     fn build(&self, pkg_src: &mut PkgSrc, what_to_build: &WhatToBuild) -> Path {
+        use conditions::nonexistent_package::cond;
+        use bad_kind = conditions::bad_kind::cond;
+
         let workspace = pkg_src.workspace.clone();
         let pkgid = pkg_src.id.clone();
 
@@ -472,11 +476,61 @@ impl CtxMethods for BuildContext {
                 }
             }
             // Build it!
-            let rs_path = pkg_src.build(self, cfgs);
+
+            // Returns a map: crate_name -> (declared_input x discovered_input)
+            let (rs_path, dep_map) = pkg_src.build(self, cfgs);
+            debug2!("rs_path = {}", rs_path.to_str());
+            let path = Path(rs_path);
+            for (c, deps) in dep_map.iter() {
+                // This is wrong for multiple crates, but they're not implemented yet anyway. #7240
+                let output_filename_opt = if is_main(&Path(*c)) {
+                    built_executable_in_workspace(&pkgid, &path)
+                } else {
+                    built_library_in_workspace(&pkgid, &path)
+                };
+                let built = match output_filename_opt {
+                    Some(p) => p,
+                    None => cond.raise((pkgid.clone(),
+                                        format!("Output filename for {} doesn't exist", *c)))
+                };
+                let built_str = built.to_str();
+
+                // Here, we have to add an entry into workcache for the built executable(s).
+                // Each one has the same declared inputs as the crate that built it.
+                do self.workcache_context.with_prep(built_str) |prep| {
+                    prep.declare_input("file",
+                                       c.to_str(),
+                                       workcache_support::digest_file_with_date(&Path(*c)));
+                    let sub_built = built.clone();
+                    let sub_deps = deps.clone();
+                    do prep.exec |exe| {
+                        for &(ref kind, ref val) in sub_deps.iter() {
+                            if *kind == ~"file" {
+                                exe.discover_input(*kind,
+                                    *val,
+                                    workcache_support::digest_file_with_date(&Path(*val)));
+                            }
+                            else if *kind == ~"binary" {
+                                exe.discover_input(*kind,
+                                                   *val,
+                                                   digest_only_date(&Path(*val)));
+                            }
+                            else {
+                                bad_kind.raise(kind.clone());
+                            }
+                        }
+                        exe.discover_output("binary",
+                                            sub_built.to_str(),
+                                            digest_only_date(&sub_built));
+                    }
+                }
+            };
             Path(rs_path)
         }
         else {
             // Just return the source workspace
+            // Note that this won't add anything to the workcache --
+            // that's the package script's job
             workspace.clone()
         }
     }
@@ -535,7 +589,8 @@ impl CtxMethods for BuildContext {
         };
         debug2!("install: destination workspace = {}, id = {}, installing to {}",
                destination_workspace, id.to_str(), actual_workspace.to_str());
-        let result = self.install_no_build(&Path(destination_workspace),
+        let result = self.install_no_build(installed_files,
+                                           &Path(destination_workspace),
                                            &actual_workspace,
                                            &id).map(|s| Path(*s));
         debug2!("install: id = {}, about to call discover_outputs, {:?}",
@@ -547,6 +602,7 @@ impl CtxMethods for BuildContext {
 
     // again, working around lack of Encodable for Path
     fn install_no_build(&self,
+                        build_inputs: &[Path],
                         source_workspace: &Path,
                         target_workspace: &Path,
                         id: &PkgId) -> ~[~str] {
@@ -564,23 +620,32 @@ impl CtxMethods for BuildContext {
                maybe_executable, maybe_library);
 
         do self.workcache_context.with_prep(id.install_tag()) |prep| {
-            for ee in maybe_executable.iter() {
-                prep.declare_input("binary",
-                                   ee.to_str(),
-                                   workcache_support::digest_only_date(ee));
-            }
-            for ll in maybe_library.iter() {
-                prep.declare_input("binary",
-                                   ll.to_str(),
-                                   workcache_support::digest_only_date(ll));
-            }
             let subex = maybe_executable.clone();
             let sublib = maybe_library.clone();
             let sub_target_ex = target_exec.clone();
             let sub_target_lib = target_lib.clone();
-
+            let sub_build_inputs = build_inputs.to_owned();
             do prep.exec |exe_thing| {
                 let mut outputs = ~[];
+                // Declare all the *inputs* to the declared input too, as inputs
+                for executable in subex.iter() {
+                    exe_thing.discover_input("binary",
+                                             executable.to_str(),
+                                             workcache_support::digest_only_date(executable));
+                }
+                for library in sublib.iter() {
+                    exe_thing.discover_input("binary",
+                                             library.to_str(),
+                                             workcache_support::digest_only_date(library));
+                }
+
+                for transitive_dependency in sub_build_inputs.iter() {
+                    exe_thing.discover_input(
+                        "file",
+                        transitive_dependency.to_str(),
+                        workcache_support::digest_file_with_date(transitive_dependency));
+                }
+
 
                 for exec in subex.iter() {
                     debug2!("Copying: {} -> {}", exec.to_str(), sub_target_ex.to_str());
@@ -589,8 +654,8 @@ impl CtxMethods for BuildContext {
                         cond.raise(((*exec).clone(), sub_target_ex.clone()));
                     }
                     exe_thing.discover_output("binary",
-                        sub_target_ex.to_str(),
-                        workcache_support::digest_only_date(&sub_target_ex));
+                                              sub_target_ex.to_str(),
+                                              workcache_support::digest_only_date(&sub_target_ex));
                     outputs.push(sub_target_ex.to_str());
                 }
                 for lib in sublib.iter() {

src/librustpkg/tests.rs

@@ -34,7 +34,6 @@ use rustc::driver::driver::{build_session, build_session_options, host_triple, o
 use syntax::diagnostic;
 use target::*;
 use package_source::PkgSrc;
-use util::datestamp;
 
 fn fake_ctxt(sysroot: Path, workspace: &Path) -> BuildContext {
     let context = workcache::Context::new(
@@ -473,7 +472,8 @@ fn touch_source_file(workspace: &Path, pkgid: &PkgId) {
     for p in contents.iter() {
         if p.filetype() == Some(".rs") {
             // should be able to do this w/o a process
-            if run::process_output("touch", [p.to_str()]).status != 0 {
+            // n.b. Bumps time up by 2 seconds to get around granularity issues
+            if run::process_output("touch", [~"-A", ~"02", p.to_str()]).status != 0 {
                 let _ = cond.raise((pkg_src_dir.clone(), ~"Bad path"));
             }
         }
@@ -960,12 +960,17 @@ fn no_rebuilding() {
     let p_id = PkgId::new("foo");
     let workspace = create_local_package(&p_id);
     command_line_test([~"build", ~"foo"], &workspace);
-    let date = datestamp(&built_library_in_workspace(&p_id,
-                                                    &workspace).expect("no_rebuilding"));
+    let foo_lib = lib_output_file_name(&workspace, "foo");
+    // Now make `foo` read-only so that subsequent rebuilds of it will fail
+    assert!(chmod_read_only(&foo_lib));
+
     command_line_test([~"build", ~"foo"], &workspace);
-    let newdate = datestamp(&built_library_in_workspace(&p_id,
-                                                       &workspace).expect("no_rebuilding (2)"));
-    assert_eq!(date, newdate);
+
+    match command_line_test_partial([~"build", ~"foo"], &workspace) {
+        Success(*) => (), // ok
+        Fail(status) if status == 65 => fail2!("no_rebuilding failed: it tried to rebuild bar"),
+        Fail(_) => fail2!("no_rebuilding failed for some other reason")
+    }
 }
 
 #[test]
@@ -975,54 +980,53 @@ fn no_rebuilding_dep() {
     let workspace = create_local_package_with_dep(&p_id, &dep_id);
     command_line_test([~"build", ~"foo"], &workspace);
     let bar_lib = lib_output_file_name(&workspace, "bar");
-    let bar_date_1 = datestamp(&bar_lib);
-
     frob_source_file(&workspace, &p_id, "main.rs");
-
     // Now make `bar` read-only so that subsequent rebuilds of it will fail
     assert!(chmod_read_only(&bar_lib));
-
     match command_line_test_partial([~"build", ~"foo"], &workspace) {
         Success(*) => (), // ok
         Fail(status) if status == 65 => fail2!("no_rebuilding_dep failed: it tried to rebuild bar"),
         Fail(_) => fail2!("no_rebuilding_dep failed for some other reason")
     }
-
-    let bar_date_2 = datestamp(&lib_output_file_name(&workspace,
-                                                   "bar"));
-    assert_eq!(bar_date_1, bar_date_2);
 }
 
 #[test]
-#[ignore]
 fn do_rebuild_dep_dates_change() {
     let p_id = PkgId::new("foo");
     let dep_id = PkgId::new("bar");
     let workspace = create_local_package_with_dep(&p_id, &dep_id);
     command_line_test([~"build", ~"foo"], &workspace);
     let bar_lib_name = lib_output_file_name(&workspace, "bar");
-    let bar_date = datestamp(&bar_lib_name);
-    debug2!("Datestamp on {} is {:?}", bar_lib_name.to_str(), bar_date);
     touch_source_file(&workspace, &dep_id);
-    command_line_test([~"build", ~"foo"], &workspace);
-    let new_bar_date = datestamp(&bar_lib_name);
-    debug2!("Datestamp on {} is {:?}", bar_lib_name.to_str(), new_bar_date);
-    assert!(new_bar_date > bar_date);
+
+    // Now make `bar` read-only so that subsequent rebuilds of it will fail
+    assert!(chmod_read_only(&bar_lib_name));
+
+    match command_line_test_partial([~"build", ~"foo"], &workspace) {
+        Success(*) => fail2!("do_rebuild_dep_dates_change failed: it didn't rebuild bar"),
+        Fail(status) if status == 65 => (), // ok
+        Fail(_) => fail2!("do_rebuild_dep_dates_change failed for some other reason")
+    }
 }
 
 #[test]
-#[ignore]
 fn do_rebuild_dep_only_contents_change() {
     let p_id = PkgId::new("foo");
     let dep_id = PkgId::new("bar");
     let workspace = create_local_package_with_dep(&p_id, &dep_id);
     command_line_test([~"build", ~"foo"], &workspace);
-    let bar_date = datestamp(&lib_output_file_name(&workspace, "bar"));
     frob_source_file(&workspace, &dep_id, "lib.rs");
+    let bar_lib_name = lib_output_file_name(&workspace, "bar");
+
+    // Now make `bar` read-only so that subsequent rebuilds of it will fail
+    assert!(chmod_read_only(&bar_lib_name));
+
     // should adjust the datestamp
-    command_line_test([~"build", ~"foo"], &workspace);
-    let new_bar_date = datestamp(&lib_output_file_name(&workspace, "bar"));
-    assert!(new_bar_date > bar_date);
+    match command_line_test_partial([~"build", ~"foo"], &workspace) {
+        Success(*) => fail2!("do_rebuild_dep_only_contents_change failed: it didn't rebuild bar"),
+        Fail(status) if status == 65 => (), // ok
+        Fail(_) => fail2!("do_rebuild_dep_only_contents_change failed for some other reason")
+    }
 }
 
 #[test]
@@ -1823,7 +1827,6 @@ fn test_rustpkg_test_output() {
 }
 
 #[test]
-#[ignore(reason = "See issue #9441")]
 fn test_rebuild_when_needed() {
     let foo_id = PkgId::new("foo");
     let foo_workspace = create_local_package(&foo_id);