From 1b8634021875d26888550160c4bcc274848f9ef7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Mon, 16 Sep 2024 11:53:52 +0800 Subject: [PATCH 1/2] 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. --- src/tools/run-make-support/src/fs.rs | 180 +++++++++++++++++++-------- 1 file changed, 130 insertions(+), 50 deletions(-) diff --git a/src/tools/run-make-support/src/fs.rs b/src/tools/run-make-support/src/fs.rs index 2c35ba52a62..2ca55ad3b3a 100644 --- a/src/tools/run-make-support/src/fs.rs +++ b/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, Q: AsRef>(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, Q: AsRef>(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, dst: impl AsRef) { fn copy_dir_all_inner(src: impl AsRef, dst: impl AsRef) -> io::Result<()> { @@ -50,7 +14,31 @@ pub fn copy_dir_all(src: impl AsRef, dst: impl AsRef) { 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, dst: impl AsRef) { } } -fn copy_symlink, Q: AsRef>(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, F: FnMut(&Path)>(dir: P, mut callback: F) { for entry in read_dir(dir) { @@ -85,8 +67,17 @@ pub fn read_dir_entries, 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>(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>(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>(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>(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>(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>(dir: P) -> Vec { } 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, Q: AsRef>(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, Q: AsRef>(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") + } +} From 7d764283b4c4fa2f6fec590a1c5db1dfc98fc449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Mon, 16 Sep 2024 14:48:47 +0800 Subject: [PATCH 2/2] tests/run-make: update for symlink helper changes --- .../invalid-symlink-search-path/rmake.rs | 17 ++++++------- tests/run-make/symlinked-extern/rmake.rs | 24 +++++++++---------- tests/run-make/symlinked-libraries/rmake.rs | 15 +++++------- tests/run-make/symlinked-rlib/rmake.rs | 2 +- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/tests/run-make/invalid-symlink-search-path/rmake.rs b/tests/run-make/invalid-symlink-search-path/rmake.rs index ed2cd9c4bd2..7b7e7c79442 100644 --- a/tests/run-make/invalid-symlink-search-path/rmake.rs +++ b/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 -// 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 //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("out/bar/deps"); - rfs::create_symlink("out/foo/libfoo.rlib", "out/bar/deps/libfoo.rlib"); + rfs::create_dir_all("out/bar/deps"); + rfs::symlink_file(path("out/foo/libfoo.rlib"), path("out/bar/deps/libfoo.rlib")); + // Check that the invalid symlink does not cause an ICE rustc() .input("in/bar/lib.rs") diff --git a/tests/run-make/symlinked-extern/rmake.rs b/tests/run-make/symlinked-extern/rmake.rs index 7a4a8ce18cb..f35d5a13f5a 100644 --- a/tests/run-make/symlinked-extern/rmake.rs +++ b/tests/run-make/symlinked-extern/rmake.rs @@ -1,22 +1,22 @@ -// 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 . //@ 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(); } diff --git a/tests/run-make/symlinked-libraries/rmake.rs b/tests/run-make/symlinked-libraries/rmake.rs index e6449b61a1c..98f156fed8f 100644 --- a/tests/run-make/symlinked-libraries/rmake.rs +++ b/tests/run-make/symlinked-libraries/rmake.rs @@ -1,18 +1,15 @@ -// 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 . //@ 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"); - rustc().input("bar.rs").library_search_path("other").run(); + rfs::symlink_file(dynamic_lib_name("foo"), path("other").join(dynamic_lib_name("foo"))); + rustc().input("bar.rs").library_search_path(cwd()).library_search_path("other").run(); } diff --git a/tests/run-make/symlinked-rlib/rmake.rs b/tests/run-make/symlinked-rlib/rmake.rs index 10ba6ba7cbb..fee432e419e 100644 --- a/tests/run-make/symlinked-rlib/rmake.rs +++ b/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(); }