Rollup merge of #134659 - jieyouxu:recursive-remove, r=ChrisDenton

test-infra: improve compiletest and run-make-support symlink handling

I was trying to implement #134656 to port `tests/run-make/incr-add-rust-src-component.rs`, but found some blockers related to symlink handling, so in this PR I tried to resolve them by improving symlink handling in compiletest and run-make-support (particularly for native windows msvc environment).

Key changes:

- I needed to copy symlinks (duplicate a symlink pointing to the same file), so I pulled out the copy symlink logic and re-exposed it as `run_make_support::rfs::copy_symlink`. This helper correctly accounts for the Windows symlink-to-file vs symlink-to-dir distinction (hereafter "Windows symlinks") when copying symlinks.
- `recursive_remove`:
    - I needed a way to remove symlinks themselves (no symlink traversal). `std::fs::remove_dir_all` handles them, but only if the root path is a directory. So I wrapped `std::fs::remove_dir_all` to also handle when the root path is a non-directory entity (e.g. file or symlink). Again, this properly accounts for Windows symlinks.
    - I wanted to use this for both compiletest and run-make-support, so I put the implementation and accompanying tests in `build_helper`.
    - In this sense, it's a reland of #129302 with proper test coverage.
    - It's a thin wrapper around `std::fs::remove_dir_all` (`remove_dir_all` correctly handles read-only entries on Windows). The helper has additional permission-setting logic for when the root path is a non-dir entry on Windows to handle read-only non-dir entry.

Fixes #126334.

src/build_helper/src/fs/mod.rs

@@ -0,0 +1,69 @@
+//! Misc filesystem related helpers for use by bootstrap and tools.
+use std::fs::Metadata;
+use std::path::Path;
+use std::{fs, io};
+
+#[cfg(test)]
+mod tests;
+
+/// Helper to ignore [`std::io::ErrorKind::NotFound`], but still propagate other
+/// [`std::io::ErrorKind`]s.
+pub fn ignore_not_found<Op>(mut op: Op) -> io::Result<()>
+where
+    Op: FnMut() -> io::Result<()>,
+{
+    match op() {
+        Ok(()) => Ok(()),
+        Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(()),
+        Err(e) => Err(e),
+    }
+}
+
+/// A wrapper around [`std::fs::remove_dir_all`] that can also be used on *non-directory entries*,
+/// including files and symbolic links.
+///
+/// - This will produce an error if the target path is not found.
+/// - Like [`std::fs::remove_dir_all`], this helper does not traverse symbolic links, will remove
+///   symbolic link itself.
+/// - This helper is **not** robust against races on the underlying filesystem, behavior is
+///   unspecified if this helper is called concurrently.
+/// - This helper is not robust against TOCTOU problems.
+///
+/// FIXME: this implementation is insufficiently robust to replace bootstrap's clean `rm_rf`
+/// implementation:
+///
+/// - This implementation currently does not perform retries.
+#[track_caller]
+pub fn recursive_remove<P: AsRef<Path>>(path: P) -> io::Result<()> {
+    let path = path.as_ref();
+    let metadata = fs::symlink_metadata(path)?;
+    #[cfg(windows)]
+    let is_dir_like = |meta: &fs::Metadata| {
+        use std::os::windows::fs::FileTypeExt;
+        meta.is_dir() || meta.file_type().is_symlink_dir()
+    };
+    #[cfg(not(windows))]
+    let is_dir_like = fs::Metadata::is_dir;
+
+    if is_dir_like(&metadata) {
+        fs::remove_dir_all(path)
+    } else {
+        try_remove_op_set_perms(fs::remove_file, path, metadata)
+    }
+}
+
+fn try_remove_op_set_perms<'p, Op>(mut op: Op, path: &'p Path, metadata: Metadata) -> io::Result<()>
+where
+    Op: FnMut(&'p Path) -> io::Result<()>,
+{
+    match op(path) {
+        Ok(()) => Ok(()),
+        Err(e) if e.kind() == io::ErrorKind::PermissionDenied => {
+            let mut perms = metadata.permissions();
+            perms.set_readonly(false);
+            fs::set_permissions(path, perms)?;
+            op(path)
+        }
+        Err(e) => Err(e),
+    }
+}

src/build_helper/src/fs/tests.rs

@@ -0,0 +1,214 @@
+#![deny(unused_must_use)]
+
+use std::{env, fs, io};
+
+use super::recursive_remove;
+
+mod recursive_remove_tests {
+    use super::*;
+
+    // Basic cases
+
+    #[test]
+    fn nonexistent_path() {
+        let tmpdir = env::temp_dir();
+        let path = tmpdir.join("__INTERNAL_BOOTSTRAP_nonexistent_path");
+        assert!(fs::symlink_metadata(&path).is_err_and(|e| e.kind() == io::ErrorKind::NotFound));
+        assert!(recursive_remove(&path).is_err_and(|e| e.kind() == io::ErrorKind::NotFound));
+    }
+
+    #[test]
+    fn file() {
+        let tmpdir = env::temp_dir();
+        let path = tmpdir.join("__INTERNAL_BOOTSTRAP_file");
+        fs::write(&path, b"").unwrap();
+        assert!(fs::symlink_metadata(&path).is_ok());
+        assert!(recursive_remove(&path).is_ok());
+        assert!(fs::symlink_metadata(&path).is_err_and(|e| e.kind() == io::ErrorKind::NotFound));
+    }
+
+    mod dir_tests {
+        use super::*;
+
+        #[test]
+        fn dir_empty() {
+            let tmpdir = env::temp_dir();
+            let path = tmpdir.join("__INTERNAL_BOOTSTRAP_dir_tests_dir_empty");
+            fs::create_dir_all(&path).unwrap();
+            assert!(fs::symlink_metadata(&path).is_ok());
+            assert!(recursive_remove(&path).is_ok());
+            assert!(
+                fs::symlink_metadata(&path).is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+        }
+
+        #[test]
+        fn dir_recursive() {
+            let tmpdir = env::temp_dir();
+            let path = tmpdir.join("__INTERNAL_BOOTSTRAP_dir_tests_dir_recursive");
+            fs::create_dir_all(&path).unwrap();
+            assert!(fs::symlink_metadata(&path).is_ok());
+
+            let file_a = path.join("a.txt");
+            fs::write(&file_a, b"").unwrap();
+            assert!(fs::symlink_metadata(&file_a).is_ok());
+
+            let dir_b = path.join("b");
+            fs::create_dir_all(&dir_b).unwrap();
+            assert!(fs::symlink_metadata(&dir_b).is_ok());
+
+            let file_c = dir_b.join("c.rs");
+            fs::write(&file_c, b"").unwrap();
+            assert!(fs::symlink_metadata(&file_c).is_ok());
+
+            assert!(recursive_remove(&path).is_ok());
+
+            assert!(
+                fs::symlink_metadata(&file_a).is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+            assert!(
+                fs::symlink_metadata(&dir_b).is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+            assert!(
+                fs::symlink_metadata(&file_c).is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+        }
+    }
+
+    /// Check that [`recursive_remove`] does not traverse symlinks and only removes symlinks
+    /// themselves.
+    ///
+    /// Symlink-to-file versus symlink-to-dir is a distinction that's important on Windows, but not
+    /// on Unix.
+    mod symlink_tests {
+        use super::*;
+
+        #[cfg(unix)]
+        #[test]
+        fn unix_symlink() {
+            let tmpdir = env::temp_dir();
+            let path = tmpdir.join("__INTERNAL_BOOTSTRAP_symlink_tests_unix_symlink");
+            let symlink_path =
+                tmpdir.join("__INTERNAL_BOOTSTRAP__symlink_tests_unix_symlink_symlink");
+            fs::write(&path, b"").unwrap();
+
+            assert!(fs::symlink_metadata(&path).is_ok());
+            assert!(
+                fs::symlink_metadata(&symlink_path)
+                    .is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+
+            std::os::unix::fs::symlink(&path, &symlink_path).unwrap();
+
+            assert!(recursive_remove(&symlink_path).is_ok());
+
+            // Check that the symlink got removed...
+            assert!(
+                fs::symlink_metadata(&symlink_path)
+                    .is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+            // ... but pointed-to file still exists.
+            assert!(fs::symlink_metadata(&path).is_ok());
+
+            fs::remove_file(&path).unwrap();
+        }
+
+        #[cfg(windows)]
+        #[test]
+        fn windows_symlink_to_file() {
+            let tmpdir = env::temp_dir();
+            let path = tmpdir.join("__INTERNAL_BOOTSTRAP_symlink_tests_windows_symlink_to_file");
+            let symlink_path = tmpdir
+                .join("__INTERNAL_BOOTSTRAP_SYMLINK_symlink_tests_windows_symlink_to_file_symlink");
+            fs::write(&path, b"").unwrap();
+
+            assert!(fs::symlink_metadata(&path).is_ok());
+            assert!(
+                fs::symlink_metadata(&symlink_path)
+                    .is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+
+            std::os::windows::fs::symlink_file(&path, &symlink_path).unwrap();
+
+            assert!(recursive_remove(&symlink_path).is_ok());
+
+            // Check that the symlink-to-file got removed...
+            assert!(
+                fs::symlink_metadata(&symlink_path)
+                    .is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+            // ... but pointed-to file still exists.
+            assert!(fs::symlink_metadata(&path).is_ok());
+
+            fs::remove_file(&path).unwrap();
+        }
+
+        #[cfg(windows)]
+        #[test]
+        fn windows_symlink_to_dir() {
+            let tmpdir = env::temp_dir();
+            let path = tmpdir.join("__INTERNAL_BOOTSTRAP_symlink_tests_windows_symlink_to_dir");
+            let symlink_path =
+                tmpdir.join("__INTERNAL_BOOTSTRAP_symlink_tests_windows_symlink_to_dir_symlink");
+            fs::create_dir_all(&path).unwrap();
+
+            assert!(fs::symlink_metadata(&path).is_ok());
+            assert!(
+                fs::symlink_metadata(&symlink_path)
+                    .is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+
+            std::os::windows::fs::symlink_dir(&path, &symlink_path).unwrap();
+
+            assert!(recursive_remove(&symlink_path).is_ok());
+
+            // Check that the symlink-to-dir got removed...
+            assert!(
+                fs::symlink_metadata(&symlink_path)
+                    .is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+            // ... but pointed-to dir still exists.
+            assert!(fs::symlink_metadata(&path).is_ok());
+
+            fs::remove_dir_all(&path).unwrap();
+        }
+    }
+
+    /// Read-only file and directories only need special handling on Windows.
+    #[cfg(windows)]
+    mod readonly_tests {
+        use super::*;
+
+        #[test]
+        fn overrides_readonly() {
+            let tmpdir = env::temp_dir();
+            let path = tmpdir.join("__INTERNAL_BOOTSTRAP_readonly_tests_overrides_readonly");
+
+            // In case of a previous failed test:
+            if let Ok(mut perms) = fs::symlink_metadata(&path).map(|m| m.permissions()) {
+                perms.set_readonly(false);
+                fs::set_permissions(&path, perms).unwrap();
+                fs::remove_file(&path).unwrap();
+            }
+
+            fs::write(&path, b"").unwrap();
+
+            let mut perms = fs::symlink_metadata(&path).unwrap().permissions();
+            perms.set_readonly(true);
+            fs::set_permissions(&path, perms).unwrap();
+
+            // Check that file exists but is read-only, and that normal `std::fs::remove_file` fails
+            // to delete the file.
+            assert!(fs::symlink_metadata(&path).is_ok_and(|m| m.permissions().readonly()));
+            assert!(
+                fs::remove_file(&path).is_err_and(|e| e.kind() == io::ErrorKind::PermissionDenied)
+            );
+
+            assert!(recursive_remove(&path).is_ok());
+
+            assert!(
+                fs::symlink_metadata(&path).is_err_and(|e| e.kind() == io::ErrorKind::NotFound)
+            );
+        }
+    }
+}

src/build_helper/src/lib.rs

@@ -2,6 +2,7 @@
 
 pub mod ci;
 pub mod drop_bomb;
+pub mod fs;
 pub mod git;
 pub mod metrics;
 pub mod stage0_parser;

src/tools/compiletest/Cargo.toml

@@ -16,7 +16,7 @@ indexmap = "2.0.0"
 miropt-test-tools = { path = "../miropt-test-tools" }
 build_helper = { path = "../../build_helper" }
 tracing = "0.1"
-tracing-subscriber = { version = "0.3.3", default-features = false, features = ["fmt", "env-filter", "smallvec", "parking_lot", "ansi"] }
+tracing-subscriber = { version = "0.3.3", default-features = false, features = ["ansi", "env-filter", "fmt", "parking_lot", "smallvec"] }
 regex = "1.0"
 semver = { version = "1.0.23", features = ["serde"] }
 serde = { version = "1.0", features = ["derive"] }

src/tools/compiletest/src/runtest.rs

@@ -2809,29 +2809,6 @@ impl<'test> TestCx<'test> {
             println!("init_incremental_test: incremental_dir={}", incremental_dir.display());
         }
     }
-
-    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)
-    }
 }
 
 struct ProcArgs {

src/tools/compiletest/src/runtest/run_make.rs

@@ -2,6 +2,8 @@ use std::path::Path;
 use std::process::{Command, Output, Stdio};
 use std::{env, fs};
 
+use build_helper::fs::{ignore_not_found, recursive_remove};
+
 use super::{ProcRes, TestCx, disable_error_reporting};
 use crate::util::{copy_dir_all, dylib_env_var};
 
@@ -27,9 +29,8 @@ impl TestCx<'_> {
         // are hopefully going away, it seems safer to leave this perilous code
         // as-is until it can all be deleted.
         let tmpdir = cwd.join(self.output_base_name());
-        if tmpdir.exists() {
-            self.aggressive_rm_rf(&tmpdir).unwrap();
-        }
+        ignore_not_found(|| recursive_remove(&tmpdir)).unwrap();
+
         fs::create_dir_all(&tmpdir).unwrap();
 
         let host = &self.config.host;
@@ -218,9 +219,8 @@ impl TestCx<'_> {
         //
         // This setup intentionally diverges from legacy Makefile run-make tests.
         let base_dir = self.output_base_dir();
-        if base_dir.exists() {
-            self.aggressive_rm_rf(&base_dir).unwrap();
-        }
+        ignore_not_found(|| recursive_remove(&base_dir)).unwrap();
+
         let rmake_out_dir = base_dir.join("rmake_out");
         fs::create_dir_all(&rmake_out_dir).unwrap();
 

src/tools/run-make-support/src/fs.rs

@@ -1,7 +1,51 @@
+use std::fs::FileType;
 use std::io;
 use std::path::{Path, PathBuf};
 
-/// Copy a directory into another.
+/// Given a symlink at `src`, read its target, then create a new symlink at `dst` also pointing to
+/// target.
+pub fn copy_symlink(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
+    let src = src.as_ref();
+    let dst = dst.as_ref();
+    let metadata = symlink_metadata(src);
+    if let Err(e) = copy_symlink_raw(metadata.file_type(), src, dst) {
+        panic!("failed to copy symlink from `{}` to `{}`: {e}", src.display(), dst.display(),);
+    }
+}
+
+fn copy_symlink_raw(ty: FileType, src: impl AsRef<Path>, dst: impl AsRef<Path>) -> io::Result<()> {
+    // Traverse symlink once to find path of target entity.
+    let target_path = std::fs::read_link(src)?;
+
+    let new_symlink_path = dst.as_ref();
+    #[cfg(windows)]
+    {
+        use std::os::windows::fs::FileTypeExt;
+        if ty.is_symlink_dir() {
+            std::os::windows::fs::symlink_dir(&target_path, new_symlink_path)?;
+        } else {
+            // Target may be a file or another symlink, in any case we can use
+            // `symlink_file` here.
+            std::os::windows::fs::symlink_file(&target_path, new_symlink_path)?;
+        }
+    }
+    #[cfg(unix)]
+    {
+        let _ = ty;
+        std::os::unix::fs::symlink(target_path, new_symlink_path)?;
+    }
+    #[cfg(not(any(windows, unix)))]
+    {
+        let _ = ty;
+        // Technically there's also wasi, but I have no clue about wasi symlink
+        // semantics and which wasi targets / environment support symlinks.
+        unimplemented!("unsupported target");
+    }
+    Ok(())
+}
+
+/// Copy a directory into another. This will not traverse symlinks; instead, it will create new
+/// symlinks pointing at target paths that symlinks in the original directory points to.
 pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
     fn copy_dir_all_inner(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> io::Result<()> {
         let dst = dst.as_ref();
@@ -14,31 +58,7 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
             if ty.is_dir() {
                 copy_dir_all_inner(entry.path(), dst.join(entry.file_name()))?;
             } else if ty.is_symlink() {
-                // Traverse symlink once to find path of target entity.
-                let target_path = std::fs::read_link(entry.path())?;
-
-                let new_symlink_path = dst.join(entry.file_name());
-                #[cfg(windows)]
-                {
-                    use std::os::windows::fs::FileTypeExt;
-                    if ty.is_symlink_dir() {
-                        std::os::windows::fs::symlink_dir(&target_path, new_symlink_path)?;
-                    } else {
-                        // Target may be a file or another symlink, in any case we can use
-                        // `symlink_file` here.
-                        std::os::windows::fs::symlink_file(&target_path, new_symlink_path)?;
-                    }
-                }
-                #[cfg(unix)]
-                {
-                    std::os::unix::fs::symlink(target_path, new_symlink_path)?;
-                }
-                #[cfg(not(any(windows, unix)))]
-                {
-                    // Technically there's also wasi, but I have no clue about wasi symlink
-                    // semantics and which wasi targets / environment support symlinks.
-                    unimplemented!("unsupported target");
-                }
+                copy_symlink_raw(ty, entry.path(), dst.join(entry.file_name()))?;
             } else {
                 std::fs::copy(entry.path(), dst.join(entry.file_name()))?;
             }
@@ -64,6 +84,21 @@ pub fn read_dir_entries<P: AsRef<Path>, F: FnMut(&Path)>(dir: P, mut callback: F
     }
 }
 
+/// A wrapper around [`build_helper::fs::recursive_remove`] which includes the file path in the
+/// panic message.
+///
+/// This handles removing symlinks on Windows (e.g. symlink-to-file will be removed via
+/// [`std::fs::remove_file`] while symlink-to-dir will be removed via [`std::fs::remove_dir`]).
+#[track_caller]
+pub fn recursive_remove<P: AsRef<Path>>(path: P) {
+    if let Err(e) = build_helper::fs::recursive_remove(path.as_ref()) {
+        panic!(
+            "failed to recursive remove filesystem entities at `{}`: {e}",
+            path.as_ref().display()
+        );
+    }
+}
+
 /// A wrapper around [`std::fs::remove_file`] which includes the file path in the panic message.
 #[track_caller]
 pub fn remove_file<P: AsRef<Path>>(path: P) {