Auto merge of rust-lang#125736 - Oneirical:run-make-file-management, r=<try>

run-make-support: add helpers for `tmp_dir().join()` and `fs` operations

Suggested by rust-lang#125728.

The point of this wrapper is to stop silent fails caused by forgetting to `unwrap` `fs` functions. However, functions like `fs::read` which return something and get stored in a variable should cause a failure on their own if they are not unwrapped (as the `Result` will be stored in the variable, and something will be done on that `Result` that should have been done to its contents). Is it still pertinent to wrap `fs::read_to_string`, `fs::metadata` and so on?

try-job: x86_64-msvc
try-job: i686-mingw

Author: bors

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

@@ -2,7 +2,8 @@ use std::path::Path;
 use std::process::Command;
 
 use crate::{
-    bin_name, cygpath_windows, env_var, handle_failed_output, is_msvc, is_windows, tmp_dir, uname,
+    bin_name, cygpath_windows, env_var, handle_failed_output, is_msvc, is_windows, rmake_out_path,
+    uname,
 };
 
 /// Construct a new platform-specific C compiler invocation.
@@ -69,13 +70,13 @@ impl Cc {
         // ```
 
         if is_msvc() {
-            let fe_path = cygpath_windows(tmp_dir().join(bin_name(name)));
-            let fo_path = cygpath_windows(tmp_dir().join(format!("{name}.obj")));
+            let fe_path = cygpath_windows(rmake_out_path(bin_name(name)));
+            let fo_path = cygpath_windows(rmake_out_path(format!("{name}.obj")));
             self.cmd.arg(format!("-Fe:{fe_path}"));
             self.cmd.arg(format!("-Fo:{fo_path}"));
         } else {
             self.cmd.arg("-o");
-            self.cmd.arg(tmp_dir().join(name));
+            self.cmd.arg(rmake_out_path(name));
         }
 
         self

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

@@ -1,7 +1,7 @@
 use std::path::Path;
 use std::process::Command;
 
-use crate::{bin_name, env_var, handle_failed_output, tmp_dir};
+use crate::{bin_name, env_var, handle_failed_output, rmake_out_path};
 
 /// Construct a new `clang` invocation. `clang` is not always available for all targets.
 pub fn clang() -> Clang {
@@ -34,7 +34,7 @@ impl Clang {
     /// extension will be determined by [`bin_name`].
     pub fn out_exe(&mut self, name: &str) -> &mut Self {
         self.cmd.arg("-o");
-        self.cmd.arg(tmp_dir().join(bin_name(name)));
+        self.cmd.arg(rmake_out_path(bin_name(name)));
         self
     }
 

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

@@ -0,0 +1,98 @@
+use std::fs;
+use std::path::Path;
+
+/// A safe wrapper around `std::fs::remove_file` that prints the file path if an operation fails.
+pub fn remove_file<P: AsRef<Path>>(path: P) {
+    fs::remove_file(path.as_ref()).expect(&format!(
+        "the file in path \"{:?}\" could not be removed.",
+        path.as_ref().display()
+    ));
+}
+
+/// A safe wrapper around `std::fs::copy` that prints the file paths if an operation fails.
+pub fn copy<P: AsRef<Path>, Q: AsRef<Path>>(from: P, to: Q) {
+    fs::copy(from.as_ref(), to.as_ref()).expect(&format!(
+        "the file \"{:?}\" could not be copied over to \"{:?}\".",
+        from.as_ref().display(),
+        to.as_ref().display(),
+    ));
+}
+
+/// A safe wrapper around `std::fs::File::create` that prints the file path if an operation fails.
+pub fn create_file<P: AsRef<Path>>(path: P) {
+    fs::File::create(path.as_ref()).expect(&format!(
+        "the file in path \"{:?}\" could not be created.",
+        path.as_ref().display()
+    ));
+}
+
+/// A safe wrapper around `std::fs::read` that prints the file path if an operation fails.
+pub fn read<P: AsRef<Path>>(path: P) -> Vec<u8> {
+    fs::read(path.as_ref())
+        .expect(&format!("the file in path \"{:?}\" could not be read.", path.as_ref().display()))
+}
+
+/// A safe wrapper around `std::fs::read_to_string` that prints the file path if an operation fails.
+pub fn read_to_string<P: AsRef<Path>>(path: P) -> String {
+    fs::read_to_string(path.as_ref()).expect(&format!(
+        "the file in path \"{:?}\" could not be read into a String.",
+        path.as_ref().display()
+    ))
+}
+
+/// A safe wrapper around `std::fs::read_dir` that prints the file path if an operation fails.
+pub fn read_dir<P: AsRef<Path>>(path: P) -> fs::ReadDir {
+    fs::read_dir(path.as_ref()).expect(&format!(
+        "the directory in path \"{:?}\" could not be read.",
+        path.as_ref().display()
+    ))
+}
+
+/// A safe wrapper around `std::fs::write` that prints the file path if an operation fails.
+pub fn write<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) {
+    fs::write(path.as_ref(), contents.as_ref()).expect(&format!(
+        "the file in path \"{:?}\" could not be written to.",
+        path.as_ref().display()
+    ));
+}
+
+/// A safe wrapper around `std::fs::remove_dir_all` that prints the file path if an operation fails.
+pub fn remove_dir_all<P: AsRef<Path>>(path: P) {
+    fs::remove_dir_all(path.as_ref()).expect(&format!(
+        "the directory in path \"{:?}\" could not be removed alongside all its contents.",
+        path.as_ref().display(),
+    ));
+}
+
+/// A safe wrapper around `std::fs::create_dir` that prints the file path if an operation fails.
+pub fn create_dir<P: AsRef<Path>>(path: P) {
+    fs::create_dir(path.as_ref()).expect(&format!(
+        "the directory in path \"{:?}\" could not be created.",
+        path.as_ref().display()
+    ));
+}
+
+/// A safe wrapper around `std::fs::create_dir_all` that prints the file path if an operation fails.
+pub fn create_dir_all<P: AsRef<Path>>(path: P) {
+    fs::create_dir_all(path.as_ref()).expect(&format!(
+        "the directory (and all its parents) in path \"{:?}\" could not be created.",
+        path.as_ref().display()
+    ));
+}
+
+/// A safe wrapper around `std::fs::metadata` that prints the file path if an operation fails.
+pub fn metadata<P: AsRef<Path>>(path: P) -> fs::Metadata {
+    fs::metadata(path.as_ref()).expect(&format!(
+        "the file's metadata in path \"{:?}\" could not be read.",
+        path.as_ref().display()
+    ))
+}
+
+/// A safe wrapper around `std::fs::rename` that prints the file paths if an operation fails.
+pub fn rename<P: AsRef<Path>, Q: AsRef<Path>>(from: P, to: Q) {
+    fs::rename(from.as_ref(), to.as_ref()).expect(&format!(
+        "the file \"{:?}\" could not be moved over to \"{:?}\".",
+        from.as_ref().display(),
+        to.as_ref().display(),
+    ));
+}

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

@@ -6,14 +6,14 @@
 pub mod cc;
 pub mod clang;
 pub mod diff;
+pub mod fs_wrapper;
 pub mod llvm_readobj;
 pub mod run;
 pub mod rustc;
 pub mod rustdoc;
 
 use std::env;
 use std::ffi::OsString;
-use std::fs;
 use std::io;
 use std::path::{Path, PathBuf};
 use std::process::{Command, Output};
@@ -46,10 +46,15 @@ pub fn env_var_os(name: &str) -> OsString {
 }
 
 /// Path of `TMPDIR` (a temporary build directory, not under `/tmp`).
-pub fn tmp_dir() -> PathBuf {
+pub fn rmake_out_dir() -> PathBuf {
     env_var_os("TMPDIR").into()
 }
 
+/// Returns the directory TMPDIR/name.
+pub fn rmake_out_path<P: AsRef<Path>>(name: P) -> PathBuf {
+    rmake_out_dir().join(name.as_ref())
+}
+
 /// `TARGET`
 pub fn target() -> String {
     env_var("TARGET")
@@ -73,7 +78,7 @@ pub fn is_darwin() -> bool {
 /// Construct a path to a static library under `$TMPDIR` given the library name. This will return a
 /// path with `$TMPDIR` joined with platform-and-compiler-specific library name.
 pub fn static_lib(name: &str) -> PathBuf {
-    tmp_dir().join(static_lib_name(name))
+    rmake_out_path(static_lib_name(name))
 }
 
 pub fn python_command() -> Command {
@@ -118,7 +123,7 @@ pub fn static_lib_name(name: &str) -> String {
 /// Construct a path to a dynamic library under `$TMPDIR` given the library name. This will return a
 /// path with `$TMPDIR` joined with platform-and-compiler-specific library name.
 pub fn dynamic_lib(name: &str) -> PathBuf {
-    tmp_dir().join(dynamic_lib_name(name))
+    rmake_out_path(dynamic_lib_name(name))
 }
 
 /// Construct the dynamic library name based on the platform.
@@ -161,7 +166,7 @@ pub fn dynamic_lib_extension() -> &'static str {
 /// Construct a path to a rust library (rlib) under `$TMPDIR` given the library name. This will return a
 /// path with `$TMPDIR` joined with the library name.
 pub fn rust_lib(name: &str) -> PathBuf {
-    tmp_dir().join(rust_lib_name(name))
+    rmake_out_path(rust_lib_name(name))
 }
 
 /// Generate the name a rust library (rlib) would have. If you want the complete path, use
@@ -240,15 +245,15 @@ 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<()> {
         let dst = dst.as_ref();
         if !dst.is_dir() {
-            fs::create_dir_all(&dst)?;
+            std::fs::create_dir_all(&dst)?;
         }
-        for entry in fs::read_dir(src)? {
+        for entry in std::fs::read_dir(src)? {
             let entry = entry?;
             let ty = entry.file_type()?;
             if ty.is_dir() {
                 copy_dir_all_inner(entry.path(), dst.join(entry.file_name()))?;
             } else {
-                fs::copy(entry.path(), dst.join(entry.file_name()))?;
+                std::fs::copy(entry.path(), dst.join(entry.file_name()))?;
             }
         }
         Ok(())
@@ -267,22 +272,15 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) {
 
 /// Check that all files in `dir1` exist and have the same content in `dir2`. Panic otherwise.
 pub fn recursive_diff(dir1: impl AsRef<Path>, dir2: impl AsRef<Path>) {
-    fn read_file(path: &Path) -> Vec<u8> {
-        match fs::read(path) {
-            Ok(c) => c,
-            Err(e) => panic!("Failed to read `{}`: {:?}", path.display(), e),
-        }
-    }
-
     let dir2 = dir2.as_ref();
     read_dir(dir1, |entry_path| {
         let entry_name = entry_path.file_name().unwrap();
         if entry_path.is_dir() {
             recursive_diff(&entry_path, &dir2.join(entry_name));
         } else {
             let path2 = dir2.join(entry_name);
-            let file1 = read_file(&entry_path);
-            let file2 = read_file(&path2);
+            let file1 = fs_wrapper::read(&entry_path);
+            let file2 = fs_wrapper::read(&path2);
 
             // We don't use `assert_eq!` because they are `Vec<u8>`, so not great for display.
             // Why not using String? Because there might be minified files or even potentially
@@ -298,7 +296,7 @@ pub fn recursive_diff(dir1: impl AsRef<Path>, dir2: impl AsRef<Path>) {
 }
 
 pub fn read_dir<F: Fn(&Path)>(dir: impl AsRef<Path>, callback: F) {
-    for entry in fs::read_dir(dir).unwrap() {
+    for entry in fs_wrapper::read_dir(dir) {
         callback(&entry.unwrap().path());
     }
 }

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

@@ -3,7 +3,7 @@ use std::io::Write;
 use std::path::Path;
 use std::process::{Command, Output, Stdio};
 
-use crate::{env_var, handle_failed_output, set_host_rpath, tmp_dir};
+use crate::{env_var, handle_failed_output, rmake_out_dir, set_host_rpath};
 
 /// Construct a new `rustc` invocation.
 pub fn rustc() -> Rustc {
@@ -28,7 +28,7 @@ fn setup_common() -> Command {
     let rustc = env_var("RUSTC");
     let mut cmd = Command::new(rustc);
     set_host_rpath(&mut cmd);
-    cmd.arg("--out-dir").arg(tmp_dir()).arg("-L").arg(tmp_dir());
+    cmd.arg("--out-dir").arg(rmake_out_dir()).arg("-L").arg(rmake_out_dir());
     cmd
 }
 

tests/run-make/artifact-incr-cache-no-obj/rmake.rs

@@ -5,10 +5,10 @@
 //
 // Fixes: rust-lang/rust#123234
 
-use run_make_support::{rustc, tmp_dir};
+use run_make_support::{rmake_out_dir, rustc};
 
 fn main() {
-    let inc_dir = tmp_dir();
+    let inc_dir = rmake_out_dir();
 
     for _ in 0..=1 {
         rustc()

tests/run-make/artifact-incr-cache/rmake.rs

@@ -7,10 +7,10 @@
 // Also see discussion at
 // <https://internals.rust-lang.org/t/interaction-between-incremental-compilation-and-emit/20551>
 
-use run_make_support::{rustc, tmp_dir};
+use run_make_support::{rmake_out_dir, rustc};
 
 fn main() {
-    let inc_dir = tmp_dir();
+    let inc_dir = rmake_out_dir();
 
     for _ in 0..=1 {
         rustc()

tests/run-make/bare-outfile/rmake.rs

@@ -3,13 +3,13 @@
 
 //@ ignore-cross-compile
 
-use run_make_support::{run, rustc, tmp_dir};
+use run_make_support::fs_wrapper;
+use run_make_support::{rmake_out_dir, rmake_out_path, run, rustc};
 use std::env;
-use std::fs;
 
 fn main() {
-    fs::copy("foo.rs", tmp_dir().join("foo.rs")).unwrap();
-    env::set_current_dir(tmp_dir());
+    fs_wrapper::copy("foo.rs", rmake_out_path("foo.rs"));
+    env::set_current_dir(rmake_out_dir()).unwrap();
     rustc().output("foo").input("foo.rs").run();
     run("foo");
 }

tests/run-make/box-struct-no-segfault/rmake.rs

@@ -5,9 +5,9 @@
 // This test checks that this bug does not resurface.
 // See https://github.com/rust-lang/rust/issues/28766
 
-use run_make_support::{rustc, tmp_dir};
+use run_make_support::{rmake_out_dir, rustc};
 
 fn main() {
     rustc().opt().input("foo.rs").run();
-    rustc().opt().library_search_path(tmp_dir()).input("main.rs").run();
+    rustc().opt().library_search_path(rmake_out_dir()).input("main.rs").run();
 }

tests/run-make/c-link-to-rust-dylib/rmake.rs

@@ -6,28 +6,28 @@
 use std::fs::remove_file;
 
 use run_make_support::{
-    cc, dynamic_lib_extension, is_msvc, read_dir, run, run_fail, rustc, tmp_dir,
+    cc, dynamic_lib_extension, is_msvc, read_dir, rmake_out_dir, run, run_fail, rustc,
 };
 
 fn main() {
     rustc().input("foo.rs").run();
 
     if is_msvc() {
-        let lib = tmp_dir().join("foo.dll.lib");
+        let lib = rmake_out_dir().join("foo.dll.lib");
 
         cc().input("bar.c").arg(lib).out_exe("bar").run();
     } else {
         cc().input("bar.c")
             .arg("-lfoo")
-            .output(tmp_dir().join("bar"))
-            .library_search_path(tmp_dir())
+            .output(rmake_out_dir().join("bar"))
+            .library_search_path(rmake_out_dir())
             .run();
     }
 
     run("bar");
 
     let expected_extension = dynamic_lib_extension();
-    read_dir(tmp_dir(), |path| {
+    read_dir(rmake_out_dir(), |path| {
         if path.is_file()
             && path.extension().is_some_and(|ext| ext == expected_extension)
             && path.file_name().and_then(|name| name.to_str()).is_some_and(|name| {

tests/run-make/c-link-to-rust-staticlib/rmake.rs

@@ -3,13 +3,14 @@
 
 //@ ignore-cross-compile
 
+use run_make_support::fs_wrapper::remove_file;
 use run_make_support::{cc, extra_c_flags, run, rustc, static_lib};
 use std::fs;
 
 fn main() {
     rustc().input("foo.rs").run();
     cc().input("bar.c").input(static_lib("foo")).out_exe("bar").args(&extra_c_flags()).run();
     run("bar");
-    fs::remove_file(static_lib("foo"));
+    remove_file(static_lib("foo"));
     run("bar");
 }