Auto merge of #11285 - jonhoo:cargo-env, r=weihanglo

Make cargo forward pre-existing CARGO if set

Currently, Cargo will always set `$CARGO` to point to what it detects its own path to be (using `std::env::current_exe`). Unfortunately, this runs into trouble when Cargo is used as a library, or when `current_exe` is not actually the binary itself (e.g., when invoked through Valgrind or `ld.so`), since `$CARGO` will not point at something that can be used as `cargo`. This, in turn, means that users can't currently rely on `$CARGO` to do the right thing, and will sometimes have to invoke `cargo` directly from `$PATH` instead, which may not reflect the `cargo` that's currently in use.

This patch makes Cargo re-use the existing value of `$CARGO` if it's already set in the environment. For Cargo subcommands, this will mean that the initial invocation of `cargo` in `cargo foo` will set `$CARGO`, and then Cargo-as-a-library inside of `cargo-foo` will inherit that (correct) value instead of overwriting it with the incorrect value `cargo-foo`. For other execution environments that do not have `cargo` in their call stack, it gives them the opportunity to set a working value for `$CARGO`.

One note about the implementation of this is that the test suite now needs to override `$CARGO` explicitly so that the _user's_ `$CARGO` does not interfere with the contents of the tests. It _could_ remove `$CARGO` instead, but overriding it seemed less error-prone.

Fixes #10119.
Fixes #10113.

crates/cargo-test-support/src/lib.rs

@@ -410,8 +410,10 @@ impl Project {
     /// Example:
     ///     p.cargo("build --bin foo").run();
     pub fn cargo(&self, cmd: &str) -> Execs {
-        let mut execs = self.process(&cargo_exe());
+        let cargo = cargo_exe();
+        let mut execs = self.process(&cargo);
         if let Some(ref mut p) = execs.process_builder {
+            p.env("CARGO", cargo);
             p.arg_line(cmd);
         }
         execs
@@ -1328,7 +1330,9 @@ impl ArgLine for snapbox::cmd::Command {
 }
 
 pub fn cargo_process(s: &str) -> Execs {
-    let mut p = process(&cargo_exe());
+    let cargo = cargo_exe();
+    let mut p = process(&cargo);
+    p.env("CARGO", cargo);
     p.arg_line(s);
     execs().with_process_builder(p)
 }

src/cargo/ops/registry/auth.rs

@@ -123,7 +123,7 @@ fn run_command(
 
     let mut cmd = Command::new(&exe);
     cmd.args(args)
-        .env("CARGO", config.cargo_exe()?)
+        .env(crate::CARGO_ENV, config.cargo_exe()?)
         .env("CARGO_REGISTRY_NAME", name)
         .env("CARGO_REGISTRY_API_URL", api_url);
     match action {

src/cargo/util/config/mod.rs

@@ -406,6 +406,18 @@ impl Config {
     pub fn cargo_exe(&self) -> CargoResult<&Path> {
         self.cargo_exe
             .try_borrow_with(|| {
+                fn from_env() -> CargoResult<PathBuf> {
+                    // Try re-using the `cargo` set in the environment already. This allows
+                    // commands that use Cargo as a library to inherit (via `cargo <subcommand>`)
+                    // or set (by setting `$CARGO`) a correct path to `cargo` when the current exe
+                    // is not actually cargo (e.g., `cargo-*` binaries, Valgrind, `ld.so`, etc.).
+                    let exe = env::var_os(crate::CARGO_ENV)
+                        .map(PathBuf::from)
+                        .ok_or_else(|| anyhow!("$CARGO not set"))?
+                        .canonicalize()?;
+                    Ok(exe)
+                }
+
                 fn from_current_exe() -> CargoResult<PathBuf> {
                     // Try fetching the path to `cargo` using `env::current_exe()`.
                     // The method varies per operating system and might fail; in particular,
@@ -431,7 +443,8 @@ impl Config {
                     paths::resolve_executable(&argv0)
                 }
 
-                let exe = from_current_exe()
+                let exe = from_env()
+                    .or_else(|_| from_current_exe())
                     .or_else(|_| from_argv())
                     .with_context(|| "couldn't get the path to cargo executable")?;
                 Ok(exe)

src/doc/src/reference/environment-variables.md

@@ -23,6 +23,12 @@ system:
 * `CARGO_TARGET_DIR` — Location of where to place all generated artifacts,
   relative to the current working directory. See [`build.target-dir`] to set
   via config.
+* `CARGO` - If set, Cargo will forward this value instead of setting it
+  to its own auto-detected path when it builds crates and when it
+  executes build scripts and external subcommands. This value is not
+  directly executed by Cargo, and should always point at a command that
+  behaves exactly like `cargo`, as that's what users of the variable
+  will be expecting.
 * `RUSTC` — Instead of running `rustc`, Cargo will execute this specified
   compiler instead. See [`build.rustc`] to set via config.
 * `RUSTC_WRAPPER` — Instead of simply running `rustc`, Cargo will execute this

tests/testsuite/build_script.rs

@@ -4,9 +4,11 @@ use cargo_test_support::compare::assert_match_exact;
 use cargo_test_support::paths::CargoPathExt;
 use cargo_test_support::registry::Package;
 use cargo_test_support::tools;
-use cargo_test_support::{basic_manifest, cross_compile, is_coarse_mtime, project, project_in};
+use cargo_test_support::{
+    basic_manifest, cargo_exe, cross_compile, is_coarse_mtime, project, project_in,
+};
 use cargo_test_support::{rustc_host, sleep_ms, slow_cpu_multiplier, symlink_supported};
-use cargo_util::paths::remove_dir_all;
+use cargo_util::paths::{self, remove_dir_all};
 use std::env;
 use std::fs;
 use std::io;
@@ -80,8 +82,15 @@ fn custom_build_env_vars() {
         )
         .file("bar/src/lib.rs", "pub fn hello() {}");
 
+    let cargo = cargo_exe().canonicalize().unwrap();
+    let cargo = cargo.to_str().unwrap();
+    let rustc = paths::resolve_executable("rustc".as_ref())
+        .unwrap()
+        .canonicalize()
+        .unwrap();
+    let rustc = rustc.to_str().unwrap();
     let file_content = format!(
-        r#"
+        r##"
             use std::env;
             use std::path::Path;
 
@@ -107,7 +116,12 @@ fn custom_build_env_vars() {
 
                 let _feat = env::var("CARGO_FEATURE_FOO").unwrap();
 
-                let _cargo = env::var("CARGO").unwrap();
+                let cargo = env::var("CARGO").unwrap();
+                if env::var_os("CHECK_CARGO_IS_RUSTC").is_some() {{
+                    assert_eq!(cargo, r#"{rustc}"#);
+                }} else {{
+                    assert_eq!(cargo, r#"{cargo}"#);
+                }}
 
                 let rustc = env::var("RUSTC").unwrap();
                 assert_eq!(rustc, "rustc");
@@ -124,7 +138,7 @@ fn custom_build_env_vars() {
                 let rustflags = env::var("CARGO_ENCODED_RUSTFLAGS").unwrap();
                 assert_eq!(rustflags, "");
             }}
-        "#,
+        "##,
         p.root()
             .join("target")
             .join("debug")
@@ -135,6 +149,11 @@ fn custom_build_env_vars() {
     let p = p.file("bar/build.rs", &file_content).build();
 
     p.cargo("build --features bar_feat").run();
+    p.cargo("build --features bar_feat")
+        // we use rustc since $CARGO is only used if it points to a path that exists
+        .env("CHECK_CARGO_IS_RUSTC", "1")
+        .env(cargo::CARGO_ENV, rustc)
+        .run();
 }
 
 #[cargo_test]

tests/testsuite/cargo_command.rs

@@ -335,13 +335,26 @@ fn cargo_subcommand_env() {
 
     let cargo = cargo_exe().canonicalize().unwrap();
     let mut path = path();
-    path.push(target_dir);
+    path.push(target_dir.clone());
     let path = env::join_paths(path.iter()).unwrap();
 
     cargo_process("envtest")
         .env("PATH", &path)
         .with_stdout(cargo.to_str().unwrap())
         .run();
+
+    // Check that subcommands inherit an overriden $CARGO
+    let envtest_bin = target_dir
+        .join("cargo-envtest")
+        .with_extension(std::env::consts::EXE_EXTENSION)
+        .canonicalize()
+        .unwrap();
+    let envtest_bin = envtest_bin.to_str().unwrap();
+    cargo_process("envtest")
+        .env("PATH", &path)
+        .env(cargo::CARGO_ENV, &envtest_bin)
+        .with_stdout(envtest_bin)
+        .run();
 }
 
 #[cargo_test]

tests/testsuite/test.rs

@@ -3485,6 +3485,19 @@ fn cargo_test_env() {
         .with_stderr_contains(cargo.to_str().unwrap())
         .with_stdout_contains("test env_test ... ok")
         .run();
+
+    // Check that `cargo test` propagates the environment's $CARGO
+    let rustc = cargo_util::paths::resolve_executable("rustc".as_ref())
+        .unwrap()
+        .canonicalize()
+        .unwrap();
+    let rustc = rustc.to_str().unwrap();
+    p.cargo("test --lib -- --nocapture")
+        // we use rustc since $CARGO is only used if it points to a path that exists
+        .env(cargo::CARGO_ENV, rustc)
+        .with_stderr_contains(rustc)
+        .with_stdout_contains("test env_test ... ok")
+        .run();
 }
 
 #[cargo_test]