Warn about CARGO environment variable in cargo proxy

src/test/mock/clitools.rs

@@ -522,7 +522,7 @@ impl Config {
         I: IntoIterator<Item = A>,
         A: AsRef<OsStr>,
     {
-        let exe_path = self.exedir.join(format!("{name}{EXE_SUFFIX}"));
+        let exe_path = self.exedir.join(name);
         let mut cmd = Command::new(exe_path);
         cmd.args(args);
         cmd.current_dir(&*self.workdir.borrow());
@@ -748,15 +748,12 @@ impl Config {
             self.run_subprocess(name, args.clone(), env)
         };
         let duration = Instant::now() - start;
-        let output = SanitizedOutput {
-            ok: matches!(out.status, Some(0)),
-            stdout: String::from_utf8(out.stdout).unwrap(),
-            stderr: String::from_utf8(out.stderr).unwrap(),
-        };
+        let status = out.status;
+        let output: SanitizedOutput = out.try_into().unwrap();
 
         println!("ran: {} {:?}", name, args);
         println!("inprocess: {inprocess}");
-        println!("status: {:?}", out.status);
+        println!("status: {:?}", status);
         println!("duration: {:.3}s", duration.as_secs_f32());
         println!("stdout:\n====\n{}\n====\n", output.stdout);
         println!("stderr:\n====\n{}\n====\n", output.stderr);
@@ -824,7 +821,10 @@ impl Config {
         for env in env {
             cmd.env(env.0, env.1);
         }
+        self.run_subprocess_cmd(cmd)
+    }
 
+    pub fn run_subprocess_cmd(&self, mut cmd: Command) -> Output {
         let mut retries = 8;
         let out = loop {
             let lock = CMD_LOCK.read().unwrap();
@@ -905,6 +905,18 @@ pub struct SanitizedOutput {
     pub stderr: String,
 }
 
+impl TryFrom<Output> for SanitizedOutput {
+    type Error = std::string::FromUtf8Error;
+    fn try_from(out: Output) -> Result<Self, Self::Error> {
+        let sanitized_output = Self {
+            ok: matches!(out.status, Some(0)),
+            stdout: String::from_utf8(out.stdout)?,
+            stderr: String::from_utf8(out.stderr)?,
+        };
+        Ok(sanitized_output)
+    }
+}
+
 pub fn cmd<I, A>(config: &Config, name: &str, args: I) -> Command
 where
     I: IntoIterator<Item = A>,
@@ -1666,3 +1678,19 @@ where
     }
     inner(original.as_ref(), link.as_ref())
 }
+
+// We need an intermediary to run cargo itself.
+// The "mock" cargo can't do that because on Windows it will check
+// for a `cargo.exe` in the current directory before checking PATH.
+//
+// The solution here is to copy from the "mock" `cargo.exe` into
+// `~/.cargo/bin/cargo-foo`. This is just for convenience to avoid
+// needing to build another executable.
+pub async fn create_cargo_foo(cx: &CliTestContext) {
+    let output = cx.config.run("rustup", ["which", "cargo"], &[]).await;
+    let real_mock_cargo = output.stdout.trim();
+    let cargo_bin_path = cx.config.cargodir.join("bin");
+    let cargo_subcommand = cargo_bin_path.join(format!("cargo-foo{}", EXE_SUFFIX));
+    fs::create_dir_all(&cargo_bin_path).unwrap();
+    fs::copy(real_mock_cargo, cargo_subcommand).unwrap();
+}

src/test/mock/mock_bin_src.rs

@@ -5,7 +5,16 @@ use std::path::{Path, PathBuf};
 use std::process::Command;
 
 fn main() {
-    let mut args = env::args_os().skip(1);
+    let mut args = env::args_os();
+    let arg0 = args.next().unwrap();
+    if let Some(cargo_subcommand) = PathBuf::from(arg0)
+        .file_stem()
+        .and_then(|s| s.to_str())
+        .and_then(|s| s.strip_prefix("cargo-"))
+    {
+        let arg1 = args.next().unwrap();
+        assert_eq!(arg1, cargo_subcommand);
+    }
     match args.next().as_ref().and_then(|s| s.to_str()) {
         Some("--version") => {
             let me = env::current_exe().unwrap();
@@ -63,7 +72,7 @@ fn main() {
         }
         Some("--recursive-cargo-subcommand") => {
             let status = Command::new("cargo-foo")
-                .arg("--recursive-cargo")
+                .args(["foo", "--recursive-cargo"])
                 .status()
                 .unwrap();
             assert!(status.success());
@@ -75,6 +84,10 @@ fn main() {
                 .unwrap();
             assert!(status.success());
         }
+        Some("--recursive-cargo-without-toolchain") => {
+            let status = Command::new("cargo").arg("--version").status().unwrap();
+            assert!(status.success());
+        }
         Some("--echo-args") => {
             let mut out = io::stderr();
             for arg in args {

src/toolchain.rs

@@ -12,7 +12,7 @@ use std::{
 
 use anyhow::{anyhow, bail, Context};
 use fs_at::OpenOptions;
-use tracing::info;
+use tracing::{info, warn};
 use url::Url;
 use wait_timeout::ChildExt;
 
@@ -159,6 +159,23 @@ impl<'a> Toolchain<'a> {
     pub fn set_env(&self, cmd: &mut Command) {
         self.set_ldpath(cmd);
 
+        let path = Path::new(cmd.get_program());
+        if path.file_stem() == Some(OsStr::new("cargo")) {
+            if let Some(value) = self.cfg.process.var_os("CARGO") {
+                if value != path {
+                    warn!("'CARGO' is not the path of the binary about to be executed");
+                    warn!("     'CARGO' value: {}", value.to_string_lossy());
+                    warn!("    to be executed: {}", path.to_string_lossy());
+                    warn!("in a future version of the cargo proxy, 'CARGO' may be cleared");
+                    // The `CARGO` environment can be cleared here if there is
+                    // consensus that that is the right approach. See the
+                    // following issue for discussion:
+                    // https://github.com/rust-lang/cargo/issues/15099
+                    // cmd.env_remove("CARGO");
+                }
+            }
+        }
+
         // Older versions of Cargo used a slightly different definition of
         // cargo home. Rustup does not read HOME on Windows whereas the older
         // versions of Cargo did. Rustup and Cargo should be in sync now (both

tests/suite/cli_misc.rs

@@ -7,11 +7,14 @@ use std::{env::consts::EXE_SUFFIX, path::Path};
 
 use rustup::for_host;
 use rustup::test::{
-    mock::clitools::{self, set_current_dist_date, CliTestContext, Config, Scenario},
+    mock::clitools::{
+        self, set_current_dist_date, CliTestContext, Config, SanitizedOutput, Scenario,
+    },
     this_host_triple,
 };
 use rustup::utils;
-use rustup::utils::raw::symlink_dir;
+use rustup::utils::raw::{self, symlink_dir};
+use tempfile::tempdir;
 
 #[tokio::test]
 async fn smoke_test() {
@@ -1239,3 +1242,68 @@ async fn toolchain_install_multi_components_comma() {
             .await;
     }
 }
+
+#[tokio::test]
+async fn clear_cargo_environment_variable() {
+    let mut cx = CliTestContext::new(Scenario::SimpleV2).await;
+
+    cx.config
+        .expect_ok(&["rustup", "toolchain", "install", "nightly"])
+        .await;
+
+    // `cargo-foo` is just a copy of the mocked cargo defined in mock_bin_src.rs. It provides a way
+    // to recursively call cargo.
+    clitools::create_cargo_foo(&cx).await;
+
+    // Because this test itself runs under cargo, the `CARGO` environment variable will be set. We
+    // use it to get the path of the real cargo binary. However, the environment variable must also
+    // be cleared before running each subcommand.
+    let real_cargo = std::env::var("CARGO").unwrap();
+
+    // The mocked cargo does not set the `CARGO` environment variable, and simulating this behavior
+    // could affect the test's fidelity. So we run the real cargo binary whose path was obtained
+    // from the `CARGO` environment variable just above.
+    let run_real_cargo = |cx: &CliTestContext, args: &[&str], should_warn: bool| {
+        let mut cmd = cx.config.cmd(&real_cargo, args);
+        cmd.env_remove("CARGO");
+        let out: SanitizedOutput = cx.config.run_subprocess_cmd(cmd).try_into().unwrap();
+        assert!(out.ok);
+        if should_warn {
+            assert!(out
+                .stderr
+                .contains("warn: 'CARGO' is not the path of the binary about to be executed"));
+        } else {
+            assert!(!out.stderr.contains("warn"));
+        }
+    };
+
+    // Verify that `cargo --version` does not produce a warning.
+    run_real_cargo(&cx, &["--version"], false);
+
+    // `cargo foo --recursive-cargo` runs `cargo +nightly --version`.
+    run_real_cargo(&cx, &["foo", "--recursive-cargo"], true);
+
+    // `cargo foo --recursive-cargo-without-toolchain` runs `cargo --version` (no `+nightly`).
+    {
+        let tempdir = tempdir().unwrap();
+        raw::write_file(&tempdir.path().join("rust-toolchain"), "nightly").unwrap();
+        let cx = cx.change_dir(tempdir.path());
+        run_real_cargo(&cx, &["foo", "--recursive-cargo-without-toolchain"], true);
+    }
+
+    {
+        let tempdir = tempdir().unwrap();
+        cx.config
+            .expect_ok(&[
+                "rustup",
+                "override",
+                "set",
+                "nightly",
+                "--path",
+                &tempdir.path().to_string_lossy(),
+            ])
+            .await;
+        let cx = cx.change_dir(tempdir.path());
+        run_real_cargo(&cx, &["foo", "--recursive-cargo-without-toolchain"], true);
+    }
+}

tests/suite/cli_rustup.rs

@@ -583,19 +583,7 @@ async fn recursive_cargo() {
     let mut cx = CliTestContext::new(Scenario::ArchivesV2).await;
     cx.config.expect_ok(&["rustup", "default", "nightly"]).await;
 
-    // We need an intermediary to run cargo itself.
-    // The "mock" cargo can't do that because on Windows it will check
-    // for a `cargo.exe` in the current directory before checking PATH.
-    //
-    // The solution here is to copy from the "mock" `cargo.exe` into
-    // `~/.cargo/bin/cargo-foo`. This is just for convenience to avoid
-    // needing to build another executable just for this test.
-    let output = cx.config.run("rustup", ["which", "cargo"], &[]).await;
-    let real_mock_cargo = output.stdout.trim();
-    let cargo_bin_path = cx.config.cargodir.join("bin");
-    let cargo_subcommand = cargo_bin_path.join(format!("cargo-foo{}", EXE_SUFFIX));
-    fs::create_dir_all(&cargo_bin_path).unwrap();
-    fs::copy(real_mock_cargo, cargo_subcommand).unwrap();
+    clitools::create_cargo_foo(&cx).await;
 
     cx.config
         .expect_stdout_ok(&["cargo", "--recursive-cargo-subcommand"], "hash-nightly-2")