Auto merge of #7306 - alexcrichton:mkdir-error, r=ehuss

Improve error messages on mkdir failure

This commit ensures that `fs::create_dir*` isn't called throughout Cargo
and is instead routed through our own wrapper `paths::create_dir_all`
which brings with it a few benefits:

* Gracefully handles when the directory already exists (which is the
  behavior we always want anyway)
* Includes the path name in the error message of what failed
* Handles races of creating a directory by default

Closes #7304

src/cargo/core/compiler/custom_build.rs

@@ -1,6 +1,5 @@
 use std::collections::hash_map::{Entry, HashMap};
 use std::collections::{BTreeSet, HashSet};
-use std::fs;
 use std::path::{Path, PathBuf};
 use std::str;
 use std::sync::Arc;
@@ -262,8 +261,8 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
     let extra_verbose = bcx.config.extra_verbose();
     let (prev_output, prev_script_out_dir) = prev_build_output(cx, unit);
 
-    fs::create_dir_all(&script_dir)?;
-    fs::create_dir_all(&script_out_dir)?;
+    paths::create_dir_all(&script_dir)?;
+    paths::create_dir_all(&script_out_dir)?;
 
     // Prepare the unit of "dirty work" which will actually run the custom build
     // command.
@@ -275,14 +274,12 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
         //
         // If we have an old build directory, then just move it into place,
         // otherwise create it!
-        if fs::metadata(&script_out_dir).is_err() {
-            fs::create_dir(&script_out_dir).chain_err(|| {
-                internal(
-                    "failed to create script output directory for \
-                     build command",
-                )
-            })?;
-        }
+        paths::create_dir_all(&script_out_dir).chain_err(|| {
+            internal(
+                "failed to create script output directory for \
+                 build command",
+            )
+        })?;
 
         // For all our native lib dependencies, pick up their metadata to pass
         // along to this custom build command. We're also careful to augment our
@@ -588,7 +585,7 @@ fn prepare_metabuild<'a, 'cfg>(
     output.push("}\n".to_string());
     let output = output.join("");
     let path = unit.pkg.manifest().metabuild_path(cx.bcx.ws.target_dir());
-    fs::create_dir_all(path.parent().unwrap())?;
+    paths::create_dir_all(path.parent().unwrap())?;
     paths::write_if_changed(path, &output)?;
     Ok(())
 }

src/cargo/core/compiler/fingerprint.rs

@@ -189,7 +189,6 @@
 
 use std::collections::hash_map::{Entry, HashMap};
 use std::env;
-use std::fs;
 use std::hash::{self, Hasher};
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
@@ -1339,7 +1338,7 @@ pub fn prepare_init<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> Ca
 
     // Doc tests have no output, thus no fingerprint.
     if !new1.exists() && !unit.mode.is_doc_test() {
-        fs::create_dir(&new1)?;
+        paths::create_dir_all(&new1)?;
     }
 
     Ok(())

src/cargo/core/compiler/layout.rs

@@ -90,12 +90,10 @@
 //! When cross-compiling, the layout is the same, except it appears in
 //! `target/$TRIPLE`.
 
-use std::fs;
-use std::io;
-use std::path::{Path, PathBuf};
-
 use crate::core::Workspace;
+use crate::util::paths;
 use crate::util::{CargoResult, FileLock};
+use std::path::{Path, PathBuf};
 
 /// Contains the paths of all target output locations.
 ///
@@ -183,21 +181,14 @@ impl Layout {
     }
 
     /// Makes sure all directories stored in the Layout exist on the filesystem.
-    pub fn prepare(&mut self) -> io::Result<()> {
-        mkdir(&self.deps)?;
-        mkdir(&self.incremental)?;
-        mkdir(&self.fingerprint)?;
-        mkdir(&self.examples)?;
-        mkdir(&self.build)?;
+    pub fn prepare(&mut self) -> CargoResult<()> {
+        paths::create_dir_all(&self.deps)?;
+        paths::create_dir_all(&self.incremental)?;
+        paths::create_dir_all(&self.fingerprint)?;
+        paths::create_dir_all(&self.examples)?;
+        paths::create_dir_all(&self.build)?;
 
         return Ok(());
-
-        fn mkdir(dir: &Path) -> io::Result<()> {
-            if fs::metadata(&dir).is_err() {
-                fs::create_dir(dir)?;
-            }
-            Ok(())
-        }
     }
 
     /// Fetch the destination path for final artifacts  (`/…/target/debug`).

src/cargo/core/compiler/mod.rs

@@ -447,9 +447,7 @@ fn link_targets<'a, 'cfg>(
             hardlink_or_copy(src, dst)?;
             if let Some(ref path) = output.export_path {
                 let export_dir = export_dir.as_ref().unwrap();
-                if !export_dir.exists() {
-                    fs::create_dir_all(export_dir)?;
-                }
+                paths::create_dir_all(export_dir)?;
 
                 hardlink_or_copy(src, path)?;
             }
@@ -628,7 +626,7 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult
     // Create the documentation directory ahead of time as rustdoc currently has
     // a bug where concurrent invocations will race to create this directory if
     // it doesn't already exist.
-    fs::create_dir_all(&doc_dir)?;
+    paths::create_dir_all(&doc_dir)?;
 
     rustdoc.arg("-o").arg(doc_dir);
 

src/cargo/ops/cargo_install.rs

@@ -347,7 +347,7 @@ fn install_one(
         (Some(tracker), duplicates)
     };
 
-    fs::create_dir_all(&dst)?;
+    paths::create_dir_all(&dst)?;
 
     // Copy all binaries to a temporary directory under `dst` first, catching
     // some failure modes (e.g., out of space) before touching the existing

src/cargo/ops/cargo_new.rs

@@ -513,7 +513,7 @@ fn init_vcs(path: &Path, vcs: VersionControl, config: &Config) -> CargoResult<()
                 // Temporary fix to work around bug in libgit2 when creating a
                 // directory in the root of a posix filesystem.
                 // See: https://github.com/libgit2/libgit2/issues/5130
-                fs::create_dir_all(path)?;
+                paths::create_dir_all(path)?;
                 GitRepo::init(path, config.cwd())?;
             }
         }
@@ -533,7 +533,7 @@ fn init_vcs(path: &Path, vcs: VersionControl, config: &Config) -> CargoResult<()
             }
         }
         VersionControl::NoVcs => {
-            fs::create_dir_all(path)?;
+            paths::create_dir_all(path)?;
         }
     };
 
@@ -650,7 +650,7 @@ edition = {}
         let path_of_source_file = path.join(i.relative_path.clone());
 
         if let Some(src_dir) = path_of_source_file.parent() {
-            fs::create_dir_all(src_dir)?;
+            paths::create_dir_all(src_dir)?;
         }
 
         let default_file_content: &[u8] = if i.bin {

src/cargo/ops/vendor.rs

@@ -75,7 +75,7 @@ fn sync(
         .map(|p| &**p)
         .unwrap_or(opts.destination);
 
-    fs::create_dir_all(&canonical_destination)?;
+    paths::create_dir_all(&canonical_destination)?;
     let mut to_remove = HashSet::new();
     if !opts.no_delete {
         for entry in canonical_destination.read_dir()? {
@@ -322,7 +322,7 @@ fn cp_sources(
             .iter()
             .fold(dst.to_owned(), |acc, component| acc.join(&component));
 
-        fs::create_dir_all(dst.parent().unwrap())?;
+        paths::create_dir_all(dst.parent().unwrap())?;
 
         fs::copy(&p, &dst)
             .chain_err(|| format!("failed to copy `{}` to `{}`", p.display(), dst.display()))?;

src/cargo/sources/git/utils.rs

@@ -1,23 +1,21 @@
-use std::env;
-use std::fmt;
-use std::fs::{self, File};
-use std::mem;
-use std::path::{Path, PathBuf};
-use std::process::Command;
-
+use crate::core::GitReference;
+use crate::util::errors::{CargoResult, CargoResultExt};
+use crate::util::paths;
+use crate::util::process_builder::process;
+use crate::util::{internal, network, Config, IntoUrl, Progress};
 use curl::easy::{Easy, List};
 use git2::{self, ObjectType};
 use log::{debug, info};
 use serde::ser;
 use serde::Serialize;
+use std::env;
+use std::fmt;
+use std::fs::File;
+use std::mem;
+use std::path::{Path, PathBuf};
+use std::process::Command;
 use url::Url;
 
-use crate::core::GitReference;
-use crate::util::errors::{CargoResult, CargoResultExt};
-use crate::util::paths;
-use crate::util::process_builder::process;
-use crate::util::{internal, network, Config, IntoUrl, Progress};
-
 #[derive(PartialEq, Clone, Debug)]
 pub struct GitRevision(git2::Oid);
 
@@ -145,10 +143,10 @@ impl GitRemote {
     }
 
     fn clone_into(&self, dst: &Path, cargo_config: &Config) -> CargoResult<git2::Repository> {
-        if fs::metadata(&dst).is_ok() {
+        if dst.exists() {
             paths::remove_dir_all(dst)?;
         }
-        fs::create_dir_all(dst)?;
+        paths::create_dir_all(dst)?;
         let mut repo = init(dst, true)?;
         fetch(
             &mut repo,
@@ -257,8 +255,7 @@ impl<'a> GitCheckout<'a> {
         config: &Config,
     ) -> CargoResult<GitCheckout<'a>> {
         let dirname = into.parent().unwrap();
-        fs::create_dir_all(&dirname)
-            .chain_err(|| format!("Couldn't mkdir {}", dirname.display()))?;
+        paths::create_dir_all(&dirname)?;
         if into.exists() {
             paths::remove_dir_all(into)?;
         }

src/cargo/sources/registry/index.rs

@@ -66,18 +66,17 @@
 //! details like invalidating caches and whatnot which are handled below, but
 //! hopefully those are more obvious inline in the code itself.
 
-use std::collections::{HashMap, HashSet};
-use std::fs;
-use std::path::Path;
-use std::str;
-
-use log::info;
-use semver::{Version, VersionReq};
-
 use crate::core::dependency::Dependency;
 use crate::core::{InternedString, PackageId, SourceId, Summary};
 use crate::sources::registry::{RegistryData, RegistryPackage};
+use crate::util::paths;
 use crate::util::{internal, CargoResult, Config, Filesystem, ToSemver};
+use log::info;
+use semver::{Version, VersionReq};
+use std::collections::{HashMap, HashSet};
+use std::fs;
+use std::path::Path;
+use std::str;
 
 /// Crates.io treats hyphen and underscores as interchangeable, but the index and old Cargo do not.
 /// Therefore, the index must store uncanonicalized version of the name so old Cargo's can find it.
@@ -559,7 +558,7 @@ impl Summaries {
         // This is opportunistic so we ignore failure here but are sure to log
         // something in case of error.
         if let Some(cache_bytes) = cache_bytes {
-            if fs::create_dir_all(cache_path.parent().unwrap()).is_ok() {
+            if paths::create_dir_all(cache_path.parent().unwrap()).is_ok() {
                 let path = Filesystem::new(cache_path.clone());
                 config.assert_package_cache_locked(&path);
                 if let Err(e) = fs::write(cache_path, cache_bytes) {

src/cargo/sources/registry/remote.rs

@@ -3,12 +3,13 @@ use crate::sources::git;
 use crate::sources::registry::MaybeLock;
 use crate::sources::registry::{RegistryConfig, RegistryData, CRATE_TEMPLATE, VERSION_TEMPLATE};
 use crate::util::errors::{CargoResult, CargoResultExt};
+use crate::util::paths;
 use crate::util::{Config, Filesystem, Sha256};
 use lazycell::LazyCell;
 use log::{debug, trace};
 use std::cell::{Cell, Ref, RefCell};
 use std::fmt::Write as FmtWrite;
-use std::fs::{self, File, OpenOptions};
+use std::fs::{File, OpenOptions};
 use std::io::prelude::*;
 use std::io::SeekFrom;
 use std::mem;
@@ -55,8 +56,8 @@ impl<'cfg> RemoteRegistry<'cfg> {
             match git2::Repository::open(&path) {
                 Ok(repo) => Ok(repo),
                 Err(_) => {
-                    drop(fs::remove_dir_all(&path));
-                    fs::create_dir_all(&path)?;
+                    drop(paths::remove_dir_all(&path));
+                    paths::create_dir_all(&path)?;
 
                     // Note that we'd actually prefer to use a bare repository
                     // here as we're not actually going to check anything out.

src/cargo/util/flock.rs

@@ -1,4 +1,4 @@
-use std::fs::{self, File, OpenOptions};
+use std::fs::{File, OpenOptions};
 use std::io;
 use std::io::{Read, Seek, SeekFrom, Write};
 use std::path::{Display, Path, PathBuf};
@@ -149,8 +149,9 @@ impl Filesystem {
     ///
     /// Handles errors where other Cargo processes are also attempting to
     /// concurrently create this directory.
-    pub fn create_dir(&self) -> io::Result<()> {
-        fs::create_dir_all(&self.root)
+    pub fn create_dir(&self) -> CargoResult<()> {
+        paths::create_dir_all(&self.root)?;
+        Ok(())
     }
 
     /// Returns an adaptor that can be used to print the path of this
@@ -221,10 +222,10 @@ impl Filesystem {
             .open(&path)
             .or_else(|e| {
                 if e.kind() == io::ErrorKind::NotFound && state == State::Exclusive {
-                    fs::create_dir_all(path.parent().unwrap())?;
-                    opts.open(&path)
+                    paths::create_dir_all(path.parent().unwrap())?;
+                    Ok(opts.open(&path)?)
                 } else {
-                    Err(e)
+                    Err(failure::Error::from(e))
                 }
             })
             .chain_err(|| format!("failed to open: {}", path.display()))?;

src/cargo/util/paths.rs

@@ -269,6 +269,15 @@ impl<'a> Iterator for PathAncestors<'a> {
     }
 }
 
+pub fn create_dir_all(p: impl AsRef<Path>) -> CargoResult<()> {
+    _create_dir_all(p.as_ref())
+}
+
+fn _create_dir_all(p: &Path) -> CargoResult<()> {
+    fs::create_dir_all(p).chain_err(|| format!("failed to create directory `{}`", p.display()))?;
+    Ok(())
+}
+
 pub fn remove_dir_all<P: AsRef<Path>>(p: P) -> CargoResult<()> {
     _remove_dir_all(p.as_ref())
 }

src/cargo/util/vcs.rs

@@ -1,9 +1,7 @@
-use std::fs::create_dir;
-use std::path::Path;
-
-use git2;
-
+use crate::util::paths;
 use crate::util::{process, CargoResult};
+use git2;
+use std::path::Path;
 
 // Check if we are in an existing repo. We define that to be true if either:
 //
@@ -68,7 +66,7 @@ impl PijulRepo {
 impl FossilRepo {
     pub fn init(path: &Path, cwd: &Path) -> CargoResult<FossilRepo> {
         // fossil doesn't create the directory so we'll do that first
-        create_dir(path)?;
+        paths::create_dir_all(path)?;
 
         // set up the paths we'll use
         let db_fname = ".fossil";

tests/testsuite/out_dir.rs

@@ -179,7 +179,7 @@ fn out_dir_is_a_file() {
     p.cargo("build -Z unstable-options --out-dir out")
         .masquerade_as_nightly_cargo()
         .with_status(101)
-        .with_stderr_contains("[ERROR] failed to link or copy [..]")
+        .with_stderr_contains("[ERROR] failed to create directory [..]")
         .run();
 }