Add environment variable query #130883

madsmtm · 2024-09-26T15:10:05Z

Generally, rustc prefers command-line arguments, but in some cases, an environment variable really is the most sensible option. We should make sure that this works properly with the compiler's change-tracking mechanisms, such that changing the relevant environment variable causes a rebuild.

This PR is a first step forwards in doing that.

Part of the work needed to do #118204, see #129342 for some discussion.

r? @petrochenkov

compiler/rustc_interface/src/passes.rs

madsmtm · 2024-09-26T15:11:37Z

compiler/rustc_interface/src/passes.rs

@@ -336,6 +337,20 @@ fn early_lint_checks(tcx: TyCtxt<'_>, (): ()) {
    )
 }

+fn env_var(tcx: TyCtxt<'_>, key: Symbol) -> Option<Symbol> {
+    let var = match std::env::var(key.as_str()) {


Is there something similar to Clippy's disallowed-methods that I could use to lint against usage of std::env::var in the compiler, and direct users towards the query instead?

See also rust-lang/cargo#11588.

There's some rustc-internal lints in https://github.com/rust-lang/rust/blob/master/compiler/rustc_lint/src/internal.rs

compiler/rustc_interface/src/passes.rs

compiler/rustc_borrowck/src/nll.rs

compiler/rustc_middle/src/query/mod.rs

…g, r=petrochenkov Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa` Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc. We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion. The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session. Tested with the same commands as in rust-lang#130435. r? `@petrochenkov`

…g, r=petrochenkov Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa` Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc. We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion. The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session. Tested with the same commands as in rust-lang#130435. r? ```@petrochenkov```

…g, r=petrochenkov Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa` Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc. We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion. The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session. Tested with the same commands as in rust-lang#130435. r? ````@petrochenkov````

…g, r=petrochenkov Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa` Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc. We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion. The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session. Tested with the same commands as in rust-lang#130435. r? `````@petrochenkov`````

…g, r=petrochenkov Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa` Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc. We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion. The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session. Tested with the same commands as in rust-lang#130435. r? ``````@petrochenkov``````

Rollup merge of rust-lang#131037 - madsmtm:move-llvm-target-versioning, r=petrochenkov Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa` Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc. We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion. The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session. Tested with the same commands as in rust-lang#130435. r? ``````@petrochenkov``````

madsmtm · 2024-11-16T18:23:48Z

@rustbot ready

Note that I've also added a test that env!/option_env! works with incremental compilation, I felt it was relevant to this PR, but feel free to tell me to do it in a different one.

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_data_structures/src/stable_hasher.rs

compiler/rustc_interface/src/passes.rs

petrochenkov · 2024-11-30T17:57:47Z

@rustbot ready

Note that I decided to remove the "Add environment variable tracking in places where it was convenient" commit, since they weren't working properly anyway without the extra support code in emit_dep_info (and then I'd rather have people add that support code when they need it).

But they did work, for incremental scenarios? Just not for cargo rebuilds.
I'd rather brought those changes back.

This currently works because it's part of expansion, and that isn't yet tracked by the query system. But we want to ensure it continues working, even if that is changed.

madsmtm · 2024-12-01T11:07:10Z

@rustbot ready

Note that I decided to remove the "Add environment variable tracking in places where it was convenient" commit, since they weren't working properly anyway without the extra support code in emit_dep_info (and then I'd rather have people add that support code when they need it).

But they did work, for incremental scenarios? Just not for cargo rebuilds. I'd rather brought those changes back.

I think so. I've brought them back.

bjorn3 · 2024-12-01T14:52:33Z

compiler/rustc_borrowck/src/nll.rs

+            let algorithm = infcx
+                .tcx
+                .env_var_os("POLONIUS_ALGORITHM".as_ref())
+                .unwrap_or_else(|| Symbol::intern("Hybrid"));


I think a better solution here would be to add a polonius_algorithm field to Options and read the POLONIUS_ALGORITHM env var in build_session_options. Then marking the field as [TRACKED] is enough to get it tracked by incr comp.

That is exactly what I did originally in #129342, but @petrochenkov preferred that we integrated it with the query system instead, hence this PR.

I have no idea what's the best solution here, I'll defer to you two to discuss and decide on that ;) .

I did prefer accessing env vars as early as possible to prevent eg proc-macros from affecting compilation options. Proc-macros are known to do things they should not be allowed to do.

Having a query for env vars probably still makes sense even if some of the env var uses are converted to options.

(One downside of early tracking is that the dependency on the env var will be registered even if the env var is not actually used.)

This won't work with Cargo's change tracking, but it should work with incremental.

petrochenkov · 2024-12-01T17:07:32Z

compiler/rustc_middle/src/util/mod.rs

@@ -29,3 +34,13 @@ impl std::ops::Deref for Providers {
        &self.queries
    }
 }
+
+impl<'tcx> TyCtxt<'tcx> {


Maybe put it to compiler\rustc_middle\src\ty\context.rs?
That's where TyCtxt is defined and many of its random inherent methods live.

petrochenkov · 2024-12-01T17:12:08Z

compiler/rustc_middle/src/util/mod.rs

@@ -29,3 +34,13 @@ impl std::ops::Deref for Providers {
        &self.queries
    }
 }
+
+impl<'tcx> TyCtxt<'tcx> {
+    pub fn env_var(self, key: &'tcx OsStr) -> Result<&'tcx str, VarError> {


Can use AsRef<OsStr> here, then the function will accept string literals.
You even use key.as_ref() below, which is unnecessary if the key is a non-generic OsStr.

petrochenkov · 2024-12-01T17:12:35Z

compiler/rustc_middle/src/util/mod.rs

+
+impl<'tcx> TyCtxt<'tcx> {
+    pub fn env_var(self, key: &'tcx OsStr) -> Result<&'tcx str, VarError> {
+        if let Some(value) = self.env_var_os(key.as_ref()) {


Style nit: if-lets with short (one line) arms typically look better as a match.

petrochenkov · 2024-12-01T17:13:34Z

compiler/rustc_middle/src/query/keys.rs

+    }
+}
+
+impl<'tcx> Key for Option<&'tcx OsStr> {


Is this necessary?
Option<&'tcx OsStr> doesn't seem to be used as a key.

petrochenkov · 2024-12-01T17:14:10Z

compiler/rustc_middle/src/query/erase.rs

@@ -255,6 +264,7 @@ trivial! {
    Option<rustc_span::def_id::DefId>,
    Option<rustc_span::def_id::LocalDefId>,
    Option<rustc_span::Span>,
+    Option<rustc_span::Symbol>,


This is likely not necessary anymore.

petrochenkov · 2024-12-01T17:14:23Z

compiler/rustc_middle/src/query/erase.rs

@@ -180,6 +185,10 @@ impl<T> EraseType for Option<&'_ [T]> {
    type Result = [u8; size_of::<Option<&'static [()]>>()];
 }

+impl EraseType for Option<&'_ OsStr> {


Probably unnecessary.

rustbot assigned petrochenkov Sep 26, 2024

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 Sep 26, 2024

madsmtm commented Sep 26, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

madsmtm force-pushed the env-var-query branch from 506b0df to af26a40 Compare September 26, 2024 19:43

This comment has been minimized.

Sign in to view

madsmtm force-pushed the env-var-query branch 2 times, most recently from dee9b61 to 0ef9570 Compare September 27, 2024 14:06

This comment has been minimized.

Sign in to view

madsmtm force-pushed the env-var-query branch from 0ef9570 to 455e468 Compare September 27, 2024 15:56

This comment has been minimized.

Sign in to view

madsmtm force-pushed the env-var-query branch from 455e468 to 46276c8 Compare September 27, 2024 16:10

madsmtm commented Sep 27, 2024

View reviewed changes

compiler/rustc_interface/src/passes.rs Outdated Show resolved Hide resolved

madsmtm mentioned this pull request Sep 29, 2024

Move versioned Apple LLVM targets from rustc_target to rustc_codegen_ssa #131037

Merged

petrochenkov reviewed Oct 22, 2024

View reviewed changes

compiler/rustc_borrowck/src/nll.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/query/mod.rs Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 22, 2024

This comment was marked as resolved.

Sign in to view

petrochenkov mentioned this pull request Nov 10, 2024

Rebuild on changes to the deployment target when compiling Apple targets #129342

Open

madsmtm force-pushed the env-var-query branch from 46276c8 to 916e404 Compare November 16, 2024 18:15

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2024

petrochenkov reviewed Nov 30, 2024

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2024

madsmtm added 3 commits December 1, 2024 11:32

Test that env! works with incremental compilation

dc910b2

This currently works because it's part of expansion, and that isn't yet tracked by the query system. But we want to ensure it continues working, even if that is changed.

Add TyCtx::env_var

ffa2135

Make OsStr usable with queries

a01330c

madsmtm force-pushed the env-var-query branch from 5db8102 to 6ca63b3 Compare December 1, 2024 11:04

madsmtm mentioned this pull request Dec 1, 2024

Support non-UTF-8 in environment variable dependency tracking #133699

Open

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 1, 2024

This comment has been minimized.

Sign in to view

madsmtm force-pushed the env-var-query branch 2 times, most recently from c89fbb7 to 745b34e Compare December 1, 2024 11:41

This comment has been minimized.

Sign in to view

madsmtm force-pushed the env-var-query branch from 745b34e to 1028b1e Compare December 1, 2024 13:24

This comment has been minimized.

Sign in to view

madsmtm force-pushed the env-var-query branch from 1028b1e to aa575ad Compare December 1, 2024 13:33

This comment has been minimized.

Sign in to view

bjorn3 reviewed Dec 1, 2024

View reviewed changes

madsmtm force-pushed the env-var-query branch from aa575ad to 2b561b8 Compare December 1, 2024 15:50

madsmtm added 2 commits December 1, 2024 16:52

Use OsStr in tracked environment variables

da9b1bc

Add environment variable tracking in places where it was convenient

60f18ba

This won't work with Cargo's change tracking, but it should work with incremental.

madsmtm force-pushed the env-var-query branch from 2b561b8 to 60f18ba Compare December 1, 2024 15:52

petrochenkov reviewed Dec 1, 2024

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add environment variable query #130883

Add environment variable query #130883

madsmtm commented Sep 26, 2024

madsmtm Sep 26, 2024

clubby789 Sep 28, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

madsmtm commented Nov 16, 2024 •

edited

Loading

petrochenkov commented Nov 30, 2024

madsmtm commented Dec 1, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bjorn3 Dec 1, 2024

madsmtm Dec 1, 2024

bjorn3 Dec 1, 2024

petrochenkov Dec 1, 2024

petrochenkov Dec 1, 2024

petrochenkov Dec 1, 2024

petrochenkov Dec 1, 2024

petrochenkov Dec 1, 2024

petrochenkov Dec 1, 2024

petrochenkov Dec 1, 2024

Add environment variable query #130883

Are you sure you want to change the base?

Add environment variable query #130883

Conversation

madsmtm commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

madsmtm commented Nov 16, 2024 • edited Loading

petrochenkov commented Nov 30, 2024

madsmtm commented Dec 1, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

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

madsmtm commented Nov 16, 2024 •

edited

Loading