fix(env): cargo:rerun-if-env-changed doesn't work with env configuration
#14027
Conversation
rustbot
added
S-waiting-on-review
| pub fn get_target_envs(&self) -> CargoResult<&BTreeMap<String, Option<OsString>>> { | ||
| return Ok(self.crate_type_process.get_envs()); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
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?
heisen-li
force-pushed
the
config_env_change_rerun
branch
from
8cd1417 to
e7897e3
Compare
| .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.
I find it strange to fallback to env::var if the envs[key] is non-UTF8. Maybe we should call env::var_os and deal with into_string at the end.
There was a problem hiding this comment.
To be honest, I didn't consider using env::var_os instead of env::var because I wanted to minimize the impact of the change and avoid affecting other behaviors. If there is an up-to-date value in the current environment variable, then use envs.get(key); otherwise the previous env::var(key) is still used.
It looks like the only difference between env::var and env::var_os is whether the environment variable is a valid Unicode.
| .and_then(|os_str| os_str.clone().into_string().ok()) | ||
| }) | ||
| .or_else(|| env::var(key).ok()); |
There was a problem hiding this comment.
iiuc on Windows, env::var calls GetEnvironmentVariableW which is case insensitive. Since we're recreating our own environment look up, should this matter?
There was a problem hiding this comment.
I don't have a more in-depth study of windows, I'm very sorry.
Do you mean that you shouldn't use env::var or env::var_os to query environment variables? That doesn't seem to work : see: rerun_if_env_changes and rerun_if_env_or_file_changes
heisen-li
force-pushed
the
config_env_change_rerun
branch
from
34a8fae to
c93867a
Compare
rustbot
added
A-cli
What does this PR try to resolve?
Fixes #10358