tidy spellcheck improvments based on code review

Author: lolbinarycat

src/build_helper/src/util.rs

@@ -1,7 +1,6 @@
-use std::env;
 use std::fs::File;
-use std::io::{self, BufRead, BufReader};
-use std::path::{Path, PathBuf};
+use std::io::{BufRead, BufReader};
+use std::path::Path;
 use std::process::Command;
 
 /// Invokes `build_helper::util::detail_exit` with `cfg!(test)`
@@ -67,65 +66,3 @@ pub fn parse_gitmodules(target_dir: &Path) -> Vec<String> {
 
     submodules_paths
 }
-
-/// If the given executable is installed with the given version, use that,
-/// otherwise install via cargo.
-pub fn ensure_version_or_cargo_install(
-    build_dir: &Path,
-    cargo: &Path,
-    pkg_name: &str,
-    bin_name: &str,
-    version: &str,
-) -> io::Result<PathBuf> {
-    // ignore the process exit code here and instead just let the version number check fail.
-    // we also importantly don't return if the program wasn't installed,
-    // instead we want to continue to the fallback.
-    'ck: {
-        // FIXME: rewrite as if-let chain once this crate is 2024 edition.
-        let Ok(output) = Command::new(bin_name).arg("--version").output() else {
-            break 'ck;
-        };
-        let Ok(s) = str::from_utf8(&output.stdout) else {
-            break 'ck;
-        };
-        let Some(v) = s.trim().split_whitespace().last() else {
-            break 'ck;
-        };
-        if v == version {
-            return Ok(PathBuf::from(bin_name));
-        }
-    }
-
-    let tool_root_dir = build_dir.join("misc-tools");
-    let tool_bin_dir = tool_root_dir.join("bin");
-    eprintln!("building external tool {bin_name} from package {pkg_name}@{version}");
-    // use --force to ensure that if the required version is bumped, we update it.
-    // use --target-dir to ensure we have a build cache so repeated invocations aren't slow.
-    // modify PATH so that cargo doesn't print a warning telling the user to modify the path.
-    let cargo_exit_code = Command::new(cargo)
-        .args(["install", "--locked", "--force", "--quiet"])
-        .arg("--root")
-        .arg(&tool_root_dir)
-        .arg("--target-dir")
-        .arg(tool_root_dir.join("target"))
-        .arg(format!("{pkg_name}@{version}"))
-        .env(
-            "PATH",
-            env::join_paths(
-                env::split_paths(&env::var("PATH").unwrap())
-                    .chain(std::iter::once(tool_bin_dir.clone())),
-            )
-            .expect("build dir contains invalid char"),
-        )
-        .spawn()?
-        .wait()?;
-    if !cargo_exit_code.success() {
-        return Err(io::Error::other("cargo install failed"));
-    }
-    let bin_path = tool_bin_dir.join(bin_name);
-    assert!(
-        matches!(bin_path.try_exists(), Ok(true)),
-        "cargo install did not produce the expected binary"
-    );
-    Ok(bin_path)
-}

src/tools/tidy/src/extra_checks/mod.rs

@@ -23,8 +23,6 @@ use std::process::Command;
 use std::str::FromStr;
 use std::{fmt, fs, io};
 
-use build_helper::util::ensure_version_or_cargo_install;
-
 use crate::CiInfo;
 
 mod rustdoc_js;
@@ -43,7 +41,6 @@ const RUFF_CONFIG_PATH: &[&str] = &["src", "tools", "tidy", "config", "ruff.toml
 const RUFF_CACHE_PATH: &[&str] = &["cache", "ruff_cache"];
 const PIP_REQ_PATH: &[&str] = &["src", "tools", "tidy", "config", "requirements.txt"];
 
-// this must be kept in sync with with .github/workflows/spellcheck.yml
 const SPELLCHECK_DIRS: &[&str] = &["compiler", "library", "src/bootstrap", "src/librustdoc"];
 
 pub fn check(
@@ -588,7 +585,8 @@ fn spellcheck_runner(
     cargo: &Path,
     args: &[&str],
 ) -> Result<(), Error> {
-    let bin_path = ensure_version_or_cargo_install(outdir, cargo, "typos-cli", "typos", "1.34.0")?;
+    let bin_path =
+        crate::ensure_version_or_cargo_install(outdir, cargo, "typos-cli", "typos", "1.34.0")?;
 
     match Command::new(bin_path).current_dir(src_root).args(args).status() {
         Ok(status) => {

src/tools/tidy/src/lib.rs

@@ -4,7 +4,9 @@
 //! to be used by tools.
 
 use std::ffi::OsStr;
+use std::path::{Path, PathBuf};
 use std::process::Command;
+use std::{env, io};
 
 use build_helper::ci::CiEnv;
 use build_helper::git::{GitConfig, get_closest_upstream_commit};
@@ -180,6 +182,69 @@ pub fn files_modified(ci_info: &CiInfo, pred: impl Fn(&str) -> bool) -> bool {
     !v.is_empty()
 }
 
+/// If the given executable is installed with the given version, use that,
+/// otherwise install via cargo.
+pub fn ensure_version_or_cargo_install(
+    build_dir: &Path,
+    cargo: &Path,
+    pkg_name: &str,
+    bin_name: &str,
+    version: &str,
+) -> io::Result<PathBuf> {
+    // ignore the process exit code here and instead just let the version number check fail.
+    // we also importantly don't return if the program wasn't installed,
+    // instead we want to continue to the fallback.
+    'ck: {
+        // FIXME: rewrite as if-let chain once this crate is 2024 edition.
+        let Ok(output) = Command::new(bin_name).arg("--version").output() else {
+            break 'ck;
+        };
+        let Ok(s) = str::from_utf8(&output.stdout) else {
+            break 'ck;
+        };
+        let Some(v) = s.trim().split_whitespace().last() else {
+            break 'ck;
+        };
+        if v == version {
+            return Ok(PathBuf::from(bin_name));
+        }
+    }
+
+    let tool_root_dir = build_dir.join("misc-tools");
+    let tool_bin_dir = tool_root_dir.join("bin");
+    eprintln!("building external tool {bin_name} from package {pkg_name}@{version}");
+    // use --force to ensure that if the required version is bumped, we update it.
+    // use --target-dir to ensure we have a build cache so repeated invocations aren't slow.
+    // modify PATH so that cargo doesn't print a warning telling the user to modify the path.
+    let cargo_exit_code = Command::new(cargo)
+        .args(["install", "--locked", "--force", "--quiet"])
+        .arg("--root")
+        .arg(&tool_root_dir)
+        .arg("--target-dir")
+        .arg(tool_root_dir.join("target"))
+        .arg(format!("{pkg_name}@{version}"))
+        .env(
+            "PATH",
+            env::join_paths(
+                env::split_paths(&env::var("PATH").unwrap())
+                    .chain(std::iter::once(tool_bin_dir.clone())),
+            )
+            .expect("build dir contains invalid char"),
+        )
+        .env("RUSTFLAGS", "-Copt-level=0")
+        .spawn()?
+        .wait()?;
+    if !cargo_exit_code.success() {
+        return Err(io::Error::other("cargo install failed"));
+    }
+    let bin_path = tool_bin_dir.join(bin_name);
+    assert!(
+        matches!(bin_path.try_exists(), Ok(true)),
+        "cargo install did not produce the expected binary"
+    );
+    Ok(bin_path)
+}
+
 pub mod alphabetical;
 pub mod bins;
 pub mod debug_artifacts;