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
| .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
There was a problem hiding this comment.
I guessed you intention is explictly to use str::to_string instead ?
There was a problem hiding this comment.
For myself, I tend to prefer to_owned as that signals the intent
| 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
linyihai
force-pushed
the
rerun-env-changed-issue-10358
branch
from
ba24555
to
0a52c09
Compare
linyihai
force-pushed
the
rerun-env-changed-issue-10358
branch
from
0a52c09
to
ab9438f
Compare
rustbot
added
A-build-execution
linyihai
force-pushed
the
rerun-env-changed-issue-10358
branch
from
1551bf3
to
697a206
Compare
There was a problem hiding this comment.
refactor: env_config supports resolve case insensitive env.
What was the intention with this last commit?
There was a problem hiding this comment.
Its intention is #14756 (comment). Should I squash it into the previous fixing commit ?
There was a problem hiding this comment.
The intention behind that is to abstract away the platform-specific logic and make it easier to do the right thing across cargo. I don't feel the current implementation meets that need. Granted, this might be a bigger change, so depending on the direction we should take with it, maybe its better to defer out of this
There was a problem hiding this comment.
Ok, I leave this as a future PR.
…rerun-if-env-changed`.
linyihai
force-pushed
the
rerun-env-changed-issue-10358
branch
2 times, most recently
from
18e5a5b
to
eeaf9bb
Compare
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.