From 667adad26f9cc1cfa4eeba8aee15035da7544f8c Mon Sep 17 00:00:00 2001 From: Tim Chevalier Date: Tue, 24 Sep 2013 12:10:44 -0700 Subject: [PATCH] rustpkg: Search for packages correctly when using the rust_path_hack Previously, any package would match any other package ID when searching using the rust_path_hack, so long as the directory had one or more crate files in it. Now, rustpkg checks that the parent directory matches the package ID. Closes #9273 --- src/librustpkg/package_source.rs | 3 +- src/librustpkg/path_util.rs | 23 +++++++++++--- src/librustpkg/tests.rs | 39 ++++++++++++++++++++++- src/librustpkg/util.rs | 43 ++++++++++++-------------- src/libstd/path.rs | 53 +++++++++++++++++++------------- 5 files changed, 109 insertions(+), 52 deletions(-) diff --git a/src/librustpkg/package_source.rs b/src/librustpkg/package_source.rs index 4bf647b011d..c2fddafd5fe 100644 --- a/src/librustpkg/package_source.rs +++ b/src/librustpkg/package_source.rs @@ -59,7 +59,8 @@ impl PkgSrc { use conditions::nonexistent_package::cond; debug!("Checking package source for package ID %s, \ - workspace = %s", id.to_str(), workspace.to_str()); + workspace = %s use_rust_path_hack = %?", + id.to_str(), workspace.to_str(), use_rust_path_hack); let mut to_try = ~[]; if use_rust_path_hack { diff --git a/src/librustpkg/path_util.rs b/src/librustpkg/path_util.rs index 3ed1b7a3a9c..7061345341f 100644 --- a/src/librustpkg/path_util.rs +++ b/src/librustpkg/path_util.rs @@ -421,11 +421,16 @@ fn dir_has_file(dir: &Path, file: &str) -> bool { pub fn find_dir_using_rust_path_hack(p: &PkgId) -> Option { let rp = rust_path(); for dir in rp.iter() { - debug!("In find_dir_using_rust_path_hack: checking dir %s", dir.to_str()); - if dir_has_file(dir, "lib.rs") || dir_has_file(dir, "main.rs") - || dir_has_file(dir, "test.rs") || dir_has_file(dir, "bench.rs") { - debug!("Did find id %s in dir %s", p.to_str(), dir.to_str()); - return Some(dir.clone()); + // Require that the parent directory match the package ID + // Note that this only matches if the package ID being searched for + // has a name that's a single component + if dir.is_parent_of(&p.path) || dir.is_parent_of(&versionize(&p.path, &p.version)) { + debug!("In find_dir_using_rust_path_hack: checking dir %s", dir.to_str()); + if dir_has_file(dir, "lib.rs") || dir_has_file(dir, "main.rs") + || dir_has_file(dir, "test.rs") || dir_has_file(dir, "bench.rs") { + debug!("Did find id %s in dir %s", p.to_str(), dir.to_str()); + return Some(dir.clone()); + } } debug!("Didn't find id %s in dir %s", p.to_str(), dir.to_str()) } @@ -440,3 +445,11 @@ pub fn user_set_rust_path() -> bool { Some(_) => true } } + +/// Append the version string onto the end of the path's filename +fn versionize(p: &Path, v: &Version) -> Path { + let q = p.file_path().to_str(); + p.with_filename(fmt!("%s-%s", q, v.to_str())) +} + + diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs index 79f137de853..52545d60420 100644 --- a/src/librustpkg/tests.rs +++ b/src/librustpkg/tests.rs @@ -1267,7 +1267,9 @@ fn test_rust_path_can_contain_package_dirs_without_flag() { #[test] fn rust_path_hack_cwd() { // Same as rust_path_hack_test, but the CWD is the dir to build out of - let cwd = mkdtemp(&os::tmpdir(), "pkg_files").expect("rust_path_hack_cwd"); + let cwd = mkdtemp(&os::tmpdir(), "foo").expect("rust_path_hack_cwd"); + let cwd = cwd.push("foo"); + assert!(os::mkdir_recursive(&cwd, U_RWX)); writeFile(&cwd.push("lib.rs"), "pub fn f() { }"); let dest_workspace = mk_empty_workspace(&Path("bar"), &NoVersion, "dest_workspace"); @@ -1763,6 +1765,41 @@ fn reinstall() { assert_built_executable_exists(&workspace, b.short_name); } +#[test] +fn correct_package_name_with_rust_path_hack() { + /* + Set rust_path_hack flag + + Try to install bar + Check that: + - no output gets produced in any workspace + - there's an error + */ + + // Set RUST_PATH to something containing only the sources for foo + let foo_id = PkgId::new("foo"); + let bar_id = PkgId::new("bar"); + let foo_workspace = create_local_package(&foo_id); + let dest_workspace = mk_empty_workspace(&Path("bar"), &NoVersion, "dest_workspace"); + + writeFile(&dest_workspace.push_many(["src", "bar-0.1", "main.rs"]), + "extern mod blat; fn main() { let _x = (); }"); + + let rust_path = Some(~[(~"RUST_PATH", fmt!("%s:%s", dest_workspace.to_str(), + foo_workspace.push_many(["src", "foo-0.1"]).to_str()))]); + // bar doesn't exist, but we want to make sure rustpkg doesn't think foo is bar + command_line_test_with_env([~"install", ~"--rust-path-hack", ~"bar"], + &dest_workspace, rust_path); + assert!(!executable_exists(&dest_workspace, "bar")); + assert!(!lib_exists(&dest_workspace, &bar_id.path.clone(), bar_id.version.clone())); + assert!(!executable_exists(&dest_workspace, "foo")); + assert!(!lib_exists(&dest_workspace, &foo_id.path.clone(), foo_id.version.clone())); + assert!(!executable_exists(&foo_workspace, "bar")); + assert!(!lib_exists(&foo_workspace, &bar_id.path.clone(), bar_id.version.clone())); + assert!(!executable_exists(&foo_workspace, "foo")); + assert!(!lib_exists(&foo_workspace, &foo_id.path.clone(), foo_id.version.clone())); +} + /// 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 5d5e895a5ad..b30ad6f2c92 100644 --- a/src/librustpkg/util.rs +++ b/src/librustpkg/util.rs @@ -439,26 +439,30 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> { let pkg_id = PkgId::new(lib_name); let workspaces = pkg_parent_workspaces(&self.context.context, &pkg_id); - let dep_workspace = if workspaces.is_empty() { - error(fmt!("Couldn't find package %s, which is needed by %s, \ - in any of the workspaces in the RUST_PATH (%?)", - lib_name, - self.parent.to_str(), - rust_path())); + let source_workspace = if workspaces.is_empty() { + error(fmt!("Couldn't find package %s \ + in any of the workspaces in the RUST_PATH (%s)", + lib_name, + rust_path().map(|s| s.to_str()).connect(":"))); cond.raise((pkg_id.clone(), ~"Dependency not found")) } - else { + else { workspaces[0] }; let (outputs_disc, inputs_disc) = - self.context.install(PkgSrc::new(dep_workspace.clone(), - false, + self.context.install(PkgSrc::new(source_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), &JustOne(Path( - lib_crate_filename))); + lib_crate_filename))); debug!("Installed %s, 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() { debug!("Discovering a binary input: %s", dep.to_str()); self.exec.discover_input("binary", @@ -471,31 +475,24 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> { *dep, digest_file_with_date(&Path(*dep))); } - else if *what == ~"binary" { + else if *what == ~"binary" { self.exec.discover_input(*what, *dep, digest_only_date(&Path(*dep))); } - else { + else { fail!("Bad kind: %s", *what); } } // Also, add an additional search path - debug!("Adding additional search path: %s", lib_name); - let installed_library = - installed_library_in_workspace(&Path(lib_name), &dep_workspace) - .expect(fmt!("rustpkg failed to install dependency %s", - lib_name)); - let install_dir = installed_library.pop(); - debug!("Installed %s into %s [%?]", lib_name, install_dir.to_str(), - datestamp(&installed_library)); - (self.save)(install_dir); + debug!("Installed %s into %s", lib_name, target_workspace.to_str()); + (self.save)(target_workspace); } - }} + } + } // Ignore `use`s _ => () } - visit::walk_view_item(self, vi, env) } } diff --git a/src/libstd/path.rs b/src/libstd/path.rs index c33a1ad11ee..af2565ec67a 100644 --- a/src/libstd/path.rs +++ b/src/libstd/path.rs @@ -236,16 +236,16 @@ pub trait GenericPath : Clone + Eq + ToStr { /// Returns `true` iff `child` is a suffix of `parent`. See the test /// case for examples. - pub fn is_parent_of(parent: &Path, child: &Path) -> bool { - if !parent.is_absolute() || child.is_absolute() - || parent.components.len() < child.components.len() - || parent.components.is_empty() { + fn is_parent_of(&self, child: &Self) -> bool { + if !self.is_absolute() || child.is_absolute() + || self.components().len() < child.components().len() + || self.components().is_empty() { return false; } let child_components = child.components().len(); - let parent_components = parent.components().len(); - let to_drop = parent.components.len() - child_components; - parent.components.slice(to_drop, parent_components) == child.components + let parent_components = self.components().len(); + let to_drop = self.components().len() - child_components; + self.components().slice(to_drop, parent_components) == child.components() } fn components<'a>(&'a self) -> &'a [~str]; @@ -1468,14 +1468,22 @@ mod tests { #[test] fn test_is_parent_of() { - assert!(is_parent_of(&PosixPath("/a/b/c/d/e"), &PosixPath("c/d/e"))); - assert!(!is_parent_of(&PosixPath("a/b/c/d/e"), &PosixPath("c/d/e"))); - assert!(!is_parent_of(&PosixPath("/a/b/c/d/e"), &PosixPath("/c/d/e"))); - assert!(!is_parent_of(&PosixPath(""), &PosixPath(""))); - assert!(!is_parent_of(&PosixPath(""), &PosixPath("a/b/c"))); - assert!(is_parent_of(&PosixPath("/a/b/c"), &PosixPath(""))); - assert!(is_parent_of(&PosixPath("/a/b/c"), &PosixPath("a/b/c"))); - assert!(!is_parent_of(&PosixPath("/a/b/c"), &PosixPath("d/e/f"))); + fn is_parent_of_pp(p: &PosixPath, q: &PosixPath) -> bool { + p.is_parent_of(q) + } + + assert!(is_parent_of_pp(&PosixPath("/a/b/c/d/e"), &PosixPath("c/d/e"))); + assert!(!is_parent_of_pp(&PosixPath("a/b/c/d/e"), &PosixPath("c/d/e"))); + assert!(!is_parent_of_pp(&PosixPath("/a/b/c/d/e"), &PosixPath("/c/d/e"))); + assert!(!is_parent_of_pp(&PosixPath(""), &PosixPath(""))); + assert!(!is_parent_of_pp(&PosixPath(""), &PosixPath("a/b/c"))); + assert!(is_parent_of_pp(&PosixPath("/a/b/c"), &PosixPath(""))); + assert!(is_parent_of_pp(&PosixPath("/a/b/c"), &PosixPath("a/b/c"))); + assert!(!is_parent_of_pp(&PosixPath("/a/b/c"), &PosixPath("d/e/f"))); + + fn is_parent_of_wp(p: &WindowsPath, q: &WindowsPath) -> bool { + p.is_parent_of(q) + } let abcde = WindowsPath("C:\\a\\b\\c\\d\\e"); let rel_abcde = WindowsPath("a\\b\\c\\d\\e"); @@ -1486,13 +1494,14 @@ mod tests { let rel_abc = WindowsPath("a\\b\\c"); let def = WindowsPath("d\\e\\f"); - assert!(is_parent_of(&abcde, &cde)); - assert!(!is_parent_of(&rel_abcde, &cde)); - assert!(!is_parent_of(&abcde, &slashcde)); - assert!(!is_parent_of(&empty, &empty)); - assert!(is_parent_of(&abc, &empty); - assert!(is_parent_of(&abc, &rel_abc)); - assert!(!is_parent_of(&abc, &def)); + assert!(is_parent_of_wp(&abcde, &cde)); + assert!(!is_parent_of_wp(&rel_abcde, &cde)); + assert!(!is_parent_of_wp(&abcde, &slashcde)); + assert!(!is_parent_of_wp(&empty, &empty)); + assert!(!is_parent_of_wp(&empty, &rel_abc)); + assert!(is_parent_of_wp(&abc, &empty)); + assert!(is_parent_of_wp(&abc, &rel_abc)); + assert!(!is_parent_of_wp(&abc, &def)); } }