fix: env table config can't trigger rebuild with rerun-if-env-changed.
#14756
Conversation
rustbot
added
A-rebuild-detection
linyihai
changed the title
rerun-if-env-changed.rerun-if-env-changed.
linyihai
force-pushed
the
rerun-env-changed-issue-10358
branch
from
0ce1d7d
to
ba24555
Compare
| @@ -376,6 +377,7 @@ use anyhow::format_err; | |||
| use anyhow::Context as _; | |||
| use cargo_util::paths; | |||
| use filetime::FileTime; | |||
| use lazycell::LazyCell; | |||
There was a problem hiding this comment.
imo we should not be adding any new uses of lazycell but should instead be trying to remove it in favor of OnceCell
| let val = if let Some(val) = env::var(key).ok() { | ||
| Some(val) | ||
| } else { | ||
| match env_config.get(key) { |
There was a problem hiding this comment.
which is supposed to have higher precedence?
| @@ -1701,6 +1725,7 @@ fn build_script_local_fingerprints( | |||
| // obvious. | |||
| let pkg_root = unit.pkg.root().to_path_buf(); | |||
| let target_dir = target_root(build_runner); | |||
| let env_config = Arc::clone(build_runner.bcx.gctx.env_config().unwrap()); | |||
There was a problem hiding this comment.
Whats the assumption we are operating under that its safe to unwrap?
| .collect::<HashMap<String, OsString>>() | ||
| }) | ||
| .get(&key.to_uppercase()) | ||
| .and_then(|s| s.to_str().map(|s| s.to_string())) |
There was a problem hiding this comment.
Please use a more specific conversion than to_string as it can mask incidental semantic conversions
| if cfg!(windows) { | ||
| env_config_insensitive |
There was a problem hiding this comment.
Rather than having to do all of this extra work, I wonder if we should make the env map itself case insensitive. Looking at how env_config() is used, I think we have several places where we don't handle case sensitivity and maybe we should?
What we could do is define a type def for EnvMap where the key is unicase::Ascii<String> and have the global context return that.
@weihanglo thoughts?
There was a problem hiding this comment.
That should be conditioned on platform, maybe only on Windows?
Cargo does that upfront for environment variables from configuration enviornment: https://github.com/rust-lang/cargo/blob/master/src/cargo/util/context/environment.rs
There was a problem hiding this comment.
Sorry, Yes, I meant conditioned on the platform (which is why the typedef would be there)
There was a problem hiding this comment.
Hmm, if we also have a normalization process, I wonder if we should apply that more generally
What does this PR try to resolve?
As #10358 said,
When a build.rs script emits cargo:rerun-if-env-changed, it is not re-run when the value of the specified variable is changed via the env configuration.Fixes #10358
How should we test and review this PR?
Add test bofore fixing to reflect the issue, the next commit fixed it.
Additional information
The PR is a sucessor of #14058, so the previous dicussion can be refer to it.