fix: env table config can't trigger rebuild with `rerun-if-env-changed`. by linyihai · Pull Request #14756 · rust-lang/cargo

linyihai · 2024-10-31T08:20:48Z

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.

rustbot · 2024-10-31T08:20:53Z

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

src/cargo/core/compiler/fingerprint/mod.rs

epage · 2024-10-31T14:08:56Z

src/cargo/core/compiler/fingerprint/mod.rs

+                                    .collect::<HashMap<String, OsString>>()
+                            })
+                            .get(&key.to_uppercase())
+                            .and_then(|s| s.to_str().map(|s| s.to_string()))


Please use a more specific conversion than to_string as it can mask incidental semantic conversions

I guessed you intention is explictly to use str::to_string instead ?

For myself, I tend to prefer to_owned as that signals the intent

epage · 2024-10-31T14:16:33Z

src/cargo/core/compiler/fingerprint/mod.rs

+                    if cfg!(windows) {
+                        env_config_insensitive


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?

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

Sorry, Yes, I meant conditioned on the platform (which is why the typedef would be there)

Hmm, if we also have a normalization process, I wonder if we should apply that more generally

epage · 2024-11-04T18:12:45Z

src/cargo/core/compiler/fingerprint/dep_info.rs

refactor: env_config supports resolve case insensitive env.

What was the intention with this last commit?

Its intention is #14756 (comment). Should I squash it into the previous fixing commit ?

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

Ok, I leave this as a future PR.

linyihai · 2024-12-31T08:12:48Z

I removed the code that makes env_config case insensitivity in Windows. Resolving env_config's case sensitivity due to operating system platform differences is far more complicated than fixing this issue, so I left it to a possible future PR and made this PR easier to merge.

weihanglo

This all makes sense. Thank you.

weihanglo · 2024-12-31T13:55:23Z

src/cargo/core/compiler/fingerprint/mod.rs

+    /// The `env_config` is used first to check if the env var is set in the config system
+    /// as some env needs to be overridden. If not, it will fallback to `std::env::var`.


I think the TODO here still holds. I don't see any reason this should be read directly from system but not our env snapshot.
(though it doesn't really make much difference in this case)

Suggested change

/// The `env_config` is used first to check if the env var is set in the config system

/// as some env needs to be overridden. If not, it will fallback to `std::env::var`.

/// The `env_config` is used first to check if the env var is set in the config system

/// as some env needs to be overridden. If not, it will fallback to `std::env::var`.

// TODO: `std::env::var` is allowed at this moment. Should figure out

// if it makes sense if permitting to read env from the env snapshot.

…rerun-if-env-changed`.

Update cargo 9 commits in d73d2caf9e41a39daf2a8d6ce60ec80bf354d2a7..fd784878cfa843e3e29a6654ecf564c62fae6735 2024-12-31 20:51:21 +0000 to 2025-01-03 20:06:26 +0000 - chore: bump gix-lock to remove thiserror@1 from `cargo` (rust-lang/cargo#15012) - refactor(manifest): Clean up field -> env var handling (rust-lang/cargo#15008) - chore(deps): update rust crate thiserror to v2 (rust-lang/cargo#14998) - test(git): Clean up shallow fetch tests (rust-lang/cargo#15002) - fix(schema): Correct and update the JSON Schema (rust-lang/cargo#15000) - chore(deps): update rust crate itertools to 0.14.0 (rust-lang/cargo#14996) - fix: env table config can't trigger rebuild with `rerun-if-env-changed`. (rust-lang/cargo#14756) - chore(deps): update alpine docker tag to v3.21 (rust-lang/cargo#14995) - fix(package): check dirtiness of symlinks source files (rust-lang/cargo#14981)

rustbot assigned epage Oct 31, 2024

rustbot added A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2024

linyihai changed the title ~~fix: env table config can trigger rebuild with rerun-if-env-changed.~~ fix: env table config can't trigger rebuild with rerun-if-env-changed. Oct 31, 2024

linyihai force-pushed the rerun-env-changed-issue-10358 branch from 0ce1d7d to ba24555 Compare October 31, 2024 08:34

linyihai marked this pull request as ready for review October 31, 2024 09:30

epage reviewed Oct 31, 2024

View reviewed changes

src/cargo/core/compiler/fingerprint/mod.rs Outdated Show resolved Hide resolved

epage reviewed Oct 31, 2024

View reviewed changes

src/cargo/core/compiler/fingerprint/mod.rs Outdated Show resolved Hide resolved

epage reviewed Oct 31, 2024

View reviewed changes

src/cargo/core/compiler/fingerprint/mod.rs Outdated Show resolved Hide resolved

epage reviewed Oct 31, 2024

View reviewed changes

linyihai force-pushed the rerun-env-changed-issue-10358 branch from ba24555 to 0a52c09 Compare November 1, 2024 09:29

weihanglo mentioned this pull request Nov 1, 2024

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

Closed

linyihai force-pushed the rerun-env-changed-issue-10358 branch from 0a52c09 to ab9438f Compare November 4, 2024 08:40

rustbot added A-build-execution Area: anything dealing with executing the compiler A-configuration Area: cargo config files and env vars labels Nov 4, 2024

linyihai force-pushed the rerun-env-changed-issue-10358 branch from 1551bf3 to 697a206 Compare November 4, 2024 10:30

epage reviewed Nov 4, 2024

View reviewed changes

linyihai force-pushed the rerun-env-changed-issue-10358 branch 2 times, most recently from 18e5a5b to eeaf9bb Compare November 6, 2024 06:39

test: add test for rerun-if-env-changed custom build script.

a014fd0

linyihai force-pushed the rerun-env-changed-issue-10358 branch from eeaf9bb to e035080 Compare December 31, 2024 07:54

weihanglo approved these changes Dec 31, 2024

View reviewed changes

fix: envs in config can trigger rebuild by custom build script with `…

76ffbe0

…rerun-if-env-changed`.

linyihai force-pushed the rerun-env-changed-issue-10358 branch from e035080 to 76ffbe0 Compare January 1, 2025 06:12

weihanglo approved these changes Jan 1, 2025

View reviewed changes

weihanglo enabled auto-merge January 1, 2025 06:35

weihanglo added this pull request to the merge queue Jan 1, 2025

Merged via the queue into rust-lang:master with commit 03a5f11 Jan 1, 2025

linyihai deleted the rerun-env-changed-issue-10358 branch January 2, 2025 01:06

weihanglo mentioned this pull request Jan 3, 2025

Update cargo rust-lang/rust#135089

Merged

rustbot added this to the 1.86.0 milestone Jan 4, 2025

		/// The `env_config` is used first to check if the env var is set in the config system
		/// as some env needs to be overridden. If not, it will fallback to `std::env::var`.

Conversation

linyihai commented Oct 31, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Oct 31, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linyihai commented Dec 31, 2024

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

weihanglo Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

weihanglo Dec 31, 2024 •

edited

Loading