env: clear environment variables that interfere with Python #375
Conversation
FFY00
force-pushed
the
env-clear-env-vars
branch
from
782bbfd
to
c64959e
Compare
Some environment variables, like PYTHONPATH, interfere with Python. They should be cleared when constructing an isolated environment so that they do not cause anything from the original environment to leak into our new environment. Fixes pypa#373 Signed-off-by: Filipe Laíns <lains@riseup.net>
FFY00
force-pushed
the
env-clear-env-vars
branch
from
c64959e
to
a38a18b
Compare
| @@ -122,6 +134,10 @@ def __exit__( | |||
| :param exc_val: The value of exception raised (if any) | |||
| :param exc_tb: The traceback of exception raised (if any) | |||
| """ | |||
| for name, old_value in self._old_env_values.items(): | |||
| if old_value is not None: | |||
| os.environ[name] = old_value | |||
There was a problem hiding this comment.
Ugh, this makes the entire class not thread safe. IMHO we should instead create a copy of os.environ, alter that one and pass it down to the subprocess calls we end up invoking.
There was a problem hiding this comment.
In that case we need to add a subprocess helper and require people to always use it, which is very limiting and requires people to change their code. Most people are using it in single threaded code, so it would be very disruptive IMO.
What about adding a keep_env/skip_env argument, defaulting to False, to disable the environment variable modification, and add both a subprocess helper and a env attribute with the env that should be used in subprocess invocations? The bad side is that people running this in multi-threaded/parallel situations would have to opt-in, but this way it would not disrupt existing code and would keep the API simple for single-threaded code, which is most of it. I feel this compromise is reasonable, what do you think?
There was a problem hiding this comment.
Sounds reasonable if that's the case 👍 I thought this might be easier considering we already overwrite the pep517 packages subprocess invocation with our own as far as I remember 🤔 (so we should now that part).
There was a problem hiding this comment.
Hum, actually, even if a bit disruptive, I am now leading towards update_env argument, with the opposite function. The API I proposed above motivates? (I don't remember the word I was looking for, something along those lines but that made sense in this sentence) non thread safe APIs, which is something we should probably avoid. Worse case scenario, people will get the exact same behavior as currently, they just might run into #373. And this would technically be a breaking change in thread safety, even though the line there is a bit blurry.
There was a problem hiding this comment.
We 'overwrite' it in the project builder for all builds. If we ovewrite it in the isolated env class then users will have to pass the isolated env's runner to the project builder and we'd need to provide some sort of function to wrap user-provided subprocess runners if they are to be used in conjunction with an isolated env. We should definitely not add a flag AND a subprocess runner wrapper AND an env attribute AND optionally mutate os.environ.
There was a problem hiding this comment.
It shouldn't need to interface with the builder, only with the isolated env, right? So you could slot in any isolated env you like:
with IsolatedEnvBuilder(...) as isolated_env:
ProjectBuilder.from_isolated_env(isolated_env)
# Or...
with MyCustomEnvBuilderWhichReturnsAnIsolatedEnvSubclass() as isolated_env:
ProjectBuilder.from_isolated_env(isolated_env)There was a problem hiding this comment.
It shouldn't need to interface with the builder, only with the isolated env, right? So you could slot in any isolated env you like:
Hum, sure. That looks good to me.
I am not sure if it would make sense to make it public API, probably not.
Actually, as long as we keep the API simple, I think it would be alright.
There was a problem hiding this comment.
Actually, as long as we keep the API simple, I think it would be alright.
Then we cannot go down by adopting my pep-517 implementation path. The API is purposefully not simple because encourages maximum flexibility. The entire frontend is public and non-trivial https://github.com/tox-dev/tox/blob/rewrite/src/tox/util/pep517/frontend.py#L1
There was a problem hiding this comment.
Could you create a new issue explaining how this differs from pep517 and how to proceed with adopting it?
There was a problem hiding this comment.
Sorry, don't have time for that. At its core it differs by:
- allow keeping alive the backend to reuse it in between commands
- provides stdout/stderr for commands executed
- frontend python 3 only and type hinted 👍
|
Let's close it. |
Some environment variables, like PYTHONPATH, interfere with Python.
They should be cleared when constructing an isolated environment so that
they do not cause anything from the original environment to leak into
our new environment.
Fixes #373
Signed-off-by: Filipe Laíns lains@riseup.net