Fix run, repl, and test --debug to have hermetic environments
#10668
Conversation
Eric-Arellano
requested review from
stuhood,
benjyw,
cosmicexplorer and
gshuflin
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
Eric-Arellano
force-pushed
the
hermetic-ip
branch
from
696192b
to
e336e38
Compare
| @@ -5,7 +5,7 @@ | |||
| import logging | |||
There was a problem hiding this comment.
The changes in this file are so that test --debug uses the same argv and env as the normal Process would be.
This rewrites it to use our new `BinaryPaths` abstraction to find the `open` program. It also fixes a cheat where we were using `Console` and `InteractiveRunner` outside of a goal rule. # 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]
|
Okay, ready for review. |
# 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]
[ci skip-rust] [ci skip-build-wheels]
|
While a hermetic environment makes sense for |
…nments (pantsbuild#10668)" This reverts commit aa6e1a4. Unfortunately it caused the wheel-building CI shards to fail in master. [ci skip-build-wheels]
|
Note: Reverted in #10688, until we can figure out why this broke the wheel building shards. |
|
I don't have a strong opinion on I do strongly believe that If we have |
|
I can see that the user might expect Not sure I agree that |
My argument with |
…etic (#10701) ## Problem See #10668 for the original implementation, which ended up getting reverted. As pointed out there and in #10644, there's an argument to keeping `run` and `repl` non-hermetic, i.e. using the env for the parent `./pants` process. Neither of those goals are cached. REPL is designed for experimentation. Run's analog is `binary`; when producing a `binary` and then running it, the `binary` would end up having the env of the host platform, rather than what Pants sets. However, we do want `test --debug` to be hermetic so that `test` and `test --debug` behave the same. ## Solution Add the argument `hermetic_env: bool` to the `InteractiveProcess` constructor, which controls whether the env gets stripped or not. We default this to `True` as, generally, hermiticity is a good default. `run.py` and `repl.py` ensure that `hermetic_env` is always set to False. ### Nuance: env var may be still be overridden If the `InteractiveProcess` includes an env var that is already set in the parent process's env, then we will override the env var with Pants's value. Tangibly, the Python implementations for `repl` and `run` will override the `$PATH` to whatever is set via `--pex-executable-search-paths`. [ci skip-build-wheels]
Previously,
InteractiveProcessused whatever theenvwas for the./pantsprocess. While this can be convenient, it is not consistent with the rest of Pants. For example, it was possible for a test to pass withtest --debugbut fail withtest --no-debug, or vice versa, due to differences with the env.This change reinforces the need for #10644, and for that to be ergonomic for users to set arbitrary env vars.
[ci skip-build-wheels]