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.

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

Stabilize --env-set option #119926

Closed

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 13, 2024

Fixes #118372.
Fixes #80792.

As discussed with meson devs, the option by itself is good enough to be used, so there isn't really a need to wait for rust-lang/rfcs#2794 to stabilize --env-set.

Summary

This PR stabilizes the --env-set option. This option flag allows specification of environment variables at compile time for the env! and option_env! macros and tracked_env::var function from the proc_macro crate.

This information will be stored in the dep-info files. For more information about dep-info files, take a look here.

When retrieving an environment variable, the value specified by --env-set will take
precedence. For example, if you want have PATH=env in your environment and pass:

rustc --env-set PATH=env

Then you will have:

assert_eq!(env!("PATH"), "env");

Crates will be fully re-compiled if --env-set arguments are changed.

Tracking issue is #118372 (it contains le list of PRs for implementing it).

Motivation

External tools like ninja or meson need to be able to specify environment variables through the command line to override current process environment. It also allows to force a recompilation if an environment variable passed with this flag is modified.

Use of this feature

It is currently used in mesonbuild/meson#10030.

Tests

List of tests for this option flag:

  • tests/ui/extenv/extenv-env-overload.rs
  • tests/ui/extenv/extenv-env.rs
  • tests/ui/extenv/extenv-not-env.rs
  • tests/ui/feature-gates/env-flag.rs
  • tests/ui/proc-macro/auxiliary/env.rs
  • tests/ui/proc-macro/env.rs

r? @davidtwco

@rustbot 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 Jan 13, 2024
@Noratrieb
Copy link
Member

Can you link to some discussion confirming that ninja has integrated this flag experimentally and that it works properly? We definitely need confirmation for that first before stabilizing anything.

@GuillaumeGomez
Copy link
Member Author

ninja? Also we can postpone approving this PR. No hurry here.

@Noratrieb
Copy link
Member

I thought this flag was added for ninja

@GuillaumeGomez
Copy link
Member Author

Ah I see. No idea. "meson devs" so not sure for what they use it exactly.

@Noratrieb Noratrieb 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 Jan 13, 2024
@Noratrieb
Copy link
Member

Marking it as waiting on author, you should prepare a stabilization report mentioning the use case again and pointing at build tools that tried to use this successfully.

@tgross35
Copy link
Contributor

I only know of ninja because it was mentioned here #80792 @dcbaker do you know any more about this?

@dcbaker
Copy link

dcbaker commented Jan 14, 2024

for us —set-env alone is a massive improvement, and the main thing we need. We can use the others, but we really need the set command

@Noratrieb
Copy link
Member

@dcbaker have you been able to integrate it and ensure that it solves all your use cases correctly?

@sdroege
Copy link
Contributor

sdroege commented Jan 15, 2024

This would be at least useful for meson, which needs something like this because of ninja's lack of support for environment variables. Other ninja-based build systems should benefit from it in the same way.

@dcbaker
Copy link

dcbaker commented Jan 15, 2024

I had planned on having a working prototype this weekend, but instead had no power or internet. Hopefully in the next day or so

@dcbaker
Copy link

dcbaker commented Jan 16, 2024

Meson with this PR: mesonbuild/meson#10030 demonstrates that --set-env does what we want. You can test this by running ./run_single_test.py "test cases/rust/23 env" with a nightly (at least 1.76) and non-nightly compiler and see that it works with the nightly and not otherwise. Eventually I need to implement a fallback path that uses a wrapper script to support versions of rust without the --set-env option, and I need to wire this into our infrastructure to build crates as native Meson projects, but this was a straightforward usecase we can start using as soon as it's available.

Edit: CI failures on that branch are expected, we don't have a nightly compiler in any of the images

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Implementation LGTM, some working changes in the documentation.

src/doc/rustc/src/command-line-arguments.md Outdated Show resolved Hide resolved
src/doc/rustc/src/command-line-arguments.md Outdated Show resolved Hide resolved
src/doc/rustc/src/command-line-arguments.md Outdated Show resolved Hide resolved
src/doc/rustc/src/command-line-arguments.md Outdated Show resolved Hide resolved
src/doc/rustc/src/command-line-arguments.md Outdated Show resolved Hide resolved
@davidtwco
Copy link
Member

Could you write a brief stabilisation report describing what is being stabilised and then I'll start an MCP?

@GuillaumeGomez
Copy link
Member Author

Thanks for the review @davidtwco! I added the stabilization report in the first comment of this PR.

@davidtwco
Copy link
Member

@rfcbot fcp merge

See stabilization report.

@rfcbot
Copy link

rfcbot commented Jan 18, 2024

Team member @davidtwco has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 18, 2024
@petrochenkov
Copy link
Contributor

Besides (option_)env! and tracked_env::var rustc also accesses several dozens of environment variables in different parts of the compiler (starting from PATH and RUSTC_BOOTSTRAP).

None of those are tracked in depinfo.
At least some of those should supposedly be tracked ("Rebuild if deployment target changes").
I suspect that some of them should also not be tracked to avoid unnecessary rebuilds.

I wonder how should all of them interact with --set-env.
Never interact? Always interact? Interact on a case-by-case basis?

@rfcbot concern other-rustc-vars

@nagisa
Copy link
Member

nagisa commented Jan 28, 2024

I have a hard time understanding how this functionality can be useful in the form that only affects the results of the env!, option_env! and perhaps a couple other APIs but not e.g. what is inherited by the spawned child executables or returned by the usual environment variable handling APIs. In particular motivation section above also claims that some tools need an ability to override environment proper, so it would seem to me that the feature as it is proposed for stabilisation does not actually satisfy the needs of those tools?

I also have a hard time wrapping my head around why the behaviour of falling back to regular environment variables would be desirable. It seems to me that once you care enough to specify --env-set, you probably care about not leaking in random junk from the environment…

@bors
Copy link
Contributor

bors commented Feb 16, 2024

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

@xclaesse
Copy link

If I want to set an environment variable to an empty value, should we allow rustc -Zunstable-options --env-set FOO? That does not seems to work currently, but rustc -Zunstable-options --env-set FOO= works.

@Dylan-DPC
Copy link
Member

After a discussion with the author, they have preferred this to be closed as they don't have a usecase or motivation for completing it.

@Dylan-DPC Dylan-DPC closed this Aug 4, 2024
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 4, 2024
@Dylan-DPC Dylan-DPC added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for --env-set flag Add option to pass environment variables to rustc with a flag