Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: trace config [env] table in dep-info. #14701

Merged
merged 3 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -2132,6 +2139,7 @@ pub fn translate_dep_info(
target_root: &Path,
rustc_cmd: &ProcessBuilder,
allow_package: bool,
env_config: &Arc<HashMap<String, OsString>>,
) -> CargoResult<()> {
let depinfo = parse_rustc_dep_info(rustc_dep_info)?;

Expand Down Expand Up @@ -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
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 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
Expand Down Expand Up @@ -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!(
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);
}
Ok(())
}
Expand Down
73 changes: 43 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<HashMap<String, OsString>>>,
/// 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,47 @@ 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<HashMap<String, OsString>>> {
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_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)
}
Expand Down
165 changes: 165 additions & 0 deletions tests/testsuite/cargo_env_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,3 +276,168 @@ 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-env

"#]])
.with_stderr_data(str![[r#"
[COMPILING] foo v0.5.0 ([ROOT]/foo)
Copy link
Member

@weihanglo weihanglo Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is funny that if you change the env config to ENV_TEST = { value = "from-config", force = true } in this test, this cargo invocation triggers a rebuild, which I expect no.

[DIRTY] foo v0.5.0 ([ROOT]/foo): the environment variable ENV_TEST changed

Before this patch, it coinciddentally didn't trigger a rebuild because [env] was not tracked in depinfo. After this rebuilds because [env] doesn't take into account here when checking the old depinfo:

} else {
gctx.get_env(key).ok()
};

I think we should add a test case covering force = true scenario, and fix it. I have one solution in mind:

  • In GlobalContext::env_config we filter out env vars that are not force = true and already exist in the env snapshot (GlobalContext::get_env). That is, env_config() always return the set of envs we need to apply.
  • Could add a doc comment for env_config() pointing out the returning is pre-filtered against env snapshot, not the literal table value in [env].
  • In addition, because of this change, we could remove this condition in apply_env_config().
  • Also, everything, including the filter of disallowed envs, should be inside the try_borrow_with if possible to avoid non-necessary computation. Maybe we could go further that also resolve values inside it, so subsequent calls don't need to do it.

What do you think?

(I am afraid of missing any more thing. Environment variables in Cargo are a bit messy)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry that I discovered this problem so late 🙇🏾.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else {
gctx.get_env(key).ok()
};

Thank your for pointing out these. Here shoudld also apply the contions to figure out which env change should trigger a rebuild.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the PR so swiftly!

[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")
linyihai marked this conversation as resolved.
Show resolved Hide resolved
.env("ENV_TEST", "from-env")
.with_stdout_data(str![[r#"
from-env

"#]])
.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#"
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]`

"#]])
.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#"
two

"#]])
.with_stderr_data(str![[r#"
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[RUNNING] `target/debug/foo[EXE]`

"#]])
.run();
}