[xtask] fix recompilations with cargo xtask clippy (#7740)

@davepacheco reported an issue where `cargo check --all-targets && cargo
xtask clippy` would cause many recompilations, starting from `ring`.
This turned out to be because:

* `cargo xtask`, when run, has `CARGO_MANIFEST_DIR` and other related
environment variables set in its environment, but a regular `cargo
check` does not
* these variables were being inherited by child Cargo processes
* ring's build script emits a bunch of `cargo:rerun-if-env-changed`
instructions for `CARGO_MANIFEST_DIR` etc
* when Cargo determines whether the env has actually changed, it only
looks at what it *sees*, not what it *sets* for scripts

To address this, remove a few environment variables from child Cargo
processes' environments. I've tested this and it solves this issue, but
it isn't a full fix. It's quite possible that we'll have to add the list
if we see more recompilations in the future, but at least we know what's
going on now.

This is probably a bug in Cargo -- in this determination it should look
at what it sets, since that's what build scripts care about.

This is also a bug in ring's build script, I think. Cargo's
documentation says in [this
section](https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-env-changed):

> Note that the environment variables here are intended for global
environment variables like CC and such, it is not possible to use this
for environment variables like TARGET that [Cargo sets for build
scripts](https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts).
The environment variables in use are those received by cargo
invocations, not those received by the executable of the build script.

I've filed ctz/ring#10 for that.

I determined this via logging both of these commands into files and
comparing them. Thanks to @mkeeter for the suggestion to use this
`CARGO_LOG` variable!

```console
$ CARGO_LOG=cargo::core::compiler::fingerprint=trace cargo clippy --all-targets --workspace
$ CARGO_LOG=cargo::core::compiler::fingerprint=trace cargo xtask clippy
```

---

I also included a `rerun-if-changed` instruction in the workspace-hack's
`build.rs` to ensure that that doesn't cause any rebuilds either. (I
should probably fix hakari's initialization template to produce this
line.)

Author: sunshowers

dev-tools/xtask/src/check_features.rs

@@ -9,6 +9,8 @@ use camino::Utf8PathBuf;
 use clap::Parser;
 use std::{collections::HashSet, process::Command};
 
+use crate::common::cargo_command;
+
 const SUPPORTED_ARCHITECTURES: [&str; 1] = ["x86_64"];
 const CI_EXCLUDED_FEATURES: [&str; 2] = ["image-trampoline", "image-standard"];
 
@@ -40,16 +42,13 @@ pub fn run_cmd(args: Args) -> Result<()> {
         bail!("cannot specify --ci and --install-version together");
     }
 
-    let cargo =
-        std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo"));
-
-    let mut command = Command::new(&cargo);
+    let mut command = cargo_command();
 
     // Add the `hack check` subcommand.
     command.args(&["hack", "check"]);
 
     if args.ci {
-        install_prebuilt_cargo_hack(&cargo)?;
+        install_prebuilt_cargo_hack()?;
 
         let ex = if let Some(mut features) = args.exclude_features {
             // Extend the list of features to exclude with the CI defaults.
@@ -68,7 +67,7 @@ pub fn run_cmd(args: Args) -> Result<()> {
         // Add the `--exclude-features` flag if we are running in CI mode.
         command.args(["--exclude-features", &ex]);
     } else {
-        install_cargo_hack(&cargo, args.install_version)?;
+        install_cargo_hack(args.install_version)?;
         // Add "only" the `--exclude-features` flag if it was provided.
         if let Some(features) = args.exclude_features {
             command.args(["--exclude-features", &features.join(",")]);
@@ -131,9 +130,9 @@ fn out_dir() -> Utf8PathBuf {
 
 /// Install `cargo-hack` if the `install-version` was specified; otherwise,
 /// download a pre-built version if it's not already in our `out` directory.
-fn install_cargo_hack(cargo: &str, version: Option<String>) -> Result<()> {
+fn install_cargo_hack(version: Option<String>) -> Result<()> {
     if let Some(version) = version {
-        let mut command = Command::new(cargo);
+        let mut command = cargo_command();
 
         eprintln!(
             "installing cargo-hack at version {} to {}",
@@ -143,7 +142,7 @@ fn install_cargo_hack(cargo: &str, version: Option<String>) -> Result<()> {
         command.args(&["install", "cargo-hack", "--version", &version]);
         exec(command)
     } else if !out_dir().exists() {
-        install_prebuilt_cargo_hack(cargo)
+        install_prebuilt_cargo_hack()
     } else {
         let out_dir = out_dir();
         eprintln!("cargo-hack found in {}", out_dir);
@@ -153,8 +152,8 @@ fn install_cargo_hack(cargo: &str, version: Option<String>) -> Result<()> {
 
 /// Download a pre-built version of `cargo-hack` to the `out` directory via the
 /// download `xtask`.
-fn install_prebuilt_cargo_hack(cargo: &str) -> Result<()> {
-    let mut command = Command::new(cargo);
+fn install_prebuilt_cargo_hack() -> Result<()> {
+    let mut command = cargo_command();
 
     let out_dir = out_dir();
     eprintln!(
@@ -185,12 +184,9 @@ fn install_prebuilt_cargo_hack(cargo: &str) -> Result<()> {
 
 /// Execute the command and check the exit status.
 fn exec(mut command: Command) -> Result<()> {
-    let cargo =
-        std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo"));
-
     eprintln!(
         "running: {:?} {}",
-        &cargo,
+        command.get_program(),
         command
             .get_args()
             .map(|arg| format!("{:?}", arg.to_str().unwrap()))

dev-tools/xtask/src/clippy.rs

@@ -4,10 +4,9 @@
 
 //! Subcommand: cargo xtask clippy
 
-use crate::common::run_subcmd;
+use crate::common::{cargo_command, run_subcmd};
 use anyhow::Result;
 use clap::Parser;
-use std::process::Command;
 
 #[derive(Parser)]
 pub struct ClippyArgs {
@@ -20,9 +19,7 @@ pub struct ClippyArgs {
 }
 
 pub fn run_cmd(args: ClippyArgs) -> Result<()> {
-    let cargo =
-        std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo"));
-    let mut command = Command::new(&cargo);
+    let mut command = cargo_command();
     command.arg("clippy");
 
     if args.fix {

dev-tools/xtask/src/common.rs

@@ -32,3 +32,48 @@ pub fn run_subcmd(mut command: Command) -> Result<()> {
 
     Ok(())
 }
+
+/// Creates and prepares a `std::process::Command` for the `cargo` executable.
+pub(crate) fn cargo_command() -> Command {
+    let cargo =
+        std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo"));
+    let mut command = Command::new(&cargo);
+
+    for (key, _) in std::env::vars_os() {
+        let Some(key) = key.to_str() else { continue };
+        if SANITIZED_ENV_VARS.matches(key) {
+            command.env_remove(key);
+        }
+    }
+
+    command
+}
+
+#[derive(Debug)]
+struct SanitizedEnvVars {
+    // At the moment we only ban some prefixes, but we may also want to ban env
+    // vars by exact name in the future.
+    prefixes: &'static [&'static str],
+}
+
+impl SanitizedEnvVars {
+    const fn new() -> Self {
+        // Remove many of the environment variables set in
+        // https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts.
+        // This is done to avoid recompilation with crates like ring between
+        // `cargo clippy` and `cargo xtask clippy`. (This is really a bug in
+        // both ring's build script and in Cargo.)
+        //
+        // The current list is informed by looking at ring's build script, so
+        // it's not guaranteed to be exhaustive and it may need to grow over
+        // time.
+        let prefixes = &["CARGO_PKG_", "CARGO_MANIFEST_", "CARGO_CFG_"];
+        Self { prefixes }
+    }
+
+    fn matches(&self, key: &str) -> bool {
+        self.prefixes.iter().any(|prefix| key.starts_with(prefix))
+    }
+}
+
+static SANITIZED_ENV_VARS: SanitizedEnvVars = SanitizedEnvVars::new();

dev-tools/xtask/src/external.rs

@@ -11,6 +11,8 @@ use std::process::Command;
 use anyhow::{Context, Result};
 use clap::Parser;
 
+use crate::common::cargo_command;
+
 /// Argument parser for external xtasks.
 ///
 /// In general we want all developer tasks to be discoverable simply by running
@@ -72,8 +74,7 @@ impl External {
 }
 
 fn new_command() -> Command {
-    let cargo = std::env::var_os("CARGO").unwrap_or_else(|| "cargo".into());
-    let mut command = Command::new(cargo);
+    let mut command = cargo_command();
     command.arg("run");
     command
 }

dev-tools/xtask/src/live_tests.rs

@@ -4,7 +4,7 @@
 
 //! Subcommand: cargo xtask live-tests
 
-use crate::common::run_subcmd;
+use crate::common::{cargo_command, run_subcmd};
 use anyhow::{Context, Result, bail};
 use clap::Parser;
 use std::process::Command;
@@ -45,9 +45,7 @@ pub fn run_cmd(_args: Args) -> Result<()> {
     std::fs::create_dir(&proto_root)
         .with_context(|| format!("mkdir {:?}", &proto_root))?;
 
-    let cargo =
-        std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo"));
-    let mut command = Command::new(&cargo);
+    let mut command = cargo_command();
 
     command.arg("nextest");
     command.arg("archive");

dev-tools/xtask/src/verify_libraries.rs

@@ -15,6 +15,7 @@ use std::{
 };
 use swrite::{SWrite, swriteln};
 
+use crate::common::cargo_command;
 use crate::load_workspace;
 
 #[derive(Parser)]
@@ -98,8 +99,7 @@ pub fn run_cmd(args: Args) -> Result<()> {
     config_path.push(".cargo/xtask.toml");
     let config = read_xtask_toml(&config_path)?;
 
-    let cargo = std::env::var("CARGO").unwrap_or_else(|_| "cargo".to_string());
-    let mut command = Command::new(cargo);
+    let mut command = cargo_command();
     command.args([
         "build",
         "--bins",

workspace-hack/build.rs

@@ -1,2 +1,4 @@
 // A build script is required for cargo to consider build dependencies.
-fn main() {}
+fn main() {
+    println!("cargo::rerun-if-changed=build.rs");
+}