Skip to content

Commit

Permalink
fix: trace env vars from [env] table
Browse files Browse the repository at this point in the history
  • Loading branch information
linyihai committed Oct 24, 2024
1 parent 4a96a40 commit d126728
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 45 deletions.
23 changes: 18 additions & 5 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,6 @@
mod dirty_reason;

use std::collections::hash_map::{Entry, HashMap};

use std::env;
use std::fmt::{self, Display};
use std::fs::{self, File};
Expand All @@ -383,6 +382,7 @@ use tracing::{debug, info};

use crate::core::compiler::unit_graph::UnitDep;
use crate::core::Package;
use crate::util::context::EnvConfigValue;
use crate::util::errors::CargoResult;
use crate::util::interning::InternedString;
use crate::util::{self, try_canonicalize};
Expand Down Expand Up @@ -849,7 +849,14 @@ impl LocalFingerprint {
.to_string(),
)
} else {
gctx.get_env(key).ok()
if let Some(value) = gctx.env_config()?.get(key) {
value
.resolve(gctx)
.to_str()
.and_then(|s| Some(s.to_string()))
} else {
gctx.get_env(key).ok()
}
};
if current == *previous {
continue;
Expand Down Expand Up @@ -2124,6 +2131,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 `config_envs` 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,
Expand All @@ -2132,6 +2142,7 @@ pub fn translate_dep_info(
target_root: &Path,
rustc_cmd: &ProcessBuilder,
allow_package: bool,
config_envs: &Arc<HashMap<String, EnvConfigValue>>,
) -> CargoResult<()> {
let depinfo = parse_rustc_dep_info(rustc_dep_info)?;

Expand Down Expand Up @@ -2168,9 +2179,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 issue#13280, We trace env vars that are defined in the `config.toml`.
on_disk_info.env.retain(|(key, _)| {
config_envs.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
Expand Down
8 changes: 3 additions & 5 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ fn rustc(
if hide_diagnostics_for_scrape_unit {
output_options.show_diagnostics = false;
}

let config_envs = 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
Expand Down Expand Up @@ -459,6 +459,7 @@ fn rustc(
&rustc,
// Do not track source files in the fingerprint for registry dependencies.
is_local,
&config_envs,
)
.with_context(|| {
internal(format!(
Expand Down Expand Up @@ -1987,10 +1988,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.resolve(gctx));
}
Ok(())
}
Expand Down
67 changes: 37 additions & 30 deletions src/cargo/util/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -227,7 +227,7 @@ pub struct GlobalContext {
target_cfgs: LazyCell<Vec<(String, TargetCfgConfig)>>,
doc_extern_map: LazyCell<RustdocExternMap>,
progress_config: ProgressConfig,
env_config: LazyCell<EnvConfig>,
env_config: LazyCell<Arc<EnvConfig>>,
/// 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`
Expand Down Expand Up @@ -1833,34 +1833,41 @@ impl GlobalContext {
&self.progress_config
}

pub fn env_config(&self) -> CargoResult<&EnvConfig> {
let env_config = self
.env_config
.try_borrow_with(|| self.get::<EnvConfig>("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<EnvConfig>> {
let env_config = self.env_config.try_borrow_with(|| {
CargoResult::Ok(Arc::new({
let env_config = self.get::<EnvConfig>("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(|(k, v)| v.is_force() || self.get_env_os(k).is_none())
.collect()
}))
})?;

Ok(env_config)
}
Expand Down
12 changes: 7 additions & 5 deletions tests/testsuite/cargo_env_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]`
Expand All @@ -328,12 +329,12 @@ from-config
p.cargo("run")
.env("ENV_TEST", "from-env")
.with_stdout_data(str![[r#"
from-config
from-env
"#]])
.with_stderr_data(str![[r#"
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[RUNNING] `target/debug/foo[EXE]`
[RUNNING] `target/debug/foo`
"#]])
.run();
Expand Down Expand Up @@ -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]`
Expand All @@ -429,7 +431,7 @@ one

p.cargo(r#"run --config 'env.ENV_TEST="two"'"#)
.with_stdout_data(str![[r#"
one
two
"#]])
.with_stderr_data(str![[r#"
Expand Down

0 comments on commit d126728

Please sign in to comment.