]> git.proxmox.com Git - cargo.git/commitdiff
Change `Config::target_dir` to return Filesystem
authorAlex Crichton <alex@alexcrichton.com>
Sat, 2 Apr 2016 01:06:20 +0000 (18:06 -0700)
committerAlex Crichton <alex@alexcrichton.com>
Tue, 5 Apr 2016 22:44:56 +0000 (15:44 -0700)
This is a shared directory among multiple Cargo processes so access to it needs
to be properly synchronized. This commit changes the API of `Config::target_dir`
and then propagates the changes outward as necessary.

One fallout of this change is now we allow release/debug builds to proceed in
parallel.

src/cargo/ops/cargo_clean.rs
src/cargo/ops/cargo_doc.rs
src/cargo/ops/cargo_install.rs
src/cargo/ops/cargo_package.rs
src/cargo/ops/cargo_rustc/layout.rs
src/cargo/ops/cargo_rustc/mod.rs
src/cargo/ops/registry.rs
src/cargo/util/config.rs
src/cargo/util/flock.rs
src/crates-io/lib.rs
tests/test_cargo_concurrent.rs

index c4918f9c352d2df317b80ea9fd208c9a2851ba55..a8ba8f28a760a6fa70c633c1e68f69ba574f21ca 100644 (file)
@@ -21,17 +21,24 @@ pub fn clean(manifest_path: &Path, opts: &CleanOptions) -> CargoResult<()> {
 
     // If we have a spec, then we need to delete some packages, otherwise, just
     // remove the whole target directory and be done with it!
+    //
+    // Note that we don't bother grabbing a lock here as we're just going to
+    // blow it all away anyway.
     if opts.spec.is_empty() {
+        let target_dir = target_dir.into_path_unlocked();
         return rm_rf(&target_dir);
     }
 
     let (resolve, packages) = try!(ops::fetch(manifest_path, opts.config));
 
     let dest = if opts.release {"release"} else {"debug"};
-    let host_layout = Layout::new(opts.config, &root, None, dest);
-    let target_layout = opts.target.map(|target| {
-        Layout::new(opts.config, &root, Some(target), dest)
-    });
+    let host_layout = try!(Layout::new(opts.config, &root, None, dest));
+    let target_layout = match opts.target {
+        Some(target) => {
+            Some(try!(Layout::new(opts.config, &root, Some(target), dest)))
+        }
+        None => None,
+    };
 
     let cx = try!(Context::new(&resolve, &packages, opts.config,
                                host_layout, target_layout,
index a818ec80e1d22c6eb029b0ff386b4f53656f78d3..5b271567eee102ea01592a3d366d1a14a41dbaaa 100644 (file)
@@ -50,8 +50,12 @@ pub fn doc(manifest_path: &Path,
             }
         };
 
+        // Don't bother locking here as if this is getting deleted there's
+        // nothing we can do about it and otherwise if it's getting overwritten
+        // then that's also ok!
         let target_dir = options.compile_opts.config.target_dir(&package);
         let path = target_dir.join("doc").join(&name).join("index.html");
+        let path = path.into_path_unlocked();
         if fs::metadata(&path).is_ok() {
             let mut shell = options.compile_opts.config.shell();
             match open_docs(&path) {
index dfb018e6ff8e5e4d915248b056485ee13d9d8b0a..916e35bd7079a3a32719c4dc10497b6f257ce831 100644 (file)
@@ -82,9 +82,9 @@ pub fn install(root: Option<&str>,
     let target_dir = if source_id.is_path() {
         config.target_dir(&pkg)
     } else {
-        config.cwd().join("target-install")
+        Filesystem::new(config.cwd().join("target-install"))
     };
-    config.set_target_dir(&target_dir);
+    config.set_target_dir(target_dir.clone());
     let compile = try!(ops::compile_pkg(&pkg, Some(source), opts).chain_error(|| {
         human(format!("failed to compile `{}`, intermediate artifacts can be \
                        found at `{}`", pkg, target_dir.display()))
@@ -108,6 +108,9 @@ pub fn install(root: Option<&str>,
     }
 
     if !source_id.is_path() {
+        // Don't bother grabbing a lock as we're going to blow it all away
+        // anyway.
+        let target_dir = target_dir.into_path_unlocked();
         try!(fs::remove_dir_all(&target_dir));
     }
 
index 277b88eae1b84eadc4c8c71c6b4a2dceebeb568a..a4758e737a081c8cddceb654bd5e94e87fffa3cd 100644 (file)
@@ -1,6 +1,7 @@
-use std::io::prelude::*;
 use std::fs::{self, File};
-use std::path::{self, Path, PathBuf};
+use std::io::SeekFrom;
+use std::io::prelude::*;
+use std::path::{self, Path};
 
 use tar::{Archive, Builder, Header};
 use flate2::{GzBuilder, Compression};
@@ -8,14 +9,14 @@ use flate2::read::GzDecoder;
 
 use core::{SourceId, Package, PackageId};
 use sources::PathSource;
-use util::{self, CargoResult, human, internal, ChainError, Config};
+use util::{self, CargoResult, human, internal, ChainError, Config, FileLock};
 use ops;
 
 pub fn package(manifest_path: &Path,
                config: &Config,
                verify: bool,
                list: bool,
-               metadata: bool) -> CargoResult<Option<PathBuf>> {
+               metadata: bool) -> CargoResult<Option<FileLock>> {
     let path = manifest_path.parent().unwrap();
     let id = try!(SourceId::for_path(path));
     let mut src = PathSource::new(path, &id, config);
@@ -39,29 +40,37 @@ pub fn package(manifest_path: &Path,
 
     let filename = format!("{}-{}.crate", pkg.name(), pkg.version());
     let dir = config.target_dir(&pkg).join("package");
-    let dst = dir.join(&filename);
-    if fs::metadata(&dst).is_ok() {
-        return Ok(Some(dst))
-    }
+    let mut dst = match dir.open_ro(&filename, config, "packaged crate") {
+        Ok(f) => return Ok(Some(f)),
+        Err(..) => {
+            let tmp = format!(".{}", filename);
+            try!(dir.open_rw(&tmp, config, "package scratch space"))
+        }
+    };
 
     // Package up and test a temporary tarball and only move it to the final
     // location if it actually passes all our tests. Any previously existing
     // tarball can be assumed as corrupt or invalid, so we just blow it away if
     // it exists.
     try!(config.shell().status("Packaging", pkg.package_id().to_string()));
-    let tmp_dst = dir.join(format!(".{}", filename));
-    let _ = fs::remove_file(&tmp_dst);
-    try!(tar(&pkg, &src, config, &tmp_dst, &filename).chain_error(|| {
+    try!(dst.file().set_len(0));
+    try!(tar(&pkg, &src, config, dst.file(), &filename).chain_error(|| {
         human("failed to prepare local package for uploading")
     }));
     if verify {
-        try!(run_verify(config, &pkg, &tmp_dst).chain_error(|| {
+        try!(dst.seek(SeekFrom::Start(0)));
+        try!(run_verify(config, &pkg, dst.file()).chain_error(|| {
             human("failed to verify package tarball")
         }))
     }
-    try!(fs::rename(&tmp_dst, &dst).chain_error(|| {
-        human("failed to move temporary tarball into final location")
-    }));
+    try!(dst.seek(SeekFrom::Start(0)));
+    {
+        let src_path = dst.path();
+        let dst_path = dst.parent().join(&filename);
+        try!(fs::rename(&src_path, &dst_path).chain_error(|| {
+            human("failed to move temporary tarball into final location")
+        }));
+    }
     Ok(Some(dst))
 }
 
@@ -103,26 +112,17 @@ fn check_metadata(pkg: &Package, config: &Config) -> CargoResult<()> {
 fn tar(pkg: &Package,
        src: &PathSource,
        config: &Config,
-       dst: &Path,
+       dst: &File,
        filename: &str) -> CargoResult<()> {
-    if fs::metadata(&dst).is_ok() {
-        bail!("destination already exists: {}", dst.display())
-    }
-
-    try!(fs::create_dir_all(dst.parent().unwrap()));
-
-    let tmpfile = try!(File::create(dst));
-
     // Prepare the encoder and its header
     let filename = Path::new(filename);
     let encoder = GzBuilder::new().filename(try!(util::path2bytes(filename)))
-                                  .write(tmpfile, Compression::Best);
+                                  .write(dst, Compression::Best);
 
     // Put all package files into a compressed archive
     let mut ar = Builder::new(encoder);
     let root = pkg.root();
     for file in try!(src.list_files(pkg)).iter() {
-        if &**file == dst { continue }
         let relative = util::without_prefix(&file, &root).unwrap();
         try!(check_filename(relative));
         let relative = try!(relative.to_str().chain_error(|| {
@@ -173,11 +173,13 @@ fn tar(pkg: &Package,
     Ok(())
 }
 
-fn run_verify(config: &Config, pkg: &Package, tar: &Path)
+fn run_verify(config: &Config,
+              pkg: &Package,
+              tar: &File)
               -> CargoResult<()> {
     try!(config.shell().status("Verifying", pkg));
 
-    let f = try!(GzDecoder::new(try!(File::open(tar))));
+    let f = try!(GzDecoder::new(tar));
     let dst = pkg.root().join(&format!("target/package/{}-{}",
                                        pkg.name(), pkg.version()));
     if fs::metadata(&dst).is_ok() {
index 4add3a7586356e01bc063ed54c5d290c3196ed97..978090658fe05041a265db81211d7c66478f7c4d 100644 (file)
@@ -50,7 +50,7 @@ use std::io;
 use std::path::{PathBuf, Path};
 
 use core::{Package, Target};
-use util::Config;
+use util::{Config, FileLock, CargoResult, Filesystem};
 use util::hex::short_hash;
 
 pub struct Layout {
@@ -60,6 +60,7 @@ pub struct Layout {
     build: PathBuf,
     fingerprint: PathBuf,
     examples: PathBuf,
+    _lock: FileLock,
 }
 
 pub struct LayoutProxy<'a> {
@@ -68,8 +69,10 @@ pub struct LayoutProxy<'a> {
 }
 
 impl Layout {
-    pub fn new(config: &Config, pkg: &Package, triple: Option<&str>,
-               dest: &str) -> Layout {
+    pub fn new(config: &Config,
+               pkg: &Package,
+               triple: Option<&str>,
+               dest: &str) -> CargoResult<Layout> {
         let mut path = config.target_dir(pkg);
         // Flexible target specifications often point at filenames, so interpret
         // the target triple as a Path and then just use the file stem as the
@@ -78,18 +81,25 @@ impl Layout {
             path.push(Path::new(triple).file_stem().unwrap());
         }
         path.push(dest);
-        Layout::at(path)
+        Layout::at(config, path)
     }
 
-    pub fn at(root: PathBuf) -> Layout {
-        Layout {
+    pub fn at(config: &Config, root: Filesystem) -> CargoResult<Layout> {
+        // For now we don't do any more finer-grained locking on the artifact
+        // directory, so just lock the entire thing for the duration of this
+        // compile.
+        let lock = try!(root.open_rw(".cargo-lock", config, "build directory"));
+        let root = root.into_path_unlocked();
+
+        Ok(Layout {
             deps: root.join("deps"),
             native: root.join("native"),
             build: root.join("build"),
             fingerprint: root.join(".fingerprint"),
             examples: root.join("examples"),
             root: root,
-        }
+            _lock: lock,
+        })
     }
 
     pub fn prepare(&mut self) -> io::Result<()> {
index 80bff8c3ef19d4658d447b4a9b70ff0da590368b..59e649f55b089a90bcef02f705648225658e2b2e 100644 (file)
@@ -3,12 +3,12 @@ use std::env;
 use std::ffi::{OsStr, OsString};
 use std::fs;
 use std::io::prelude::*;
-use std::path::{self, PathBuf, Path};
+use std::path::{self, PathBuf};
 use std::sync::Arc;
 
 use core::{Package, PackageId, PackageSet, Target, Resolve};
 use core::{Profile, Profiles};
-use util::{self, CargoResult, human, Filesystem};
+use util::{self, CargoResult, human};
 use util::{Config, internal, ChainError, profile, join_paths};
 
 use self::job::{Job, Work};
@@ -80,17 +80,13 @@ pub fn compile_targets<'a, 'cfg: 'a>(pkg_targets: &'a PackagesToBuild<'a>,
 
     let dest = if build_config.release {"release"} else {"debug"};
     let root = try!(packages.get(resolve.root()));
-    let host_layout = Layout::new(config, root, None, &dest);
-    let target_layout = build_config.requested_target.as_ref().map(|target| {
-        layout::Layout::new(config, root, Some(&target), &dest)
-    });
-
-    // For now we don't do any more finer-grained locking on the artifact
-    // directory, so just lock the entire thing for the duration of this
-    // compile.
-    let fs = Filesystem::new(host_layout.root().to_path_buf());
-    let path = Path::new(".cargo-lock");
-    let _lock = try!(fs.open_rw(path, config, "build directory"));
+    let host_layout = try!(Layout::new(config, root, None, &dest));
+    let target_layout = match build_config.requested_target.as_ref() {
+        Some(target) => {
+            Some(try!(layout::Layout::new(config, root, Some(&target), &dest)))
+        }
+        None => None,
+    };
 
     let mut cx = try!(Context::new(resolve, packages, config,
                                    host_layout, target_layout,
index a02b80a6ba27c01f9787b09335feae7942e3d403..266e4accff0d47f62f638e315ac8caf163203254 100644 (file)
@@ -1,6 +1,6 @@
 use std::collections::HashMap;
 use std::env;
-use std::fs;
+use std::fs::{self, File};
 use std::io::prelude::*;
 use std::iter::repeat;
 use std::path::{Path, PathBuf};
@@ -51,7 +51,7 @@ pub fn publish(manifest_path: &Path,
 
     // Upload said tarball to the specified destination
     try!(config.shell().status("Uploading", pkg.package_id().to_string()));
-    try!(transmit(&pkg, &tarball, &mut registry));
+    try!(transmit(&pkg, tarball.file(), &mut registry));
 
     Ok(())
 }
@@ -74,7 +74,7 @@ fn verify_dependencies(pkg: &Package, registry_src: &SourceId)
     Ok(())
 }
 
-fn transmit(pkg: &Package, tarball: &Path, registry: &mut Registry)
+fn transmit(pkg: &Package, tarball: &File, registry: &mut Registry)
             -> CargoResult<()> {
     let deps = pkg.dependencies().iter().map(|dep| {
         NewCrateDependency {
index 66268be8792cd71b65158bb4883fd065b32e3694..05e53c40de4534a495760ef2636e4eb4301ededc 100644 (file)
@@ -30,7 +30,7 @@ pub struct Config {
     cwd: PathBuf,
     rustc: PathBuf,
     rustdoc: PathBuf,
-    target_dir: RefCell<Option<PathBuf>>,
+    target_dir: RefCell<Option<Filesystem>>,
 }
 
 impl Config {
@@ -110,14 +110,14 @@ impl Config {
 
     pub fn cwd(&self) -> &Path { &self.cwd }
 
-    pub fn target_dir(&self, pkg: &Package) -> PathBuf {
+    pub fn target_dir(&self, pkg: &Package) -> Filesystem {
         self.target_dir.borrow().clone().unwrap_or_else(|| {
-            pkg.root().join("target")
+            Filesystem::new(pkg.root().join("target"))
         })
     }
 
-    pub fn set_target_dir(&self, path: &Path) {
-        *self.target_dir.borrow_mut() = Some(path.to_owned());
+    pub fn set_target_dir(&self, path: Filesystem) {
+        *self.target_dir.borrow_mut() = Some(path);
     }
 
     fn get(&self, key: &str) -> CargoResult<Option<ConfigValue>> {
@@ -327,9 +327,9 @@ impl Config {
 
     fn scrape_target_dir_config(&mut self) -> CargoResult<()> {
         if let Some(dir) = env::var_os("CARGO_TARGET_DIR") {
-            *self.target_dir.borrow_mut() = Some(self.cwd.join(dir));
+            *self.target_dir.borrow_mut() = Some(Filesystem::new(self.cwd.join(dir)));
         } else if let Some(val) = try!(self.get_path("build.target-dir")) {
-            *self.target_dir.borrow_mut() = Some(val.val);
+            *self.target_dir.borrow_mut() = Some(Filesystem::new(val.val));
         }
         Ok(())
     }
index d40978dbc7cba8cff6a67aa5488a3b8fc69eb76a..98a27672368e71daa3399c96b70ef9a2009ebea1 100644 (file)
@@ -1,7 +1,7 @@
 use std::fs::{self, File, OpenOptions};
 use std::io::*;
 use std::io;
-use std::path::{Path, PathBuf};
+use std::path::{Path, PathBuf, Display};
 
 use term::color::CYAN;
 use fs2::{FileExt, lock_contended_error};
@@ -102,7 +102,7 @@ impl Drop for FileLock {
 /// The `Path` of a filesystem cannot be learned unless it's done in a locked
 /// fashion, and otherwise functions on this structure are prepared to handle
 /// concurrent invocations across multiple instances of Cargo.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct Filesystem {
     root: PathBuf,
 }
@@ -119,6 +119,11 @@ impl Filesystem {
         Filesystem::new(self.root.join(other))
     }
 
+    /// Like `Path::push`, pushes a new path component onto this filesystem.
+    pub fn push<T: AsRef<Path>>(&mut self, other: T) {
+        self.root.push(other);
+    }
+
     /// Consumes this filesystem and returns the underlying `PathBuf`.
     ///
     /// Note that this is a relatively dangerous operation and should be used
@@ -135,6 +140,12 @@ impl Filesystem {
         return create_dir_all(&self.root);
     }
 
+    /// Returns an adaptor that can be used to print the path of this
+    /// filesystem.
+    pub fn display(&self) -> Display {
+        self.root.display()
+    }
+
     /// Opens exclusive access to a file, returning the locked version of a
     /// file.
     ///
index fd98a772c6d8239c8d49c6068b7b4263b5acb338..10ab68a728f87c30a459fa39fc74afd7f87a1701 100644 (file)
@@ -4,10 +4,9 @@ extern crate rustc_serialize;
 
 use std::collections::HashMap;
 use std::fmt;
-use std::fs::{self, File};
+use std::fs::File;
 use std::io::prelude::*;
 use std::io::{self, Cursor};
-use std::path::Path;
 use std::result;
 
 use curl::http;
@@ -143,7 +142,7 @@ impl Registry {
         Ok(try!(json::decode::<Users>(&body)).users)
     }
 
-    pub fn publish(&mut self, krate: &NewCrate, tarball: &Path) -> Result<()> {
+    pub fn publish(&mut self, krate: &NewCrate, tarball: &File) -> Result<()> {
         let json = try!(json::encode(krate));
         // Prepare the body. The format of the upload request is:
         //
@@ -151,7 +150,7 @@ impl Registry {
         //      <json request> (metadata for the package)
         //      <le u32 of tarball>
         //      <source tarball>
-        let stat = try!(fs::metadata(tarball).map_err(Error::Io));
+        let stat = try!(tarball.metadata().map_err(Error::Io));
         let header = {
             let mut w = Vec::new();
             w.extend([
@@ -169,7 +168,6 @@ impl Registry {
             ].iter().map(|x| *x));
             w
         };
-        let tarball = try!(File::open(tarball).map_err(Error::Io));
         let size = stat.len() as usize + header.len();
         let mut body = Cursor::new(header).chain(tarball);
 
index 4e8c1a8dbe597fc6b47cd4aed30898b202293a44..99dc624c0fa3be3b805826d08e6e766e6469c8a6 100644 (file)
@@ -8,7 +8,7 @@ use std::thread;
 use git2;
 use hamcrest::{assert_that, existing_file};
 
-use support::{execs, project, ERROR};
+use support::{execs, project, ERROR, COMPILING};
 use support::git;
 use support::registry::Package;
 use test_cargo_install::{cargo_home, has_installed_exe};
@@ -350,3 +350,35 @@ test!(killing_cargo_releases_the_lock {
     assert!(!a.status.success());
     assert_that(b, execs().with_status(0));
 });
+
+test!(debug_release_ok {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [package]
+            name = "foo"
+            authors = []
+            version = "0.0.0"
+        "#)
+        .file("src/main.rs", "fn main() {}");
+    p.build();
+
+    assert_that(p.cargo("build"), execs().with_status(0));
+    fs::remove_dir_all(p.root().join("target")).unwrap();
+
+    let mut a = p.cargo("build").build_command();
+    let mut b = p.cargo("build").arg("--release").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_that(a, execs().with_status(0).with_stdout(&format!("\
+{compiling} foo v0.0.0 [..]
+", compiling = COMPILING)));
+    assert_that(b, execs().with_status(0).with_stdout(&format!("\
+{compiling} foo v0.0.0 [..]
+", compiling = COMPILING)));
+});