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

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

Merged
merged 3 commits into from
Dec 17, 2023
+56 −1
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ce2fef
Add support for `--env` on `tracked_env::var`
GuillaumeGomez Dec 11, 2023
5967ceb
Update rust-analyzer to support new `injected_env_var` function
GuillaumeGomez Dec 11, 2023
453ff1e
Add tests for `--env` usage with `tracked_env::var`
GuillaumeGomez Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions compiler/rustc_expand/src/proc_macro_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,10 @@ impl server::Types for Rustc<'_, '_> {
}

impl server::FreeFunctions for Rustc<'_, '_> {
fn injected_env_var(&mut self, var: &str) -> Option<String> {
self.ecx.sess.opts.logical_env.get(var).cloned()
}

fn track_env_var(&mut self, var: &str, value: Option<&str>) {
self.sess()
.env_depinfo
Expand Down
1 change: 1 addition & 0 deletions library/proc_macro/src/bridge/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ macro_rules! with_api {
$m! {
FreeFunctions {
fn drop($self: $S::FreeFunctions);
fn injected_env_var(var: &str) -> Option<String>;
fn track_env_var(var: &str, value: Option<&str>);
fn track_path(path: &str);
fn literal_from_str(s: &str) -> Result<Literal<$S::Span, $S::Symbol>, ()>;
Expand Down
3 changes: 2 additions & 1 deletion library/proc_macro/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member

@Noratrieb Noratrieb Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@Noratrieb Noratrieb Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@Noratrieb Noratrieb Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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>

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

value
}
Expand Down
4 changes: 4 additions & 0 deletions src/tools/rust-analyzer/crates/proc-macro-srv/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ impl server::Types for RustAnalyzer {
}

impl server::FreeFunctions for RustAnalyzer {
fn injected_env_var(&mut self, _var: &str) -> Option<String> {
None
}

fn track_env_var(&mut self, _var: &str, _value: Option<&str>) {
// FIXME: track env var accesses
// https://github.com/rust-lang/rust/pull/71858
Expand Down
28 changes: 28 additions & 0 deletions tests/ui/proc-macro/auxiliary/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// force-host
// no-prefer-dynamic

#![crate_type = "proc-macro"]
#![feature(proc_macro_tracked_env)]

extern crate proc_macro;

use proc_macro::TokenStream;
use proc_macro::tracked_env::var;

#[proc_macro]
pub fn generate_const(input: TokenStream) -> TokenStream {
let the_const = match var("THE_CONST") {
Ok(x) if x == "12" => {
"const THE_CONST: u32 = 12;"
}
_ => {
"const THE_CONST: u32 = 0;"
}
};
let another = if var("ANOTHER").is_ok() {
"const ANOTHER: u32 = 1;"
} else {
"const ANOTHER: u32 = 2;"
};
format!("{the_const}{another}").parse().unwrap()
}
17 changes: 17 additions & 0 deletions tests/ui/proc-macro/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// aux-build:env.rs
// run-pass
// rustc-env: THE_CONST=1
// compile-flags: -Zunstable-options --env THE_CONST=12 --env ANOTHER=4

#![crate_name = "foo"]

extern crate env;

use env::generate_const;

generate_const!();

fn main() {
assert_eq!(THE_CONST, 12);
assert_eq!(ANOTHER, 1);
}
Loading