Require explicit environment usage #11641
Conversation
[ci skip-rust] [ci skip-build-wheels]
…ronment or a new filtered Environment type. [ci skip-build-wheels] [ci skip-rust]
[ci skip-build-wheels]
stuhood
force-pushed
the
stuhood/explicit-env
branch
from
d6286eb
to
130b280
Compare
|
Commits are useful to review independently. |
| @rule | ||
| async def complete_environment(session_values: SessionValues) -> CompleteEnvironment: | ||
| return session_values[CompleteEnvironment] | ||
|
|
||
|
|
||
| @rule | ||
| async def environment_subset( |
There was a problem hiding this comment.
This can be a non async function which begs the question of why not just a utility. Are you making room here?
There was a problem hiding this comment.
Good point: can drop the async. But the reason to expose this as a @rule is that we want to discourage direct access to the CompleteEnvironment and expose an API that encourages requesting a filtered copy, because it should allow @rules to be cleaned with early-cutoff in more cases. When a @rule requests the entire environment, it will be invalidated for any environment change.
There was a problem hiding this comment.
Could you s/encourage/force/ by just changing the relevant Process field type?
There was a problem hiding this comment.
Possibly...? I think that a majority of users of Process right now are constructing an environment for the process by hand, and only some cases are pulling it in from the outside world. Most users of InteractiveProcess will use the CompleteEnvironment, but not all, etc.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…ronment as deprecated. [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
stuhood
force-pushed
the
stuhood/explicit-env
branch
from
1a08dba
to
79b8ae0
Compare
…nvironment usage to unblock pantsbuild#11641. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…caching (#11678) ### Problem #11641 changed how `ExecutionOptions` are computed for the "bootstrap" `Scheduler` that is used for plugin resolution (in order to allow any local-affecting options to still be used, while only disabling remote execution). But the `local_only` flag was sloppily applied, and did not affect the boolean flags that actually enable or disable remote caching and execution. ### Solution Apply the `ExecutionOptions.from_options(local_only)` flag more thoroughly, so that both caching and execution are disabled (in addition to skipping the auth plugin). ### Result Repositories that use auth plugins and remote execution will not experience errors during plugin bootstrapping. [ci skip-build-wheels] [ci skip-rust]
) Since pantsbuild#11641, `sys.argv` has not been the right way to get the args for a particular run of pants. Explicitly pass in the args via the `OptionsBootstrapper`, and fix the test (which was actually testing its _own_ `sys.argv`). Fixes pantsbuild#11930. [ci skip-rust] [ci skip-build-wheels]
Problem
In order to allow concurrent runs of Pants in #11639 (part of #7654), all global mutable singletons must be either 1) thread-local, 2) replaced with explicitly passed values.
os.environandsys.argvare two of the last cases we're concerned with.Solution
Although we could hypothetically redefine
os.environandos.getenvas thread-local, they seem to be a better fit to be explicitly passed. So this change:PantsEnvironmenttoCompleteEnvironmentEnvironmentandEnvironmentRequest, which filter theCompleteEnvironmentto a smaller subset, and should be used more frequently by@rules.os.environusage in thePythonSetupsubsystem by explicitly selecting interpreter-selection env vars (HOME/PATH/PYENV_ROOT)Finally, we empty the environment for
pantsdruns to enforce explicit usage ofCompleteEnvironment/Environment.Result
The last (?) global mutable variable blocking #7654 is removed.
[ci skip-build-wheels]