From 08b774626aa26f1f5fc52b9d247f9c93b4780544 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 12 Nov 2024 15:37:58 -0500 Subject: [PATCH 1/2] test(env): show rebuild when overriding env set by cargo This is a side effect when fixing 13280 via PR 14701. --- tests/testsuite/cargo_env_config.rs | 56 +++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/testsuite/cargo_env_config.rs b/tests/testsuite/cargo_env_config.rs index 5f420d31142..93a500bb09e 100644 --- a/tests/testsuite/cargo_env_config.rs +++ b/tests/testsuite/cargo_env_config.rs @@ -441,3 +441,59 @@ two "#]]) .run(); } + +#[cargo_test] +fn override_env_set_by_cargo() { + // Cargo disallows overridding envs set by itself. + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file( + "src/main.rs", + r#" + use std::env; + fn main() { + println!( "{}", env!("CARGO_MANIFEST_DIR") ); + println!( "{}", env!("CARGO_PKG_NAME") ); + } + "#, + ) + .build(); + + let args = [ + "--config", + "env.CARGO_MANIFEST_DIR='Sauron'", + "--config", + "env.CARGO_PKG_NAME='Saruman'", + ]; + + p.cargo("run") + .args(&args) + .with_stdout_data(str![[r#" +[ROOT]/foo +foo + +"#]]) + .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(); + + // The second run shouldn't trigger a rebuild + p.cargo("run") + .args(&args) + .with_stdout_data(str![[r#" +[ROOT]/foo +foo + +"#]]) + .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(); +} From 31295158261c89c723cfe050fc76b618f5e82ebf Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 12 Nov 2024 15:46:33 -0500 Subject: [PATCH 2/2] fix(env): dont track envs set by cargo in dep-info file This patch is quite hacky since the "set-by-Cargo" env vars are hardcoded. However, not all environment variables set by Cargo are statically known. To prevent the actual environment variables applied to rustc invocation get out of sync from what we hard-code, a debug time assertion is put to help discover missing environment variables earlier. --- crates/cargo-util/src/paths.rs | 2 +- src/cargo/core/compiler/compilation.rs | 95 +++++++++++++++++++ .../core/compiler/fingerprint/dep_info.rs | 9 +- src/cargo/core/compiler/mod.rs | 5 + tests/testsuite/cargo_env_config.rs | 1 - 5 files changed, 109 insertions(+), 3 deletions(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 5d7e3c5a681..c7fc5b19965 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -35,7 +35,7 @@ pub fn join_paths>(paths: &[T], env: &str) -> Result { /// Returns the name of the environment variable used for searching for /// dynamic libraries. -pub fn dylib_path_envvar() -> &'static str { +pub const fn dylib_path_envvar() -> &'static str { if cfg!(windows) { "PATH" } else if cfg!(target_os = "macos") { diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 7531f0fa61b..e2a50907839 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -527,3 +527,98 @@ fn target_linker(bcx: &BuildContext<'_, '_>, kind: CompileKind) -> CargoResult` or +/// `-Zbindeps` related env vars, we compare only their prefixes to determine +/// if they are internal. +/// See [`is_env_set_by_cargo`] and +/// . +const ENV_VARS_SET_FOR_CRATES: [&str; 23] = [ + crate::CARGO_ENV, + "CARGO_MANIFEST_DIR", + "CARGO_MANIFEST_PATH", + "CARGO_PKG_VERSION", + "CARGO_PKG_VERSION_MAJOR", + "CARGO_PKG_VERSION_MINOR", + "CARGO_PKG_VERSION_PATCH", + "CARGO_PKG_VERSION_PRE", + "CARGO_PKG_AUTHORS", + "CARGO_PKG_NAME", + "CARGO_PKG_DESCRIPTION", + "CARGO_PKG_HOMEPAGE", + "CARGO_PKG_REPOSITORY", + "CARGO_PKG_LICENSE", + "CARGO_PKG_LICENSE_FILE", + "CARGO_PKG_RUST_VERSION", + "CARGO_PKG_README", + "CARGO_CRATE_NAME", + "CARGO_BIN_NAME", + "OUT_DIR", + "CARGO_PRIMARY_PACKAGE", + "CARGO_TARGET_TMPDIR", + paths::dylib_path_envvar(), +]; +/// Asserts if the given env vars are controlled by Cargo. +/// +/// This is only for reminding Cargo developer to keep newly added environment +/// variables in sync with [`ENV_VARS_SET_FOR_CRATES`]. +#[cfg(debug_assertions)] +pub(crate) fn assert_only_envs_set_by_cargo<'a>( + keys: impl Iterator>, + env_config: &HashMap, +) { + for key in keys { + let key = key.as_ref(); + // When running Cargo's test suite, + // we're fine if it is from the `[env]` table + if env_config.contains_key(key) { + continue; + } + assert!( + is_env_set_by_cargo(key), + "`{key}` is not tracked as an environment variable set by Cargo\n\ + Add it to `ENV_VARS_SET_FOR_CRATES` if you intend to introduce a new one" + ); + } +} + +/// True if the given env var is controlled or set by Cargo. +/// See [`ENV_VARS_SET_FOR_CRATES`]. +pub(crate) fn is_env_set_by_cargo(key: &str) -> bool { + ENV_VARS_SET_FOR_CRATES.contains(&key) + || key.starts_with("CARGO_BIN_EXE_") + || key.starts_with("__CARGO") // internal/test-only envs + || key == "RUSTC_BOOTSTRAP" // for -Zbuild-std + || is_artifact_dep_env_vars(key) +} + +/// Whether an env var is set because of `-Zbindeps`. +fn is_artifact_dep_env_vars(key: &str) -> bool { + let Some(key) = key.strip_prefix("CARGO_") else { + return false; + }; + let Some(key) = key + .strip_prefix("BIN_") + .or_else(|| key.strip_prefix("CDYLIB_")) + .or_else(|| key.strip_prefix("STATICLIB_")) + else { + return false; + }; + key.starts_with("FILE_") || key.starts_with("DIR_") +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use super::ENV_VARS_SET_FOR_CRATES; + + #[test] + fn ensure_env_vars_set_for_crates_unique() { + let set: HashSet<&str> = HashSet::from_iter(ENV_VARS_SET_FOR_CRATES); + assert_eq!(ENV_VARS_SET_FOR_CRATES.len(), set.len()); + } +} diff --git a/src/cargo/core/compiler/fingerprint/dep_info.rs b/src/cargo/core/compiler/fingerprint/dep_info.rs index e8628f34ebf..0a3f926b8fb 100644 --- a/src/cargo/core/compiler/fingerprint/dep_info.rs +++ b/src/cargo/core/compiler/fingerprint/dep_info.rs @@ -19,6 +19,7 @@ use cargo_util::paths; use cargo_util::ProcessBuilder; use cargo_util::Sha256; +use crate::core::compiler::is_env_set_by_cargo; use crate::CargoResult; use crate::CARGO_ENV; @@ -334,7 +335,13 @@ pub fn translate_dep_info( // // 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 + if env_config.contains_key(key) && !is_env_set_by_cargo(key) { + return true; + } + if !rustc_cmd.get_envs().contains_key(key) { + return true; + } + key == CARGO_ENV }); let serialize_path = |file| { diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 5dc34616d91..6b7526c08a5 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -74,6 +74,7 @@ pub use self::build_context::{ }; use self::build_plan::BuildPlan; pub use self::build_runner::{BuildRunner, Metadata, UnitHash}; +pub(crate) use self::compilation::is_env_set_by_cargo; pub use self::compilation::{Compilation, Doctest, UnitOutput}; pub use self::compile_kind::{CompileKind, CompileTarget}; pub use self::crate_type::CrateType; @@ -331,6 +332,10 @@ fn rustc( output_options.show_diagnostics = false; } let env_config = Arc::clone(build_runner.bcx.gctx.env_config()?); + + #[cfg(debug_assertions)] + compilation::assert_only_envs_set_by_cargo(rustc.get_envs().keys(), &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 diff --git a/tests/testsuite/cargo_env_config.rs b/tests/testsuite/cargo_env_config.rs index 93a500bb09e..ffd4a687af4 100644 --- a/tests/testsuite/cargo_env_config.rs +++ b/tests/testsuite/cargo_env_config.rs @@ -490,7 +490,6 @@ foo "#]]) .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]`