fix(env): dont track envs set by cargo in dep-info file by weihanglo · Pull Request #14811 · rust-lang/cargo

weihanglo · 2024-11-12T22:03:20Z

What does this PR try to resolve?

Don't track environment variables set by Cargo1 in Cargo's dep-info file.

This patch is quite hacky since the "set-by-Cargo" env vars are
hardcoded. However, not all environment variables set by Cargo are
statically known. To prevent the actual environment variables applied to
rustc invocation get out of sync from what we hard-code, a debug time
assertion is put to help discover missing environment variables earlier.

Fixes #14798

How should we test and review this PR?

A new test is added to show no rebuild triggered.

Try adding a random new env here and see if a debug build fails.

rustbot · 2024-11-12T22:03:25Z

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/compilation.rs

tests/testsuite/cargo_env_config.rs

epage · 2024-11-12T22:41:16Z

src/cargo/core/compiler/compilation.rs

+#[cfg(debug_assertions)]
+pub(crate) fn assert_only_envs_set_by_cargo<'a>(
+    keys: impl Iterator<Item = impl AsRef<str>>,
+    env_config: &HashMap<String, OsString>,
+) {
+    for key in keys {
+        let key = key.as_ref();
+        // When running Cargo's test suite,
+        // we're fine if it is from the `[env]` table
+        if env_config.contains_key(key) {
+            continue;
+        }
+        assert!(
+            is_env_set_by_cargo(key),
+            "`{key}` is not tracked as an environment variable set by Cargo\n\
+            Add it to `ENV_VARS_SET_FOR_CRATES` if you intend to introduce a new one"
+        );
+    }
+}


This patch is quite hacky since the "set-by-Cargo" env vars are
hardcoded. However, not all environment variables set by Cargo are
statically known. To prevent the actual environment variables applied to
rustc invocation get out of sync from what we hard-code, a debug time
assertion is put to help discover missing environment variables earlier.

@ehuss I'm curious about your thoughts on this

### What does this PR try to resolve? This was found when doing #14811. Other counterparts display environment variables, but rustdoc does not. This patch makes rustdoc follow suit. ### How should we test and review this PR? Run `cargo doc -vv`

linyihai · 2024-11-14T02:32:27Z

src/cargo/core/compiler/compilation.rs

    }
    Ok(matching_linker.map(|(_k, linker)| linker.val.clone().resolve_program(bcx.gctx)))
 }
+


In order to better discover these variables, whether to put this part of the content in a separate file management is better?

A simple search for ENV_VARS_SET_FOR_CRATES could lead the way, though no objection to this proposal, just lacking of a better module name (naming is hard 😆).

bors · 2024-11-16T02:02:46Z

☔ The latest upstream changes (presumably 69e5959) made this pull request unmergeable. Please resolve the merge conflicts.

This is a side effect when fixing 13280 via PR 14701.

This patch is quite hacky since the "set-by-Cargo" env vars are hardcoded. However, not all environment variables set by Cargo are statically known. To prevent the actual environment variables applied to rustc invocation get out of sync from what we hard-code, a debug time assertion is put to help discover missing environment variables earlier.

epage · 2024-12-11T03:00:16Z

src/cargo/core/compiler/compilation.rs

+    "CARGO_PKG_DESCRIPTION",
+    "CARGO_PKG_HOMEPAGE",
+    "CARGO_PKG_REPOSITORY",
+    "CARGO_PKG_LICENSE",


To remove these requires that we are tracking these somewhere else. For things like the package name, thats more obvious. For ones like this, where is that happening? Should we have a test that changing the license causes a rebuild?

That's a good catch. We actually only rebuild for authors, homepage, repository, and repository.

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

Line 1534 in 298e403

let metadata = util::hash_u64((&m.authors, &m.description, &m.homepage, &m.repository));

If we're going to hand-pick some of them, I would rather drop this out.

Since we were trying to fix #14811, the alternative is a “wontfix” and documenting it.

If you're setting CARGO_SOMETHING in [env], you are screwed because it won't work and may hurt your rebuild detection.

Apparently, package.description was tracked by fingerprint because of the usage of CARGO_PKG_DESCRIPTION #3857.

Okay, maybe the other way round—We do hand-pick whatever we want to "remove" from cargo's dep-info file. The other stay in dep-info so they are happy to rebuild when requires by env!. And we don't even need to track them in fingerprint.

I think it makes sense for us to track metadata in dep info rather than fingerprint so we only rebuild if the user cares.

As for the other part, I'm unusre

cargo/src/cargo/core/manifest.rs

Lines 145 to 146 in 3cd245c

pub links: Option<String>,

pub rust_version: Option<RustVersion>,

It is so fun that package.links and package.rust-version are considered metadata and and won't trigger any rebuild when changed.

imo there isn't a reason to rebiild on rust-version change unless the user is using it.

That is fair. It is a kind of metadata for the actual compilation.

weihanglo · 2024-12-18T01:31:29Z

Going to close this.

It is not good that a patch relying on debug assertions
We may want to track metadata by Cargo's dep-info file, insteado of manual track in fingerprint. fix(env): dont track envs set by cargo in dep-info file #14811 (comment)
CARGO_RUSTC_CURRENT_DIR has been removed. If other folks has bumped into the same situation of setting internal envs in [env] table, we can at least tell them it is not supported.

epage · 2024-12-18T02:30:29Z

CARGO_RUSTC_CURRENT_DIR may come back because file! is being specified, see rust-lang/rust#134442

rustbot assigned epage Nov 12, 2024

rustbot added A-build-execution Area: anything dealing with executing the compiler A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2024

epage reviewed Nov 12, 2024

View reviewed changes

src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved

epage reviewed Nov 12, 2024

View reviewed changes

tests/testsuite/cargo_env_config.rs Show resolved Hide resolved

epage reviewed Nov 12, 2024

View reviewed changes

weihanglo mentioned this pull request Nov 13, 2024

feat(rustdoc): diplay env vars in extra verbose mode #14812

Merged

weihanglo force-pushed the env branch from d853d5d to cbb2164 Compare November 13, 2024 21:06

epage approved these changes Nov 13, 2024

View reviewed changes

linyihai reviewed Nov 14, 2024

View reviewed changes

weihanglo force-pushed the env branch from cbb2164 to 45a4e98 Compare November 16, 2024 16:49

weihanglo added 2 commits December 10, 2024 21:01

test(env): show rebuild when overriding env set by cargo

08b7746

This is a side effect when fixing 13280 via PR 14701.

weihanglo force-pushed the env branch from 45a4e98 to 3129515 Compare December 11, 2024 02:01

epage reviewed Dec 11, 2024

View reviewed changes

weihanglo closed this Dec 18, 2024

weihanglo deleted the env branch December 18, 2024 01:34

weihanglo mentioned this pull request Dec 18, 2024

CARGO_RUSTC_CURRENT_DIR is causing forced rebuilds #14798

Open

weihanglo mentioned this pull request Dec 21, 2024

Moved manifest metadata tracking from fingerprint to dep info #14973

Merged

	pub links: Option<String>,
	pub rust_version: Option<RustVersion>,

Conversation

weihanglo commented Nov 12, 2024

What does this PR try to resolve?

How should we test and review this PR?

Uh oh!

rustbot commented Nov 12, 2024

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!

bors commented Nov 16, 2024

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!

weihanglo commented Dec 18, 2024

Uh oh!

epage commented Dec 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants