Fix removal of symbolic links on Windows #81
Merged
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
ericcornelissen
force-pushed
the
symlinks-windows
branch
from
a0fcdaa
to
ace7cba
Compare
Add conditional ignore attributes to all tests involving symlinks to
prevent them from running on Windows. Per [1,2], creating symlinks is
considered a privileged action on Windows systems. As a result, all
these tests currently fail on Windows systems unless they're run as
administrator (or similar)[^1].
To address this, the test_feature "test-symlink" has been created to
allow Windows contributors to run tests involving symbolic links
conditionally. This is documented in the Contributing Guidelines. This
test_feature is enabled by default when running `ci-*` variants of Just
commands (like other test_features).
Note that tests with pre-existing ignores for Windows due to symlinks
(i.e. those with the ignore comment "TODO: investigate symlink test
errors on Windows") are not included here. This is because those tests
fail due to a bug in the implementation of this software for Windows
where symbolic links to folders cannot be removed.
--
^1: This was not the case in the CI because (I presume) GitHub Actions
runs things as administrator (or similar) by default.
--
1. https://doc.rust-lang.org/stable/std/os/windows/fs/fn.symlink_dir.html
2. https://doc.rust-lang.org/stable/std/os/windows/fs/fn.symlink_file.html
ericcornelissen
force-pushed
the
symlinks-windows
branch
from
ace7cba
to
236d37e
Compare
ericcornelissen
changed the title
Codecov Report
@@ Coverage Diff @@
## main #81 +/- ##
=======================================
Coverage 98.97% 98.98%
=======================================
Files 1 1
Lines 294 295 +1
=======================================
+ Hits 291 292 +1
Misses 3 3
|
Update the implementation of `rm::remove` to, conditionally on the target platform, remove symbolic files either: 1. On non-Windows systems always as files (unchanged), or 2. On Windows systems as either a directory or file depending on what the link is pointing to. The latter is necessary for removal of symbolic links on Windows because otherwise users will get a `PermissionDenied` error. All tests that were disabled on Windows for this reason previously now run conditionally on the `test-symlink` test feature, just like all other symlink-related tests. Except for the unit test named `symlink_to_ dir_at_ location_of_a_dir_toctou`. With the current implementation of the `remove` function this TOCTOU scenario is handled differently on Windows from non-Windows systems, but both outcomes are considered okay (both versions of the test are kept to notice changes/regressions on any platform).
ericcornelissen
force-pushed
the
symlinks-windows
branch
from
236d37e
to
7f45c96
Compare
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.
Closes #1
Summary
Update how symlinks on Windows are removed as well as how it is tested.
Tests involving symlinks are now run conditionally on the (new)
test-symlinkfeature on Windows in order to avoid unexpected test failures due to the elevated privileges requirement for working with symlinks on Windows.The implementation for removing symlinks is updated such that, on Windows, removing a symlink of a directory uses
std::fs::remove_dir. This appears to be necessary for removing symbolic links of directories, while usingstd::fs::remove_fileis appears to be necessary for removing symbolic links of files.