cargo clean failure with dangling readonly dir-symlinks on windows

[rbtcollins]

Problem

Rustup creates symlinks within target/ in its test suite.

Here is an example:

stat target/tests/running-test-5D1PNf/rustupiIcEHF/toolchains/default-from-path
  File: target/tests/running-test-5D1PNf/rustupiIcEHF/toolchains/default-from-path -> /c/Users/username/Documents/src/rustup.rs/target/tests/running-test-5D1PNf/rustup-customsOflBt/custom-1
  Size: 102             Blocks: 0          IO Block: 65536  symbolic link
Device: de41ba52h/3728849490d   Inode: 9007199256791307  Links: 1
Access: (0777/lrwxrwxrwx)  Uid: (197609/ robertc)   Gid: (197609/ UNKNOWN)
Access: 2023-02-27 11:44:54.557419600 +0100
Modify: 2023-02-27 11:44:54.557419600 +0100
Change: 2023-02-27 11:44:54.559622400 +0100
 Birth: 2023-02-27 11:44:54.557419600 +0100

and in cmd:

dir
 Volume in drive C has no label.
 Volume Serial Number is DE41-BA52

 Directory of C:\Users\username\Documents\src\rustup.rs\target\tests\running-test-5D1PNf\rustupiIcEHF\toolchains

27/02/2023  11:44 AM    <DIR>          .
20/03/2023  11:22 PM    <DIR>          ..
27/02/2023  11:44 AM    <JUNCTION>     default-from-path [\??\C:\Users\username\Documents\src\rustup.rs\target\tests\running-test-5D1PNf\rustup-customsOflBt\custom-1]
               0 File(s)              0 bytes
               3 Dir(s)  127,863,406,592 bytes free

Inspecting with explorer shows that the link is a readonly dir link.

This is a dangling link (likely because cargo clean deleted the target first).

stat /c/Users/username/Documents/src/rustup.rs/target/tests/running-test-5D1PNf/rustup-customsOflBt/
custom-1
stat: cannot stat '/c/Users/username/Documents/src/rustup.rs/target/tests/running-test-5D1PNf/rustup-customsOflBt/custom-1': No such file or directory

cargo clean fails:

cargo clean
error: failed to remove build artifact

Caused by:
  failed to remove file `C:\Users\username\Documents\src\rustup.rs\target\tests\running-test-5D1PNf\rustupiIcEHF\toolchains\default-from-path`

Caused by:
  Access is denied. (os error 5)

rm from within bash deletes the file successfully, which permits cargo clean to proceed.

I haven't tested yet, but I suspect the remove_dir_all function in the stdlib would handle this correctly. I have tested the remove_dir_all crate's equivalent API, and it does handle this correctly.

The code in _remove_file calls a helper that doesn't use symlink_metadata, rather metadata, which will fail on dangling symlinks.

Separately I also note that the entire routine is based on path based calls rather than _at style calls, which will incur higher traversal costs in-kernel, and is serial, even though directory removal is embarrassingly parallel : if you're interested I could look at what changes would be needed to the remove_dir_all crate to support the UI cargo wants, to permit switching to that. Parallel deletion of installed toolchains had a measurable speed improvement for rustup, both in our test suite and for users.

Steps

No response

Possible Solution(s)

No response

Notes

No response

Version

$ cargo version --verbose
cargo 1.67.0-beta.2 (f6e737b1e 2022-12-02)
release: 1.67.0-beta.2
commit-hash: f6e737b1e3386adb89333bf06a01f68a91ac5306
commit-date: 2022-12-02
host: x86_64-pc-windows-msvc
libgit2: 1.5.0 (sys:0.15.0 vendored)
libcurl: 7.86.0-DEV (sys:0.4.59+curl-7.86.0 vendored ssl:Schannel)
os: Windows 10.0.22621 (Windows 10 Pro) [64-bit]

[weihanglo]

somewhat related #11442

[HamishPoole]

Mentioned on #11442

Root cause is std::fs::remove_dir_all .

The question is:

Do you continue to reimplement std::fs::remove_dir_all in cargo-util::paths::*?

Or attempt to change std::fs::remove_dir_all to resolve like rbtcollins suggest?

I'm unfamiliar with how difficult submitting a change to std is in Rust.

[weihanglo]

#11442 has landed on beta channel. Is this issue still relevant?

[dmikis]

Yes, the problem persists. The solution is to carefully go about every place where cargo decides how to delete a fs entry and check FileTypeExt::is_symlink_dir on Windows. I've dug a little bit, and it seems to me there are at least two places to patch: cargo_util::paths::remove_dir_all and CleanContext::rm_rf in cargo_clean.rs. I can come up with a patch.

BTW, std::fs::remove_dir_all does the right thing.