Auto merge of #129413 - jieyouxu:revert-remove-dir-all, r=compiler-errors

Revert #129187 and #129302

The two PRs naively switched to `std::fs::remove_dir_all`, but failed to gracefully handle the failure case where the top-level directory entry does not exist, causing #129187 (comment) `./x clean` to fail locally when `tmp` does not exist.

I plan to reland the two PRs with fixed top-level dir entry handling and more testing, but let's quickly revert to unblock people.

Reverts #129187.
Reverts #129302.

r? bootstrap

Author: bors

src/bootstrap/src/core/build_steps/clean.rs

@@ -6,6 +6,7 @@
 //! directory unless the `--all` flag is present.
 
 use std::fs;
+use std::io::{self, ErrorKind};
 use std::path::Path;
 
 use crate::core::builder::{crate_description, Builder, RunConfig, ShouldRun, Step};
@@ -100,11 +101,11 @@ fn clean(build: &Build, all: bool, stage: Option<u32>) {
         return;
     }
 
-    remove_dir_recursive("tmp");
+    rm_rf("tmp".as_ref());
 
     // Clean the entire build directory
     if all {
-        remove_dir_recursive(&build.out);
+        rm_rf(&build.out);
         return;
     }
 
@@ -135,17 +136,17 @@ fn clean_specific_stage(build: &Build, stage: u32) {
             }
 
             let path = t!(entry.path().canonicalize());
-            remove_dir_recursive(&path);
+            rm_rf(&path);
         }
     }
 }
 
 fn clean_default(build: &Build) {
-    remove_dir_recursive(build.out.join("tmp"));
-    remove_dir_recursive(build.out.join("dist"));
-    remove_dir_recursive(build.out.join("bootstrap").join(".last-warned-change-id"));
-    remove_dir_recursive(build.out.join("bootstrap-shims-dump"));
-    remove_dir_recursive(build.out.join("rustfmt.stamp"));
+    rm_rf(&build.out.join("tmp"));
+    rm_rf(&build.out.join("dist"));
+    rm_rf(&build.out.join("bootstrap").join(".last-warned-change-id"));
+    rm_rf(&build.out.join("bootstrap-shims-dump"));
+    rm_rf(&build.out.join("rustfmt.stamp"));
 
     let mut hosts: Vec<_> = build.hosts.iter().map(|t| build.out.join(t)).collect();
     // After cross-compilation, artifacts of the host architecture (which may differ from build.host)
@@ -165,16 +166,78 @@ fn clean_default(build: &Build) {
                 continue;
             }
             let path = t!(entry.path().canonicalize());
-            remove_dir_recursive(&path);
+            rm_rf(&path);
         }
     }
 }
 
-/// Wrapper for [`std::fs::remove_dir_all`] that panics on failure and prints the `path` we failed
-/// on.
-fn remove_dir_recursive<P: AsRef<Path>>(path: P) {
-    let path = path.as_ref();
-    if let Err(e) = fs::remove_dir_all(path) {
-        panic!("failed to `remove_dir_all` at `{}`: {e}", path.display());
+fn rm_rf(path: &Path) {
+    match path.symlink_metadata() {
+        Err(e) => {
+            if e.kind() == ErrorKind::NotFound {
+                return;
+            }
+            panic!("failed to get metadata for file {}: {}", path.display(), e);
+        }
+        Ok(metadata) => {
+            if metadata.file_type().is_file() || metadata.file_type().is_symlink() {
+                do_op(path, "remove file", |p| match fs::remove_file(p) {
+                    #[cfg(windows)]
+                    Err(e)
+                        if e.kind() == std::io::ErrorKind::PermissionDenied
+                            && p.file_name().and_then(std::ffi::OsStr::to_str)
+                                == Some("bootstrap.exe") =>
+                    {
+                        eprintln!("WARNING: failed to delete '{}'.", p.display());
+                        Ok(())
+                    }
+                    r => r,
+                });
+
+                return;
+            }
+
+            for file in t!(fs::read_dir(path)) {
+                rm_rf(&t!(file).path());
+            }
+
+            do_op(path, "remove dir", |p| match fs::remove_dir(p) {
+                // Check for dir not empty on Windows
+                // FIXME: Once `ErrorKind::DirectoryNotEmpty` is stabilized,
+                // match on `e.kind()` instead.
+                #[cfg(windows)]
+                Err(e) if e.raw_os_error() == Some(145) => Ok(()),
+                r => r,
+            });
+        }
+    };
+}
+
+fn do_op<F>(path: &Path, desc: &str, mut f: F)
+where
+    F: FnMut(&Path) -> io::Result<()>,
+{
+    match f(path) {
+        Ok(()) => {}
+        // On windows we can't remove a readonly file, and git will often clone files as readonly.
+        // As a result, we have some special logic to remove readonly files on windows.
+        // This is also the reason that we can't use things like fs::remove_dir_all().
+        #[cfg(windows)]
+        Err(ref e) if e.kind() == ErrorKind::PermissionDenied => {
+            let m = t!(path.symlink_metadata());
+            let mut p = m.permissions();
+            p.set_readonly(false);
+            t!(fs::set_permissions(path, p));
+            f(path).unwrap_or_else(|e| {
+                // Delete symlinked directories on Windows
+                if m.file_type().is_symlink() && path.is_dir() && fs::remove_dir(path).is_ok() {
+                    return;
+                }
+                panic!("failed to {} {}: {}", desc, path.display(), e);
+            });
+        }
+        Err(e) => {
+            panic!("failed to {} {}: {}", desc, path.display(), e);
+        }
     }
 }

src/tools/compiletest/src/runtest.rs

@@ -3265,7 +3265,7 @@ impl<'test> TestCx<'test> {
 
         let tmpdir = cwd.join(self.output_base_name());
         if tmpdir.exists() {
-            fs::remove_dir_all(&tmpdir).unwrap();
+            self.aggressive_rm_rf(&tmpdir).unwrap();
         }
         create_dir_all(&tmpdir).unwrap();
 
@@ -3404,6 +3404,29 @@ impl<'test> TestCx<'test> {
         }
     }
 
+    fn aggressive_rm_rf(&self, path: &Path) -> io::Result<()> {
+        for e in path.read_dir()? {
+            let entry = e?;
+            let path = entry.path();
+            if entry.file_type()?.is_dir() {
+                self.aggressive_rm_rf(&path)?;
+            } else {
+                // Remove readonly files as well on windows (by default we can't)
+                fs::remove_file(&path).or_else(|e| {
+                    if cfg!(windows) && e.kind() == io::ErrorKind::PermissionDenied {
+                        let mut meta = entry.metadata()?.permissions();
+                        meta.set_readonly(false);
+                        fs::set_permissions(&path, meta)?;
+                        fs::remove_file(&path)
+                    } else {
+                        Err(e)
+                    }
+                })?;
+            }
+        }
+        fs::remove_dir(path)
+    }
+
     fn run_rmake_v2_test(&self) {
         // For `run-make` V2, we need to perform 2 steps to build and run a `run-make` V2 recipe
         // (`rmake.rs`) to run the actual tests. The support library is already built as a tool rust
@@ -3452,7 +3475,7 @@ impl<'test> TestCx<'test> {
         // This setup intentionally diverges from legacy Makefile run-make tests.
         let base_dir = self.output_base_name();
         if base_dir.exists() {
-            fs::remove_dir_all(&base_dir).unwrap();
+            self.aggressive_rm_rf(&base_dir).unwrap();
         }
         let rmake_out_dir = base_dir.join("rmake_out");
         create_dir_all(&rmake_out_dir).unwrap();