Enable more fs tests on Windows #31360
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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| // `SeCreateSymbolicLinkPrivilege`). Instead of disabling these test on Windows, use this | ||
| // function to test whether we have permission, and return otherwise. This way, we still don't | ||
| // run these tests most of the time, but at least we do if the user has the right permissions. | ||
| pub fn got_symlink_permission(tmpdir: &TempDir) -> bool { |
There was a problem hiding this comment.
Could this function always return true on unix? I'm somewhat wary about us auto-ignoring tests as it may be easy for the tests to accidentally be ignored in the future.
There was a problem hiding this comment.
No problem, that was my first try.
|
Updated. Thanks for the good review! |
|
I have added the test Also this uncovered a bug with create_dir_all: it fails if the path contains a dangling symlink. |
| } | ||
| } | ||
| #[cfg(not(windows))] | ||
| #[allow(unused_variables)] |
There was a problem hiding this comment.
Instead of conditional compilation here (e.g. defining the same signature twice and having an #[allow] this could also do something like
if cfg!(unix) {
return true
}That should be more resilient to cross-compilation concerns in the future at least
|
Thanks @pitdicker! Looks good to me modulo a few last nits but otherwise r=me |
|
Updated. I should really learn to use the right line length... |
|
⌛ Testing commit 0bd55e5 with merge f2b7960... |
|
💔 Test failed - auto-win-msvc-64-opt |
|
Oops. It fails because |
|
It should be ok now |
bors
added a commit
that referenced
this pull request
Feb 4, 2016
- use `symlink_file` and `symlink_dir` instead of the old `soft_link`
- create a junction instead of a directory symlink for testing recursive_rmdir (as it causes the
same troubles, but can be created by users without `SeCreateSymbolicLinkPrivilege`)
- `remove_dir_all` was unable to remove directory symlinks and junctions
- only run tests that create symlinks if we have the right permissions.
- rename `Path2` to `Path`
- remove the global `#[allow(deprecated)]` and outdated comments
- After factoring out `create_junction()` from the test `directory_junctions_are_directories` and
removing needlessly complex code, what I was left with was:
```
#[test]
#[cfg(windows)]
fn directory_junctions_are_directories() {
use sys::fs::create_junction;
let tmpdir = tmpdir();
let foo = tmpdir.join("foo");
let bar = tmpdir.join("bar");
fs::create_dir(&foo).unwrap();
check!(create_junction(&foo, &bar));
assert!(bar.metadata().unwrap().is_dir());
}
```
It test whether a junction is a directory instead of a reparse point. But it actually test the
target of the junction (which is a directory if it exists) instead of the junction itself, which
should always be a symlink. So this test is invalid, and I expect it only exists because the
author was suprised by it. So I removed it.
Some things that do not yet work right:
- relative symlinks do not accept forward slashes
- the conversion of paths for `create_junction` is hacky
- `remove_dir_all` now messes with the internal data of `FileAttr` to be able to remove symlinks.
We should add some method like `is_symlink_dir()` to it, so code outside the standard library
can see the difference between file and directory symlinks too.
|
⌛ Testing commit fb172b6 with merge d0ef740... |
This was referenced Feb 4, 2016
|
@pitdicker oh one thing I just remembered, if you're gonna be working some more in this area could you add a regression test for |
|
I found the probleem because all tests that create directory symlinks or junctions failed when cleaning up... But having an explicit test can make it clearer I suppose. I will add one when a next pr is ready. |
|
ah right good point, thanks! |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 29, 2019
…t, r=sfackler
std: Fix a failing `fs` test on Windows
In testing 4-core machines on Azure the `realpath_works_tricky` test in
the standard library is failing with "The directory name is invalid". In
attempting to debug this test I was able to reproduce the failure
locally on my machine, and after inspecing the test it I believe is
exploiting Unix-specific behavior that seems to only sometimes work on
Windows. Specifically the test basically executes:
mkdir -p a/b
mkdir -p a/d
touch a/f
ln -s a/b/c ../d/e
ln -s a/d/e ../f
and then asserts that `canonicalize("a/b/c")` and
`canonicalize("a/d/e")` are equivalent to `a/f`. On Windows however the
first symlink is a "directory symlink" and the second is a file symlink.
In both cases, though, they're pointing to files. This means that for
whatever reason locally and on the 4-core environment the call to
`canonicalize` is failing. On Azure today it seems to be passing, and
I'm not entirely sure why. I'm sort of presuming that there's some sort
of internals going on here where there's some global Windows setting
which makes symlinks behavior more unix-like and ignore the directory
hint.
In any case this should keep the test working and also fixes the test
locally for me. It's also worth pointing out that this test was made Windows compatible in rust-lang#31360, a pretty ancient PR at this point.
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 29, 2019
…t, r=sfackler
std: Fix a failing `fs` test on Windows
In testing 4-core machines on Azure the `realpath_works_tricky` test in
the standard library is failing with "The directory name is invalid". In
attempting to debug this test I was able to reproduce the failure
locally on my machine, and after inspecing the test it I believe is
exploiting Unix-specific behavior that seems to only sometimes work on
Windows. Specifically the test basically executes:
mkdir -p a/b
mkdir -p a/d
touch a/f
ln -s a/b/c ../d/e
ln -s a/d/e ../f
and then asserts that `canonicalize("a/b/c")` and
`canonicalize("a/d/e")` are equivalent to `a/f`. On Windows however the
first symlink is a "directory symlink" and the second is a file symlink.
In both cases, though, they're pointing to files. This means that for
whatever reason locally and on the 4-core environment the call to
`canonicalize` is failing. On Azure today it seems to be passing, and
I'm not entirely sure why. I'm sort of presuming that there's some sort
of internals going on here where there's some global Windows setting
which makes symlinks behavior more unix-like and ignore the directory
hint.
In any case this should keep the test working and also fixes the test
locally for me. It's also worth pointing out that this test was made Windows compatible in rust-lang#31360, a pretty ancient PR at this point.
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 30, 2019
…t, r=sfackler
std: Fix a failing `fs` test on Windows
In testing 4-core machines on Azure the `realpath_works_tricky` test in
the standard library is failing with "The directory name is invalid". In
attempting to debug this test I was able to reproduce the failure
locally on my machine, and after inspecing the test it I believe is
exploiting Unix-specific behavior that seems to only sometimes work on
Windows. Specifically the test basically executes:
mkdir -p a/b
mkdir -p a/d
touch a/f
ln -s a/b/c ../d/e
ln -s a/d/e ../f
and then asserts that `canonicalize("a/b/c")` and
`canonicalize("a/d/e")` are equivalent to `a/f`. On Windows however the
first symlink is a "directory symlink" and the second is a file symlink.
In both cases, though, they're pointing to files. This means that for
whatever reason locally and on the 4-core environment the call to
`canonicalize` is failing. On Azure today it seems to be passing, and
I'm not entirely sure why. I'm sort of presuming that there's some sort
of internals going on here where there's some global Windows setting
which makes symlinks behavior more unix-like and ignore the directory
hint.
In any case this should keep the test working and also fixes the test
locally for me. It's also worth pointing out that this test was made Windows compatible in rust-lang#31360, a pretty ancient PR at this point.
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 30, 2019
…t, r=sfackler
std: Fix a failing `fs` test on Windows
In testing 4-core machines on Azure the `realpath_works_tricky` test in
the standard library is failing with "The directory name is invalid". In
attempting to debug this test I was able to reproduce the failure
locally on my machine, and after inspecing the test it I believe is
exploiting Unix-specific behavior that seems to only sometimes work on
Windows. Specifically the test basically executes:
mkdir -p a/b
mkdir -p a/d
touch a/f
ln -s a/b/c ../d/e
ln -s a/d/e ../f
and then asserts that `canonicalize("a/b/c")` and
`canonicalize("a/d/e")` are equivalent to `a/f`. On Windows however the
first symlink is a "directory symlink" and the second is a file symlink.
In both cases, though, they're pointing to files. This means that for
whatever reason locally and on the 4-core environment the call to
`canonicalize` is failing. On Azure today it seems to be passing, and
I'm not entirely sure why. I'm sort of presuming that there's some sort
of internals going on here where there's some global Windows setting
which makes symlinks behavior more unix-like and ignore the directory
hint.
In any case this should keep the test working and also fixes the test
locally for me. It's also worth pointing out that this test was made Windows compatible in rust-lang#31360, a pretty ancient PR at this point.
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.
use
symlink_fileandsymlink_dirinstead of the oldsoft_linkcreate a junction instead of a directory symlink for testing recursive_rmdir (as it causes the
same troubles, but can be created by users without
SeCreateSymbolicLinkPrivilege)remove_dir_allwas unable to remove directory symlinks and junctionsonly run tests that create symlinks if we have the right permissions.
rename
Path2toPathremove the global
#[allow(deprecated)]and outdated commentsAfter factoring out
create_junction()from the testdirectory_junctions_are_directoriesandremoving needlessly complex code, what I was left with was:
It test whether a junction is a directory instead of a reparse point. But it actually test the
target of the junction (which is a directory if it exists) instead of the junction itself, which
should always be a symlink. So this test is invalid, and I expect it only exists because the
author was suprised by it. So I removed it.
Some things that do not yet work right:
create_junctionis hackyremove_dir_allnow messes with the internal data ofFileAttrto be able to remove symlinks.We should add some method like
is_symlink_dir()to it, so code outside the standard librarycan see the difference between file and directory symlinks too.