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(env): cargo:rerun-if-env-changed doesn't work with env configuration #14058

Closed
wants to merge 3 commits into from
Closed
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
6 changes: 6 additions & 0 deletions src/cargo/core/compiler/build_context/target_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ use cargo_util::{paths, ProcessBuilder};
use serde::{Deserialize, Serialize};
use std::cell::RefCell;
use std::collections::hash_map::{Entry, HashMap};
use std::collections::BTreeMap;
use std::ffi::OsString;
use std::path::{Path, PathBuf};
use std::str::{self, FromStr};

Expand Down Expand Up @@ -588,6 +590,10 @@ impl TargetInfo {
.iter()
.any(|sup| sup.as_str() == split.as_str())
}

pub fn get_target_envs(&self) -> &BTreeMap<String, Option<OsString>> {
return self.crate_type_process.get_envs();
Copy link
Member

Choose a reason for hiding this comment

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

Accessing crate_type_process seems like a hacky shortcut to me. The process is for learning target info from rustc, but fingerprint is for actual rustc compilation. One extension point for putting it here is for supporting #10273, though I believe that will need other rounds of env var system overhual in Cargo.

Copy link
Contributor

@ehuss ehuss Jul 24, 2024

Choose a reason for hiding this comment

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

I would agree, the chance of this having unintended consequences seems kinda high since it isn't obvious when looking at TargetInfo that this would also influence build-script environment fingerprinting.

Would it be possible to rewrite apply_env_config so that there is a separate function that yields a map of resolved config values? Then the code in the fingerprint module could call that and collect the results into a map that it could then pass into the closure.

(I also wonder if it ends up creating a new map, if it could pre-normalize it on Windows to avoid the inner case-check loop.)

}
}

/// Takes rustc output (using specialized command line args), and calculates the file prefix and
Expand Down
51 changes: 43 additions & 8 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,9 @@ mod dirty_reason;

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

use std::collections::BTreeMap;
use std::env;
use std::ffi::OsString;
use std::hash::{self, Hash, Hasher};
use std::io;
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -768,14 +770,35 @@ pub enum StaleItem {
impl LocalFingerprint {
/// Read the environment variable of the given env `key`, and creates a new
/// [`LocalFingerprint::RerunIfEnvChanged`] for it.
///
// TODO: This is allowed at this moment. Should figure out if it makes
// sense if permitting to read env from the config system.
Comment on lines -772 to -773
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing this, can we have a comment explaining why the allow can't be removed? It's not obvious to me that there is some fundamental problem that is preventing this.

I presume the problem is that the closure cannot access the GlobalContext, and thus is unable to access the env snapshot, and thus must access the raw environment directly? It seems like there are still options we can consider, such as wrapping the environment in Arc or something like that?

Copy link
Member

Choose a reason for hiding this comment

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

such as wrapping the environment in Arc

Agree Arc is the way to resolve this.

#[allow(clippy::disallowed_methods)]
fn from_env<K: AsRef<str>>(key: K) -> LocalFingerprint {
let key = key.as_ref();
fn from_env<K: AsRef<str>>(
epage marked this conversation as resolved.
Show resolved Hide resolved
key: K,
envs: &BTreeMap<String, Option<OsString>>,
) -> LocalFingerprint {
fn get_envs_case_insensitive(
key: &str,
envs: &BTreeMap<String, Option<OsString>>,
) -> Option<OsString> {
let upper_case_key: String = key.to_uppercase();
for (k, v) in envs {
Copy link
Member

Choose a reason for hiding this comment

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

This conversion seems to be in a deeply nested loop. M rerun_if_env_changed * N [env] keys on Windows. We should probably find a way to do it once and for all.

This doesn't really seem to be a performance issue for general usage though.

Copy link
Contributor

Choose a reason for hiding this comment

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

A small fix would be to check the map with the key as-is, and then only check for the case-insensitive if that fails. 99% of the time the case will match. EnvConfig::get_env_os does something similar.

if k.to_uppercase().eq(&upper_case_key) {
return v.to_owned();
}
}
None
}

let key: &str = key.as_ref();
let var = key.to_owned();
let val = env::var(key).ok();

let val: Option<String> = if cfg!(windows) {
get_envs_case_insensitive(key, envs)
} else {
envs.get(key).and_then(|v| v.to_owned())
}
.or(env::var_os(key))
Copy link
Member

Choose a reason for hiding this comment

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

Lazy evaluation

Suggested change
.or(env::var_os(key))
.or_else(|| env::var_os(key))

.and_then(|os_str| os_str.into_string().ok());

LocalFingerprint::RerunIfEnvChanged { var, val }
}

Expand Down Expand Up @@ -1608,6 +1631,12 @@ fn build_script_local_fingerprints(
bool,
) {
assert!(unit.mode.is_run_custom_build());
let envs = build_runner
.bcx
.target_data
.info(unit.kind)
.get_target_envs()
.clone();
epage marked this conversation as resolved.
Show resolved Hide resolved
// First up, if this build script is entirely overridden, then we just
// return the hash of what we overrode it with. This is the easy case!
if let Some(fingerprint) = build_script_override_fingerprint(build_runner, unit) {
Expand Down Expand Up @@ -1660,7 +1689,12 @@ fn build_script_local_fingerprints(
// Ok so now we're in "new mode" where we can have files listed as
// dependencies as well as env vars listed as dependencies. Process
// them all here.
Ok(Some(local_fingerprints_deps(deps, &target_dir, &pkg_root)))
Ok(Some(local_fingerprints_deps(
deps,
&target_dir,
&pkg_root,
&envs,
)))
};

// Note that `false` == "not overridden"
Expand Down Expand Up @@ -1695,6 +1729,7 @@ fn local_fingerprints_deps(
deps: &BuildDeps,
target_root: &Path,
pkg_root: &Path,
envs: &BTreeMap<String, Option<OsString>>,
) -> Vec<LocalFingerprint> {
debug!("new local fingerprints deps {:?}", pkg_root);
let mut local = Vec::new();
Expand All @@ -1719,7 +1754,7 @@ fn local_fingerprints_deps(
local.extend(
deps.rerun_if_env_changed
.iter()
.map(LocalFingerprint::from_env),
.map(|v| LocalFingerprint::from_env(v, &envs)),
);

local
Expand Down
6 changes: 2 additions & 4 deletions src/doc/src/reference/build-scripts.md
Original file line number Diff line number Diff line change
Expand Up @@ -387,10 +387,8 @@ The `rerun-if-env-changed` instruction tells Cargo to re-run the build script
if the value of an environment variable of the given name has changed.

Note that the environment variables here are intended for global environment
variables like `CC` and such, it is not possible to use this for environment
variables like `TARGET` that [Cargo sets for build scripts][build-env]. The
environment variables in use are those received by `cargo` invocations, not
those received by the executable of the build script.
variables like `CC` and such, it is not necessary to use this for environment
variables like `TARGET` that Cargo sets.

## The `links` Manifest Key

Expand Down
142 changes: 142 additions & 0 deletions tests/testsuite/build_script_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,148 @@ use cargo_test_support::project;
use cargo_test_support::sleep_ms;
use cargo_test_support::str;

#[cargo_test]
fn rerun_if_env_changes_config() {
epage marked this conversation as resolved.
Show resolved Hide resolved
let p = project()
.file("Cargo.toml", &basic_manifest("foo", "0.1.0"))
.file("src/main.rs", "fn main() {}")
.file(
".cargo/config.toml",
r#"
[env]
FOO = "good"
"#,
)
.file(
"build.rs",
r#"
fn main() {
Copy link
Member

Choose a reason for hiding this comment

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

This formatting change shouldn't have happened, or should merge into the first commit adding the test.

println!("cargo:rerun-if-env-changed=FOO");
if let Ok(foo) = std::env::var("FOO") {
assert!(&foo != "bad");
}
}
"#,
)
.build();

p.cargo("check")
.with_stderr_data(str![[r#"
[COMPILING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
.run();

p.change_file(
".cargo/config.toml",
r#"
[env]
FOO = "bad"
"#,
);

p.cargo("check")
.with_status(101)
.with_stderr_data(str![[r#"
[COMPILING] foo v0.1.0 ([ROOT]/foo)
[ERROR] failed to run custom build command for `foo v0.1.0 ([ROOT]/foo)`
...
Copy link
Member

Choose a reason for hiding this comment

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

We should better check if our assertion actually causes the error message. Perhaps add an assertion message assert!(..., "rerun due to FOO changed") and see if the message contain it.

"#]])
.run();
}

#[cfg(windows)]
#[cargo_test]
fn rerun_if_env_changes_config_in_windows() {
heisen-li marked this conversation as resolved.
Show resolved Hide resolved
let p = project()
.file("Cargo.toml", &basic_manifest("foo", "0.1.0"))
.file("src/main.rs", "fn main() {}")
.file(
".cargo/config.toml",
r#"
[env]
Foo = "good"
Copy link
Member

Choose a reason for hiding this comment

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

Could you merge this into the commit that the test first appeared?

"#,
)
.file(
"build.rs",
r#"
fn main() {
println!("cargo:rerun-if-env-changed=foo");
Copy link
Member

Choose a reason for hiding this comment

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

double colons syntax is preferrable

Suggested change
println!("cargo:rerun-if-env-changed=foo");
println!("cargo::rerun-if-env-changed=foo");

if let Ok(foo) = std::env::var("FOo") {
assert!(&foo != "bad");
}
}
"#,
)
.build();

p.cargo("check")
.with_stderr_data(str![[r#"
[COMPILING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
.run();

p.change_file(
".cargo/config.toml",
r#"
[env]
FoO = "bad"
"#,
);

p.cargo("check")
.with_status(101)
.with_stderr_data(str![[r#"
[COMPILING] foo v0.1.0 ([ROOT]/foo)
[ERROR] failed to run custom build command for `foo v0.1.0 ([ROOT]/foo)`
...
"#]])
.run();
}

#[cargo_test]
fn rerun_if_env_changes_cargo_set_env_variable() {
let p = project()
.file("src/main.rs", "fn main() {}")
.file(
"build.rs",
r#"
fn main() {
println!("cargo:rerun-if-env-changed=OUT_DIR");
}
"#,
)
.build();

p.cargo("check")
.with_stderr_data(str![[r#"
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
.run();

p.change_file(
".cargo/config.toml",
r#"
[env]
OUT_DIR = "somedir"
"#,
);
Comment on lines +133 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

The intention of this test is for detecting changes made by cargo, and not the user. Us manually setting one runs counter to that

Copy link
Member

Choose a reason for hiding this comment

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

I know this is for #8693 and planning to revert #12482. However Cargo doesn't really respect env vars that are set by Cargo internally. Why this should recompile?
This fixes a confusion with the other new confusion introduced.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too sure what the concern is.

Copy link
Member

Choose a reason for hiding this comment

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

Setting env.OUR_DIR has no effect on build script execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that. The intent of my comment was to say that we should not test this with [env] but making a change that causes the cargo-provided env to change.


p.cargo("check")
.with_stderr_data(str![[r#"
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
.run();
}

#[cargo_test]
fn rerun_if_env_changes() {
let p = project()
Expand Down