Auto merge of rust-lang#130427 - jieyouxu:rmake-symlink, r=<try>

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.

r? `@ghost`

try-job: x86_64-msvc
try-job: aarch64-apple

Author: bors

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::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, link) {
+        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};

tests/run-make/invalid-symlink-search-path/rmake.rs

@@ -1,7 +1,8 @@
-// In this test, the symlink created is invalid (valid relative to the root, but not
-// relatively to where it is located), and used to cause an internal
-// compiler error (ICE) when passed as a library search path. This was fixed in #26044,
-// and this test checks that the invalid symlink is instead simply ignored.
+// In this test, the symlink created is invalid (valid relative to the root, but not relatively to
+// where it is located), and used to cause an internal compiler error (ICE) when passed as a library
+// search path. This was fixed in #26044, and this test checks that the invalid symlink is instead
+// simply ignored.
+//
 // See https://github.com/rust-lang/rust/issues/26006
 
 //@ needs-symlink
@@ -22,7 +23,22 @@ fn main() {
         .run();
     rfs::create_dir("out/bar");
     rfs::create_dir("out/bar/deps");
-    rfs::create_symlink("out/foo/libfoo.rlib", "out/bar/deps/libfoo.rlib");
+    #[cfg(unix)]
+    {
+        rfs::unix::symlink("out/foo/libfoo.rlib", "out/bar/deps/libfoo.rlib");
+    }
+    #[cfg(windows)]
+    {
+        rfs::windows::symlink_file("out/foo/libfoo.rlib", "out/bar/deps/libfoo.rlib");
+    }
+    #[cfg(not(any(unix, windows)))]
+    {
+        // FIXME(jieyouxu): this is not supported, but we really should implement a only-* directive
+        // that accepts multiple targets, like e.g. `//@ only-target-families: unix, windows`. That
+        // way, it properly shows up as an ignored test instead of silently passing.
+        return;
+    }
+
     // Check that the invalid symlink does not cause an ICE
     rustc()
         .input("in/bar/lib.rs")

tests/run-make/symlinked-extern/rmake.rs

@@ -1,12 +1,11 @@
-// Crates that are resolved normally have their path canonicalized and all
-// symlinks resolved. This did not happen for paths specified
-// using the --extern option to rustc, which could lead to rustc thinking
-// that it encountered two different versions of a crate, when it's
-// actually the same version found through different paths.
-// See https://github.com/rust-lang/rust/pull/16505
-
-// This test checks that --extern and symlinks together
-// can result in successful compilation.
+// Crates that are resolved normally have their path canonicalized and all symlinks resolved. This
+// did not happen for paths specified using the `--extern` option to rustc, which could lead to
+// rustc thinking that it encountered two different versions of a crate, when it's actually the same
+// version found through different paths.
+//
+// This test checks that `--extern` and symlinks together can result in successful compilation.
+//
+// See <https://github.com/rust-lang/rust/pull/16505>.
 
 //@ ignore-cross-compile
 //@ needs-symlink
@@ -16,7 +15,26 @@ use run_make_support::{cwd, rfs, rustc};
 fn main() {
     rustc().input("foo.rs").run();
     rfs::create_dir_all("other");
-    rfs::create_symlink("libfoo.rlib", "other");
+    #[cfg(unix)]
+    {
+        rfs::unix::symlink(cwd().join("libfoo.rlib"), cwd().join("other").join("libfoo.rlib"));
+    }
+    #[cfg(windows)]
+    {
+        rfs::windows::symlink_file(
+            cwd().join("libfoo.rlib"),
+            cwd().join("other").join("libfoo.rlib"),
+        );
+    }
+    #[cfg(not(any(unix, windows)))]
+    {
+        // FIXME(jieyouxu): this is not supported, but we really should implement a only-* directive
+        // that accepts multiple targets, like e.g. `//@ only-target-families: unix, windows`. That
+        // way, it properly shows up as an ignored test instead of silently passing.
+        return;
+    }
+
     rustc().input("bar.rs").library_search_path(cwd()).run();
-    rustc().input("baz.rs").extern_("foo", "other").library_search_path(cwd()).run();
+
+    rustc().input("baz.rs").extern_("foo", "other/libfoo.rlib").library_search_path(cwd()).run();
 }

tests/run-make/symlinked-libraries/rmake.rs

@@ -1,18 +1,37 @@
-// When a directory and a symlink simultaneously exist with the same name,
-// setting that name as the library search path should not cause rustc
-// to avoid looking in the symlink and cause an error. This test creates
-// a directory and a symlink named "other", and places the library in the symlink.
-// If it succeeds, the library was successfully found.
-// See https://github.com/rust-lang/rust/issues/12459
+// Avoid erroring on symlinks pointing to the same file that are present in the library search path.
+//
+// See <https://github.com/rust-lang/rust/issues/12459>.
 
 //@ ignore-cross-compile
 //@ needs-symlink
 
-use run_make_support::{dynamic_lib_name, rfs, rustc};
+use run_make_support::{cwd, dynamic_lib_name, rfs, rustc};
 
 fn main() {
     rustc().input("foo.rs").arg("-Cprefer-dynamic").run();
     rfs::create_dir_all("other");
-    rfs::create_symlink(dynamic_lib_name("foo"), "other");
-    rustc().input("bar.rs").library_search_path("other").run();
+
+    #[cfg(unix)]
+    {
+        rfs::unix::symlink(
+            cwd().join(dynamic_lib_name("foo")),
+            cwd().join("other").join(dynamic_lib_name("foo")),
+        );
+    }
+    #[cfg(windows)]
+    {
+        rfs::windows::symlink_file(
+            cwd().join(dynamic_lib_name("foo")),
+            cwd().join("other").join(dynamic_lib_name("foo")),
+        );
+    }
+    #[cfg(not(any(unix, windows)))]
+    {
+        // FIXME(jieyouxu): this is not supported, but we really should implement a only-* directive
+        // that accepts multiple targets, like e.g. `//@ only-target-families: unix, windows`. That
+        // way, it properly shows up as an ignored test instead of silently passing.
+        return;
+    }
+
+    rustc().input("bar.rs").library_search_path(cwd()).library_search_path("other").run();
 }

tests/run-make/symlinked-rlib/rmake.rs

@@ -12,6 +12,20 @@ use run_make_support::{cwd, rfs, rustc};
 
 fn main() {
     rustc().input("foo.rs").crate_type("rlib").output("foo.xxx").run();
-    rfs::create_symlink("foo.xxx", "libfoo.rlib");
+    #[cfg(unix)]
+    {
+        rfs::unix::symlink("foo.xxx", "libfoo.rlib");
+    }
+    #[cfg(windows)]
+    {
+        rfs::windows::symlink_file("foo.xxx", "libfoo.rlib");
+    }
+    #[cfg(not(any(unix, windows)))]
+    {
+        // FIXME(jieyouxu): this is not supported, but we really should implement a only-* directive
+        // that accepts multiple targets, like e.g. `//@ only-target-families: unix, windows`. That
+        // way, it properly shows up as an ignored test instead of silently passing.
+        return;
+    }
     rustc().input("bar.rs").library_search_path(cwd()).run();
 }