From b07456e4efe63bf77a95927d7fac40923dd5b3ef Mon Sep 17 00:00:00 2001 From: trevyn <230691+trevyn@users.noreply.github.com> Date: Wed, 30 Nov 2022 10:22:49 +0400 Subject: [PATCH 1/5] cargo clean: use `remove_dir_all` --- src/cargo/ops/cargo_clean.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index de3139c9947..948c556be7d 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -297,7 +297,8 @@ fn rm_rf(path: &Path, config: &Config, progress: &mut dyn CleaningProgressBar) - let entry = entry?; progress.on_clean()?; if entry.file_type().is_dir() { - paths::remove_dir(entry.path()).with_context(|| "could not remove build directory")?; + paths::remove_dir_all(entry.path()) + .with_context(|| "could not remove build directory")?; } else { paths::remove_file(entry.path()).with_context(|| "failed to remove build artifact")?; } From ddc49783ce299ef182f883cabd39b11f5849b374 Mon Sep 17 00:00:00 2001 From: trevyn <230691+trevyn@users.noreply.github.com> Date: Sun, 28 May 2023 17:57:49 +0400 Subject: [PATCH 2/5] Fall back to `fs::remove_dir_all` on error --- crates/cargo-util/src/paths.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 5e153de04cc..4c399d6a064 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -417,7 +417,10 @@ fn _create_dir_all(p: &Path) -> Result<()> { /// /// This does *not* follow symlinks. pub fn remove_dir_all>(p: P) -> Result<()> { - _remove_dir_all(p.as_ref()) + _remove_dir_all(p.as_ref()).or_else(|_| { + fs::remove_dir_all(p.as_ref()) + .with_context(|| format!("failed to remove directory `{}`", p.as_ref().display())) + }) } fn _remove_dir_all(p: &Path) -> Result<()> { From 0b8c12f5c66a7b533568dfd53f341c6b2f684461 Mon Sep 17 00:00:00 2001 From: trevyn <230691+trevyn@users.noreply.github.com> Date: Tue, 30 May 2023 07:51:07 +0400 Subject: [PATCH 3/5] Document reasoning and report previous error chain --- crates/cargo-util/src/paths.rs | 16 ++++++++++++---- src/cargo/ops/cargo_clean.rs | 1 + 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 4c399d6a064..5d7f31b6500 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -413,13 +413,21 @@ fn _create_dir_all(p: &Path) -> Result<()> { Ok(()) } -/// Recursively remove all files and directories at the given directory. +/// Equivalent to [`std::fs::remove_dir_all`] with better error messages. /// /// This does *not* follow symlinks. pub fn remove_dir_all>(p: P) -> Result<()> { - _remove_dir_all(p.as_ref()).or_else(|_| { - fs::remove_dir_all(p.as_ref()) - .with_context(|| format!("failed to remove directory `{}`", p.as_ref().display())) + _remove_dir_all(p.as_ref()).or_else(|prev_err| { + // `std::fs::remove_dir_all` is highly specialized for different platforms + // and may be more reliable than a simple walk. We try the walk first in + // order to report more detailed errors. + fs::remove_dir_all(p.as_ref()).with_context(|| { + format!( + "failed to remove directory `{}` \n\n---\nPrevious error: {:?}\n---", + p.as_ref().display(), + prev_err + ) + }) }) } diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 948c556be7d..1a9e3f1f031 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -297,6 +297,7 @@ fn rm_rf(path: &Path, config: &Config, progress: &mut dyn CleaningProgressBar) - let entry = entry?; progress.on_clean()?; if entry.file_type().is_dir() { + // `remove_dir_all` is used here due to https://github.com/rust-lang/cargo/issues/11441 paths::remove_dir_all(entry.path()) .with_context(|| "could not remove build directory")?; } else { From 5dfae689fa2b03e7d58de01cc05a378d61b1daa2 Mon Sep 17 00:00:00 2001 From: trevyn <230691+trevyn@users.noreply.github.com> Date: Tue, 30 May 2023 13:55:05 +0400 Subject: [PATCH 4/5] Improve error chain formatting --- crates/cargo-util/src/paths.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 5d7f31b6500..4a917821b7e 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -423,9 +423,9 @@ pub fn remove_dir_all>(p: P) -> Result<()> { // order to report more detailed errors. fs::remove_dir_all(p.as_ref()).with_context(|| { format!( - "failed to remove directory `{}` \n\n---\nPrevious error: {:?}\n---", + "{:?}\n\nError: failed to remove directory `{}`", + prev_err, p.as_ref().display(), - prev_err ) }) }) From ac3ccdd703ee037371d7ef410ac2642f0de9eedf Mon Sep 17 00:00:00 2001 From: trevyn <230691+trevyn@users.noreply.github.com> Date: Wed, 31 May 2023 10:11:36 +0400 Subject: [PATCH 5/5] Add stand-alone explanation --- src/cargo/ops/cargo_clean.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 1a9e3f1f031..b9b33690e45 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -297,7 +297,10 @@ fn rm_rf(path: &Path, config: &Config, progress: &mut dyn CleaningProgressBar) - let entry = entry?; progress.on_clean()?; if entry.file_type().is_dir() { - // `remove_dir_all` is used here due to https://github.com/rust-lang/cargo/issues/11441 + // The contents should have been removed by now, but sometimes a race condition is hit + // where other files have been added by the OS. `paths::remove_dir_all` also falls back + // to `std::fs::remove_dir_all`, which may be more reliable than a simple walk in + // platform-specific edge cases. paths::remove_dir_all(entry.path()) .with_context(|| "could not remove build directory")?; } else {