From 9ffeeba61f1ef8497b5206ad0598dbf924c2234b Mon Sep 17 00:00:00 2001
From: Tim Chevalier <chevalier@alum.wellesley.edu>
Date: Thu, 14 Nov 2013 14:38:57 -0800
Subject: [PATCH 1/2] rustpkg: Handle requested versions in `extern mod`
 directives properly

Closes #8711
---
 src/librustc/metadata/creader.rs    |  36 +++-
 src/librustc/metadata/filesearch.rs |  24 +++
 src/librustpkg/lib.rs               |  12 +-
 src/librustpkg/package_source.rs    | 312 ++++++++++++++++++----------
 src/librustpkg/path_util.rs         |   2 +-
 src/librustpkg/source_control.rs    |   2 +-
 src/librustpkg/tests.rs             |  54 ++++-
 src/librustpkg/util.rs              |   4 +-
 src/librustpkg/version.rs           |   1 -
 9 files changed, 309 insertions(+), 138 deletions(-)

diff --git a/src/librustc/metadata/creader.rs b/src/librustc/metadata/creader.rs
index f7b1955191abd..76a29539b6066 100644
--- a/src/librustc/metadata/creader.rs
+++ b/src/librustc/metadata/creader.rs
@@ -13,12 +13,11 @@
 
 use metadata::cstore;
 use metadata::decoder;
-use metadata::filesearch::FileSearch;
+use metadata::filesearch::{FileSearch, split_version};
 use metadata::loader;
 
 use std::hashmap::HashMap;
 use syntax::ast;
-use std::vec;
 use syntax::attr;
 use syntax::attr::AttrMetaMethods;
 use syntax::codemap::{Span, dummy_sp};
@@ -143,15 +142,30 @@ fn visit_view_item(e: @mut Env, i: &ast::view_item) {
           let meta_items = match path_opt {
               None => meta_items.clone(),
               Some((p, _path_str_style)) => {
-                  let p_path = Path::new(p);
-                  match p_path.filestem_str() {
-                      None|Some("") =>
-                          e.diag.span_bug(i.span, "Bad package path in `extern mod` item"),
-                      Some(s) =>
-                          vec::append(
-                              ~[attr::mk_name_value_item_str(@"package_id", p),
-                               attr::mk_name_value_item_str(@"name", s.to_managed())],
-                              *meta_items)
+                  match split_version(p) {
+                      None => {
+                          let p_path = Path::new(p);
+                          let s = match p_path.filestem_str() {
+                                None|Some("") =>
+                                    e.diag.span_bug(i.span,
+                                                    "Bad package path in `extern mod` item"),
+                                Some(s) => s,
+                          };
+                            ~[attr::mk_name_value_item_str(@"package_id", p),
+                              attr::mk_name_value_item_str(@"name", s.to_managed())]
+                      }
+                      Some((name, version)) => {
+                          let p_path = Path::new(name);
+                          let s = match p_path.filestem_str() {
+                                None|Some("") =>
+                                    e.diag.span_bug(i.span,
+                                                    "Bad package path in `extern mod` item"),
+                                Some(s) => s,
+                          };
+                          ~[attr::mk_name_value_item_str(@"package_id", name.to_managed()),
+                            attr::mk_name_value_item_str(@"name", s.to_managed()),
+                            attr::mk_name_value_item_str(@"vers", version.to_managed())]
+                      }
                   }
             }
           };
diff --git a/src/librustc/metadata/filesearch.rs b/src/librustc/metadata/filesearch.rs
index 237c50ab29410..c625ce31354f9 100644
--- a/src/librustc/metadata/filesearch.rs
+++ b/src/librustc/metadata/filesearch.rs
@@ -236,3 +236,27 @@ pub fn rust_path() -> ~[Path] {
 pub fn libdir() -> ~str {
     (env!("CFG_LIBDIR")).to_owned()
 }
+
+
+/// If s is of the form foo#bar, where bar is a valid version
+/// number, return the prefix before the # and the version.
+/// Otherwise, return None.
+pub fn split_version<'a>(s: &'a str) -> Option<(&'a str, &'a str)> {
+    // Check for extra '#' characters separately
+    if s.split_iter('#').len() > 2 {
+        return None;
+    }
+    split_version_general(s, '#')
+}
+
+pub fn split_version_general<'a>(s: &'a str, sep: char) -> Option<(&'a str, &'a str)> {
+    match s.rfind(sep) {
+        Some(i) => {
+            let path = s.slice(0, i);
+            Some((path, s.slice(i + 1, s.len())))
+        }
+        None => {
+            None
+        }
+    }
+}
diff --git a/src/librustpkg/lib.rs b/src/librustpkg/lib.rs
index 0891038e8d5f3..6a67d84964eed 100644
--- a/src/librustpkg/lib.rs
+++ b/src/librustpkg/lib.rs
@@ -433,17 +433,19 @@ impl CtxMethods for BuildContext {
         let pkgid = pkg_src.id.clone();
 
         debug!("build: workspace = {} (in Rust path? {:?} is git dir? {:?} \
-                pkgid = {} pkgsrc start_dir = {}", workspace.display(),
+               pkgid = {} pkgsrc start_dir = {}", workspace.display(),
                in_rust_path(&workspace), is_git_dir(&workspace.join(&pkgid.path)),
-               pkgid.to_str(), pkg_src.start_dir.display());
+               pkgid.to_str(),
+               pkg_src.start_dir.display());
+        let pkg_dir = workspace.join(&pkgid.path);
         debug!("build: what to build = {:?}", what_to_build);
 
         // If workspace isn't in the RUST_PATH, and it's a git repo,
         // then clone it into the first entry in RUST_PATH, and repeat
-        if !in_rust_path(&workspace) && is_git_dir(&workspace.join(&pkgid.path)) {
+        if !in_rust_path(&workspace) && is_git_dir(&pkg_dir) {
             let mut out_dir = default_workspace().join("src");
             out_dir.push(&pkgid.path);
-            let git_result = source_control::safe_git_clone(&workspace.join(&pkgid.path),
+            let git_result = source_control::safe_git_clone(&pkg_dir,
                                                             &pkgid.version,
                                                             &out_dir);
             match git_result {
@@ -516,7 +518,7 @@ impl CtxMethods for BuildContext {
                 JustOne(ref p) => {
                     // We expect that p is relative to the package source's start directory,
                     // so check that assumption
-                    debug!("JustOne: p = {}", p.display());
+                    debug!("JustOne: p = {} and {}", p.display(), pkg_src.start_dir.display());
                     assert!(pkg_src.start_dir.join(p).exists());
                     if is_lib(p) {
                         PkgSrc::push_crate(&mut pkg_src.libs, 0, p);
diff --git a/src/librustpkg/package_source.rs b/src/librustpkg/package_source.rs
index b6edab3d65ad0..8db8df7480168 100644
--- a/src/librustpkg/package_source.rs
+++ b/src/librustpkg/package_source.rs
@@ -19,7 +19,7 @@ use context::*;
 use crate::Crate;
 use messages::*;
 use source_control::{safe_git_clone, git_clone_url, DirToUse, CheckedOutSources};
-use source_control::make_read_only;
+use source_control::{is_git_dir, make_read_only};
 use path_util::{find_dir_using_rust_path_hack, make_dir_rwx_recursive, default_workspace};
 use path_util::{target_build_dir, versionize, dir_has_crate_file};
 use util::{compile_crate, DepMap};
@@ -70,10 +70,23 @@ condition! {
     build_err: (~str) -> ~str;
 }
 
+// Package source files can be in a workspace
+// subdirectory, or in a source control repository
+enum StartDir { InWorkspace(Path), InRepo(Path) }
+
+impl ToStr for StartDir {
+    fn to_str(&self) -> ~str {
+        match *self {
+            InWorkspace(ref p) => format!("workspace({})", p.display()),
+            InRepo(ref p)      => format!("repository({})", p.display())
+        }
+    }
+}
+
 impl PkgSrc {
 
     pub fn new(mut source_workspace: Path,
-               destination_workspace: Path,
+               mut destination_workspace: Path,
                use_rust_path_hack: bool,
                id: PkgId) -> PkgSrc {
         use conditions::nonexistent_package::cond;
@@ -85,54 +98,94 @@ impl PkgSrc {
                 destination_workspace.display(),
                 use_rust_path_hack);
 
-        let mut destination_workspace = destination_workspace.clone();
-
-        let mut to_try = ~[];
-        let mut output_names = ~[];
-        let build_dir = target_build_dir(&source_workspace);
+        // Possible locations to look for sources
+        let mut paths_to_search = ~[];
+        // Possible places to put the checked-out sources (with or without version)
+        let mut possible_cache_dirs = ~[];
+        // The directory in which to put build products for this package
+        let where_to_build = target_build_dir(&source_workspace);
 
         if use_rust_path_hack {
-            to_try.push(source_workspace.clone());
+            paths_to_search.push(InWorkspace(source_workspace.clone()));
         } else {
             // We search for sources under both src/ and build/ , because build/ is where
             // automatically-checked-out sources go.
-            let mut result = source_workspace.join("src");
-            result.push(&id.path.dir_path());
-            result.push(format!("{}-{}", id.short_name, id.version.to_str()));
-            to_try.push(result);
-            let mut result = source_workspace.join("src");
-            result.push(&id.path);
-            to_try.push(result);
-
-            let mut result = build_dir.join("src");
-            result.push(&id.path.dir_path());
-            result.push(format!("{}-{}", id.short_name, id.version.to_str()));
-            to_try.push(result.clone());
-            output_names.push(result);
-            let mut other_result = build_dir.join("src");
-            other_result.push(&id.path);
-            to_try.push(other_result.clone());
-            output_names.push(other_result);
+
+            paths_to_search.push(
+                InWorkspace(source_workspace.join("src").
+                            join(id.path.clone().dir_path()).
+                            join(format!("{}-{}",
+                                         id.short_name.clone(),
+                                         id.version.to_str().clone()))));
+            paths_to_search.push(InWorkspace(source_workspace.join("src").join(id.path.clone())));
+            // Try it without the src/ too, in the case of a local git repo
+            paths_to_search.push(InRepo(source_workspace.join(id.path.clone().dir_path()).
+                                        join(format!("{}-{}",
+                                                     id.short_name.clone(),
+                                                     id.version.to_str().clone()))));
+            paths_to_search.push(InRepo(source_workspace.join(id.path.clone())));
+
+
+            // Search the CWD
+            let cwd = os::getcwd();
+            paths_to_search.push(if dir_has_crate_file(&cwd) {
+                    InRepo(cwd) } else { InWorkspace(cwd) });
+
+            // Search the build dir too, since that's where automatically-downloaded sources go
+            let in_build_dir =
+                where_to_build.join("src").
+                               join(id.path.clone().dir_path()).join(
+                                          format!("{}-{}",
+                                                  id.short_name.clone(),
+                                                  id.version.to_str()));
+            paths_to_search.push(InWorkspace(in_build_dir.clone()));
+            possible_cache_dirs.push(in_build_dir);
+            let in_build_dir = where_to_build.join("src").join(id.path.clone());
+            paths_to_search.push(InWorkspace(in_build_dir.clone()));
+            possible_cache_dirs.push(in_build_dir);
 
         }
 
-        debug!("Checking dirs: {:?}", to_try.map(|p| p.display().to_str()).connect(":"));
+        debug!("Searching for sources in the following locations: {:?}",
+               paths_to_search.map(|p| p.to_str()).connect(":"));
 
-        let path = to_try.iter().find(|&d| d.is_dir()
-                                      && dir_has_crate_file(d));
+        let dir_containing_sources_opt =
+            paths_to_search.iter().find(|&d| match d {
+                &InWorkspace(ref d) | &InRepo(ref 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;
-        debug!("1. build_in_destination = {:?}", build_in_destination);
 
-        let dir: Path = match path {
-            Some(d) => (*d).clone(),
+        let mut start_dir = None;
+        // Used only if the sources are found in a local git repository
+        let mut local_git_dir = None;
+        // If we determine that the sources are findable without git
+        // this gets set to false
+        let mut use_git = true;
+        // Now we determine the top-level directory that contains the sources for this
+        // package. Normally we would expect it to be <source-workspace>/src/<id.short_name>,
+        // but there are some special cases.
+        match dir_containing_sources_opt {
+            Some(top_dir) => {
+                // If this is a local git repository, we have to check it
+                // out into a workspace
+                match *top_dir {
+                    InWorkspace(ref d) => {
+                        use_git = false;
+                        start_dir = Some(d.clone());
+                    }
+                    InRepo(ref repo_dir) if is_git_dir(repo_dir) => {
+                        local_git_dir = Some(repo_dir.clone());
+                    }
+                    InRepo(_) => {} // Weird, but do nothing
+                }
+            }
             None => {
                 // See if any of the prefixes of this package ID form a valid package ID
                 // That is, is this a package ID that points into the middle of a workspace?
                 for (prefix, suffix) in id.prefixes_iter() {
                     let package_id = PkgId::new(prefix.as_str().unwrap());
-                    let path = build_dir.join(&package_id.path);
+                    let path = where_to_build.join(&package_id.path);
                     debug!("in loop: checking if {} is a directory", path.display());
                     if path.is_dir() {
                         let ps = PkgSrc::new(source_workspace,
@@ -163,97 +216,121 @@ impl PkgSrc {
 
                     };
                 }
+            }
+        }
 
-                // Ok, no prefixes work, so try fetching from git
-                let mut ok_d = None;
-                for w in output_names.iter() {
-                    debug!("Calling fetch_git on {}", w.display());
-                    let target_dir_opt = PkgSrc::fetch_git(w, &id);
-                    for p in target_dir_opt.iter() {
-                        ok_d = Some(p.clone());
-                        build_in_destination = true;
-                        debug!("2. build_in_destination = {:?}", build_in_destination);
-                        break;
-                    }
-                    match ok_d {
-                        Some(ref d) => {
-                            if d.is_ancestor_of(&id.path)
-                                || d.is_ancestor_of(&versionize(&id.path, &id.version)) {
-                                // Strip off the package ID
-                                source_workspace = d.clone();
-                                for _ in id.path.component_iter() {
+        if use_git {
+            // At this point, we don't know whether it's named `foo-0.2` or just `foo`
+            for cache_dir in possible_cache_dirs.iter() {
+                let mut target_dir_for_checkout = None;
+                debug!("Calling fetch_git on {}", cache_dir.display());
+                let checkout_target_dir_opt = PkgSrc::fetch_git(cache_dir,
+                                                                &id,
+                                                                &local_git_dir);
+                for checkout_target_dir in checkout_target_dir_opt.iter() {
+                    target_dir_for_checkout = Some(checkout_target_dir.clone());
+                    // In this case, we put the build products in the destination
+                    // workspace, because this package originated from a non-workspace.
+                    build_in_destination = true;
+                }
+                match target_dir_for_checkout {
+                    Some(ref checkout_target_dir) => {
+                        if checkout_target_dir.is_ancestor_of(&id.path)
+                            || checkout_target_dir.is_ancestor_of(&versionize(&id.path,
+                                                                              &id.version)) {
+                            // This means that we successfully checked out the git sources
+                            // into `checkout_target_dir`.
+                            // Now determine whether or not the source was actually a workspace
+                            // (that is, whether it has a `src` subdirectory)
+                            // Strip off the package ID
+                            source_workspace = checkout_target_dir.clone();
+                            for _ in id.path.component_iter() {
+                                source_workspace.pop();
+                            }
+                            // Strip off the src/ part
+                            match source_workspace.filename_str() {
+                                None => (),
+                                Some("src") => {
                                     source_workspace.pop();
                                 }
-                                // Strip off the src/ part
-                                source_workspace.pop();
-                                // Strip off the build/<target-triple> part to get the workspace
-                                destination_workspace = source_workspace.clone();
-                                destination_workspace.pop();
-                                destination_workspace.pop();
+                                _ => ()
                             }
-                            break;
+                        // Strip off the build/<target-triple> part to get the workspace
+                        destination_workspace = source_workspace.clone();
+                        destination_workspace.pop();
+                        destination_workspace.pop();
                         }
-                        None => ()
+                        start_dir = Some(checkout_target_dir.clone());
+                        break;
                     }
-                }
-                match ok_d {
-                    Some(d) => d,
+                    // In this case, the git checkout did not succeed.
                     None => {
-                        // See if the sources are in $CWD
-                        let cwd = os::getcwd();
-                        if dir_has_crate_file(&cwd) {
-                            return PkgSrc {
-                                // In this case, source_workspace isn't really a workspace.
-                                // This data structure needs yet more refactoring.
-                                source_workspace: cwd.clone(),
-                                destination_workspace: default_workspace(),
-                                build_in_destination: true,
-                                start_dir: cwd,
-                                id: id,
-                                libs: ~[],
-                                mains: ~[],
-                                benchs: ~[],
-                                tests: ~[]
-                            }
-                        } else if use_rust_path_hack {
-                            match find_dir_using_rust_path_hack(&id) {
-                                Some(d) => d,
-                                None => {
-                                    cond.raise((id.clone(),
-                                        ~"supplied path for package dir does not \
-                                        exist, and couldn't interpret it as a URL fragment"))
-                                }
-                            }
-                        }
-                        else {
-                            cond.raise((id.clone(),
-                                ~"supplied path for package dir does not \
-                                exist, and couldn't interpret it as a URL fragment"))
-                        }
+                        debug!("With cache_dir = {}, checkout failed", cache_dir.display());
                     }
                 }
             }
-        };
-        debug!("3. build_in_destination = {:?}", build_in_destination);
-        debug!("source: {} dest: {}", source_workspace.display(), destination_workspace.display());
-
-        debug!("For package id {}, returning {}", id.to_str(), dir.display());
-
-        if !dir.is_dir() {
-            cond.raise((id.clone(), ~"supplied path for package dir is a \
-                                        non-directory"));
         }
 
-        PkgSrc {
-            source_workspace: source_workspace.clone(),
-            build_in_destination: build_in_destination,
-            destination_workspace: destination_workspace,
-            start_dir: dir,
-            id: id,
-            libs: ~[],
-            mains: ~[],
-            tests: ~[],
-            benchs: ~[]
+        // If we still haven't found it yet...
+        if start_dir.is_none() {
+            // See if the sources are in $CWD
+            let cwd = os::getcwd();
+            if dir_has_crate_file(&cwd) {
+                return PkgSrc {
+                    // In this case, source_workspace isn't really a workspace.
+                    // This data structure needs yet more refactoring.
+                    source_workspace: cwd.clone(),
+                    destination_workspace: default_workspace(),
+                    build_in_destination: true,
+                    start_dir: cwd,
+                    id: id,
+                    libs: ~[],
+                    mains: ~[],
+                    benchs: ~[],
+                    tests: ~[]
+                }
+            } else if use_rust_path_hack {
+                let dir_opt = find_dir_using_rust_path_hack(&id);
+                for dir_with_sources in dir_opt.iter() {
+                    start_dir = Some(dir_with_sources.clone());
+                }
+            }
+        };
+
+        match start_dir {
+            None => {
+                PkgSrc {
+                    source_workspace: source_workspace.clone(),
+                    build_in_destination: build_in_destination,
+                    destination_workspace: destination_workspace,
+                    start_dir: cond.raise((id.clone(),
+                                            format!("supplied path for package {} does not \
+                              exist, and couldn't interpret it as a URL fragment", id.to_str()))),
+                    id: id,
+                    libs: ~[],
+                    mains: ~[],
+                    tests: ~[],
+                    benchs: ~[]
+                }
+            }
+            Some(start_dir) => {
+                if !start_dir.is_dir() {
+                    cond.raise((id.clone(), format!("supplied path `{}` for package dir is a \
+                                non-directory", start_dir.display())));
+                }
+                debug!("For package id {}, returning {}", id.to_str(), start_dir.display());
+                PkgSrc {
+                    source_workspace: source_workspace.clone(),
+                    build_in_destination: build_in_destination,
+                    destination_workspace: destination_workspace,
+                    start_dir: start_dir,
+                    id: id,
+                    libs: ~[],
+                    mains: ~[],
+                    tests: ~[],
+                    benchs: ~[]
+                }
+            }
         }
     }
 
@@ -262,16 +339,21 @@ impl PkgSrc {
     /// if this was successful, None otherwise. Similarly, if the package id
     /// refers to a git repo on the local version, also check it out.
     /// (right now we only support git)
-    pub fn fetch_git(local: &Path, pkgid: &PkgId) -> Option<Path> {
+    /// If parent_dir.is_some(), then we treat the pkgid's path as relative to it
+    pub fn fetch_git(local: &Path, pkgid: &PkgId, parent_dir: &Option<Path>) -> Option<Path> {
         use conditions::git_checkout_failed::cond;
 
         let cwd = os::getcwd();
+        let pkgid_path = match parent_dir {
+            &Some(ref p) => p.clone(),
+            &None    => pkgid.path.clone()
+        };
         debug!("Checking whether {} (path = {}) exists locally. Cwd = {}, does it? {:?}",
-                pkgid.to_str(), pkgid.path.display(),
+                pkgid.to_str(), pkgid_path.display(),
                 cwd.display(),
-                pkgid.path.exists());
+                pkgid_path.exists());
 
-        match safe_git_clone(&pkgid.path, &pkgid.version, local) {
+        match safe_git_clone(&pkgid_path, &pkgid.version, local) {
             CheckedOutSources => {
                 make_read_only(local);
                 Some(local.clone())
diff --git a/src/librustpkg/path_util.rs b/src/librustpkg/path_util.rs
index 921005fdaab57..0ee562a869700 100644
--- a/src/librustpkg/path_util.rs
+++ b/src/librustpkg/path_util.rs
@@ -78,7 +78,7 @@ pub fn workspace_contains_package_id_(pkgid: &PkgId, workspace: &Path,
                 let pf = p.filename_str();
                 do pf.iter().any |&g| {
                     match split_version_general(g, '-') {
-                        None => false,
+                        None => g == pkgid.short_name,
                         Some((ref might_match, ref vers)) => {
                             *might_match == pkgid.short_name
                                 && (pkgid.version == *vers || pkgid.version == NoVersion)
diff --git a/src/librustpkg/source_control.rs b/src/librustpkg/source_control.rs
index 702a849e4addd..951fd6d4f8ff1 100644
--- a/src/librustpkg/source_control.rs
+++ b/src/librustpkg/source_control.rs
@@ -46,7 +46,7 @@ pub fn safe_git_clone(source: &Path, v: &Version, target: &Path) -> CloneResult
                     &ExactRevision(ref s) => {
                         let git_dir = target.join(".git");
                         debug!("`Running: git --work-tree={} --git-dir={} checkout {}",
-                                *s, target.display(), git_dir.display());
+                               target.display(), git_dir.display(), format!("{}", *s));
                         // FIXME (#9639: This needs to handle non-utf8 paths
                         let outp = run::process_output("git",
                             [format!("--work-tree={}", target.as_str().unwrap().to_owned()),
diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs
index bf62e7068f325..9833637f17007 100644
--- a/src/librustpkg/tests.rs
+++ b/src/librustpkg/tests.rs
@@ -15,8 +15,6 @@ use std::{os, run, str, task};
 use std::io;
 use std::io::fs;
 use std::io::File;
-use std::io::process;
-use std::io::process::ProcessExit;
 use extra::arc::Arc;
 use extra::arc::RWArc;
 use extra::tempfile::TempDir;
@@ -176,6 +174,27 @@ fn init_git_repo(p: &Path) -> TempDir {
     tmp
 }
 
+fn init_git_repo_with_src<'a>(p: &Path) -> TempDir {
+    assert!(!p.is_absolute());
+    let tmp = TempDir::new("git_local").expect("couldn't create temp dir");
+    let mut work_dir = tmp.path().clone();
+    work_dir.push("src");
+    let work_dir = work_dir.join(p);
+    let work_dir_for_opts = work_dir.clone();
+    fs::mkdir_recursive(&work_dir, io::UserRWX);
+    debug!("Running: git init in {}", work_dir.display());
+    run_git([~"init"], None, &work_dir_for_opts,
+        format!("Couldn't initialize git repository in {}", work_dir.display()));
+    // Add stuff to the dir so that git tag succeeds
+    writeFile(&work_dir.join("README"), "");
+    run_git([~"add", ~"README"],
+            None,
+            &work_dir_for_opts,
+            format!("Couldn't add in {}", work_dir.display()));
+    git_commit(&work_dir_for_opts, ~"whatever");
+    tmp
+}
+
 fn add_all_and_commit(repo: &Path) {
     git_add_all(repo);
     git_commit(repo, ~"floop");
@@ -1879,7 +1898,8 @@ fn pkgid_pointing_to_subdir() {
     // The actual repo is mockgithub.com/mozilla/some_repo
     // rustpkg should recognize that and treat the part after some_repo/ as a subdir
     let workspace = TempDir::new("parent_repo").expect("Couldn't create temp dir");
-    let workspace = workspace.path();
+    let workspace = workspace.unwrap();
+    let workspace = &workspace;
     fs::mkdir_recursive(&workspace.join_many(["src", "mockgithub.com",
                                                 "mozilla", "some_repo"]),
                           io::UserRWX);
@@ -2218,6 +2238,34 @@ fn test_installed_read_only() {
     assert!(is_read_only(&src2));
 }
 
+#[test]
+fn test_import_specific_version() {
+    let p_id = PkgId::new("foo");
+    let dep_id = git_repo_pkg();
+    let workspace = create_local_package(&p_id);
+    let workspace = workspace.unwrap();
+    let repo = init_git_repo(&dep_id.path);
+    let repo = repo.path();
+    let rust_path = Some(~[(~"RUST_PATH", repo.as_str().unwrap().to_owned())]);
+
+    let repo_subdir = repo.join(&dep_id.path);
+    fs::mkdir_recursive(&repo_subdir, io::UserRWX);
+    debug!("Writing files in: {}", repo_subdir.display());
+    writeFile(&repo_subdir.join("lib.rs"),
+              "pub fn f() { let _x = (); }");
+    add_git_tag(&repo_subdir, ~"1.0");
+    writeFile(&repo_subdir.join("lib.rs"),
+              "pub fn g(i: int) { assert_eq!(i, i) }");
+    add_git_tag(&repo_subdir, ~"2.0");
+
+    writeFile(&workspace.join_many([~"src", ~"foo-0.1", ~"main.rs"]),
+              "extern mod bar = \"mockgithub.com/catamorphism/test-pkg#1.0\"; \
+              use bar::f; fn main() { f(); }");
+
+    command_line_test_with_env([~"install", ~"foo"], &workspace, rust_path);
+    assert_executable_exists(&workspace, "foo");
+}
+
 #[test]
 fn test_installed_local_changes() {
     let temp_pkg_id = git_repo_pkg();
diff --git a/src/librustpkg/util.rs b/src/librustpkg/util.rs
index 6156cc0838ab7..1f18e2d479835 100644
--- a/src/librustpkg/util.rs
+++ b/src/librustpkg/util.rs
@@ -530,6 +530,7 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                                     self.parent_crate.as_str().unwrap().to_owned(),
                                     (~"binary", dep.as_str().unwrap().to_owned()));
 
+                            // FIXME (#9639): This needs to handle non-utf8 paths
                             // Also, add an additional search path
                             let dep_dir = dep.dir_path();
                             debug!("Installed {} into {}", dep.display(), dep_dir.display());
@@ -567,13 +568,14 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                                     lib_name, target_workspace.as_str().unwrap().to_owned());
                             (self.save)(target_workspace.clone());
                         }
+                        debug!("Installed {} into {}", lib_name, dest_workspace.display());
                     }
                 }
             }
             // Ignore `use`s
             _ => ()
         }
-        visit::walk_view_item(self, vi, env)
+        visit::walk_view_item(self, vi, env);
     }
 }
 
diff --git a/src/librustpkg/version.rs b/src/librustpkg/version.rs
index d40a104ccdad8..c3a9e98c642e1 100644
--- a/src/librustpkg/version.rs
+++ b/src/librustpkg/version.rs
@@ -185,7 +185,6 @@ enum ParseState {
 
 pub fn try_parsing_version(s: &str) -> Option<Version> {
     let s = s.trim();
-    debug!("Attempting to parse: {}", s);
     let mut parse_state = Start;
     for c in s.iter() {
         if char::is_digit(c) {

From ece2e9a74a8babd708c9bccefdad2f44ef4a5a7f Mon Sep 17 00:00:00 2001
From: Tim Chevalier <chevalier@alum.wellesley.edu>
Date: Fri, 15 Nov 2013 17:09:55 -0800
Subject: [PATCH 2/2] wip

---
 src/librustpkg/path_util.rs |  7 -------
 src/librustpkg/tests.rs     | 36 +++++++++++++++++++++++++++++++++---
 src/librustpkg/util.rs      | 14 ++++++--------
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/src/librustpkg/path_util.rs b/src/librustpkg/path_util.rs
index 0ee562a869700..72b38e4add221 100644
--- a/src/librustpkg/path_util.rs
+++ b/src/librustpkg/path_util.rs
@@ -214,20 +214,15 @@ pub fn system_library(sysroot: &Path, lib_name: &str) -> Option<Path> {
 }
 
 fn library_in(short_name: &str, version: &Version, dir_to_search: &Path) -> Option<Path> {
-    debug!("Listing directory {}", dir_to_search.display());
     let dir_contents = do io::ignore_io_error { fs::readdir(dir_to_search) };
-    debug!("dir has {:?} entries", dir_contents.len());
 
     let lib_prefix = format!("{}{}", os::consts::DLL_PREFIX, short_name);
     let lib_filetype = os::consts::DLL_EXTENSION;
 
-    debug!("lib_prefix = {} and lib_filetype = {}", lib_prefix, lib_filetype);
-
     // Find a filename that matches the pattern:
     // (lib_prefix)-hash-(version)(lib_suffix)
     let mut libraries = do dir_contents.iter().filter |p| {
         let extension = p.extension_str();
-        debug!("p = {}, p's extension is {:?}", p.display(), extension);
         match extension {
             None => false,
             Some(ref s) => lib_filetype == *s
@@ -248,12 +243,10 @@ fn library_in(short_name: &str, version: &Version, dir_to_search: &Path) -> Opti
             if f_name.is_empty() { break; }
             match f_name.rfind('-') {
                 Some(i) => {
-                    debug!("Maybe {} is a version", f_name.slice(i + 1, f_name.len()));
                     match try_parsing_version(f_name.slice(i + 1, f_name.len())) {
                        Some(ref found_vers) if version == found_vers => {
                            match f_name.slice(0, i).rfind('-') {
                                Some(j) => {
-                                   debug!("Maybe {} equals {}", f_name.slice(0, j), lib_prefix);
                                    if f_name.slice(0, j) == lib_prefix {
                                        result_filename = Some(p_path.clone());
                                    }
diff --git a/src/librustpkg/tests.rs b/src/librustpkg/tests.rs
index 9833637f17007..b295e3969a33b 100644
--- a/src/librustpkg/tests.rs
+++ b/src/librustpkg/tests.rs
@@ -2243,7 +2243,7 @@ fn test_import_specific_version() {
     let p_id = PkgId::new("foo");
     let dep_id = git_repo_pkg();
     let workspace = create_local_package(&p_id);
-    let workspace = workspace.unwrap();
+    let workspace = workspace.path();
     let repo = init_git_repo(&dep_id.path);
     let repo = repo.path();
     let rust_path = Some(~[(~"RUST_PATH", repo.as_str().unwrap().to_owned())]);
@@ -2262,8 +2262,38 @@ fn test_import_specific_version() {
               "extern mod bar = \"mockgithub.com/catamorphism/test-pkg#1.0\"; \
               use bar::f; fn main() { f(); }");
 
-    command_line_test_with_env([~"install", ~"foo"], &workspace, rust_path);
-    assert_executable_exists(&workspace, "foo");
+    command_line_test_with_env([~"install", ~"foo"], workspace, rust_path);
+    assert_executable_exists(workspace, "foo");
+}
+
+#[test]
+fn test_import_nonexistent_version() {
+    // Request a revision that isn't in the version control history. It
+    // should fail.
+    let p_id = PkgId::new("foo");
+    let dep_id = git_repo_pkg();
+    let workspace = create_local_package(&p_id);
+    let workspace = workspace.path();
+    let repo = init_git_repo(&dep_id.path);
+    let repo = repo.path();
+    let rust_path = Some(~[(~"RUST_PATH", repo.as_str().unwrap().to_owned())]);
+
+    let repo_subdir = repo.join(&dep_id.path);
+    fs::mkdir_recursive(&repo_subdir, io::UserRWX);
+    debug!("Writing files in: {}", repo_subdir.display());
+    writeFile(&repo_subdir.join("lib.rs"),
+              "pub fn f() { let _x = (); }");
+    add_git_tag(&repo_subdir, ~"1.0");
+
+    writeFile(&workspace.join_many([~"src", ~"foo-0.1", ~"main.rs"]),
+              "extern mod bar = \"mockgithub.com/catamorphism/test-pkg#monkeys\"; \
+              use bar::f; fn main() { f(); }");
+
+    match command_line_test_with_env([~"install", ~"foo"], workspace, rust_path) {
+        Fail(ref error) if error.status.matches_exit_status(70) => (), // ok
+        Fail(_)  => fail!("Wrong error"),
+        Success(*)   => fail!("Bad-revision test succeeded when it should fail")
+    }
 }
 
 #[test]
diff --git a/src/librustpkg/util.rs b/src/librustpkg/util.rs
index 1f18e2d479835..77e70b86e837b 100644
--- a/src/librustpkg/util.rs
+++ b/src/librustpkg/util.rs
@@ -210,9 +210,8 @@ pub fn compile_input(context: &BuildContext,
                           + context.flag_strs()
                           + cfgs.flat_map(|c| { ~[~"--cfg", (*c).clone()] }),
                           driver::optgroups()).unwrap();
-    debug!("rustc flags: {:?}", matches);
-
-    // Hack so that rustpkg can run either out of a rustc target dir,
+ 
+   // Hack so that rustpkg can run either out of a rustc target dir,
     // or the host dir
     let sysroot_to_use = @if !in_target(&context.sysroot()) {
         context.sysroot()
@@ -443,10 +442,9 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                 debug!("Finding and installing... {}", lib_name);
                 // Check standard Rust library path first
                 let whatever = system_library(&self.context.sysroot(), lib_name);
-                debug!("system library returned {:?}", whatever);
                 match whatever {
                     Some(ref installed_path) => {
-                        debug!("It exists: {}", installed_path.display());
+                        debug!("system_library returned: {}", installed_path.display());
                         // Say that [path for c] has a discovered dependency on
                         // installed_path
                         // For binary files, we only hash the datestamp, not the contents.
@@ -462,8 +460,8 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                     }
                     None => {
                         // FIXME #8711: need to parse version out of path_opt
-                        debug!("Trying to install library {}, rebuilding it",
-                               lib_name.to_str());
+                        debug!("Didn't find system library; trying to install
+                               library {}, rebuilding it", lib_name.to_str());
                         // Try to install it
                         let pkg_id = PkgId::new(lib_name);
                         // Find all the workspaces in the RUST_PATH that contain this package.
@@ -498,7 +496,7 @@ impl<'self> Visitor<()> for ViewItemVisitor<'self> {
                                  error(format!("Package {} depends on {}, but I don't know \
                                                how to find it",
                                                self.parent.path.display(),
-                                               pkg_id.path.display()));
+                                               pkg_id.to_str()));
                                  fail!()
                         }).inside {
                             PkgSrc::new(source_workspace.clone(),