Auto merge of #2547 - alexcrichton:more-locking, r=brson

Add file locks in a few more locations

One was discovered by making `target_dir` return a `Filesystem`, the other was discovered by failing tests on the nightly OSX bots.

Author: bors

src/cargo/ops/cargo_clean.rs

@@ -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,

src/cargo/ops/cargo_doc.rs

@@ -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) {

src/cargo/ops/cargo_generate_lockfile.rs

@@ -23,7 +23,7 @@ pub fn generate_lockfile(manifest_path: &Path, config: &Config)
     let resolve = try!(ops::resolve_with_previous(&mut registry, &package,
                                                   Method::Everything,
                                                   None, None));
-    try!(ops::write_pkg_lockfile(&package, &resolve));
+    try!(ops::write_pkg_lockfile(&package, &resolve, config));
     Ok(())
 }
 
@@ -106,7 +106,7 @@ pub fn update_lockfile(manifest_path: &Path,
         }
     }
 
-    try!(ops::write_pkg_lockfile(&package, &resolve));
+    try!(ops::write_pkg_lockfile(&package, &resolve, opts.config));
     return Ok(());
 
     fn fill_with_deps<'a>(resolve: &'a Resolve, dep: &'a PackageId,

src/cargo/ops/cargo_install.rs

@@ -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));
     }
 

src/cargo/ops/cargo_package.rs

@@ -1,21 +1,22 @@
-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};
 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() {

src/cargo/ops/cargo_pkgid.rs

@@ -8,9 +8,7 @@ pub fn pkgid(manifest_path: &Path,
              spec: Option<&str>,
              config: &Config) -> CargoResult<PackageIdSpec> {
     let package = try!(Package::for_path(manifest_path, config));
-
-    let lockfile = package.root().join("Cargo.lock");
-    let resolve = match try!(ops::load_lockfile(&lockfile, &package, config)) {
+    let resolve = match try!(ops::load_pkg_lockfile(&package, config)) {
         Some(resolve) => resolve,
         None => bail!("a Cargo.lock must exist for this command"),
     };

src/cargo/ops/cargo_rustc/layout.rs

@@ -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<()> {

src/cargo/ops/cargo_rustc/mod.rs

@@ -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,