run_make_support: rectify symlink handling

Avoid confusing Unix symlinks and Windows symlinks, and since their
semantics are quite different we should avoid trying to make it to
automagic in how symlinks are created and deleted. Notably, the tests
should reflect what type of symlinks are to be created to match what std
does to make it less surprising for test readers.

Author: jieyouxu

src/tools/run-make-support/src/fs.rs renamed to src/tools/run-make-support/src/fs/mod.rs

@@ -1,41 +1,10 @@
 use std::io;
 use std::path::{Path, PathBuf};
 
-// FIXME(jieyouxu): modify create_symlink to panic on windows.
-
-/// Creates a new symlink to a path on the filesystem, adjusting for Windows or Unix.
-#[cfg(target_family = "windows")]
-pub fn create_symlink<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) {
-    if link.as_ref().exists() {
-        std::fs::remove_dir(link.as_ref()).unwrap();
-    }
-    if original.as_ref().is_file() {
-        std::os::windows::fs::symlink_file(original.as_ref(), link.as_ref()).expect(&format!(
-            "failed to create symlink {:?} for {:?}",
-            link.as_ref().display(),
-            original.as_ref().display(),
-        ));
-    } else {
-        std::os::windows::fs::symlink_dir(original.as_ref(), link.as_ref()).expect(&format!(
-            "failed to create symlink {:?} for {:?}",
-            link.as_ref().display(),
-            original.as_ref().display(),
-        ));
-    }
-}
-
-/// Creates a new symlink to a path on the filesystem, adjusting for Windows or Unix.
-#[cfg(target_family = "unix")]
-pub fn create_symlink<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) {
-    if link.as_ref().exists() {
-        std::fs::remove_dir(link.as_ref()).unwrap();
-    }
-    std::os::unix::fs::symlink(original.as_ref(), link.as_ref()).expect(&format!(
-        "failed to create symlink {:?} for {:?}",
-        link.as_ref().display(),
-        original.as_ref().display(),
-    ));
-}
+#[cfg(unix)]
+pub mod unix;
+#[cfg(windows)]
+pub mod windows;
 
 /// Copy a directory into another.
 pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
@@ -50,7 +19,34 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
             if ty.is_dir() {
                 copy_dir_all_inner(entry.path(), dst.join(entry.file_name()))?;
             } else if ty.is_symlink() {
-                copy_symlink(entry.path(), dst.join(entry.file_name()))?;
+                // Traverse symlink once to find path of target entity.
+                let target_path = std::fs::read_link(entry.path())?;
+
+                let new_symlink_path = dst.join(entry.file_name());
+                #[cfg(windows)]
+                {
+                    // Find metadata about target entity (shallow, no further symlink traversal in
+                    // case target is also a symlink).
+                    let target_metadata = std::fs::symlink_metadata(&target_path)?;
+                    let target_file_type = target_metadata.file_type();
+                    if target_file_type.is_dir() {
+                        std::os::windows::fs::symlink_dir(&target_path, new_symlink_path)?;
+                    } else {
+                        // Target may be a file or another symlink, in any case we can use
+                        // `symlink_file` here.
+                        std::os::windows::fs::symlink_file(&target_path, new_symlink_path)?;
+                    }
+                }
+                #[cfg(unix)]
+                {
+                    std::os::unix::fs::symlink(target_path, new_symlink_path)?;
+                }
+                #[cfg(not(any(windows, unix)))]
+                {
+                    // Technically there's also wasi, but I have no clue about wasi symlink
+                    // semantics and which wasi targets / environment support symlinks.
+                    unimplemented!("unsupported target");
+                }
             } else {
                 std::fs::copy(entry.path(), dst.join(entry.file_name()))?;
             }
@@ -69,12 +65,6 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
     }
 }
 
-fn copy_symlink<P: AsRef<Path>, Q: AsRef<Path>>(from: P, to: Q) -> io::Result<()> {
-    let target_path = std::fs::read_link(from).unwrap();
-    create_symlink(target_path, to);
-    Ok(())
-}
-
 /// Helper for reading entries in a given directory.
 pub fn read_dir_entries<P: AsRef<Path>, F: FnMut(&Path)>(dir: P, mut callback: F) {
     for entry in read_dir(dir) {
@@ -85,8 +75,17 @@ pub fn read_dir_entries<P: AsRef<Path>, F: FnMut(&Path)>(dir: P, mut callback: F
 /// A wrapper around [`std::fs::remove_file`] which includes the file path in the panic message.
 #[track_caller]
 pub fn remove_file<P: AsRef<Path>>(path: P) {
-    std::fs::remove_file(path.as_ref())
-        .expect(&format!("the file in path \"{}\" could not be removed", path.as_ref().display()));
+    if let Err(e) = std::fs::remove_file(path.as_ref()) {
+        panic!("failed to remove file at `{}`: {e}", path.as_ref().display());
+    }
+}
+
+/// A wrapper around [`std::fs::remove_dir`] which includes the directory path in the panic message.
+#[track_caller]
+pub fn remove_dir<P: AsRef<Path>>(path: P) {
+    if let Err(e) = std::fs::remove_dir(path.as_ref()) {
+        panic!("failed to remove directory at `{}`: {e}", path.as_ref().display());
+    }
 }
 
 /// A wrapper around [`std::fs::copy`] which includes the file path in the panic message.
@@ -165,13 +164,32 @@ pub fn create_dir_all<P: AsRef<Path>>(path: P) {
     ));
 }
 
-/// A wrapper around [`std::fs::metadata`] which includes the file path in the panic message.
+/// A wrapper around [`std::fs::metadata`] which includes the file path in the panic message. Note
+/// that this will traverse symlinks and will return metadata about the target file. Use
+/// [`symlink_metadata`] if you don't want to traverse symlinks.
+///
+/// See [`std::fs::metadata`] docs for more details.
 #[track_caller]
 pub fn metadata<P: AsRef<Path>>(path: P) -> std::fs::Metadata {
-    std::fs::metadata(path.as_ref()).expect(&format!(
-        "the file's metadata in path \"{}\" could not be read",
-        path.as_ref().display()
-    ))
+    match std::fs::metadata(path.as_ref()) {
+        Ok(m) => m,
+        Err(e) => panic!("failed to read file metadata at `{}`: {e}", path.as_ref().display()),
+    }
+}
+
+/// A wrapper around [`std::fs::symlink_metadata`] which includes the file path in the panic
+/// message. Note that this will not traverse symlinks and will return metadata about the filesystem
+/// entity itself. Use [`metadata`] if you want to traverse symlinks.
+///
+/// See [`std::fs::symlink_metadata`] docs for more details.
+#[track_caller]
+pub fn symlink_metadata<P: AsRef<Path>>(path: P) -> std::fs::Metadata {
+    match std::fs::symlink_metadata(path.as_ref()) {
+        Ok(m) => m,
+        Err(e) => {
+            panic!("failed to read file metadata (shallow) at `{}`: {e}", path.as_ref().display())
+        }
+    }
 }
 
 /// A wrapper around [`std::fs::rename`] which includes the file path in the panic message.

src/tools/run-make-support/src/fs/unix.rs

@@ -0,0 +1,16 @@
+use std::path::Path;
+
+/// Create a new symbolic link (soft link) at `link` pointing to target `original`. Unix symlinks
+/// should be removed via [`fs::remove_file`](crate::fs::remove_file).
+///
+/// This is an infallible wrapper around [`std::os::unix::fs::symlink`], and will panic if we failed
+/// to create a symlink at `link` for target `original`.
+pub fn symlink<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) {
+    if let Err(e) = std::os::unix::fs::symlink(original.as_ref(), link.as_ref()) {
+        panic!(
+            "failed to create symlink: original=`{}`, link=`{}`: {e}",
+            original.as_ref().display(),
+            link.as_ref().display()
+        );
+    }
+}

src/tools/run-make-support/src/fs/windows.rs

@@ -0,0 +1,49 @@
+/// Windows symbolic link helpers. Unlike Unix symbolic links (soft links), on Windows there is a
+/// distinction between symlink-to-file versus symlink-to-directory. In particular, a
+/// symlink-to-file need to be deleted with [`fs::remove_file`](crate::fs::remove_file) while a
+/// symlink-to-dir need to be deleted with [`fs::remove_dir`](crate::fs::remove_dir). See
+/// [`CreateSymbolicLinkW`] for details.
+///
+/// # Notes
+///
+/// - Windows symbolic links are distinct from junction points or hard links.
+/// - A Unix symlink created via WSL but on the Windows host NTFS filesystem need to be deleted with
+///   [`fs::remove_file`](crate::fs::remove_file) even if that symlink may point to a directory.
+/// - Hard links and junction points need additional handling, which is currently not implemented by
+///   this support library.
+///
+/// # References
+///
+/// [`CreateSymbolicLinkW`]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinkw
+pub mod symlinks {
+    use std::path::Path;
+
+    /// Create a new symbolic link to a directory. A symlink-to-directory needs to be removed with a
+    /// corresponding [`fs::remove_dir`](crate::fs::remove_dir) and not
+    /// [`fs::remove_file`](crate::fs::remove_file).
+    pub fn symlink_dir<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) {
+        if let Err(e) = std::os::windows::fs::symlink_dir(original.as_ref(), link.as_ref()) {
+            panic!(
+                "failed to create symlink-to-directory: original=`{}`, link=`{}`: {e}",
+                original.as_ref().display(),
+                link.as_ref().display()
+            );
+        }
+    }
+
+    /// Create a new symbolic link to a file. A symlink-to-file needs to be removed with a
+    /// corresponding [`fs::remove_file`](crate::fs::remove_file) and not
+    /// [`fs::remove_dir`](crate::fs::remove_dir).
+    pub fn symlink_file<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) {
+        if let Err(e) = std::os::windows::fs::symlink_file(original.as_ref(), link.as_ref()) {
+            panic!(
+                "failed to create symlink-to-file: original=`{}`, link=`{}`: {e}",
+                original.as_ref().display(),
+                link.as_ref().display()
+            );
+        }
+    }
+}
+
+// Re-export to flatten module hierarchy, but the module is there for documentation purposes.
+pub use symlinks::{symlink_dir, symlink_file};