test-infra: improve compiletest and run-make-support symlink handling

[jieyouxu]

I was trying to implement #134656 to port tests/run-make/incr-add-rust-src-component.rs, but found some blockers related to symlink handling, so in this PR I tried to resolve them by improving symlink handling in compiletest and run-make-support (particularly for native windows msvc environment).

Key changes:

• I needed to copy symlinks (duplicate a symlink pointing to the same file), so I pulled out the copy symlink logic and re-exposed it as run_make_support::rfs::copy_symlink. This helper correctly accounts for the Windows symlink-to-file vs symlink-to-dir distinction (hereafter "Windows symlinks") when copying symlinks.
• recursive_remove:
  • I needed a way to remove symlinks themselves (no symlink traversal). std::fs::remove_dir_all handles them, but only if the root path is a directory. So I wrapped std::fs::remove_dir_all to also handle when the root path is a non-directory entity (e.g. file or symlink). Again, this properly accounts for Windows symlinks.
  • I wanted to use this for both compiletest and run-make-support, so I put the implementation and accompanying tests in build_helper.
  • In this sense, it's a reland of compiletest: use std::fs::remove_dir_all now that it is available #129302 with proper test coverage.
  • It's a thin wrapper around std::fs::remove_dir_all (remove_dir_all correctly handles read-only entries on Windows). The helper has additional permission-setting logic for when the root path is a non-dir entry on Windows to handle read-only non-dir entry.

Fixes #126334.

[ChrisDenton]

Can I ask why you need to FORCE PERMISSIONS? remove_dir_all should remove readonly file on Windows 10 and 11.

[jieyouxu]

Can I ask why you need to FORCE PERMISSIONS? remove_dir_all should remove readonly file on Windows 10 and 11.

... good question. For some reason I completely forgot about this. Let me revert it real quick.

EDIT: ah, but I will need to force perms when the root path is a non-dir entry. Does std::fs::remove_file handle read-only files? Based on locally testing, not remove_file (but remove_dir_all will).

[ChrisDenton]

It should do. I'd be minded to try something like:

#[cfg(windows)]
use std::os::windows::fs::FileTypeExt;

pub fn recursive_remove<P: AsRef<Path>>(path: P) -> io::Result<()> {
    let path = path.as_ref();
    let metadata = fs::symlink_metadata(path)?;
    #[cfg(windows)]
    let is_dir = |meta: &fs::Metadata| meta.is_dir() || meta.file_type().is_symlink_dir();
    #[cfg(not(windows))]
    let is_dir = fs::Metadata::is_dir;

    if is_dir(&metadata) { fs::remove_dir_all(path) } else { fs::remove_file(path) }
}

[jieyouxu]

It should do. I'd be minded to try something like:

#[cfg(windows)]
use std::os::windows::fs::FileTypeExt;

pub fn recursive_remove<P: AsRef<Path>>(path: P) -> io::Result<()> {
    let path = path.as_ref();
    let metadata = fs::symlink_metadata(path)?;
    #[cfg(windows)]
    let is_dir = |meta: &fs::Metadata| meta.is_dir() || meta.file_type().is_symlink_dir();
    #[cfg(not(windows))]
    let is_dir = fs::Metadata::is_dir;

    if is_dir(&metadata) { fs::remove_dir_all(path) } else { fs::remove_file(path) }
}

Thanks, that indeed is what I needed to do, just needed to additionally handle the special case of root path being a non-dir read-only entry.

[jieyouxu]

Chris said I can
r? @ChrisDenton

[ChrisDenton]

std should probably make remove_file just work on readonly files. But that's a job for another time.

[jieyouxu]

@bors rollup=iffy (who knows what this will fail on)

[ChrisDenton]

Ok, this version looks good to me. And I do like having a range of tests.

@bors r+

[bors]

📌 Commit 5bb55fe has been approved by ChrisDenton

It is now in the queue for this repository.

[ChrisDenton]

@bors r-

I was so focused on Windows I missed the Linux, ha

[jieyouxu]

I just added a #![allow(unused_variables)] to copy_symlink_raw, let me know if you prefer let _ = ty instead.

[ChrisDenton]

Hm, yeah, I'd prefer let _ in the long run even if it's a bit more verbose. Because when things change it's more obvious what was intended to be ignored and what wasn't.

[jieyouxu]

Changed to use let _ = ty; instead of allow(unused_variables).

[jieyouxu]

@rustbot ready

[ChrisDenton]

@bors r+

[bors]

📌 Commit 4d3bf1f has been approved by ChrisDenton

It is now in the queue for this repository.