From 9b41df94612a87dfcfdd2e8b7ce47518cfef6c8c Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Tue, 25 Oct 2022 18:49:23 +0000 Subject: [PATCH] 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 | 8 ++++-- src/cargo/ops/registry/auth.rs | 2 +- src/cargo/util/config/mod.rs | 15 ++++++++++- .../src/reference/environment-variables.md | 6 +++++ tests/testsuite/build_script.rs | 25 ++++++++++++++++--- tests/testsuite/cargo_command.rs | 13 +++++++++- tests/testsuite/test.rs | 13 ++++++++++ 7 files changed, 74 insertions(+), 8 deletions(-) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 37976e0f50ec..4ce0162bfbd2 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/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) } diff --git a/src/cargo/ops/registry/auth.rs b/src/cargo/ops/registry/auth.rs index 648e051e6dc2..85b51a50ab0e 100644 --- a/src/cargo/ops/registry/auth.rs +++ b/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 { diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 765c93db7042..d2fa0b5caf97 100644 --- a/src/cargo/util/config/mod.rs +++ b/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 { + // Try re-using the `cargo` set in the environment already. This allows + // commands that use Cargo as a library to inherit (via `cargo `) + // 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 { // 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) diff --git a/src/doc/src/reference/environment-variables.md b/src/doc/src/reference/environment-variables.md index 42417b8a05b7..d49922afbfde 100644 --- a/src/doc/src/reference/environment-variables.md +++ b/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 diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 8157b9e73aa7..feac01a49533 100644 --- a/tests/testsuite/build_script.rs +++ b/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,6 +82,13 @@ 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#" use std::env; @@ -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, "{rustc}"); + }} else {{ + assert_eq!(cargo, "{cargo}"); + }} let rustc = env::var("RUSTC").unwrap(); assert_eq!(rustc, "rustc"); @@ -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] diff --git a/tests/testsuite/cargo_command.rs b/tests/testsuite/cargo_command.rs index ad969d1ba6eb..e0cf47b54f85 100644 --- a/tests/testsuite/cargo_command.rs +++ b/tests/testsuite/cargo_command.rs @@ -335,13 +335,24 @@ 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); + 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] diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index b6e21775cc51..88f5f34c2ff7 100644 --- a/tests/testsuite/test.rs +++ b/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]