Skip to content

Commit 9ad3e3f

Browse files
jieyouxujieyouxu
authored
Rollup merge of #129187 - jieyouxu:squeaky-clean-windows-symlinks, r=Kobzol
bootstrap: fix clean's remove_dir_all implementation It turns out bootstrap's `clean.rs`'s hand-rolled `rm_rf` (which probably comes before `std::fs::remove_dir_all` was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use `std::fs::remove_dir_all`. This is a blocker for compiletest's own `rm_rf` implementation #129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port #128562 that heavily exercises symlinks (I was reviewing #128562 and testing it on native Windows which is how I found out). I also left a FIXME for `detect_src_and_out` due to a failing assertion on native Windows (opened #129188): ``` ---- core::config::tests::detect_src_and_out stdout ---- thread 'core::config::tests::detect_src_and_out' panicked at src\core\config\tests.rs:72:13: assertion `left == right` failed left: "E:\\tmp" right: "C:\\tmp" ``` Fixes #112544 (because now we handle Windows symlinks properly). try-job: x86_64-msvc try-job: i686-mingw try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: aarch64-gnu
2 parents 331da08 + 1687c55 commit 9ad3e3f

File tree

1 file changed

+15
-78
lines changed
  • src/bootstrap/src/core/build_steps

1 file changed

+15
-78
lines changed

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

+15-78
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
//! directory unless the `--all` flag is present.
77
88
use std::fs;
9-
use std::io::{self, ErrorKind};
109
use std::path::Path;
1110

1211
use crate::core::builder::{crate_description, Builder, RunConfig, ShouldRun, Step};
@@ -101,11 +100,11 @@ fn clean(build: &Build, all: bool, stage: Option<u32>) {
101100
return;
102101
}
103102

104-
rm_rf("tmp".as_ref());
103+
remove_dir_recursive("tmp");
105104

106105
// Clean the entire build directory
107106
if all {
108-
rm_rf(&build.out);
107+
remove_dir_recursive(&build.out);
109108
return;
110109
}
111110

@@ -136,17 +135,17 @@ fn clean_specific_stage(build: &Build, stage: u32) {
136135
}
137136

138137
let path = t!(entry.path().canonicalize());
139-
rm_rf(&path);
138+
remove_dir_recursive(&path);
140139
}
141140
}
142141
}
143142

144143
fn clean_default(build: &Build) {
145-
rm_rf(&build.out.join("tmp"));
146-
rm_rf(&build.out.join("dist"));
147-
rm_rf(&build.out.join("bootstrap").join(".last-warned-change-id"));
148-
rm_rf(&build.out.join("bootstrap-shims-dump"));
149-
rm_rf(&build.out.join("rustfmt.stamp"));
144+
remove_dir_recursive(build.out.join("tmp"));
145+
remove_dir_recursive(build.out.join("dist"));
146+
remove_dir_recursive(build.out.join("bootstrap").join(".last-warned-change-id"));
147+
remove_dir_recursive(build.out.join("bootstrap-shims-dump"));
148+
remove_dir_recursive(build.out.join("rustfmt.stamp"));
150149

151150
let mut hosts: Vec<_> = build.hosts.iter().map(|t| build.out.join(t)).collect();
152151
// After cross-compilation, artifacts of the host architecture (which may differ from build.host)
@@ -166,78 +165,16 @@ fn clean_default(build: &Build) {
166165
continue;
167166
}
168167
let path = t!(entry.path().canonicalize());
169-
rm_rf(&path);
168+
remove_dir_recursive(&path);
170169
}
171170
}
172171
}
173172

174-
fn rm_rf(path: &Path) {
175-
match path.symlink_metadata() {
176-
Err(e) => {
177-
if e.kind() == ErrorKind::NotFound {
178-
return;
179-
}
180-
panic!("failed to get metadata for file {}: {}", path.display(), e);
181-
}
182-
Ok(metadata) => {
183-
if metadata.file_type().is_file() || metadata.file_type().is_symlink() {
184-
do_op(path, "remove file", |p| match fs::remove_file(p) {
185-
#[cfg(windows)]
186-
Err(e)
187-
if e.kind() == std::io::ErrorKind::PermissionDenied
188-
&& p.file_name().and_then(std::ffi::OsStr::to_str)
189-
== Some("bootstrap.exe") =>
190-
{
191-
eprintln!("WARNING: failed to delete '{}'.", p.display());
192-
Ok(())
193-
}
194-
r => r,
195-
});
196-
197-
return;
198-
}
199-
200-
for file in t!(fs::read_dir(path)) {
201-
rm_rf(&t!(file).path());
202-
}
203-
204-
do_op(path, "remove dir", |p| match fs::remove_dir(p) {
205-
// Check for dir not empty on Windows
206-
// FIXME: Once `ErrorKind::DirectoryNotEmpty` is stabilized,
207-
// match on `e.kind()` instead.
208-
#[cfg(windows)]
209-
Err(e) if e.raw_os_error() == Some(145) => Ok(()),
210-
r => r,
211-
});
212-
}
213-
};
214-
}
215-
216-
fn do_op<F>(path: &Path, desc: &str, mut f: F)
217-
where
218-
F: FnMut(&Path) -> io::Result<()>,
219-
{
220-
match f(path) {
221-
Ok(()) => {}
222-
// On windows we can't remove a readonly file, and git will often clone files as readonly.
223-
// As a result, we have some special logic to remove readonly files on windows.
224-
// This is also the reason that we can't use things like fs::remove_dir_all().
225-
#[cfg(windows)]
226-
Err(ref e) if e.kind() == ErrorKind::PermissionDenied => {
227-
let m = t!(path.symlink_metadata());
228-
let mut p = m.permissions();
229-
p.set_readonly(false);
230-
t!(fs::set_permissions(path, p));
231-
f(path).unwrap_or_else(|e| {
232-
// Delete symlinked directories on Windows
233-
if m.file_type().is_symlink() && path.is_dir() && fs::remove_dir(path).is_ok() {
234-
return;
235-
}
236-
panic!("failed to {} {}: {}", desc, path.display(), e);
237-
});
238-
}
239-
Err(e) => {
240-
panic!("failed to {} {}: {}", desc, path.display(), e);
241-
}
173+
/// Wrapper for [`std::fs::remove_dir_all`] that panics on failure and prints the `path` we failed
174+
/// on.
175+
fn remove_dir_recursive<P: AsRef<Path>>(path: P) {
176+
let path = path.as_ref();
177+
if let Err(e) = fs::remove_dir_all(path) {
178+
panic!("failed to `remove_dir_all` at `{}`: {e}", path.display());
242179
}
243180
}

0 commit comments

Comments
 (0)