From 02d2c77d8780ec2455b0cd3e4febebed404dd44f Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Wed, 16 Oct 2024 17:42:19 +0800 Subject: [PATCH 1/3] test: add test to describe the issue#13280 Cargo fails to detect environment variable is newly set and rebuild if the environment variable is set in `config.toml` --- tests/testsuite/cargo_env_config.rs | 163 ++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/tests/testsuite/cargo_env_config.rs b/tests/testsuite/cargo_env_config.rs index 369a5a8699e..3f10efd2fab 100644 --- a/tests/testsuite/cargo_env_config.rs +++ b/tests/testsuite/cargo_env_config.rs @@ -276,3 +276,166 @@ MAIN ENV_TEST:from-env "#]]) .run(); } + +#[cargo_test] +fn env_changed_defined_in_config_toml() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file( + "src/main.rs", + r#" + use std::env; + fn main() { + println!( "{}", env!("ENV_TEST") ); + } + "#, + ) + .file( + ".cargo/config.toml", + r#" + [env] + ENV_TEST = "from-config" + "#, + ) + .build(); + + p.cargo("run") + .with_stdout_data(str![[r#" +from-config + +"#]]) + .with_stderr_data(str![[r#" +[COMPILING] foo v0.5.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); + + p.cargo("run") + .env("ENV_TEST", "from-env") + .with_stdout_data(str![[r#" +from-config + +"#]]) + .with_stderr_data(str![[r#" +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); + // This identical cargo invocation is to ensure no rebuild happen. + p.cargo("run") + .env("ENV_TEST", "from-env") + .with_stdout_data(str![[r#" +from-config + +"#]]) + .with_stderr_data(str![[r#" +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); +} + +#[cargo_test] +fn forced_env_changed_defined_in_config_toml() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file( + "src/main.rs", + r#" + use std::env; + fn main() { + println!( "{}", env!("ENV_TEST") ); + } + "#, + ) + .file( + ".cargo/config.toml", + r#" + [env] + ENV_TEST = {value = "from-config", force = true} + "#, + ) + .build(); + + p.cargo("run") + .with_stdout_data(str![[r#" +from-config + +"#]]) + .with_stderr_data(str![[r#" +[COMPILING] foo v0.5.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); + + p.cargo("run") + .env("ENV_TEST", "from-env") + .with_stdout_data(str![[r#" +from-config + +"#]]) + .with_stderr_data(str![[r#" +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); +} + +#[cargo_test] +fn env_changed_defined_in_config_args() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file( + "src/main.rs", + r#" + use std::env; + fn main() { + println!( "{}", env!("ENV_TEST") ); + } + "#, + ) + .build(); + p.cargo(r#"run --config 'env.ENV_TEST="one"'"#) + .with_stdout_data(str![[r#" +one + +"#]]) + .with_stderr_data(str![[r#" +[COMPILING] foo v0.5.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); + + p.cargo(r#"run --config 'env.ENV_TEST="two"'"#) + .with_stdout_data(str![[r#" +one + +"#]]) + .with_stderr_data(str![[r#" +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); + // This identical cargo invocation is to ensure no rebuild happen. + p.cargo(r#"run --config 'env.ENV_TEST="two"'"#) + .with_stdout_data(str![[r#" +one + +"#]]) + .with_stderr_data(str![[r#" +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +[RUNNING] `target/debug/foo[EXE]` + +"#]]) + .run(); +} From edd073210869e6e36c96e479c23e13444827d636 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Fri, 25 Oct 2024 11:12:36 +0800 Subject: [PATCH 2/3] refactor: Use `Arc` wraps `GlobalContext::env_config` and resolve value in advance. --- src/cargo/core/compiler/mod.rs | 5 +-- src/cargo/util/context/mod.rs | 73 ++++++++++++++++++++-------------- 2 files changed, 44 insertions(+), 34 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index ee5fdbbecd6..ff830c66d88 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -1987,10 +1987,7 @@ pub(crate) fn apply_env_config( if cmd.get_envs().contains_key(key) { continue; } - - if value.is_force() || gctx.get_env_os(key).is_none() { - cmd.env(key, value.resolve(gctx)); - } + cmd.env(key, value); } Ok(()) } diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index c38fd6fd55a..e16bffb3c7a 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -63,7 +63,7 @@ use std::io::SeekFrom; use std::mem; use std::path::{Path, PathBuf}; use std::str::FromStr; -use std::sync::Once; +use std::sync::{Arc, Once}; use std::time::Instant; use self::ConfigValue as CV; @@ -227,7 +227,7 @@ pub struct GlobalContext { target_cfgs: LazyCell>, doc_extern_map: LazyCell, progress_config: ProgressConfig, - env_config: LazyCell, + env_config: LazyCell>>, /// This should be false if: /// - this is an artifact of the rustc distribution process for "stable" or for "beta" /// - this is an `#[test]` that does not opt in with `enable_nightly_features` @@ -1833,34 +1833,47 @@ impl GlobalContext { &self.progress_config } - pub fn env_config(&self) -> CargoResult<&EnvConfig> { - let env_config = self - .env_config - .try_borrow_with(|| self.get::("env"))?; - - // Reasons for disallowing these values: - // - // - CARGO_HOME: The initial call to cargo does not honor this value - // from the [env] table. Recursive calls to cargo would use the new - // value, possibly behaving differently from the outer cargo. - // - // - RUSTUP_HOME and RUSTUP_TOOLCHAIN: Under normal usage with rustup, - // this will have no effect because the rustup proxy sets - // RUSTUP_HOME and RUSTUP_TOOLCHAIN, and that would override the - // [env] table. If the outer cargo is executed directly - // circumventing the rustup proxy, then this would affect calls to - // rustc (assuming that is a proxy), which could potentially cause - // problems with cargo and rustc being from different toolchains. We - // consider this to be not a use case we would like to support, - // since it will likely cause problems or lead to confusion. - for disallowed in &["CARGO_HOME", "RUSTUP_HOME", "RUSTUP_TOOLCHAIN"] { - if env_config.contains_key(*disallowed) { - bail!( - "setting the `{disallowed}` environment variable is not supported \ - in the `[env]` configuration table" - ); - } - } + /// Get the env vars from the config `[env]` table which + /// are `force = true` or don't exist in the env snapshot [`GlobalContext::get_env`]. + pub fn env_config(&self) -> CargoResult<&Arc>> { + let env_config = self.env_config.try_borrow_with(|| { + CargoResult::Ok(Arc::new({ + let env_config = self.get::("env")?; + // Reasons for disallowing these values: + // + // - CARGO_HOME: The initial call to cargo does not honor this value + // from the [env] table. Recursive calls to cargo would use the new + // value, possibly behaving differently from the outer cargo. + // + // - RUSTUP_HOME and RUSTUP_TOOLCHAIN: Under normal usage with rustup, + // this will have no effect because the rustup proxy sets + // RUSTUP_HOME and RUSTUP_TOOLCHAIN, and that would override the + // [env] table. If the outer cargo is executed directly + // circumventing the rustup proxy, then this would affect calls to + // rustc (assuming that is a proxy), which could potentially cause + // problems with cargo and rustc being from different toolchains. We + // consider this to be not a use case we would like to support, + // since it will likely cause problems or lead to confusion. + for disallowed in &["CARGO_HOME", "RUSTUP_HOME", "RUSTUP_TOOLCHAIN"] { + if env_config.contains_key(*disallowed) { + bail!( + "setting the `{disallowed}` environment variable is not supported \ + in the `[env]` configuration table" + ); + } + } + env_config + .into_iter() + .filter_map(|(k, v)| { + if v.is_force() || self.get_env_os(&k).is_none() { + Some((k, v.resolve(self).to_os_string())) + } else { + None + } + }) + .collect() + })) + })?; Ok(env_config) } From 668d82e3f0461ebbcb8eb2d86a8150be494a9427 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Fri, 25 Oct 2024 11:21:52 +0800 Subject: [PATCH 3/3] fix: trace `[env]` config table into fingerprint. --- src/cargo/core/compiler/fingerprint/mod.rs | 20 +++++++++++++++----- src/cargo/core/compiler/mod.rs | 3 ++- tests/testsuite/cargo_env_config.rs | 10 ++++++---- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index e6fff90932a..0b8a5505919 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -362,8 +362,8 @@ mod dirty_reason; use std::collections::hash_map::{Entry, HashMap}; - use std::env; +use std::ffi::OsString; use std::fmt::{self, Display}; use std::fs::{self, File}; use std::hash::{self, Hash, Hasher}; @@ -849,7 +849,11 @@ impl LocalFingerprint { .to_string(), ) } else { - gctx.get_env(key).ok() + if let Some(value) = gctx.env_config()?.get(key) { + value.to_str().and_then(|s| Some(s.to_string())) + } else { + gctx.get_env(key).ok() + } }; if current == *previous { continue; @@ -2124,6 +2128,9 @@ enum DepInfoPathType { /// /// The serialized Cargo format will contain a list of files, all of which are /// relative if they're under `root`. or absolute if they're elsewhere. +/// +/// The `env_config` argument is a set of environment variables that are +/// defined in `[env]` table of the `config.toml`. pub fn translate_dep_info( rustc_dep_info: &Path, cargo_dep_info: &Path, @@ -2132,6 +2139,7 @@ pub fn translate_dep_info( target_root: &Path, rustc_cmd: &ProcessBuilder, allow_package: bool, + env_config: &Arc>, ) -> CargoResult<()> { let depinfo = parse_rustc_dep_info(rustc_dep_info)?; @@ -2168,9 +2176,11 @@ pub fn translate_dep_info( // This also includes `CARGO` since if the code is explicitly wanting to // know that path, it should be rebuilt if it changes. The CARGO path is // not tracked elsewhere in the fingerprint. - on_disk_info - .env - .retain(|(key, _)| !rustc_cmd.get_envs().contains_key(key) || key == CARGO_ENV); + // + // For cargo#13280, We trace env vars that are defined in the `[env]` config table. + on_disk_info.env.retain(|(key, _)| { + env_config.contains_key(key) || !rustc_cmd.get_envs().contains_key(key) || key == CARGO_ENV + }); let serialize_path = |file| { // The path may be absolute or relative, canonical or not. Make sure diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index ff830c66d88..5d0e2962c22 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -330,7 +330,7 @@ fn rustc( if hide_diagnostics_for_scrape_unit { output_options.show_diagnostics = false; } - + let env_config = Arc::clone(build_runner.bcx.gctx.env_config()?); return Ok(Work::new(move |state| { // Artifacts are in a different location than typical units, // hence we must assure the crate- and target-dependent @@ -459,6 +459,7 @@ fn rustc( &rustc, // Do not track source files in the fingerprint for registry dependencies. is_local, + &env_config, ) .with_context(|| { internal(format!( diff --git a/tests/testsuite/cargo_env_config.rs b/tests/testsuite/cargo_env_config.rs index 3f10efd2fab..5f420d31142 100644 --- a/tests/testsuite/cargo_env_config.rs +++ b/tests/testsuite/cargo_env_config.rs @@ -315,10 +315,11 @@ from-config p.cargo("run") .env("ENV_TEST", "from-env") .with_stdout_data(str![[r#" -from-config +from-env "#]]) .with_stderr_data(str![[r#" +[COMPILING] foo v0.5.0 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s [RUNNING] `target/debug/foo[EXE]` @@ -328,7 +329,7 @@ from-config p.cargo("run") .env("ENV_TEST", "from-env") .with_stdout_data(str![[r#" -from-config +from-env "#]]) .with_stderr_data(str![[r#" @@ -417,10 +418,11 @@ one p.cargo(r#"run --config 'env.ENV_TEST="two"'"#) .with_stdout_data(str![[r#" -one +two "#]]) .with_stderr_data(str![[r#" +[COMPILING] foo v0.5.0 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s [RUNNING] `target/debug/foo[EXE]` @@ -429,7 +431,7 @@ one // This identical cargo invocation is to ensure no rebuild happen. p.cargo(r#"run --config 'env.ENV_TEST="two"'"#) .with_stdout_data(str![[r#" -one +two "#]]) .with_stderr_data(str![[r#"