Auto merge of #2610 - japaric:install-from-tempdir, r=alexcrichton

bors · bors · commit 191f65f5fad6 · 2016-04-23T22:15:19.000-07:00
cargo-install: prefer building artifacts in the system temporary directory and each cargo-install instance creates and uses its own build directory. This allows running several cargo-install instances in parallel. If we fail to create a temporary directory for whatever reason fallback to creating and using a target-install directory in the current directory. closes #2606 --- r? @alexcrichton Qs: - Should we preserve the current behavior (`target-install` in `cwd`) as a fallback or remove it and error if we can't create a `TempDir` in `env::temp_dir()`? (we currently error if we can't create `target-install` directory in `cwd`) - Should I add tests for the issues I raised at #2606? If yes, how can I test `cargo-install` parallelism? Lack of "Blocking waiting for file lock on build directory" in the output of the `cargo` commands? or something else?
diff --git a/Cargo.toml b/Cargo.toml
@@ -38,14 +38,14 @@ regex = "0.1"
 rustc-serialize = "0.3"
 semver = "0.2.2"
 tar = "0.4"
+tempdir = "0.3"
 term = "0.4.4"
 time = "0.1"
 toml = "0.1"
 url = "0.2"
 winapi = "0.2"
 
 [dev-dependencies]
-tempdir = "0.3"
 hamcrest = "0.1"
 bufstream = "0.1"
 filetime = "0.1"
diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs
@@ -19,6 +19,7 @@ extern crate regex;
 extern crate rustc_serialize;
 extern crate semver;
 extern crate tar;
+extern crate tempdir;
 extern crate term;
 extern crate time;
 extern crate toml;
diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs
@@ -7,6 +7,7 @@ use std::io::prelude::*;
 use std::io::SeekFrom;
 use std::path::{Path, PathBuf};
 
+use tempdir::TempDir;
 use toml;
 
 use core::{SourceId, Source, Package, Registry, Dependency, PackageIdSpec};
@@ -79,13 +80,25 @@ pub fn install(root: Option<&str>,
         try!(check_overwrites(&dst, &pkg, &opts.filter, &list));
     }
 
+    let mut td_opt = None;
     let target_dir = if source_id.is_path() {
         config.target_dir(&pkg)
     } else {
-        Filesystem::new(config.cwd().join("target-install"))
+        if let Ok(td) = TempDir::new("cargo-install") {
+            let p = td.path().to_owned();
+            td_opt = Some(td);
+            Filesystem::new(p)
+        } else {
+            Filesystem::new(config.cwd().join("target-install"))
+        }
     };
     config.set_target_dir(target_dir.clone());
     let compile = try!(ops::compile_pkg(&pkg, Some(source), opts).chain_error(|| {
+        if let Some(td) = td_opt.take() {
+            // preserve the temporary directory, so the user can inspect it
+            td.into_path();
+        }
+
         human(format!("failed to compile `{}`, intermediate artifacts can be \
                        found at `{}`", pkg, target_dir.display()))
     }));
diff --git a/tests/test_cargo_concurrent.rs b/tests/test_cargo_concurrent.rs
@@ -1,4 +1,4 @@
-use std::env;
+use std::{env, str};
 use std::fs::{self, File};
 use std::io::Write;
 use std::net::TcpListener;
@@ -15,6 +15,12 @@ use test_cargo_install::{cargo_home, has_installed_exe};
 
 fn setup() {}
 
+fn pkg(name: &str, vers: &str) {
+    Package::new(name, vers)
+        .file("src/main.rs", "fn main() {{}}")
+        .publish();
+}
+
 test!(multiple_installs {
     let p = project("foo")
         .file("a/Cargo.toml", r#"
@@ -52,6 +58,34 @@ test!(multiple_installs {
     assert_that(cargo_home(), has_installed_exe("bar"));
 });
 
+test!(concurrent_installs {
+    const LOCKED_BUILD: &'static str = "waiting for file lock on build directory";
+
+    pkg("foo", "0.0.1");
+    pkg("bar", "0.0.1");
+
+    let mut a = ::cargo_process().arg("install").arg("foo").build_command();
+    let mut b = ::cargo_process().arg("install").arg("bar").build_command();
+
+    a.stdout(Stdio::piped()).stderr(Stdio::piped());
+    b.stdout(Stdio::piped()).stderr(Stdio::piped());
+
+    let a = a.spawn().unwrap();
+    let b = b.spawn().unwrap();
+    let a = thread::spawn(move || a.wait_with_output().unwrap());
+    let b = b.wait_with_output().unwrap();
+    let a = a.join().unwrap();
+
+    assert!(!str::from_utf8(&a.stderr).unwrap().contains(LOCKED_BUILD));
+    assert!(!str::from_utf8(&b.stderr).unwrap().contains(LOCKED_BUILD));
+
+    assert_that(a, execs().with_status(0));
+    assert_that(b, execs().with_status(0));
+
+    assert_that(cargo_home(), has_installed_exe("foo"));
+    assert_that(cargo_home(), has_installed_exe("bar"));
+});
+
 test!(one_install_should_be_bad {
     let p = project("foo")
         .file("a/Cargo.toml", r#"
diff --git a/tests/test_cargo_install.rs b/tests/test_cargo_install.rs
@@ -659,3 +659,18 @@ test!(q_silences_warnings {
     assert_that(cargo_process("install").arg("-q").arg("--path").arg(p.root()),
                 execs().with_status(0).with_stderr(""));
 });
+
+test!(readonly_dir {
+    pkg("foo", "0.0.1");
+
+    let root = paths::root();
+    let dir = &root.join("readonly");
+    fs::create_dir(root.join("readonly")).unwrap();
+    let mut perms = fs::metadata(dir).unwrap().permissions();
+    perms.set_readonly(true);
+    fs::set_permissions(dir, perms).unwrap();
+
+    assert_that(cargo_process("install").arg("foo").cwd(dir),
+                execs().with_status(0));
+    assert_that(cargo_home(), has_installed_exe("foo"));
+});
