Handle symlinks in src/bootstrap/clean.rs (mostly) -- resolves #40860. #41026
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. :-)
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
@bors: r+ Thanks! |
|
📌 Commit efd6eab has been approved by |
|
You're welcome. :-) |
frewsxcv
added a commit
to frewsxcv/rust
that referenced
this pull request
Apr 6, 2017
Handle symlinks in src/bootstrap/clean.rs (mostly) -- resolves rust-lang#40860. In response to rust-lang#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. :-)
frewsxcv
added a commit
to frewsxcv/rust
that referenced
this pull request
Apr 6, 2017
Handle symlinks in src/bootstrap/clean.rs (mostly) -- resolves rust-lang#40860. In response to rust-lang#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. :-)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In response to #40860
The broken condition can be replicated with:
src/bootstrap/clean.rshas a custom implementation of removing a treefn rm_rfthat usedstd::path::Path::{is_file, is_dir, exists}whilerecursively deleting directories and files. Unfortunately,
Path'simplementation of
is_file()andis_dir()andexists()alwaysunconditionally 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 offollowing them.
So it looks like the fix is to use
.symlink_metadata()to figure outwhether 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/$MYARCHdirectory, because those initial entries are run throughPath::canonicalize(), which panics with broken symlinks. So lets justnever use symlinks in that one directory. :-)