From efd6eab366ad44ac6e9b0b12cd4b9688ab71df6d Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Sun, 2 Apr 2017 16:26:43 -0600 Subject: [PATCH] Handle symlinks in src/bootstrap/clean.rs (mostly) -- resolves #40860. The broken condition can be replicated with: ``shell export MYARCH=x86_64-apple-darwin && mkdir -p build/$MYARCH/subdir && touch build/$MYARCH/subdir/file && ln -s build/$MYARCH/subdir/file build/$MYARCH/subdir/symlink `` `src/bootstrap/clean.rs` has a custom implementation of removing a tree `fn rm_rf` that used `std::path::Path::{is_file, is_dir, exists}` while recursively deleting directories and files. Unfortunately, `Path`'s implementation of `is_file()` and `is_dir()` and `exists()` always unconditionally follow symlinks, which is the exact opposite of standard implementations of deleting file trees. It appears that this custom implementation is being used to workaround a behavior in Windows where the files often get marked as read-only, which prevents us from simply using something nice and simple like `std::fs::remove_dir_all`, which properly deletes links instead of following them. So it looks like the fix is to use `.symlink_metadata()` to figure out whether tree items are files/symlinks/directories. The one corner case this won't cover is if there is a broken symlink in the "root" `build/$MYARCH` directory, because those initial entries are run through `Path::canonicalize()`, which panics with broken symlinks. So lets just never use symlinks in that one directory. :-) --- src/bootstrap/clean.rs | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/bootstrap/clean.rs b/src/bootstrap/clean.rs index e9547ee42d077..308a0ab3076dd 100644 --- a/src/bootstrap/clean.rs +++ b/src/bootstrap/clean.rs @@ -44,26 +44,25 @@ pub fn clean(build: &Build) { } fn rm_rf(path: &Path) { - if !path.exists() { - return - } - if path.is_file() { - return do_op(path, "remove file", |p| fs::remove_file(p)); - } - - for file in t!(fs::read_dir(path)) { - let file = t!(file).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| fs::remove_file(p)); + return; + } - if file.is_dir() { - rm_rf(&file); - } else { - // 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. - do_op(&file, "remove file", |p| fs::remove_file(p)); - } - } - do_op(path, "remove dir", |p| fs::remove_dir(p)); + for file in t!(fs::read_dir(path)) { + rm_rf(&t!(file).path()); + } + do_op(path, "remove dir", |p| fs::remove_dir(p)); + }, + }; } fn do_op(path: &Path, desc: &str, mut f: F) @@ -71,9 +70,12 @@ fn do_op(path: &Path, desc: &str, mut f: F) { 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(). Err(ref e) if cfg!(windows) && e.kind() == ErrorKind::PermissionDenied => { - let mut p = t!(path.metadata()).permissions(); + let mut p = t!(path.symlink_metadata()).permissions(); p.set_readonly(false); t!(fs::set_permissions(path, p)); f(path).unwrap_or_else(|e| {