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.

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

@@ -21,10 +21,11 @@ use source_control::{safe_git_clone, git_clone_url, DirToUse, CheckedOutSources}
 use source_control::make_read_only;
 use path_util::{find_dir_using_rust_path_hack, make_dir_rwx_recursive};
 use path_util::{target_build_dir, versionize};
-use util::compile_crate;
+use util::{compile_crate, DepMap};
 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.
@@ -370,6 +371,7 @@ impl PkgSrc {
 
     fn build_crates(&self,
                     ctx: &BuildContext,
+                    deps: &mut DepMap,
                     crates: &[Crate],
                     cfgs: &[~str],
                     what: OutputType) {
@@ -389,12 +391,14 @@ impl PkgSrc {
                 let id = self.id.clone();
                 let sub_dir = self.build_workspace().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,
@@ -428,24 +432,27 @@ impl PkgSrc {
         }
     }
 
-    // It would be better if build returned a Path, but then Path would have to derive
-    // Encodable.
     pub fn build(&self,
                  build_context: &BuildContext,
-                 cfgs: ~[~str]) {
+                 // DepMap is a map from str (crate name) to (kind, name) --
+                 // it tracks discovered dependencies per-crate
+                 cfgs: ~[~str]) -> DepMap {
+        let mut deps = TreeMap::new();
+
         let libs = self.libs.clone();
         let mains = self.mains.clone();
         let tests = self.tests.clone();
         let benchs = self.benchs.clone();
         debug2!("Building libs in {}, destination = {}",
                self.source_workspace.display(), self.build_workspace().display());
-        self.build_crates(build_context, libs, cfgs, Lib);
+        self.build_crates(build_context, &mut deps, libs, cfgs, Lib);
         debug2!("Building mains");
-        self.build_crates(build_context, mains, cfgs, Main);
+        self.build_crates(build_context, &mut deps, mains, cfgs, Main);
         debug2!("Building tests");
-        self.build_crates(build_context, tests, cfgs, Test);
+        self.build_crates(build_context, &mut deps, tests, cfgs, Test);
         debug2!("Building benches");
-        self.build_crates(build_context, benchs, cfgs, Bench);
+        self.build_crates(build_context, &mut deps, benchs, cfgs, Bench);
+        deps
     }
 
     /// Return the workspace to put temporary files in. See the comment on `PkgSrc`

src/librustpkg/rustpkg.rs

@@ -197,7 +197,8 @@ pub trait CtxMethods {
     fn install(&self, src: PkgSrc, what: &WhatToBuild) -> (~[Path], ~[(~str, ~str)]);
     /// Returns a list of installed files
     fn install_no_build(&self,
-                        source_workspace: &Path,
+                        build_workspace: &Path,
+                        build_inputs: &[Path],
                         target_workspace: &Path,
                         id: &PkgId) -> ~[~str];
     fn prefer(&self, _id: &str, _vers: Option<~str>);
@@ -542,6 +543,7 @@ impl CtxMethods for BuildContext {
 
         let mut installed_files = ~[];
         let mut inputs = ~[];
+        let mut build_inputs = ~[];
 
         debug2!("Installing package source: {}", pkg_src.to_str());
 
@@ -554,14 +556,16 @@ impl CtxMethods for BuildContext {
         debug2!("In declare inputs for {}", id.to_str());
         for cs in to_do.iter() {
             for c in cs.iter() {
-                let path = pkg_src.start_dir.join(&c.file);
+                let path = pkg_src.start_dir.join(&c.file).normalize();
                 debug2!("Recording input: {}", path.display());
                 // FIXME (#9639): This needs to handle non-utf8 paths
                 inputs.push((~"file", path.as_str().unwrap().to_owned()));
+                build_inputs.push(path);
             }
         }
 
         let result = self.install_no_build(pkg_src.build_workspace(),
+                                           build_inputs,
                                            &pkg_src.destination_workspace,
                                            &id).map(|s| Path::new(s.as_slice()));
         debug2!("install: id = {}, about to call discover_outputs, {:?}",
@@ -576,6 +580,7 @@ impl CtxMethods for BuildContext {
     // again, working around lack of Encodable for Path
     fn install_no_build(&self,
                         build_workspace: &Path,
+                        build_inputs: &[Path],
                         target_workspace: &Path,
                         id: &PkgId) -> ~[~str] {
         use conditions::copy_failed::cond;
@@ -612,9 +617,28 @@ impl CtxMethods for BuildContext {
             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.display(), sub_target_ex.display());

src/librustpkg/tests.rs

@@ -37,7 +37,6 @@ 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 {
     let context = workcache::Context::new(
@@ -507,6 +506,7 @@ fn output_file_name(workspace: &Path, short_name: ~str) -> Path {
                                                                          os::EXE_SUFFIX))
 }
 
+#[cfg(target_os = "linux")]
 fn touch_source_file(workspace: &Path, pkgid: &PkgId) {
     use conditions::bad_path::cond;
     let pkg_src_dir = workspace.join_many([~"src", pkgid.to_str()]);
@@ -515,7 +515,26 @@ fn touch_source_file(workspace: &Path, pkgid: &PkgId) {
         if p.extension_str() == Some("rs") {
             // should be able to do this w/o a process
             // FIXME (#9639): This needs to handle non-utf8 paths
-            if run::process_output("touch", [p.as_str().unwrap().to_owned()]).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"));
+            }
+        }
+    }
+}
+
+#[cfg(not(target_os = "linux"))]
+fn touch_source_file(workspace: &Path, pkgid: &PkgId) {
+    use conditions::bad_path::cond;
+    let pkg_src_dir = workspace.join_many([~"src", pkgid.to_str()]);
+    let contents = os::list_dir_path(&pkg_src_dir);
+    for p in contents.iter() {
+        if p.extension_str() == Some("rs") {
+            // should be able to do this w/o a process
+            // FIXME (#9639): This needs to handle non-utf8 paths
+            // n.b. Bumps time up by 2 seconds to get around granularity issues
+            if run::process_output("touch", [~"-A02",
+                                             p.as_str().unwrap().to_owned()]).status != 0 {
                 let _ = cond.raise((pkg_src_dir.clone(), ~"Bad path"));
             }
         }
@@ -1033,12 +1052,17 @@ fn no_rebuilding() {
     let workspace = create_local_package(&p_id);
     let workspace = workspace.path();
     command_line_test([~"build", ~"foo"], workspace);
-    let date = datestamp(&built_library_in_workspace(&p_id,
+    let foo_lib = lib_output_file_name(workspace, "foo");
-                                                     workspace).expect("no_rebuilding"));
+    // 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)"));
+    match command_line_test_partial([~"build", ~"foo"], workspace) {
-    assert_eq!(date, newdate);
+        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]
@@ -1049,25 +1073,17 @@ fn no_rebuilding_dep() {
     let workspace = workspace.path();
     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(&bar_lib);
-    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");
@@ -1075,29 +1091,37 @@ fn do_rebuild_dep_dates_change() {
     let workspace = workspace.path();
     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.display(), bar_date);
     touch_source_file(workspace, &dep_id);
-    command_line_test([~"build", ~"foo"], workspace);
+
-    let new_bar_date = datestamp(&bar_lib_name);
+    // Now make `bar` read-only so that subsequent rebuilds of it will fail
-    debug2!("Datestamp on {} is {:?}", bar_lib_name.display(), new_bar_date);
+    assert!(chmod_read_only(&bar_lib_name));
-    assert!(new_bar_date > bar_date);
+
+    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);
     let workspace = workspace.path();
     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);
+    match command_line_test_partial([~"build", ~"foo"], workspace) {
-    let new_bar_date = datestamp(&lib_output_file_name(workspace, "bar"));
+        Success(*) => fail2!("do_rebuild_dep_only_contents_change failed: it didn't rebuild bar"),
-    assert!(new_bar_date > bar_date);
+        Fail(status) if status == 65 => (), // ok
+        Fail(_) => fail2!("do_rebuild_dep_only_contents_change failed for some other reason")
+    }
 }
 
 #[test]
@@ -2003,7 +2027,7 @@ fn test_rustpkg_test_output() {
 }
 
 #[test]
-#[ignore(reason = "See issue #9441")]
+#[ignore(reason = "Issue 9441")]
 fn test_rebuild_when_needed() {
     let foo_id = PkgId::new("foo");
     let foo_workspace = create_local_package(&foo_id);

src/librustpkg/util.rs

@@ -30,6 +30,8 @@ use workspace::pkg_parent_workspaces;
 use path_util::{U_RWX, system_library, target_build_dir};
 use path_util::{default_workspace, built_library_in_workspace};
 pub use target::{OutputType, Main, Lib, Bench, Test, JustOne, lib_name_of, lib_crate_filename};
+pub use target::{Target, Build, Install};
+use extra::treemap::TreeMap;
 use workcache_support::{digest_file_with_date, digest_only_date};
 
 // It would be nice to have the list of commands in just one place -- for example,
@@ -166,6 +168,7 @@ pub fn compile_input(context: &BuildContext,
                      pkg_id: &PkgId,
                      in_file: &Path,
                      workspace: &Path,
+                     deps: &mut DepMap,
                      flags: &[~str],
                      cfgs: &[~str],
                      opt: bool,
@@ -265,7 +268,7 @@ pub fn compile_input(context: &BuildContext,
     let mut crate = driver::phase_1_parse_input(sess, cfg.clone(), &input);
     crate = driver::phase_2_configure_and_expand(sess, cfg.clone(), crate);
 
-    find_and_install_dependencies(context, pkg_id, sess, exec, &crate,
+    find_and_install_dependencies(context, pkg_id, in_file, sess, exec, &crate, deps,
                                   |p| {
                                       debug2!("a dependency: {}", p.display());
                                       // Pass the directory containing a dependency
@@ -329,6 +332,7 @@ pub fn compile_input(context: &BuildContext,
 // If crate_opt is present, then finish compilation. If it's None, then
 // call compile_upto and return the crate
 // also, too many arguments
+// Returns list of discovered dependencies
 pub fn compile_crate_from_input(input: &Path,
                                 exec: &mut workcache::Exec,
                                 stop_before: StopBefore,
@@ -390,29 +394,34 @@ pub fn exe_suffix() -> ~str { ~"" }
 pub fn compile_crate(ctxt: &BuildContext,
                      exec: &mut workcache::Exec,
                      pkg_id: &PkgId,
-                     crate: &Path, workspace: &Path,
+                     crate: &Path,
-                     flags: &[~str], cfgs: &[~str], opt: bool,
+                     workspace: &Path,
+                     deps: &mut DepMap,
+                     flags: &[~str],
+                     cfgs: &[~str],
+                     opt: bool,
                      what: OutputType) -> Option<Path> {
     debug2!("compile_crate: crate={}, workspace={}", crate.display(), workspace.display());
     debug2!("compile_crate: short_name = {}, flags =...", pkg_id.to_str());
     for fl in flags.iter() {
         debug2!("+++ {}", *fl);
     }
-    compile_input(ctxt, exec, pkg_id, crate, workspace, flags, cfgs, opt, what)
+    compile_input(ctxt, exec, pkg_id, crate, workspace, deps, flags, cfgs, opt, what)
 }
 
 struct ViewItemVisitor<'self> {
     context: &'self BuildContext,
     parent: &'self PkgId,
+    parent_crate: &'self Path,
     sess: session::Session,
     exec: &'self mut workcache::Exec,
     c: &'self ast::Crate,
     save: &'self fn(Path),
+    deps: &'self mut DepMap
 }
 
 impl<'self> Visitor<()> for ViewItemVisitor<'self> {
     fn visit_view_item(&mut self, vi: &ast::view_item, env: ()) {
-        debug2!("A view item!");
         match vi.node {
             // ignore metadata, I guess
             ast::view_item_extern_mod(lib_ident, path_opt, _, _) => {
@@ -432,6 +441,8 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                         // Now we know that this crate has a discovered dependency on
                         // installed_path
                         // FIXME (#9639): This needs to handle non-utf8 paths
+                        add_dep(self.deps, self.parent_crate.to_str(),
+                                (~"binary", installed_path.to_str()));
                         self.exec.discover_input("binary",
                                                  installed_path.as_str().unwrap(),
                                                  digest_only_date(installed_path));
@@ -465,7 +476,7 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                         // Use the rust_path_hack to search for dependencies iff
                         // we were already using it
                                                   self.context.context.use_rust_path_hack,
-                                                  pkg_id);
+                                                  pkg_id.clone());
                         let (outputs_disc, inputs_disc) =
                             self.context.install(pkg_src, &JustOne(Path::new(lib_crate_filename)));
                         debug2!("Installed {}, returned {:?} dependencies and \
@@ -486,14 +497,35 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                             debug2!("Installed {} into {}", dep.display(), dep_dir.display());
                             (self.save)(dep_dir);
                         }
+                        debug2!("Installed {}, returned {} dependencies and \
+                                {} transitive dependencies",
+                                lib_name, outputs_disc.len(), inputs_disc.len());
+                        // It must have installed *something*...
+                        assert!(!outputs_disc.is_empty());
+                        let target_workspace = outputs_disc[0].pop();
+                        for dep in outputs_disc.iter() {
+                            debug2!("Discovering a binary input: {}", dep.to_str());
+                            self.exec.discover_input("binary", dep.to_str(),
+                                                     digest_only_date(dep));
+                            add_dep(self.deps,
+                                    self.parent_crate.to_str(),
+                                    (~"binary", dep.to_str()));
+                        }
                         for &(ref what, ref dep) in inputs_disc.iter() {
                             if *what == ~"file" {
+                                add_dep(self.deps,
+                                        self.parent_crate.to_str(),
+                                        (~"file", dep.to_str()));
+
                                 self.exec.discover_input(*what,
                                                          *dep,
                                                          digest_file_with_date(
                                                              &Path::new(dep.as_slice())));
                             }
                                 else if *what == ~"binary" {
+                                add_dep(self.deps,
+                                        self.parent_crate.to_str(),
+                                        (~"binary", dep.to_str()));
                                 self.exec.discover_input(*what,
                                                          *dep,
                                                          digest_only_date(
@@ -502,6 +534,10 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                                 else {
                                 fail2!("Bad kind: {}", *what);
                             }
+                            // Also, add an additional search path
+                            debug2!("Installed {} into {}",
+                                    lib_name, target_workspace.to_str());
+                            (self.save)(target_workspace.clone());
                         }
                     }
                 }
@@ -518,18 +554,22 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
 /// can't be found.
 pub fn find_and_install_dependencies(context: &BuildContext,
                                      parent: &PkgId,
+                                     parent_crate: &Path,
                                      sess: session::Session,
                                      exec: &mut workcache::Exec,
                                      c: &ast::Crate,
+                                     deps: &mut DepMap,
                                      save: &fn(Path)) {
     debug2!("In find_and_install_dependencies...");
     let mut visitor = ViewItemVisitor {
         context: context,
         parent: parent,
+        parent_crate: parent_crate,
         sess: sess,
         exec: exec,
         c: c,
         save: save,
+        deps: deps
     };
     visit::walk_crate(&mut visitor, c, ())
 }
@@ -579,3 +619,19 @@ pub fn datestamp(p: &Path) -> Option<libc::time_t> {
     debug2!("Date = {:?}", out);
     out.map(|t| { t as libc::time_t })
 }
+
+pub type DepMap = TreeMap<~str, ~[(~str, ~str)]>;
+
+/// Records a dependency from `parent` to the kind and value described by `info`,
+/// in `deps`
+fn add_dep(deps: &mut DepMap, parent: ~str, info: (~str, ~str)) {
+    let mut done = false;
+    let info_clone = info.clone();
+    match deps.find_mut(&parent) {
+        None => { }
+        Some(v) => { done = true; (*v).push(info) }
+    };
+    if !done {
+        deps.insert(parent, ~[info_clone]);
+    }
+}