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

Use explicit environment on subprocess #1546

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

mrclary
Copy link

@mrclary mrclary commented Apr 15, 2020

Resolves #1540.

Use an explicit environment for subprocess to ensure that existing environment variables are not inherited. This ensures more reliable results.

@mrclary
Copy link
Author

mrclary commented Apr 15, 2020

Tested locally on my system, under both conda and pyenv environments, TestSetupReadline.test_import has one failure due to assert len(difference) < 22 where len(difference) is 47 for the conda environment and 45 for th pyenv environment. Both environments have only parso==0.7.0 and pytest explicitly installed (Python 3.7.7, MacOS 10.14.6).

I'm not sure what purpose this assertion serves, since you've increased it recently from 21...
Nevertheless, all other tests pass locally for me.

Note that I do not know what effects there will be on Windows systems. In the subproccess.Popen documentation, I found:

Note: If specified, env must provide any variables required for the program to execute. On Windows, in order to run a side-by-side assembly the specified env must include a valid SystemRoot.

@davidhalter
Copy link
Owner

davidhalter commented Apr 15, 2020

Thanks for this patch!

Note that I do not know what effects there will be on Windows systems

That's fine I guess, the Windows CI passes. If it doesn't work for people, I guess I will hear from them :).

Tested locally on my system, under both conda and pyenv environments, TestSetupReadline.test_import has one failure due to assert len(difference) < 22 where len(difference) is 47 for the conda environment and 45 for th pyenv environment. Both environments have only parso==0.7.0 and pytest explicitly installed (Python 3.7.7, MacOS 10.14.6).

This is test is there to ensure that the actual completions for os are not too different from typeshed's definitions. It would probably need a bit of love to make it pass on virtualenvs as well. (Just raising the numbers is not really what I want).

env = {}
if os.name == 'nt':
# Windows requires SYSTEMROOT
env.update(SYSTEMROOT=os.environ.get('SYSTEMROOT'))
Copy link
Owner

Choose a reason for hiding this comment

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

Minor issue: While SYSTEMROOT is probably defined most of the time, if it is not we should just not set it. The problem is essentially that it could be empty. If it is we cannot set it:

>>> os.environ['asdf'] = None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.5/os.py", line 730, in __setitem__
    value = self.encodevalue(value)
  File "/usr/lib/python3.5/os.py", line 798, in encode
    raise TypeError("str expected, not %s" % type(value).__name__)
TypeError: str expected, not NoneType

Copy link
Author

Choose a reason for hiding this comment

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

Another issue: Windows environment variables are case insensitive, so it also be SYSTEMROOT, SystemRoot, or maybe systemroot. I'll put in some robustness and if none are present, I'll refrain from setting.

…environment variables are not inherited. This ensures more reliable results, see issue davidhalter#1540.

* Attempt to send SYSTEMROOT variable to Windows subprocess
@davidhalter
Copy link
Owner

Thanks again for you work here and the good reasoning :).

@davidhalter davidhalter merged commit 803c3cb into davidhalter:master Apr 15, 2020
@mrclary mrclary deleted the clean-environment branch April 16, 2020 14:36
@mrclary mrclary mentioned this pull request Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jedi has wrong sys.path when started from pyenv virtual environment
2 participants