Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(env): cargo:rerun-if-env-changed doesn't work with env configuration #14027

Closed
wants to merge 4 commits into from
Closed

fix(env): cargo:rerun-if-env-changed doesn't work with env configuration #14027

wants to merge 4 commits into from

Conversation

heisen-li
Copy link
Contributor

What does this PR try to resolve?

Fixes #10358

@rustbot
Copy link
Collaborator

rustbot commented Jun 7, 2024

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-cfg-expr Area: Platform cfg expressions A-rebuild-detection Area: rebuild detection and fingerprinting labels Jun 7, 2024
@heisen-li heisen-li marked this pull request as ready for review June 7, 2024 12:47
epage
epage reviewed Jun 10, 2024
View reviewed changes
src/cargo/core/compiler/build_context/target_info.rs
Comment on lines +594 to 597
pub fn get_target_envs(&self) -> CargoResult<&BTreeMap<String, Option<OsString>>> {
return Ok(self.crate_type_process.get_envs());
}
}
Copy link
Contributor

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.

Copy link
Contributor

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?

epage
epage reviewed Jun 12, 2024
View reviewed changes
src/cargo/core/compiler/fingerprint/mod.rs Outdated
Comment on lines 786 to 790
.and_then(|opt| {
opt.as_ref()
.and_then(|os_str| os_str.clone().into_string().ok())
})
.or_else(|| env::var(key).ok());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

epage
epage reviewed Jun 12, 2024
View reviewed changes
src/cargo/core/compiler/fingerprint/mod.rs Outdated
Comment on lines 788 to 790
.and_then(|os_str| os_str.clone().into_string().ok())
})
.or_else(|| env::var(key).ok());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc on Windows, env::var calls GetEnvironmentVariableW which is case insensitive. Since we're recreating our own environment look up, should this matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-configuration Area: cargo config files and env vars A-dependency-resolution Area: dependency resolution and the resolver A-documenting-cargo-itself Area: Cargo's documentation A-features2 Area: issues specifically related to the v2 feature resolver A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-layout Area: target output directory layout, naming, and organization A-manifest Area: Cargo.toml issues A-registries Area: registries A-testing-cargo-itself Area: cargo's tests A-workspaces Area: workspaces Command-add Command-bench labels Jun 13, 2024
@heisen-li heisen-li closed this Jun 13, 2024
@heisen-li heisen-li mentioned this pull request Jun 13, 2024
Closed
@heisen-li heisen-li deleted the config_env_change_rerun branch June 24, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@epage epage

Labels
A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-configuration Area: cargo config files and env vars A-dependency-resolution Area: dependency resolution and the resolver A-documenting-cargo-itself Area: Cargo's documentation A-features2 Area: issues specifically related to the v2 feature resolver A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-layout Area: target output directory layout, naming, and organization A-manifest Area: Cargo.toml issues A-rebuild-detection Area: rebuild detection and fingerprinting A-registries Area: registries A-testing-cargo-itself Area: cargo's tests A-workspaces Area: workspaces Command-add Command-bench Command-build Command-clean Command-fetch Command-fix Command-install Command-metadata Command-package Command-remove Command-test Command-tree Command-update Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cargo:rerun-if-env-changed doesn't work with env configuration
3 participants
@heisen-li @rustbot @epage