Add support for `--env` on `tracked_env::var` #118830

GuillaumeGomez · 2023-12-11T15:45:52Z

Follow-up of #118368.
Part of #80792 and of #118372.

It adds support of the --env option for proc-macros through tracked_env::var.

rustbot · 2023-12-11T15:46:01Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Noratrieb · 2023-12-11T20:21:26Z

library/proc_macro/src/lib.rs

@@ -1503,7 +1503,8 @@ pub mod tracked_env {
    #[unstable(feature = "proc_macro_tracked_env", issue = "99515")]
    pub fn var<K: AsRef<OsStr> + AsRef<str>>(key: K) -> Result<String, VarError> {
        let key: &str = key.as_ref();
-        let value = env::var(key);
+        let value = crate::bridge::client::FreeFunctions::injected_env_var(key)
+            .map_or_else(|| env::var(key), Ok);
        crate::bridge::client::FreeFunctions::track_env_var(key, value.as_deref().ok());


Is it correct to track --env here? I don't think it is. Tracked env vars end up in depinfo, implying that the compiler invocation depended on this environment variable, but it didn't, there is no environment variable and even if there is, the compiler didn't care, it just depended on a flag. I think it should only track when env::var is used.

Well, if this function is called, it means the environment variable was used no? Or did I misunderstand what you meant?

From an outside perspective, no "environment variable" was used. It just took a value for the flag. A build system doesn't have to check whether an environment variables changes to figure out whether a rebuild is needed, it needs to look at arguments instead (which it of course already does).

Well, I'm fine with removing this call to track_env_var. Please just confirm it's what you want I do do it. :)

Yes
I also just checked your previous PR and I missed that there:
https://github.com/rust-lang/rust/pull/118368/files#diff-0eaa1095d59f2f3d9c015005d15fd8f7cda13121a8404d77b2955fc5fcd40449R37

The name here also makes it more clear what tracking is for: it's for depinfo which is used by builds systems or other consumers to figure out what went into the compilation (for example when things need to be rebuilt). It's essential that rustc records env variables it reads with env::var. But --env are not actually env variables in the build environment, so they don't need to be tracked. I'd also be happy to review a PR changing the behavior for the macros to not track when the var is read from --env>

That's a flag though and not an environment variable. A build system already needs to handle flags changing. Depinfo tells the build system "hey, I read this environment variable, check it again next time" but in reality it didn't read any environment variable, it got it from the flag.

If it is not tracked in the dep info, any change to a --env argument would need to trigger recompilation of all crates even if they don't use those env!() at all.

I see, for build systems that pass env vars explicitly like that it would be nice if they could detect whether it was actually used. But I don't think the normal env depinfo is the right place for that. The case I was thinking about was for example having A=x in the environment and using --env A=x. Then, changing A=y should not cause any rebuilds as it wasn't used, only the --env A=x value was.

The case I was thinking about was for example having A=x in the environment and using --env A=x. Then, changing A=y should not cause any rebuilds as it wasn't used, only the --env A=x value was.

And in this case it won't trigger a rebuild because the value is same. The --env option uses the same mechanism as the env! macro: variables are tracked if used and will trigger a rebuild if their value changed. So unless I missed something, I think it's doing exactly what we want?

So we're relying on build systems correctly doing the "dep info env -> when I passed --env" translation themselves, since they know they passed the flag.
That makes sense, I didn't think of that, I was too focused on the env depinfo meaning actual environment variable, but I guess the build system is supposed to know what it's doing.
In that case, sounds good. Maybe it should be documented explicitly in the documentation for --env that it ends up in depinfo as env vars.

Noratrieb · 2023-12-16T21:05:24Z

My point was cleared up, thanks!
@bors r+ rollup

bors · 2023-12-16T21:05:27Z

📌 Commit 453ff1e has been approved by Nilstrieb

It is now in the queue for this repository.

…Nilstrieb Add support for `--env` on `tracked_env::var` Follow-up of rust-lang#118368. Part of Part of rust-lang#80792. It adds support of the `--env` option for proc-macros through `tracked_env::var`. r? `@Nilstrieb`

…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#118644 (Add test for Apple's `-weak_framework` linker argument) - rust-lang#118828 (Remove dead codes in rustc_codegen_gcc) - rust-lang#118830 (Add support for `--env` on `tracked_env::var`) - rust-lang#119001 (rustdoc-search: remove parallel searchWords array) - rust-lang#119020 (remove `hex` dependency in bootstrap) r? `@ghost` `@rustbot` modify labels: rollup

…Nilstrieb Add support for `--env` on `tracked_env::var` Follow-up of rust-lang#118368. Part of Part of rust-lang#80792. It adds support of the `--env` option for proc-macros through `tracked_env::var`. r? ``@Nilstrieb``

…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#118828 (Remove dead codes in rustc_codegen_gcc) - rust-lang#118830 (Add support for `--env` on `tracked_env::var`) - rust-lang#119001 (rustdoc-search: remove parallel searchWords array) - rust-lang#119011 (coverage: Regression test for `assert!(!false)`) - rust-lang#119020 (remove `hex` dependency in bootstrap) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2023-12-17T04:23:11Z

⌛ Testing commit 453ff1e with merge 5e70254...

bors · 2023-12-17T06:19:43Z

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 5e70254 to master...

rust-timer · 2023-12-17T07:39:54Z

Finished benchmarking commit (5e70254): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.8%, -0.7%]	3
Improvements ✅ (secondary)	-0.5%	[-0.7%, -0.3%]	7
All ❌✅ (primary)	-0.7%	[-0.8%, -0.7%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	1.5%	[-0.9%, 3.8%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.2s -> 674.824s (0.39%)
Artifact size: 312.39 MiB -> 312.45 MiB (0.02%)

Update documentation for `--env` compilation flag Part of rust-lang#80792. As mentioned in rust-lang#118830. It adds a mention to `tracked_env::var` and also clarifies what triggers a new compilation. r? `@Nilstrieb`

Rollup merge of rust-lang#119115 - GuillaumeGomez:env-docs, r=Nilstrieb Update documentation for `--env` compilation flag Part of rust-lang#80792. As mentioned in rust-lang#118830. It adds a mention to `tracked_env::var` and also clarifies what triggers a new compilation. r? `@Nilstrieb`

…lstrieb Add support for `--env` on `tracked_env::var` Follow-up of rust-lang#118368. Part of Part of rust-lang#80792. It adds support of the `--env` option for proc-macros through `tracked_env::var`. r? `@Nilstrieb`

GuillaumeGomez added 3 commits December 11, 2023 16:44

Add support for --env on tracked_env::var

6ce2fef

Update rust-analyzer to support new injected_env_var function

5967ceb

Add tests for --env usage with tracked_env::var

453ff1e

rustbot assigned Noratrieb Dec 11, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 11, 2023

Noratrieb reviewed Dec 11, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2023

GuillaumeGomez mentioned this pull request Dec 16, 2023

Rollup of 5 pull requests #119027

Closed

GuillaumeGomez mentioned this pull request Dec 16, 2023

Rollup of 5 pull requests #119030

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2023

bors merged commit 5e70254 into rust-lang:master Dec 17, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Dec 17, 2023

GuillaumeGomez deleted the env-tracked_env branch December 17, 2023 10:56

GuillaumeGomez mentioned this pull request Dec 19, 2023

Update documentation for --env compilation flag #119115

Merged

GuillaumeGomez mentioned this pull request Jan 4, 2024

Tracking issue for --env-set flag #118372

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `--env` on `tracked_env::var` #118830

Add support for `--env` on `tracked_env::var` #118830

GuillaumeGomez commented Dec 11, 2023 •

edited

Loading

rustbot commented Dec 11, 2023

Noratrieb Dec 11, 2023 •

edited

Loading

GuillaumeGomez Dec 11, 2023

Noratrieb Dec 11, 2023 •

edited

Loading

GuillaumeGomez Dec 11, 2023

Noratrieb Dec 11, 2023 •

edited

Loading

Noratrieb Dec 12, 2023

bjorn3 Dec 12, 2023

Noratrieb Dec 12, 2023

GuillaumeGomez Dec 12, 2023

Noratrieb Dec 16, 2023

Noratrieb commented Dec 16, 2023

bors commented Dec 16, 2023

bors commented Dec 17, 2023

bors commented Dec 17, 2023

rust-timer commented Dec 17, 2023

Add support for --env on tracked_env::var #118830

Add support for --env on tracked_env::var #118830

Conversation

GuillaumeGomez commented Dec 11, 2023 • edited Loading

rustbot commented Dec 11, 2023

Noratrieb Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Dec 16, 2023

bors commented Dec 16, 2023

bors commented Dec 17, 2023

bors commented Dec 17, 2023

rust-timer commented Dec 17, 2023

Overall result: ✅ improvements - no action needed

Add support for `--env` on `tracked_env::var` #118830

Add support for `--env` on `tracked_env::var` #118830

GuillaumeGomez commented Dec 11, 2023 •

edited

Loading

Noratrieb Dec 11, 2023 •

edited

Loading

Noratrieb Dec 11, 2023 •

edited

Loading

Noratrieb Dec 11, 2023 •

edited

Loading