-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
base: master
Are you sure you want to change the base?
Conversation
0a264a9
to
877da76
Compare
513422b
to
cbe3643
Compare
08bbb58
to
d19af49
Compare
d19af49
to
83eaae3
Compare
83eaae3
to
fd2e75e
Compare
p.change_file( | ||
".cargo/config.toml", | ||
r#" | ||
[env] | ||
OUT_DIR = "somedir" | ||
"#, | ||
); |
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.
The intention of this test is for detecting changes made by cargo, and not the user. Us manually setting one runs counter to that
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.
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.
I'm not too sure what the concern is.
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.
Setting env.OUR_DIR
has no effect on build script execution.
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.
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.
"build.rs", | ||
r#" | ||
fn main() { | ||
println!("cargo:rerun-if-env-changed=foo"); |
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.
double colons syntax is preferrable
println!("cargo:rerun-if-env-changed=foo"); | |
println!("cargo::rerun-if-env-changed=foo"); |
} | ||
} | ||
"#, | ||
fn main() { |
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 formatting change shouldn't have happened, or should merge into the first commit adding the test.
|
||
[COMPILING] foo v0.1.0 ([ROOT]/foo) | ||
[ERROR] failed to run custom build command for `foo v0.1.0 ([ROOT]/foo)` | ||
... |
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.
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.
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" | ||
Foo = "good" |
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.
Could you merge this into the commit that the test first appeared?
p.change_file( | ||
".cargo/config.toml", | ||
r#" | ||
[env] | ||
OUT_DIR = "somedir" | ||
"#, | ||
); |
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.
} else { | ||
envs.get(key).and_then(|v| v.to_owned()) | ||
} | ||
.or(env::var_os(key)) |
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.
Lazy evaluation
.or(env::var_os(key)) | |
.or_else(|| env::var_os(key)) |
envs: &BTreeMap<String, Option<OsString>>, | ||
) -> Option<OsString> { | ||
let upper_case_key: String = key.to_uppercase(); | ||
for (k, v) in envs { |
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 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.
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.
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.
@@ -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(); |
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.
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.
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.
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.)
// TODO: This is allowed at this moment. Should figure out if it makes | ||
// sense if permitting to read env from the config system. |
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.
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?
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.
such as wrapping the environment in Arc
Agree Arc
is the way to resolve this.
envs: &BTreeMap<String, Option<OsString>>, | ||
) -> Option<OsString> { | ||
let upper_case_key: String = key.to_uppercase(); | ||
for (k, v) in envs { |
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.
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.
I'm not clear, how does this fix #8693? |
What does this PR try to resolve?
Fixes #10358
Fixes #8693
Additional information
Due to my stupidity, I screwed up the previous PR. I didn't know what to do with it, so I created a new PR.
r?@epage