tidy: make npm_install use root_path and improve error reporting

Author: lolbinarycat

src/tools/tidy/src/ext_tool_checks.rs

@@ -296,7 +296,7 @@ fn check_impl(
     }
 
     if js_lint || js_typecheck {
-        rustdoc_js::npm_install(outdir)?;
+        rustdoc_js::npm_install(root_path, outdir)?;
     }
 
     if js_lint {

src/tools/tidy/src/ext_tool_checks/rustdoc_js.rs

@@ -2,22 +2,34 @@
 //! characters.
 
 use std::ffi::OsStr;
-use std::fs;
+use std::{fs, io};
 use std::path::{Path, PathBuf};
-use std::process::Command;
+use std::process::{Child, Command};
 
 use build_helper::ci::CiEnv;
 use ignore::DirEntry;
 
 use crate::walk::walk_no_read;
 
 fn node_module_bin(outdir: &Path, name: &str) -> PathBuf {
-    outdir.join(name)
+    outdir.join("node_modules/.bin").join(name)
+}
+
+fn spawn_cmd(cmd: &mut Command) -> Result<Child, io::Error> {
+    cmd.spawn().map_err(|err| {
+        eprintln!("unable to run {cmd:?} due to {err:?}");
+        err
+    })
 }
 
 /// install all js dependencies from package.json.
-pub(super) fn npm_install(outdir: &Path) -> Result<(), super::Error> {
-    let copy_to_build = |p| fs::copy(p, outdir.join(p));
+pub(super) fn npm_install(root_path: &Path, outdir: &Path) -> Result<(), super::Error> {
+    let copy_to_build = |p| {
+        fs::copy(root_path.join(p), outdir.join(p)).map_err(|e| {
+            eprintln!("unable to copy {p:?} to build directory: {e:?}");
+            e
+        })
+    };
     // copy stuff to the output directory to make node_modules get put there.
     copy_to_build("package.json")?;
     copy_to_build("package-lock.json")?;
@@ -34,7 +46,7 @@ pub(super) fn npm_install(outdir: &Path) -> Result<(), super::Error> {
     // of repeated tidy invokations.
     cmd.args(&["--audit=false", "--save=false", "--fund=false"]);
     cmd.current_dir(outdir);
-    let mut child = cmd.spawn()?;
+    let mut child = spawn_cmd(cmd)?;
     match child.wait() {
         Ok(exit_status) => {
             if exit_status.success() {
@@ -59,11 +71,10 @@ fn rustdoc_js_files(librustdoc_path: &Path) -> Vec<PathBuf> {
 }
 
 fn run_eslint(outdir: &Path, args: &[PathBuf], config_folder: PathBuf) -> Result<(), super::Error> {
-    let mut child = Command::new(node_module_bin(outdir, "eslint"))
+    let mut child = spawn_cmd(Command::new(node_module_bin(outdir, "eslint"))
         .arg("-c")
         .arg(config_folder.join(".eslintrc.js"))
-        .args(args)
-        .spawn()?;
+        .args(args))?;
     match child.wait() {
         Ok(exit_status) => {
             if exit_status.success() {
@@ -75,21 +86,6 @@ fn run_eslint(outdir: &Path, args: &[PathBuf], config_folder: PathBuf) -> Result
     }
 }
 
-fn get_eslint_version_inner(global: bool) -> Option<String> {
-    let mut command = Command::new("npm");
-    command.arg("list").arg("--parseable").arg("--long").arg("--depth=0");
-    if global {
-        command.arg("--global");
-    }
-    let output = command.output().ok()?;
-    let lines = String::from_utf8_lossy(&output.stdout);
-    lines.lines().find_map(|l| l.split(':').nth(1)?.strip_prefix("eslint@")).map(|v| v.to_owned())
-}
-
-fn get_eslint_version() -> Option<String> {
-    get_eslint_version_inner(false).or_else(|| get_eslint_version_inner(true))
-}
-
 pub(super) fn lint(
     outdir: &Path,
     librustdoc_path: &Path,
@@ -104,33 +100,6 @@ pub(super) fn lint(
             return Err(error.into());
         }
     };
-    // Having the correct `eslint` version installed via `npm` isn't strictly necessary, since we're invoking it via `npx`,
-    // but this check allows the vast majority that is not working on the rustdoc frontend to avoid the penalty of running
-    // `eslint` in tidy. See also: https://github.com/rust-lang/rust/pull/142851
-    match get_eslint_version() {
-        Some(version) => {
-            if version != eslint_version {
-                // unfortunatly we can't use `Error::Version` here becuse `str::trim` isn't const and
-                // Version::required must be a static str
-                return Err(super::Error::Generic(format!(
-                    "⚠️ Installed version of eslint (`{version}`) is different than the \
-                     one used in the CI (`{eslint_version}`)\n\
-                    You can install this version using `npm update eslint` or by using \
-                    `npm install eslint@{eslint_version}`\n
-"
-                )));
-            }
-        }
-        None => {
-            //eprintln!("`eslint` doesn't seem to be installed. Skipping tidy check for JS files.");
-            //eprintln!("You can install it using `npm install eslint@{eslint_version}`");
-            return Err(super::Error::MissingReq(
-                "eslint",
-                "js lint checks",
-                Some(format!("You can install it using `npm install eslint@{eslint_version}`")),
-            ));
-        }
-    }
     let files_to_check = rustdoc_js_files(librustdoc_path);
     println!("Running eslint on rustdoc JS files");
     run_eslint(outdir, &files_to_check, librustdoc_path.join("html/static"))?;
@@ -146,10 +115,9 @@ pub(super) fn lint(
 
 pub(super) fn typecheck(outdir: &Path, librustdoc_path: &Path) -> Result<(), super::Error> {
     // use npx to ensure correct version
-    let mut child = Command::new(node_module_bin(outdir, "tsc"))
+    let mut child = spawn_cmd(Command::new(node_module_bin(outdir, "tsc"))
         .arg("-p")
-        .arg(librustdoc_path.join("html/static/js/tsconfig.json"))
-        .spawn()?;
+        .arg(librustdoc_path.join("html/static/js/tsconfig.json")))?;
     match child.wait() {
         Ok(exit_status) => {
             if exit_status.success() {
@@ -168,7 +136,7 @@ pub(super) fn es_check(outdir: &Path, librustdoc_path: &Path) -> Result<(), supe
     for f in files_to_check {
         cmd.arg(f);
     }
-    match cmd.spawn()?.wait() {
+    match spawn_cmd(cmd)?.wait() {
         Ok(exit_status) => {
             if exit_status.success() {
                 return Ok(());