-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix VIRTUALENV_PYTHON
environment lookup
#1998
Changes from 4 commits
4c3e4e2
e0351cd
3a6fe60
a59e31c
4f891ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Fix processing of the ``VIRTUALENV_PYTHON`` environment variable and make it | ||
multi-value as well (separated by comma) - by :user:`pneff`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
|
||
import pytest | ||
|
||
from virtualenv.config.cli.parser import VirtualEnvOptions | ||
from virtualenv.config.ini import IniConfig | ||
from virtualenv.run import session_via_cli | ||
from virtualenv.util.path import Path | ||
|
@@ -31,6 +32,20 @@ def test_value_bad(monkeypatch, caplog, empty_conf): | |
assert "invalid literal" in caplog.messages[0] | ||
|
||
|
||
def test_python_via_env_var(monkeypatch): | ||
options = VirtualEnvOptions() | ||
monkeypatch.setenv(str("VIRTUALENV_PYTHON"), str("python3")) | ||
session_via_cli(["venv"], options=options) | ||
assert options.python == ["python3"] | ||
|
||
|
||
def test_python_multi_value_via_env_var(monkeypatch): | ||
options = VirtualEnvOptions() | ||
monkeypatch.setenv(str("VIRTUALENV_PYTHON"), str("python3,python2")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, I did not realize that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, what we have is correct, but instead of having different split character depending on the type, let's always accept both. I don't expect people to use comma in paths, so IMHO, this is alright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am pushing a commit which implements this change. I opted for only accepting one split option at a time - so if the environment variable contains newlines, then we don't also accept commas. Do you agree with that decision? I quickly went back to the legacy code as well, and it seems that newline support has only been added with the rewrite. The previous version had space separation for path names. Thus I would expect the backwards compatibility impact of this change to be minimal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. Can you add a test demonstrating/validating this? Also, the documentation should explicitly state that only of the two will be accepted, and not both at the same time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those tests have been added now. |
||
session_via_cli(["venv"], options=options) | ||
assert options.python == ["python3", "python2"] | ||
|
||
|
||
def test_extra_search_dir_via_env_var(tmp_path, monkeypatch): | ||
monkeypatch.chdir(tmp_path) | ||
value = "a{}0{}b{}c".format(os.linesep, os.linesep, os.pathsep) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test to validate that
python2,python3
works as expected too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extended the code to provide that functionality. Multi-value was split with newline so far, though it seems the only other multi-value field is
extra-search-dir
.Let me know if you are happy with the approach taken or would prefer a different way to do this.