auto merge of #8831 : catamorphism/rust/extend_rust_path, r=catamorphism

r? @brson

@metajack requested the ability to violate the "only workspaces can be in the RUST_PATH" rule for the purpose of bootstrapping Servo without having to restructure all the directories. This patch gives rustpkg the ability to find sources if a directory in the RUST_PATH directly contains one of rustpkg's "special" files (lib.rs, main.rs, bench.rs, or test.rs), even if it's not a workspace. In this case, it puts the build artifacts in the first workspace in the RUST_PATH.

I'm not sure whether or not it's a good idea to keep this feature in rustpkg permanently. Thus, I added a flag, ```--use-rust-path-hack```, and only enabled it when the flag is set.

src/librustc/middle/kind.rs

@@ -129,7 +129,7 @@ fn check_impl_of_trait(cx: Context, it: @item, trait_ref: &trait_ref, self_type:
 
     // If this trait has builtin-kind supertraits, meet them.
     let self_ty: ty::t = ty::node_id_to_type(cx.tcx, it.id);
-    error!("checking impl with self type %?", ty::get(self_ty).sty);
+    debug!("checking impl with self type %?", ty::get(self_ty).sty);
     do check_builtin_bounds(cx, self_ty, trait_def.bounds) |missing| {
         cx.tcx.sess.span_err(self_type.span,
             fmt!("the type `%s', which does not fulfill `%s`, cannot implement this \

src/librustpkg/api.rs

@@ -20,7 +20,12 @@ use std::hashmap::*;
 /// Convenience functions intended for calling from pkg.rs
 
 fn default_ctxt(p: @Path) -> Ctx {
-    Ctx { sysroot_opt: Some(p), json: false, dep_cache: @mut HashMap::new() }
+    Ctx {
+        use_rust_path_hack: false,
+        sysroot_opt: Some(p),
+        json: false,
+        dep_cache: @mut HashMap::new()
+    }
 }
 
 pub fn build_lib(sysroot: @Path, root: Path, name: ~str, version: Version,

src/librustpkg/conditions.rs

@@ -36,3 +36,11 @@ condition! {
 condition! {
     no_rust_path: (~str) -> super::Path;
 }
+
+condition! {
+    not_a_workspace: (~str) -> super::Path;
+}
+
+condition! {
+    failed_to_create_temp_dir: (~str) -> super::Path;
+}

src/librustpkg/context.rs

@@ -15,6 +15,11 @@ use std::hashmap::HashMap;
 use std::os;
 
 pub struct Ctx {
+    // If use_rust_path_hack is true, rustpkg searches for sources
+    // in *package* directories that are in the RUST_PATH (for example,
+    // FOO/src/bar-0.1 instead of FOO). The flag doesn't affect where
+    // rustpkg stores build artifacts.
+    use_rust_path_hack: bool,
     // Sysroot -- if this is None, uses rustc filesearch's
     // idea of the default
     sysroot_opt: Option<@Path>,

src/librustpkg/package_source.rs

@@ -8,6 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+extern mod extra;
+
 use target::*;
 use package_id::PkgId;
 use std::path::Path;
@@ -16,8 +18,9 @@ use context::*;
 use crate::Crate;
 use messages::*;
 use source_control::{git_clone, git_clone_general};
-use path_util::pkgid_src_in_workspace;
+use path_util::{pkgid_src_in_workspace, find_dir_using_rust_path_hack, default_workspace};
 use util::compile_crate;
+use workspace::is_workspace;
 
 // An enumeration of the unpacked source of a package workspace.
 // This contains a list of files found in the source workspace.
@@ -48,7 +51,7 @@ impl PkgSrc {
     }
 
 
-    fn check_dir(&self) -> Path {
+    fn check_dir(&self, cx: &Ctx) -> Path {
         use conditions::nonexistent_package::cond;
 
         debug!("Pushing onto root: %s | %s", self.id.path.to_str(), self.root.to_str());
@@ -59,12 +62,21 @@ impl PkgSrc {
 
         let dir = match path {
             Some(d) => (*d).clone(),
-            None => match self.fetch_git() {
-                Some(d) => d,
-                None => cond.raise((self.id.clone(), ~"supplied path for package dir does not \
-                                      exist, and couldn't interpret it as a URL fragment"))
+            None => {
+                match self.fetch_git() {
+                    Some(d) => d,
+                    None => {
+                        match find_dir_using_rust_path_hack(cx, &self.id) {
+                            Some(d) => d,
+                            None => cond.raise((self.id.clone(),
+                               ~"supplied path for package dir does not \
+                                 exist, and couldn't interpret it as a URL fragment"))
+                        }
+                    }
+                }
             }
         };
+        debug!("For package id %s, returning %s", self.id.to_str(), dir.to_str());
         if !os::path_is_dir(&dir) {
             cond.raise((self.id.clone(), ~"supplied path for package dir is a \
                                         non-directory"));
@@ -79,11 +91,19 @@ impl PkgSrc {
     /// refers to a git repo on the local version, also check it out.
     /// (right now we only support git)
     pub fn fetch_git(&self) -> Option<Path> {
+        use conditions::failed_to_create_temp_dir::cond;
+
+        // We use a temporary directory because if the git clone fails,
+        // it creates the target directory anyway and doesn't delete it
+
+        let scratch_dir = extra::tempfile::mkdtemp(&os::tmpdir(), "rustpkg");
+        let clone_target = match scratch_dir {
+            Some(d) => d.push("rustpkg_temp"),
+            None    => cond.raise(~"Failed to create temporary directory for fetching git sources")
+        };
 
         let mut local = self.root.push("src");
         local = local.push(self.id.to_str());
-        // Git can't clone into a non-empty directory
-        os::remove_dir_recursive(&local);
 
         debug!("Checking whether %s exists locally. Cwd = %s, does it? %?",
                self.id.path.to_str(),
@@ -93,15 +113,28 @@ impl PkgSrc {
         if os::path_exists(&self.id.path) {
             debug!("%s exists locally! Cloning it into %s",
                    self.id.path.to_str(), local.to_str());
+            // Ok to use local here; we know it will succeed
             git_clone(&self.id.path, &local, &self.id.version);
             return Some(local);
         }
 
+        if (self.id.path.clone()).components().len() < 2 {
+            // If a non-URL, don't bother trying to fetch
+            return None;
+        }
+
         let url = fmt!("https://%s", self.id.path.to_str());
         note(fmt!("Fetching package: git clone %s %s [version=%s]",
-                  url, local.to_str(), self.id.version.to_str()));
-        if git_clone_general(url, &local, &self.id.version) {
-            Some(local)
+                  url, clone_target.to_str(), self.id.version.to_str()));
+
+        if git_clone_general(url, &clone_target, &self.id.version) {
+            // since the operation succeeded, move clone_target to local
+            if !os::rename_file(&clone_target, &local) {
+                 None
+            }
+            else {
+                 Some(local)
+            }
         }
         else {
             None
@@ -138,10 +171,10 @@ impl PkgSrc {
 
     /// Infers crates to build. Called only in the case where there
     /// is no custom build logic
-    pub fn find_crates(&mut self) {
+    pub fn find_crates(&mut self, cx: &Ctx) {
         use conditions::missing_pkg_files::cond;
 
-        let dir = self.check_dir();
+        let dir = self.check_dir(cx);
         debug!("Called check_dir, I'm in %s", dir.to_str());
         let prefix = dir.components.len();
         debug!("Matching against %?", self.id.short_name);
@@ -183,6 +216,7 @@ impl PkgSrc {
     fn build_crates(&self,
                     ctx: &Ctx,
                     src_dir: &Path,
+                    destination_dir: &Path,
                     crates: &[Crate],
                     cfgs: &[~str],
                     what: OutputType) {
@@ -194,8 +228,8 @@ impl PkgSrc {
             let result = compile_crate(ctx,
                                        &self.id,
                                        path,
-                                       // compile_crate wants the workspace
-                                       &self.root,
+                                       // compile_crate wants the destination workspace
+                                       destination_dir,
                                        crate.flags,
                                        crate.cfgs + cfgs,
                                        false,
@@ -209,15 +243,39 @@ impl PkgSrc {
         }
     }
 
-    pub fn build(&self, ctx: &Ctx, cfgs: ~[~str]) {
-        let dir = self.check_dir();
-        debug!("Building libs in %s", dir.to_str());
-        self.build_crates(ctx, &dir, self.libs, cfgs, Lib);
+    pub fn build(&self, ctx: &Ctx, cfgs: ~[~str]) -> Path {
+        use conditions::not_a_workspace::cond;
+
+        // Determine the destination workspace (which depends on whether
+        // we're using the rust_path_hack)
+        let destination_workspace = if is_workspace(&self.root) {
+            debug!("%s is indeed a workspace", self.root.to_str());
+            self.root.clone()
+        }
+        else {
+            // It would be nice to have only one place in the code that checks
+            // for the use_rust_path_hack flag...
+            if ctx.use_rust_path_hack {
+                let rs = default_workspace();
+                debug!("Using hack: %s", rs.to_str());
+                rs
+            }
+            else {
+                cond.raise(fmt!("Package root %s is not a workspace; pass in --rust_path_hack \
+                                if you want to treat it as a package source", self.root.to_str()))
+            }
+        };
+
+        let dir = self.check_dir(ctx);
+        debug!("Building libs in %s, destination = %s", dir.to_str(),
+            destination_workspace.to_str());
+        self.build_crates(ctx, &dir, &destination_workspace, self.libs, cfgs, Lib);
         debug!("Building mains");
-        self.build_crates(ctx, &dir, self.mains, cfgs, Main);
+        self.build_crates(ctx, &dir, &destination_workspace, self.mains, cfgs, Main);
         debug!("Building tests");
-        self.build_crates(ctx, &dir, self.tests, cfgs, Test);
+        self.build_crates(ctx, &dir, &destination_workspace, self.tests, cfgs, Test);
         debug!("Building benches");
-        self.build_crates(ctx, &dir, self.benchs, cfgs, Bench);
+        self.build_crates(ctx, &dir, &destination_workspace, self.benchs, cfgs, Bench);
+        destination_workspace
     }
 }

src/librustpkg/path_util.rs

@@ -14,6 +14,7 @@ pub use package_id::PkgId;
 pub use target::{OutputType, Main, Lib, Test, Bench, Target, Build, Install};
 pub use version::{Version, NoVersion, split_version_general, try_parsing_version};
 pub use rustc::metadata::filesearch::rust_path;
+use context::Ctx;
 
 use std::libc::consts::os::posix88::{S_IRUSR, S_IWUSR, S_IXUSR};
 use std::os::mkdir_recursive;
@@ -51,18 +52,23 @@ pub fn make_dir_rwx(p: &Path) -> bool { os::make_dir(p, U_RWX) }
 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());
+    workspace_contains_package_id_(pkgid, workspace, |p| { p.push("src") }).is_some()
+}
 
-    let src_dir = workspace.push("src");
+pub fn workspace_contains_package_id_(pkgid: &PkgId, workspace: &Path,
+// Returns the directory it was actually found in
+             workspace_to_src_dir: &fn(&Path) -> Path) -> Option<Path> {
+    let src_dir = workspace_to_src_dir(workspace);
 
-    let mut found = false;
+    let mut found = None;
     do os::walk_dir(&src_dir) |p| {
         debug!("=> p = %s", p.to_str());
 
-        let was_found = os::path_is_dir(p) && {
+        if os::path_is_dir(p) {
             debug!("p = %s, path = %s [%s]", p.to_str(), pkgid.path.to_str(),
                    src_dir.push_rel(&pkgid.path).to_str());
 
-            *p == src_dir.push_rel(&pkgid.path) || {
+            if *p == src_dir.push_rel(&pkgid.path) || {
                 let pf = p.filename();
                 do pf.iter().any |pf| {
                     let g = pf.to_str();
@@ -76,16 +82,15 @@ pub fn workspace_contains_package_id(pkgid: &PkgId, workspace: &Path) -> bool {
                         }
                     }
                 }
+            } {
+                found = Some(p.clone());
             }
-        };
 
-        if was_found {
-            found = true
-        }
+        };
         true
     };
 
-    debug!(if found { fmt!("Found %s in %s", pkgid.to_str(), workspace.to_str()) }
+    debug!(if found.is_some() { 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
 }
@@ -123,8 +128,7 @@ pub fn built_executable_in_workspace(pkgid: &PkgId, workspace: &Path) -> Option<
         Some(result)
     }
     else {
-        // This is not an error, but it's worth logging it
-        error!(fmt!("built_executable_in_workspace: %s does not exist", result.to_str()));
+        debug!("built_executable_in_workspace: %s does not exist", result.to_str());
         None
     }
 }
@@ -164,7 +168,7 @@ pub fn built_library_in_workspace(pkgid: &PkgId, workspace: &Path) -> Option<Pat
 
 /// Does the actual searching stuff
 pub fn installed_library_in_workspace(short_name: &str, workspace: &Path) -> Option<Path> {
-    // NOTE: this could break once we're handling multiple versions better... want a test for it
+    // This could break once we're handling multiple versions better -- I should add a test for it
     library_in_workspace(&Path(short_name), short_name, Install, workspace, "lib", &NoVersion)
 }
 
@@ -246,8 +250,8 @@ pub fn library_in_workspace(path: &Path, short_name: &str, where: Target,
     } // for
 
     if result_filename.is_none() {
-        warn(fmt!("library_in_workspace didn't find a library in %s for %s",
-                  dir_to_search.to_str(), short_name));
+        debug!("warning: library_in_workspace didn't find a library in %s for %s",
+                  dir_to_search.to_str(), short_name);
     }
 
     // Return the filename that matches, which we now know exists
@@ -392,3 +396,25 @@ pub fn uninstall_package_from(workspace: &Path, pkgid: &PkgId) {
     }
 
 }
+
+fn dir_has_file(dir: &Path, file: &str) -> bool {
+    assert!(dir.is_absolute());
+    os::path_exists(&dir.push(file))
+}
+
+pub fn find_dir_using_rust_path_hack(cx: &Ctx, p: &PkgId) -> Option<Path> {
+    if !cx.use_rust_path_hack {
+        return None;
+    }
+    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());
+        }
+        debug!("Didn't find id %s in dir %s", p.to_str(), dir.to_str())
+    }
+    None
+}