Rollup merge of #112084 - ozkanonur:improvements, r=clubby789

enhancements on  build_helper utilization and rustdoc-gui-test

This change provides codebase improvements, resolves `FIXME` in `rustdoc-gui-test` and makes `rustdoc-gui` test able to find local `node_modules` directory outside of the source root.

Cargo.lock

@@ -4289,6 +4289,7 @@ dependencies = [
 name = "rustdoc-gui-test"
 version = "0.1.0"
 dependencies = [
+ "build_helper",
  "compiletest",
  "getopts",
  "walkdir",

src/bootstrap/builder.rs

@@ -381,7 +381,7 @@ fn run(v: &[StepDescription], builder: &Builder<'_>, paths: &[PathBuf]) {
             eprintln!(
                 "note: if you are adding a new Step to bootstrap itself, make sure you register it with `describe!`"
             );
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
     }
 }
@@ -1355,7 +1355,7 @@ pub fn cargo(
                         "error: `x.py clippy` requires a host `rustc` toolchain with the `clippy` component"
                     );
                     eprintln!("help: try `rustup component add clippy`");
-                    crate::detail_exit(1);
+                    crate::detail_exit_macro!(1);
                 });
                 if !t!(std::str::from_utf8(&output.stdout)).contains("nightly") {
                     rustflags.arg("--cfg=bootstrap");

src/bootstrap/compile.rs

@@ -1686,7 +1686,7 @@ pub fn run_cargo(
     });
 
     if !ok {
-        crate::detail_exit(1);
+        crate::detail_exit_macro!(1);
     }
 
     // Ok now we need to actually find all the files listed in `toplevel`. We've

src/bootstrap/config.rs

@@ -23,6 +23,7 @@
 pub use crate::flags::Subcommand;
 use crate::flags::{Color, Flags, Warnings};
 use crate::util::{exe, output, t};
+use build_helper::detail_exit_macro;
 use once_cell::sync::OnceCell;
 use serde::{Deserialize, Deserializer};
 use serde_derive::Deserialize;
@@ -579,7 +580,7 @@ fn merge(&mut self, other: Self, replace: ReplaceOpt) {
                                         panic!("overriding existing option")
                                     } else {
                                         eprintln!("overriding existing option: `{}`", stringify!($field));
-                                        crate::detail_exit(2);
+                                        detail_exit_macro!(2);
                                     }
                                 } else {
                                     self.$field = other.$field;
@@ -678,7 +679,7 @@ fn merge(&mut self, other: Self, replace: ReplaceOpt) {
                             panic!("overriding existing option")
                         } else {
                             eprintln!("overriding existing option");
-                            crate::detail_exit(2);
+                            detail_exit_macro!(2);
                         }
                     } else {
                         *self = other;
@@ -944,7 +945,7 @@ fn get_toml(file: &Path) -> TomlConfig {
                 .and_then(|table: toml::Value| TomlConfig::deserialize(table))
                 .unwrap_or_else(|err| {
                     eprintln!("failed to parse TOML configuration '{}': {err}", file.display());
-                    crate::detail_exit(2);
+                    detail_exit_macro!(2);
                 })
         }
         Self::parse_inner(args, get_toml)
@@ -978,7 +979,7 @@ fn parse_inner(args: &[String], get_toml: impl Fn(&Path) -> TomlConfig) -> Confi
             eprintln!(
                 "Cannot use both `llvm_bolt_profile_generate` and `llvm_bolt_profile_use` at the same time"
             );
-            crate::detail_exit(1);
+            detail_exit_macro!(1);
         }
 
         // Infer the rest of the configuration.
@@ -1094,7 +1095,7 @@ fn get_table(option: &str) -> Result<TomlConfig, toml::de::Error> {
                 }
             }
             eprintln!("failed to parse override `{option}`: `{err}");
-            crate::detail_exit(2)
+            detail_exit_macro!(2)
         }
         toml.merge(override_toml, ReplaceOpt::Override);
 
@@ -1810,7 +1811,7 @@ fn download_ci_rustc_commit(&self, download_rustc: Option<StringOrBool>) -> Opti
             println!("help: maybe your repository history is too shallow?");
             println!("help: consider disabling `download-rustc`");
             println!("help: or fetch enough history to include one upstream commit");
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
 
         // Warn if there were changes to the compiler or standard library since the ancestor commit.

src/bootstrap/download.rs

@@ -7,6 +7,7 @@
     process::{Command, Stdio},
 };
 
+use build_helper::util::try_run;
 use once_cell::sync::OnceCell;
 use xz2::bufread::XzDecoder;
 
@@ -14,7 +15,7 @@
     config::RustfmtMetadata,
     llvm::detect_llvm_sha,
     t,
-    util::{check_run, exe, program_out_of_date, try_run},
+    util::{check_run, exe, program_out_of_date},
     Config,
 };
 
@@ -245,7 +246,7 @@ fn download_http_with_retries(&self, tempfile: &Path, url: &str, help_on_error:
             if !help_on_error.is_empty() {
                 eprintln!("{}", help_on_error);
             }
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
     }
 

src/bootstrap/flags.rs

@@ -193,7 +193,7 @@ struct HelpVerboseOnly {
             } else {
                 panic!("No paths available for subcommand `{}`", subcommand.as_str());
             }
-            crate::detail_exit(0);
+            crate::detail_exit_macro!(0);
         }
 
         Flags::parse_from(it)
@@ -538,7 +538,7 @@ pub fn get_completion<G: clap_complete::Generator>(shell: G, path: &Path) -> Opt
     } else {
         std::fs::read_to_string(path).unwrap_or_else(|_| {
             eprintln!("couldn't read {}", path.display());
-            crate::detail_exit(1)
+            crate::detail_exit_macro!(1)
         })
     };
     let mut buf = Vec::new();

src/bootstrap/format.rs

@@ -40,7 +40,7 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
                         code, run `./x.py fmt` instead.",
                 cmd_debug,
             );
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
         true
     }
@@ -196,7 +196,7 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
 
     let rustfmt_path = build.initial_rustfmt().unwrap_or_else(|| {
         eprintln!("./x.py fmt is not supported on this channel");
-        crate::detail_exit(1);
+        crate::detail_exit_macro!(1);
     });
     assert!(rustfmt_path.exists(), "{}", rustfmt_path.display());
     let src = build.src.clone();

src/bootstrap/lib.rs

@@ -27,6 +27,7 @@
 use std::str;
 
 use build_helper::ci::{gha, CiEnv};
+use build_helper::detail_exit_macro;
 use channel::GitInfo;
 use config::{DryRun, Target};
 use filetime::FileTime;
@@ -699,7 +700,7 @@ pub fn build(&mut self) {
             for failure in failures.iter() {
                 eprintln!("  - {}\n", failure);
             }
-            detail_exit(1);
+            detail_exit_macro!(1);
         }
 
         #[cfg(feature = "build-metrics")]
@@ -1482,7 +1483,7 @@ fn read_stamp_file(&self, stamp: &Path) -> Vec<(PathBuf, DependencyType)> {
                 "Error: Unable to find the stamp file {}, did you try to keep a nonexistent build stage?",
                 stamp.display()
             );
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
 
         let mut paths = Vec::new();
@@ -1674,7 +1675,7 @@ fn ninja(&self) -> bool {
 to download LLVM rather than building it.
 "
                 );
-                detail_exit(1);
+                detail_exit_macro!(1);
             }
         }
 
@@ -1739,18 +1740,6 @@ fn chmod(path: &Path, perms: u32) {
 #[cfg(windows)]
 fn chmod(_path: &Path, _perms: u32) {}
 
-/// If code is not 0 (successful exit status), exit status is 101 (rust's default error code.)
-/// If the test is running and code is an error code, it will cause a panic.
-fn detail_exit(code: i32) -> ! {
-    // if in test and code is an error code, panic with status code provided
-    if cfg!(test) {
-        panic!("status code: {}", code);
-    } else {
-        // otherwise,exit with provided status code
-        std::process::exit(code);
-    }
-}
-
 impl Compiler {
     pub fn with_stage(mut self, stage: u32) -> Compiler {
         self.stage = stage;

src/bootstrap/render_tests.rs

@@ -30,7 +30,7 @@ pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool {
 
     if !run_tests(builder, cmd) {
         if builder.fail_fast {
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         } else {
             let mut failures = builder.delayed_failures.borrow_mut();
             failures.push(format!("{cmd:?}"));

src/bootstrap/sanity.rs

@@ -104,7 +104,7 @@ pub fn check(build: &mut Build) {
 than building it.
 "
             );
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
     }
 

src/bootstrap/setup.rs

@@ -194,7 +194,7 @@ fn setup_config_toml(path: &PathBuf, profile: Profile, config: &Config) {
             "note: this will use the configuration in {}",
             profile.include_path(&config.src).display()
         );
-        crate::detail_exit(1);
+        crate::detail_exit_macro!(1);
     }
 
     let settings = format!(
@@ -380,7 +380,7 @@ fn parse_with_abbrev(input: &str) -> Result<Profile, String> {
         io::stdin().read_line(&mut input)?;
         if input.is_empty() {
             eprintln!("EOF on stdin, when expecting answer to question.  Giving up.");
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
         break match parse_with_abbrev(&input) {
             Ok(profile) => profile,

src/bootstrap/test.rs

@@ -773,7 +773,7 @@ fn run(self, builder: &Builder<'_>) {
         }
 
         if !builder.config.cmd.bless() {
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
 
         let mut cargo = builder.cargo(compiler, Mode::ToolRustc, SourceType::InTree, host, "run");
@@ -1085,7 +1085,7 @@ fn run(self, builder: &Builder<'_>) {
                     PATH = inferred_rustfmt_dir.display(),
                     CHAN = builder.config.channel,
                 );
-                crate::detail_exit(1);
+                crate::detail_exit_macro!(1);
             }
             crate::format::format(&builder, !builder.config.cmd.bless(), &[]);
         }
@@ -1108,7 +1108,7 @@ fn run(self, builder: &Builder<'_>) {
                 eprintln!(
                     "x.py completions were changed; run `x.py run generate-completions` to update them"
                 );
-                crate::detail_exit(1);
+                crate::detail_exit_macro!(1);
             }
         }
     }
@@ -1329,7 +1329,7 @@ fn run(self, builder: &Builder<'_>) {
 help: to test the standard library, use `--stage 0 library/std` instead
 note: if you're sure you want to do this, please open an issue as to why. In the meantime, you can override this with `COMPILETEST_FORCE_STAGE0=1`."
             );
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
 
         let mut compiler = self.compiler;

src/bootstrap/tool.rs

@@ -116,7 +116,7 @@ fn run(self, builder: &Builder<'_>) -> Option<PathBuf> {
 
         if !is_expected {
             if !is_optional_tool {
-                crate::detail_exit(1);
+                crate::detail_exit_macro!(1);
             } else {
                 None
             }

src/bootstrap/toolstate.rs

@@ -91,7 +91,7 @@ fn print_error(tool: &str, submodule: &str) {
     eprintln!("If you do NOT intend to update '{}', please ensure you did not accidentally", tool);
     eprintln!("change the submodule at '{}'. You may ask your reviewer for the", submodule);
     eprintln!("proper steps.");
-    crate::detail_exit(3);
+    crate::detail_exit_macro!(3);
 }
 
 fn check_changed_files(toolstates: &HashMap<Box<str>, ToolState>) {
@@ -106,7 +106,7 @@ fn check_changed_files(toolstates: &HashMap<Box<str>, ToolState>) {
         Ok(o) => o,
         Err(e) => {
             eprintln!("Failed to get changed files: {:?}", e);
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
     };
 
@@ -177,7 +177,7 @@ fn run(self, builder: &Builder<'_>) {
         }
 
         if did_error {
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
 
         check_changed_files(&toolstates);
@@ -223,7 +223,7 @@ fn run(self, builder: &Builder<'_>) {
         }
 
         if did_error {
-            crate::detail_exit(1);
+            crate::detail_exit_macro!(1);
         }
 
         if builder.config.channel == "nightly" && env::var_os("TOOLSTATE_PUBLISH").is_some() {

src/bootstrap/util.rs

@@ -3,6 +3,7 @@
 //! Simple things like testing the various filesystem operations here and there,
 //! not a lot of interesting happenings here unfortunately.
 
+use build_helper::util::{fail, try_run};
 use std::env;
 use std::fs;
 use std::io;
@@ -230,25 +231,10 @@ pub fn is_valid_test_suite_arg<'a, P: AsRef<Path>>(
 
 pub fn run(cmd: &mut Command, print_cmd_on_fail: bool) {
     if !try_run(cmd, print_cmd_on_fail) {
-        crate::detail_exit(1);
+        crate::detail_exit_macro!(1);
     }
 }
 
-pub fn try_run(cmd: &mut Command, print_cmd_on_fail: bool) -> bool {
-    let status = match cmd.status() {
-        Ok(status) => status,
-        Err(e) => fail(&format!("failed to execute command: {:?}\nerror: {}", cmd, e)),
-    };
-    if !status.success() && print_cmd_on_fail {
-        println!(
-            "\n\ncommand did not execute successfully: {:?}\n\
-             expected success, got: {}\n\n",
-            cmd, status
-        );
-    }
-    status.success()
-}
-
 pub fn check_run(cmd: &mut Command, print_cmd_on_fail: bool) -> bool {
     let status = match cmd.status() {
         Ok(status) => status,
@@ -269,7 +255,7 @@ pub fn check_run(cmd: &mut Command, print_cmd_on_fail: bool) -> bool {
 
 pub fn run_suppressed(cmd: &mut Command) {
     if !try_run_suppressed(cmd) {
-        crate::detail_exit(1);
+        crate::detail_exit_macro!(1);
     }
 }
 
@@ -374,11 +360,6 @@ fn dir_up_to_date(src: &Path, threshold: SystemTime) -> bool {
     })
 }
 
-fn fail(s: &str) -> ! {
-    eprintln!("\n\n{}\n\n", s);
-    crate::detail_exit(1);
-}
-
 /// Copied from `std::path::absolute` until it stabilizes.
 ///
 /// FIXME: this shouldn't exist.

src/tools/build_helper/src/lib.rs

@@ -1,2 +1,3 @@
 pub mod ci;
 pub mod git;
+pub mod util;

src/tools/build_helper/src/util.rs

@@ -0,0 +1,41 @@
+use std::process::Command;
+
+/// Invokes `build_helper::util::detail_exit` with `cfg!(test)`
+#[macro_export]
+macro_rules! detail_exit_macro {
+    ($code:expr) => {
+        build_helper::util::detail_exit($code, cfg!(test));
+    };
+}
+
+/// If code is not 0 (successful exit status), exit status is 101 (rust's default error code.)
+/// If `is_test` true and code is an error code, it will cause a panic.
+pub fn detail_exit(code: i32, is_test: bool) -> ! {
+    // if in test and code is an error code, panic with status code provided
+    if is_test {
+        panic!("status code: {}", code);
+    } else {
+        // otherwise,exit with provided status code
+        std::process::exit(code);
+    }
+}
+
+pub fn fail(s: &str) -> ! {
+    eprintln!("\n\n{}\n\n", s);
+    detail_exit(1, cfg!(test));
+}
+
+pub fn try_run(cmd: &mut Command, print_cmd_on_fail: bool) -> bool {
+    let status = match cmd.status() {
+        Ok(status) => status,
+        Err(e) => fail(&format!("failed to execute command: {:?}\nerror: {}", cmd, e)),
+    };
+    if !status.success() && print_cmd_on_fail {
+        println!(
+            "\n\ncommand did not execute successfully: {:?}\n\
+             expected success, got: {}\n\n",
+            cmd, status
+        );
+    }
+    status.success()
+}

src/tools/rustdoc-gui-test/Cargo.toml

@@ -4,6 +4,7 @@ version = "0.1.0"
 edition = "2021"
 
 [dependencies]
+build_helper = { path = "../build_helper" }
 compiletest = { path = "../compiletest" }
 getopts = "0.2"
 walkdir = "2"

src/tools/rustdoc-gui-test/src/main.rs

@@ -1,3 +1,4 @@
+use build_helper::util::try_run;
 use compiletest::header::TestProps;
 use config::Config;
 use std::path::{Path, PathBuf};
@@ -60,23 +61,6 @@ fn find_librs<P: AsRef<Path>>(path: P) -> Option<PathBuf> {
     None
 }
 
-// FIXME: move `bootstrap::util::try_run` into `build_helper` crate
-// and use that one instead of creating this function.
-fn try_run(cmd: &mut Command, print_cmd_on_fail: bool) -> bool {
-    let status = match cmd.status() {
-        Ok(status) => status,
-        Err(e) => panic!("failed to execute command: {:?}\nerror: {}", cmd, e),
-    };
-    if !status.success() && print_cmd_on_fail {
-        println!(
-            "\n\ncommand did not execute successfully: {:?}\n\
-             expected success, got: {}\n\n",
-            cmd, status
-        );
-    }
-    status.success()
-}
-
 fn main() {
     let config = Arc::new(Config::from_args(env::args().collect()));
 
@@ -143,6 +127,16 @@ fn main() {
     }
 
     let mut command = Command::new(&config.nodejs);
+
+    if let Ok(current_dir) = env::current_dir() {
+        let local_node_modules = current_dir.join("node_modules");
+        if local_node_modules.exists() {
+            // Link the local node_modules if exists.
+            // This is useful when we run rustdoc-gui-test from outside of the source root.
+            env::set_var("NODE_PATH", local_node_modules);
+        }
+    }
+
     command
         .arg(config.rust_src.join("src/tools/rustdoc-gui/tester.js"))
         .arg("--jobs")