From 3e4e1a274a7b3bf760c7a09d1a004728c8590362 Mon Sep 17 00:00:00 2001 From: Tim Chevalier Date: Fri, 23 Aug 2013 11:51:45 -0700 Subject: [PATCH] rustpkg: Test that different copies of the same package ID can exist in multiple workspaces The test checks that rustpkg uses the first one, rather than complaining about multiple matches. Closes #7241 --- src/librustc/metadata/filesearch.rs | 53 +++++++++++++++++++---------- src/librustc/metadata/loader.rs | 14 ++++---- src/librustpkg/path_util.rs | 6 ++++ src/librustpkg/rustpkg.rs | 2 ++ src/librustpkg/search.rs | 18 +++++++++- src/librustpkg/tests.rs | 31 +++++++++++++++-- src/librustpkg/util.rs | 35 +++++++++++++------ src/librustpkg/version.rs | 6 ++-- 8 files changed, 122 insertions(+), 43 deletions(-) diff --git a/src/librustc/metadata/filesearch.rs b/src/librustc/metadata/filesearch.rs index 33d517f3981bb..757c53354a24f 100644 --- a/src/librustc/metadata/filesearch.rs +++ b/src/librustc/metadata/filesearch.rs @@ -13,13 +13,15 @@ use std::option; use std::os; use std::hashmap::HashSet; +pub enum FileMatch { FileMatches, FileDoesntMatch } + // A module for searching for libraries // FIXME (#2658): I'm not happy how this module turned out. Should // probably just be folded into cstore. /// Functions with type `pick` take a parent directory as well as /// a file found in that directory. -pub type pick<'self, T> = &'self fn(path: &Path) -> Option; +pub type pick<'self> = &'self fn(path: &Path) -> FileMatch; pub fn pick_file(file: Path, path: &Path) -> Option { if path.file_path() == file { @@ -31,7 +33,7 @@ pub fn pick_file(file: Path, path: &Path) -> Option { pub trait FileSearch { fn sysroot(&self) -> @Path; - fn for_each_lib_search_path(&self, f: &fn(&Path) -> bool) -> bool; + fn for_each_lib_search_path(&self, f: &fn(&Path) -> FileMatch); fn get_target_lib_path(&self) -> Path; fn get_target_lib_file_path(&self, file: &Path) -> Path; } @@ -47,13 +49,17 @@ pub fn mk_filesearch(maybe_sysroot: &Option<@Path>, } impl FileSearch for FileSearchImpl { fn sysroot(&self) -> @Path { self.sysroot } - fn for_each_lib_search_path(&self, f: &fn(&Path) -> bool) -> bool { + fn for_each_lib_search_path(&self, f: &fn(&Path) -> FileMatch) { let mut visited_dirs = HashSet::new(); + let mut found = false; debug!("filesearch: searching additional lib search paths [%?]", self.addl_lib_search_paths.len()); for path in self.addl_lib_search_paths.iter() { - f(path); + match f(path) { + FileMatches => found = true, + FileDoesntMatch => () + } visited_dirs.insert(path.to_str()); } @@ -61,20 +67,33 @@ pub fn mk_filesearch(maybe_sysroot: &Option<@Path>, let tlib_path = make_target_lib_path(self.sysroot, self.target_triple); if !visited_dirs.contains(&tlib_path.to_str()) { - if !f(&tlib_path) { - return false; + match f(&tlib_path) { + FileMatches => found = true, + FileDoesntMatch => () } } visited_dirs.insert(tlib_path.to_str()); // Try RUST_PATH - let rustpath = rust_path(); - for path in rustpath.iter() { + if !found { + let rustpath = rust_path(); + for path in rustpath.iter() { + debug!("is %s in visited_dirs? %?", + path.push("lib").to_str(), + visited_dirs.contains(&path.push("lib").to_str())); + if !visited_dirs.contains(&path.push("lib").to_str()) { - f(&path.push("lib")); visited_dirs.insert(path.push("lib").to_str()); + // Don't keep searching the RUST_PATH if one match turns up -- + // if we did, we'd get a "multiple matching crates" error + match f(&path.push("lib")) { + FileMatches => { + break; + } + FileDoesntMatch => () + } } + } } - true } fn get_target_lib_path(&self) -> Path { make_target_lib_path(self.sysroot, self.target_triple) @@ -93,28 +112,26 @@ pub fn mk_filesearch(maybe_sysroot: &Option<@Path>, } as @FileSearch } -pub fn search(filesearch: @FileSearch, pick: pick) -> Option { - let mut rslt = None; +pub fn search(filesearch: @FileSearch, pick: pick) { do filesearch.for_each_lib_search_path() |lib_search_path| { debug!("searching %s", lib_search_path.to_str()); let r = os::list_dir_path(lib_search_path); + let mut rslt = FileDoesntMatch; for path in r.iter() { debug!("testing %s", path.to_str()); let maybe_picked = pick(path); match maybe_picked { - Some(_) => { + FileMatches => { debug!("picked %s", path.to_str()); - rslt = maybe_picked; - break; + rslt = FileMatches; } - None => { + FileDoesntMatch => { debug!("rejected %s", path.to_str()); } } } - rslt.is_none() + rslt }; - return rslt; } pub fn relative_target_lib_path(target_triple: &str) -> Path { diff --git a/src/librustc/metadata/loader.rs b/src/librustc/metadata/loader.rs index d0060931a6607..fbf6e3fcff8a7 100644 --- a/src/librustc/metadata/loader.rs +++ b/src/librustc/metadata/loader.rs @@ -14,7 +14,7 @@ use lib::llvm::{False, llvm, mk_object_file, mk_section_iter}; use metadata::decoder; use metadata::encoder; -use metadata::filesearch::FileSearch; +use metadata::filesearch::{FileSearch, FileMatch, FileMatches, FileDoesntMatch}; use metadata::filesearch; use syntax::codemap::span; use syntax::diagnostic::span_handler; @@ -92,10 +92,10 @@ fn find_library_crate_aux( // want: crate_name.dir_part() + prefix + crate_name.file_part + "-" let prefix = fmt!("%s%s-", prefix, crate_name); let mut matches = ~[]; - filesearch::search(filesearch, |path| -> Option<()> { + filesearch::search(filesearch, |path| -> FileMatch { let path_str = path.filename(); match path_str { - None => None, + None => FileDoesntMatch, Some(path_str) => if path_str.starts_with(prefix) && path_str.ends_with(suffix) { debug!("%s is a candidate", path.to_str()); @@ -104,20 +104,20 @@ fn find_library_crate_aux( if !crate_matches(cvec, cx.metas, cx.hash) { debug!("skipping %s, metadata doesn't match", path.to_str()); - None + FileDoesntMatch } else { debug!("found %s with matching metadata", path.to_str()); matches.push((path.to_str(), cvec)); - None + FileMatches }, _ => { debug!("could not load metadata for %s", path.to_str()); - None + FileDoesntMatch } } } else { - None + FileDoesntMatch } } }); diff --git a/src/librustpkg/path_util.rs b/src/librustpkg/path_util.rs index 2dbd054ef60bc..467477ca479de 100644 --- a/src/librustpkg/path_util.rs +++ b/src/librustpkg/path_util.rs @@ -49,6 +49,9 @@ pub fn make_dir_rwx(p: &Path) -> bool { os::make_dir(p, U_RWX) } /// True if there's a directory in with /// pkgid's short name pub fn workspace_contains_package_id(pkgid: &PkgId, workspace: &Path) -> bool { + debug!("Checking in src dir of %s for %s", + workspace.to_str(), pkgid.to_str()); + let src_dir = workspace.push("src"); let mut found = false; @@ -81,6 +84,9 @@ pub fn workspace_contains_package_id(pkgid: &PkgId, workspace: &Path) -> bool { } true }; + + debug!(if found { fmt!("Found %s in %s", pkgid.to_str(), workspace.to_str()) } + else { fmt!("Didn't find %s in %s", pkgid.to_str(), workspace.to_str()) }); found } diff --git a/src/librustpkg/rustpkg.rs b/src/librustpkg/rustpkg.rs index f5dc17851e5cc..b78460dc2243b 100644 --- a/src/librustpkg/rustpkg.rs +++ b/src/librustpkg/rustpkg.rs @@ -250,6 +250,8 @@ impl CtxMethods for Ctx { // argument let pkgid = PkgId::new(args[0]); let workspaces = pkg_parent_workspaces(&pkgid); + debug!("package ID = %s, found it in %? workspaces", + pkgid.to_str(), workspaces.len()); if workspaces.is_empty() { let rp = rust_path(); assert!(!rp.is_empty()); diff --git a/src/librustpkg/search.rs b/src/librustpkg/search.rs index ea0389fed7727..9862f870bcafb 100644 --- a/src/librustpkg/search.rs +++ b/src/librustpkg/search.rs @@ -8,7 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use path_util::installed_library_in_workspace; +use path_util::{installed_library_in_workspace, rust_path}; +use version::Version; /// If a library with path `p` matching pkg_id's name exists under sroot_opt, /// return Some(p). Return None if there's no such path or if sroot_opt is None. @@ -19,3 +20,18 @@ pub fn find_library_in_search_path(sroot_opt: Option<@Path>, short_name: &str) - installed_library_in_workspace(short_name, sroot) } } + +/// If some workspace `p` in the RUST_PATH contains a package matching short_name, +/// return Some(p) (returns the first one of there are multiple matches.) Return +/// None if there's no such path. +/// FIXME #8711: This ignores the desired version. +pub fn find_installed_library_in_rust_path(short_name: &str, _version: &Version) -> Option { + let rp = rust_path(); + for p in rp.iter() { + match installed_library_in_workspace(short_name, p) { + Some(path) => return Some(path), + None => () + } + } + None +} diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs index 0efa0782da902..fc40c2cd5f79d 100644 --- a/src/librustpkg/tests.rs +++ b/src/librustpkg/tests.rs @@ -686,7 +686,7 @@ fn package_script_with_default_build() { push("testsuite").push("pass").push("src").push("fancy-lib").push("pkg.rs"); debug!("package_script_with_default_build: %s", source.to_str()); if !os::copy_file(&source, - & dir.push("src").push("fancy-lib-0.1").push("pkg.rs")) { + &dir.push("src").push("fancy-lib-0.1").push("pkg.rs")) { fail!("Couldn't copy file"); } command_line_test([~"install", ~"fancy-lib"], &dir); @@ -890,20 +890,28 @@ fn no_rebuilding_dep() { assert!(bar_date < foo_date); } +// n.b. The following two tests are ignored; they worked "accidentally" before, +// when the behavior was "always rebuild libraries" (now it's "never rebuild +// libraries if they already exist"). They can be un-ignored once #7075 is done. #[test] +#[ignore(reason = "Workcache not yet implemented -- see #7075")] 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_date = datestamp(&lib_output_file_name(&workspace, "build", "bar")); + let bar_lib_name = lib_output_file_name(&workspace, "build", "bar"); + let bar_date = datestamp(&bar_lib_name); + debug!("Datestamp on %s 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(&lib_output_file_name(&workspace, "build", "bar")); + let new_bar_date = datestamp(&bar_lib_name); + debug!("Datestamp on %s is %?", bar_lib_name.to_str(), new_bar_date); assert!(new_bar_date > bar_date); } #[test] +#[ignore(reason = "Workcache not yet implemented -- see #7075")] fn do_rebuild_dep_only_contents_change() { let p_id = PkgId::new("foo"); let dep_id = PkgId::new("bar"); @@ -1060,6 +1068,23 @@ fn test_macro_pkg_script() { os::EXE_SUFFIX)))); } +#[test] +fn multiple_workspaces() { +// Make a package foo; build/install in directory A +// Copy the exact same package into directory B and install it +// Set the RUST_PATH to A:B +// Make a third package that uses foo, make sure we can build/install it + let a_loc = mk_temp_workspace(&Path("foo"), &NoVersion).pop().pop(); + let b_loc = mk_temp_workspace(&Path("foo"), &NoVersion).pop().pop(); + debug!("Trying to install foo in %s", a_loc.to_str()); + command_line_test([~"install", ~"foo"], &a_loc); + debug!("Trying to install foo in %s", b_loc.to_str()); + command_line_test([~"install", ~"foo"], &b_loc); + let env = Some(~[(~"RUST_PATH", fmt!("%s:%s", a_loc.to_str(), b_loc.to_str()))]); + let c_loc = create_local_package_with_dep(&PkgId::new("bar"), &PkgId::new("foo")); + command_line_test_with_env([~"install", ~"bar"], &c_loc, env); +} + /// Returns true if p exists and is executable fn is_executable(p: &Path) -> bool { use std::libc::consts::os::posix88::{S_IXUSR}; diff --git a/src/librustpkg/util.rs b/src/librustpkg/util.rs index 41c1c7e31aeff..4bdb442c1e619 100644 --- a/src/librustpkg/util.rs +++ b/src/librustpkg/util.rs @@ -20,9 +20,10 @@ use rustc::back::link::output_type_exe; use rustc::driver::session::{lib_crate, bin_crate}; use context::{Ctx, in_target}; use package_id::PkgId; -use search::find_library_in_search_path; +use search::{find_library_in_search_path, find_installed_library_in_rust_path}; use path_util::{target_library_in_workspace, U_RWX}; pub use target::{OutputType, Main, Lib, Bench, Test}; +use version::NoVersion; // 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 @@ -360,18 +361,32 @@ pub fn find_and_install_dependencies(ctxt: &Ctx, debug!("It exists: %s", installed_path.to_str()); } None => { - // Try to install it - let pkg_id = PkgId::new(lib_name); - my_ctxt.install(&my_workspace, &pkg_id); - // Also, add an additional search path - debug!("let installed_path...") - let installed_path = target_library_in_workspace(&pkg_id, + // FIXME #8711: need to parse version out of path_opt + match find_installed_library_in_rust_path(lib_name, &NoVersion) { + Some(installed_path) => { + debug!("Found library %s, not rebuilding it", + installed_path.to_str()); + // Once workcache is implemented, we'll actually check + // whether or not the library at installed_path is fresh + save(installed_path.pop()); + } + None => { + debug!("Trying to install library %s, rebuilding it", + lib_name.to_str()); + // Try to install it + let pkg_id = PkgId::new(lib_name); + my_ctxt.install(&my_workspace, &pkg_id); + // Also, add an additional search path + debug!("let installed_path...") + let installed_path = target_library_in_workspace(&pkg_id, &my_workspace).pop(); - debug!("Great, I installed %s, and it's in %s", - lib_name, installed_path.to_str()); - save(installed_path); + debug!("Great, I installed %s, and it's in %s", + lib_name, installed_path.to_str()); + save(installed_path); + } } } + } } // Ignore `use`s _ => () diff --git a/src/librustpkg/version.rs b/src/librustpkg/version.rs index f8658517cdff4..4528a02db1978 100644 --- a/src/librustpkg/version.rs +++ b/src/librustpkg/version.rs @@ -213,11 +213,9 @@ fn is_url_like(p: &Path) -> bool { pub fn split_version<'a>(s: &'a str) -> Option<(&'a str, Version)> { // Check for extra '#' characters separately if s.split_iter('#').len() > 2 { - None - } - else { - split_version_general(s, '#') + return None; } + split_version_general(s, '#') } pub fn split_version_general<'a>(s: &'a str, sep: char) -> Option<(&'a str, Version)> {