From d853d5dfcaa2c4ec3ef4daecf1e75b23daad2633 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 12 Nov 2024 15:46:33 -0500 Subject: [PATCH] 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. --- src/cargo/core/compiler/compilation.rs | 90 ++++++++++++++++++- .../core/compiler/fingerprint/dep_info.rs | 9 +- src/cargo/core/compiler/mod.rs | 5 ++ tests/testsuite/cargo_env_config.rs | 7 +- 4 files changed, 106 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 4830677b6d27..86736bc6ec62 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -1,8 +1,11 @@ //! Type definitions for the result of a compilation. -use std::collections::{BTreeSet, HashMap}; +use std::collections::BTreeSet; +use std::collections::HashMap; +use std::collections::HashSet; use std::ffi::{OsStr, OsString}; use std::path::PathBuf; +use std::sync::LazyLock; use cargo_platform::CfgExpr; use cargo_util::{paths, ProcessBuilder}; @@ -529,3 +532,88 @@ 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 +/// . +static ENV_VARS_SET_FOR_CRATES: LazyLock> = LazyLock::new(|| { + HashSet::from_iter([ + 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_") +} diff --git a/src/cargo/core/compiler/fingerprint/dep_info.rs b/src/cargo/core/compiler/fingerprint/dep_info.rs index e8628f34ebfd..595b85b465b2 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 key == CARGO_ENV { + return true; + } + if !rustc_cmd.get_envs().contains_key(key) { + return true; + } + env_config.contains_key(key) && !is_env_set_by_cargo(key) }); let serialize_path = |file| { diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 61d88a683839..407ba39dfbdf 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}; +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; @@ -334,6 +335,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 7c6bc35afa13..ffd4a687af43 100644 --- a/tests/testsuite/cargo_env_config.rs +++ b/tests/testsuite/cargo_env_config.rs @@ -460,8 +460,10 @@ fn override_env_set_by_cargo() { .build(); let args = [ - "--config", "env.CARGO_MANIFEST_DIR='Sauron'", - "--config", "env.CARGO_PKG_NAME='Saruman'", + "--config", + "env.CARGO_MANIFEST_DIR='Sauron'", + "--config", + "env.CARGO_PKG_NAME='Saruman'", ]; p.cargo("run") @@ -488,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]`