Rollup merge of #130427 - jieyouxu:rmake-symlink, r=Kobzol

run_make_support: rectify symlink handling

Avoid confusing Unix symlinks and Windows symlinks. Since their
semantics are quite different, we should avoid trying to make it
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.

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

@@ -1,42 +1,6 @@
 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(),
-    ));
-}
-
 /// Copy a directory into another.
 pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
     fn copy_dir_all_inner(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> io::Result<()> {
@@ -50,7 +14,31 @@ 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)]
+                {
+                    use std::os::windows::fs::FileTypeExt;
+                    if ty.is_symlink_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 +57,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 +67,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())
+    if let Err(e) = std::fs::remove_file(path.as_ref()) {
-        .expect(&format!("the file in path \"{}\" could not be removed", path.as_ref().display()));
+        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 +156,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!(
+    match std::fs::metadata(path.as_ref()) {
-        "the file's metadata in path \"{}\" could not be read",
+        Ok(m) => m,
-        path.as_ref().display()
+        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.
@@ -205,3 +215,73 @@ pub fn shallow_find_dir_entries<P: AsRef<Path>>(dir: P) -> Vec<PathBuf> {
     }
     output
 }
+
+/// Create a new symbolic link to a directory.
+///
+/// # Removing the symlink
+///
+/// - On Windows, a symlink-to-directory needs to be removed with a corresponding [`fs::remove_dir`]
+///   and not [`fs::remove_file`].
+/// - On Unix, remove the symlink with [`fs::remove_file`].
+///
+/// [`fs::remove_dir`]: crate::fs::remove_dir
+/// [`fs::remove_file`]: crate::fs::remove_file
+pub fn symlink_dir<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) {
+    #[cfg(unix)]
+    {
+        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()
+            );
+        }
+    }
+    #[cfg(windows)]
+    {
+        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()
+            );
+        }
+    }
+    #[cfg(not(any(windows, unix)))]
+    {
+        unimplemented!("target family not currently supported")
+    }
+}
+
+/// Create a new symbolic link to a file.
+///
+/// # Removing the symlink
+///
+/// On both Windows and Unix, 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) {
+    #[cfg(unix)]
+    {
+        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()
+            );
+        }
+    }
+    #[cfg(windows)]
+    {
+        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()
+            );
+        }
+    }
+    #[cfg(not(any(windows, unix)))]
+    {
+        unimplemented!("target family not currently supported")
+    }
+}

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

@@ -1,13 +1,14 @@
-// In this test, the symlink created is invalid (valid relative to the root, but not
+// In this test, the symlink created is invalid (valid relative to the root, but not relatively to
-// relatively to where it is located), and used to cause an internal
+// where it is located), and used to cause an internal compiler error (ICE) when passed as a library
-// compiler error (ICE) when passed as a library search path. This was fixed in #26044,
+// search path. This was fixed in #26044, and this test checks that the invalid symlink is instead
-// and this test checks that the invalid symlink is instead simply ignored.
+// simply ignored.
+//
 // See https://github.com/rust-lang/rust/issues/26006
 
 //@ needs-symlink
 //Reason: symlink requires elevated permission in Windows
 
-use run_make_support::{rfs, rustc};
+use run_make_support::{path, rfs, rustc};
 
 fn main() {
     // We create two libs: `bar` which depends on `foo`. We need to compile `foo` first.
@@ -20,9 +21,9 @@ fn main() {
         .metadata("foo")
         .output("out/foo/libfoo.rlib")
         .run();
-    rfs::create_dir("out/bar");
+    rfs::create_dir_all("out/bar/deps");
-    rfs::create_dir("out/bar/deps");
+    rfs::symlink_file(path("out/foo/libfoo.rlib"), path("out/bar/deps/libfoo.rlib"));
-    rfs::create_symlink("out/foo/libfoo.rlib", "out/bar/deps/libfoo.rlib");
+
     // Check that the invalid symlink does not cause an ICE
     rustc()
         .input("in/bar/lib.rs")

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

@@ -1,22 +1,22 @@
-// Crates that are resolved normally have their path canonicalized and all
+// Crates that are resolved normally have their path canonicalized and all symlinks resolved. This
-// symlinks resolved. This did not happen for paths specified
+// did not happen for paths specified using the `--extern` option to rustc, which could lead to
-// using the --extern option to rustc, which could lead to rustc thinking
+// rustc thinking that it encountered two different versions of a crate, when it's actually the same
-// that it encountered two different versions of a crate, when it's
+// version found through different paths.
-// 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.
-
+//
-// This test checks that --extern and symlinks together
+// See <https://github.com/rust-lang/rust/pull/16505>.
-// can result in successful compilation.
 
 //@ ignore-cross-compile
 //@ needs-symlink
 
-use run_make_support::{cwd, rfs, rustc};
+use run_make_support::{cwd, path, rfs, rustc};
 
 fn main() {
     rustc().input("foo.rs").run();
     rfs::create_dir_all("other");
-    rfs::create_symlink("libfoo.rlib", "other");
+    rfs::symlink_file(path("libfoo.rlib"), path("other").join("libfoo.rlib"));
+
     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,15 @@
-// When a directory and a symlink simultaneously exist with the same name,
+// Avoid erroring on symlinks pointing to the same file that are present in the library search path.
-// 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
+// See <https://github.com/rust-lang/rust/issues/12459>.
-// 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
 
 //@ ignore-cross-compile
 //@ needs-symlink
 
-use run_make_support::{dynamic_lib_name, rfs, rustc};
+use run_make_support::{cwd, dynamic_lib_name, path, 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");
+    rfs::symlink_file(dynamic_lib_name("foo"), path("other").join(dynamic_lib_name("foo")));
-    rustc().input("bar.rs").library_search_path("other").run();
+    rustc().input("bar.rs").library_search_path(cwd()).library_search_path("other").run();
 }

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

@@ -12,6 +12,6 @@ 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");
+    rfs::symlink_file("foo.xxx", "libfoo.rlib");
     rustc().input("bar.rs").library_search_path(cwd()).run();
 }