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

Improve discovery on Windows and provide escape hatchet #2046

Merged
merged 1 commit into from
Jan 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/changelog/1986.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
On Windows python ``3.7+`` distributions where the exe shim is missing fallback to the old ways - by :user:`gaborbernat`.
2 changes: 2 additions & 0 deletions docs/changelog/2046.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
When discovering interpreters on Windows, via the PEP-514, prefer ``PythonCore`` releases over other ones. virtualenv
is used via pip mostly by this distribution, so prefer it over other such as conda - by :user:`gaborbernat`.
3 changes: 3 additions & 0 deletions docs/changelog/2046.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
The builtin discovery takes now a ``--try-first-with`` argument and is first attempted as valid interpreters. One can
use this to force discovery of a given python executable when the discovery order/mechanism raises errors -
by :user:`gaborbernat`.
19 changes: 13 additions & 6 deletions src/virtualenv/create/via_global_ref/builtin/cpython/cpython3.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,27 @@ def setup_meta(cls, interpreter):
def sources(cls, interpreter):
for src in super(CPython3Windows, cls).sources(interpreter):
yield src
if not cls.venv_37p(interpreter):
if not cls.has_shim(interpreter):
for src in cls.include_dll_and_pyd(interpreter):
yield src

@staticmethod
def venv_37p(interpreter):
return interpreter.version_info.minor >= 7
@classmethod
def has_shim(cls, interpreter):
return interpreter.version_info.minor >= 7 and cls.shim(interpreter) is not None

@classmethod
def shim(cls, interpreter):
shim = Path(interpreter.system_stdlib) / "venv" / "scripts" / "nt" / "python.exe"
if shim.exists():
return shim
return None

@classmethod
def host_python(cls, interpreter):
if cls.venv_37p(interpreter):
if cls.has_shim(interpreter):
# starting with CPython 3.7 Windows ships with a venvlauncher.exe that avoids the need for dll/pyd copies
# it also means the wrapper must be copied to avoid bugs such as https://bugs.python.org/issue42013
return Path(interpreter.system_stdlib) / "venv" / "scripts" / "nt" / "python.exe"
return cls.shim(interpreter)
return super(CPython3Windows, cls).host_python(interpreter)

@classmethod
Expand Down
28 changes: 24 additions & 4 deletions src/virtualenv/discovery/builtin.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def __init__(self, options):
super(Builtin, self).__init__(options)
self.python_spec = options.python if options.python else [sys.executable]
self.app_data = options.app_data
self.try_first_with = options.try_first_with

@classmethod
def add_parser_arguments(cls, parser):
Expand All @@ -31,10 +32,19 @@ def add_parser_arguments(cls, parser):
help="interpreter based on what to create environment (path/identifier) "
"- by default use the interpreter where the tool is installed - first found wins",
)
parser.add_argument(
"--try-first-with",
dest="try_first_with",
metavar="py_exe",
type=str,
action="append",
default=[],
help="try first these interpreters before starting the discovery",
)

def run(self):
for python_spec in self.python_spec:
result = get_interpreter(python_spec, self.app_data)
result = get_interpreter(python_spec, self.try_first_with, self.app_data)
if result is not None:
return result
return None
Expand All @@ -47,11 +57,11 @@ def __unicode__(self):
return "{} discover of python_spec={!r}".format(self.__class__.__name__, spec)


def get_interpreter(key, app_data=None):
def get_interpreter(key, try_first_with, app_data=None):
spec = PythonSpec.from_string_spec(key)
logging.info("find interpreter for spec %r", spec)
proposed_paths = set()
for interpreter, impl_must_match in propose_interpreters(spec, app_data):
for interpreter, impl_must_match in propose_interpreters(spec, try_first_with, app_data):
key = interpreter.system_executable, impl_must_match
if key in proposed_paths:
continue
Expand All @@ -62,7 +72,17 @@ def get_interpreter(key, app_data=None):
proposed_paths.add(key)


def propose_interpreters(spec, app_data):
def propose_interpreters(spec, try_first_with, app_data):
# 0. try with first
for py_exe in try_first_with:
path = os.path.abspath(py_exe)
try:
os.lstat(path) # Windows Store Python does not work with os.path.exists, but does for os.lstat
except OSError:
pass
else:
yield PythonInfo.from_exe(os.path.abspath(path), app_data), True

# 1. if it's a path and exists
if spec.path is not None:
try:
Expand Down
5 changes: 4 additions & 1 deletion src/virtualenv/discovery/windows/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ def propose_interpreters(spec, cache_dir):
# see if PEP-514 entries are good

# start with higher python versions in an effort to use the latest version available
# and prefer PythonCore over conda pythons (as virtualenv is mostly used by non conda tools)
existing = list(discover_pythons())
existing.sort(key=lambda i: tuple(-1 if j is None else j for j in i[1:4]), reverse=True)
existing.sort(
key=lambda i: tuple(-1 if j is None else j for j in i[1:4]) + (1 if i[0] == "PythonCore" else 0,), reverse=True
)

for name, major, minor, arch, exe, _ in existing:
# pre-filter
Expand Down
2 changes: 1 addition & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ def temp_app_data(monkeypatch, tmp_path):
@pytest.fixture(scope="session")
def cross_python(is_inside_ci, session_app_data):
spec = str(2 if sys.version_info[0] == 3 else 3)
interpreter = get_interpreter(spec, session_app_data)
interpreter = get_interpreter(spec, [], session_app_data)
if interpreter is None:
msg = "could not find {}".format(spec)
if is_inside_ci:
Expand Down
10 changes: 5 additions & 5 deletions tests/unit/discovery/test_discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ def test_discovery_via_path(monkeypatch, case, tmp_path, caplog, session_app_dat
(target / pyvenv_cfg.name).write_bytes(pyvenv_cfg.read_bytes())
new_path = os.pathsep.join([str(target)] + os.environ.get(str("PATH"), str("")).split(os.pathsep))
monkeypatch.setenv(str("PATH"), new_path)
interpreter = get_interpreter(core)
interpreter = get_interpreter(core, [])

assert interpreter is not None


def test_discovery_via_path_not_found(tmp_path, monkeypatch):
monkeypatch.setenv(str("PATH"), str(tmp_path))
interpreter = get_interpreter(uuid4().hex)
interpreter = get_interpreter(uuid4().hex, [])
assert interpreter is None


Expand All @@ -52,13 +52,13 @@ def test_relative_path(tmp_path, session_app_data, monkeypatch):
cwd = sys_executable.parents[1]
monkeypatch.chdir(str(cwd))
relative = str(sys_executable.relative_to(cwd))
result = get_interpreter(relative, session_app_data)
result = get_interpreter(relative, [], session_app_data)
assert result is not None


def test_discovery_fallback_fail(session_app_data, caplog):
caplog.set_level(logging.DEBUG)
builtin = Builtin(Namespace(app_data=session_app_data, python=["magic-one", "magic-two"]))
builtin = Builtin(Namespace(app_data=session_app_data, try_first_with=[], python=["magic-one", "magic-two"]))

result = builtin.run()
assert result is None
Expand All @@ -68,7 +68,7 @@ def test_discovery_fallback_fail(session_app_data, caplog):

def test_discovery_fallback_ok(session_app_data, caplog):
caplog.set_level(logging.DEBUG)
builtin = Builtin(Namespace(app_data=session_app_data, python=["magic-one", sys.executable]))
builtin = Builtin(Namespace(app_data=session_app_data, try_first_with=[], python=["magic-one", sys.executable]))

result = builtin.run()
assert result is not None, caplog.text
Expand Down