From e0900a1bb79a6c844fadb4b27fe8e0793d952476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 22 Aug 2024 15:54:52 +0000 Subject: [PATCH 1/2] Revert "bootstrap: fix clean's `remove_dir_all` implementation" This reverts commit 1687c55168f3837506afcd2240a8a0b6eadcc1eb. --- src/bootstrap/src/core/build_steps/clean.rs | 93 +++++++++++++++++---- 1 file changed, 78 insertions(+), 15 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clean.rs b/src/bootstrap/src/core/build_steps/clean.rs index bcbe490c36a0f..f608e5d715e49 100644 --- a/src/bootstrap/src/core/build_steps/clean.rs +++ b/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) { 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>(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(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); + } } } From 3957f3933904075a348eda68ae3d34f9f4914116 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 22 Aug 2024 15:55:06 +0000 Subject: [PATCH 2/2] Revert "compiletest: use `std::fs::remove_dir_all` now that it is available" This reverts commit 75ed08972703798888fceff12b3217408ca6a4b5. --- src/tools/compiletest/src/runtest.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 6d45040345ad4..eca21e559896a 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/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();