-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
#14027
Changes from 2 commits
d202e52
e7897e3
1cf0b56
c93867a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}; | ||
|
@@ -772,10 +774,21 @@ impl LocalFingerprint { | |
// TODO: This is allowed at this moment. Should figure out if it makes | ||
// sense if permitting to read env from the config system. | ||
#[allow(clippy::disallowed_methods)] | ||
fn from_env<K: AsRef<str>>(key: K) -> LocalFingerprint { | ||
fn from_env<K: AsRef<str>>( | ||
key: K, | ||
envs: &BTreeMap<String, Option<OsString>>, | ||
) -> LocalFingerprint { | ||
let key = key.as_ref(); | ||
let var = key.to_owned(); | ||
let val = env::var(key).ok(); | ||
|
||
let val = envs | ||
.get(key) | ||
.and_then(|opt| { | ||
opt.as_ref() | ||
.and_then(|os_str| os_str.clone().into_string().ok()) | ||
}) | ||
.or_else(|| env::var(key).ok()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it strange to fallback to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I didn't consider using It looks like the only difference between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iiuc on Windows, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a more in-depth study of windows, I'm very sorry. Do you mean that you shouldn't use |
||
|
||
LocalFingerprint::RerunIfEnvChanged { var, val } | ||
} | ||
|
||
|
@@ -1608,6 +1621,13 @@ 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() | ||
.unwrap() | ||
.clone(); | ||
// 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) { | ||
|
@@ -1660,7 +1680,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" | ||
|
@@ -1695,6 +1720,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(); | ||
|
@@ -1719,7 +1745,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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels very brittle because any change to how we pass in environment variables automatically gets used as fingerprinting, rather than intentionally choosing what we fingerprint. Just to check to see if this is correct is requiring checking several other places.
In fact, I think this will cause every build to rebuild everything when using jobserver because I think we are capturing the env after that is configured and that env includes file descriptors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was overlooking that we aren't fingerprinting all of it, only what the user requests. So maybe its not all that of a problem?