From cbbf465f82e4020cfe85563ce0eece4bdf2878a2 Mon Sep 17 00:00:00 2001 From: Ofek Lev Date: Sat, 27 Apr 2024 14:43:00 -0400 Subject: [PATCH] Fix PATH-based Python discovery on Windows (#2712) --- .github/workflows/check.yml | 8 +++-- docs/changelog/2712.bugfix.rst | 1 + src/virtualenv/discovery/builtin.py | 44 ++++++++++++++++++++------ src/virtualenv/discovery/py_spec.py | 10 +++++- src/virtualenv/info.py | 5 +++ tests/unit/discovery/test_discovery.py | 8 +++-- 6 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 docs/changelog/2712.bugfix.rst diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index cd47eb571..0fb1b5fc8 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -28,11 +28,9 @@ jobs: - "3.10" - "3.9" - "3.8" - - "3.7" - pypy-3.10 - pypy-3.9 - pypy-3.8 - - pypy-3.7 os: - ubuntu-latest - macos-latest @@ -42,6 +40,12 @@ jobs: - { os: macos-latest, py: "brew@3.10" } - { os: macos-latest, py: "brew@3.9" } - { os: macos-latest, py: "brew@3.8" } + - { os: ubuntu-latest, py: "3.7" } + - { os: windows-latest, py: "3.7" } + - { os: macos-12, py: "3.7" } + - { os: ubuntu-latest, py: "pypy-3.7" } + - { os: windows-latest, py: "pypy-3.7" } + - { os: macos-12, py: "pypy-3.7" } steps: - uses: taiki-e/install-action@cargo-binstall - name: Install OS dependencies diff --git a/docs/changelog/2712.bugfix.rst b/docs/changelog/2712.bugfix.rst new file mode 100644 index 000000000..fee543691 --- /dev/null +++ b/docs/changelog/2712.bugfix.rst @@ -0,0 +1 @@ +fix PATH-based Python discovery on Windows - by :user:`ofek`. diff --git a/src/virtualenv/discovery/builtin.py b/src/virtualenv/discovery/builtin.py index 51de52903..2a10f8292 100644 --- a/src/virtualenv/discovery/builtin.py +++ b/src/virtualenv/discovery/builtin.py @@ -6,7 +6,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Callable -from virtualenv.info import IS_WIN +from virtualenv.info import IS_WIN, fs_path_id from .discover import Discover from .py_info import PythonInfo @@ -84,7 +84,7 @@ def get_interpreter( return None -def propose_interpreters( # noqa: C901, PLR0912 +def propose_interpreters( # noqa: C901, PLR0912, PLR0915 spec: PythonSpec, try_first_with: Iterable[str], app_data: AppData | None = None, @@ -92,6 +92,7 @@ def propose_interpreters( # noqa: C901, PLR0912 ) -> Generator[tuple[PythonInfo, bool], None, None]: # 0. try with first env = os.environ if env is None else env + tested_exes: set[str] = set() for py_exe in try_first_with: path = os.path.abspath(py_exe) try: @@ -99,7 +100,12 @@ def propose_interpreters( # noqa: C901, PLR0912 except OSError: pass else: - yield PythonInfo.from_exe(os.path.abspath(path), app_data, env=env), True + exe_raw = os.path.abspath(path) + exe_id = fs_path_id(exe_raw) + if exe_id in tested_exes: + continue + tested_exes.add(exe_id) + yield PythonInfo.from_exe(exe_raw, app_data, env=env), True # 1. if it's a path and exists if spec.path is not None: @@ -109,29 +115,44 @@ def propose_interpreters( # noqa: C901, PLR0912 if spec.is_abs: raise else: - yield PythonInfo.from_exe(os.path.abspath(spec.path), app_data, env=env), True + exe_raw = os.path.abspath(spec.path) + exe_id = fs_path_id(exe_raw) + if exe_id not in tested_exes: + tested_exes.add(exe_id) + yield PythonInfo.from_exe(exe_raw, app_data, env=env), True if spec.is_abs: return else: # 2. otherwise try with the current - yield PythonInfo.current_system(app_data), True + current_python = PythonInfo.current_system(app_data) + exe_raw = str(current_python.executable) + exe_id = fs_path_id(exe_raw) + if exe_id not in tested_exes: + tested_exes.add(exe_id) + yield current_python, True # 3. otherwise fallback to platform default logic if IS_WIN: from .windows import propose_interpreters # noqa: PLC0415 for interpreter in propose_interpreters(spec, app_data, env): + exe_raw = str(interpreter.executable) + exe_id = fs_path_id(exe_raw) + if exe_id in tested_exes: + continue + tested_exes.add(exe_id) yield interpreter, True # finally just find on path, the path order matters (as the candidates are less easy to control by end user) - tested_exes = set() find_candidates = path_exe_finder(spec) for pos, path in enumerate(get_paths(env)): logging.debug(LazyPathDump(pos, path, env)) for exe, impl_must_match in find_candidates(path): - if exe in tested_exes: + exe_raw = str(exe) + exe_id = fs_path_id(exe_raw) + if exe_id in tested_exes: continue - tested_exes.add(exe) - interpreter = PathPythonInfo.from_exe(str(exe), app_data, raise_on_error=False, env=env) + tested_exes.add(exe_id) + interpreter = PathPythonInfo.from_exe(exe_raw, app_data, raise_on_error=False, env=env) if interpreter is not None: yield interpreter, impl_must_match @@ -180,7 +201,10 @@ def path_exe_finder(spec: PythonSpec) -> Callable[[Path], Generator[tuple[Path, def path_exes(path: Path) -> Generator[tuple[Path, bool], None, None]: # 4. then maybe it's something exact on PATH - if it was direct lookup implementation no longer counts - yield (path / direct), False + direct_path = path / direct + if direct_path.exists(): + yield direct_path, False + # 5. or from the spec we can deduce if a name on path matches for exe in path.iterdir(): match = pat.fullmatch(exe.name) diff --git a/src/virtualenv/discovery/py_spec.py b/src/virtualenv/discovery/py_spec.py index 5f173a2ed..dcd84f423 100644 --- a/src/virtualenv/discovery/py_spec.py +++ b/src/virtualenv/discovery/py_spec.py @@ -79,9 +79,17 @@ def generate_re(self, *, windows: bool) -> re.Pattern: ) impl = "python" if self.implementation is None else f"python|{re.escape(self.implementation)}" suffix = r"\.exe" if windows else "" + version_conditional = ( + "?" + # Windows Python executables are almost always unversioned + if windows + # Spec is an empty string + or self.major is None + else "" + ) # Try matching `direct` first, so the `direct` group is filled when possible. return re.compile( - rf"(?P{impl})(?P{version}){suffix}$", + rf"(?P{impl})(?P{version}){version_conditional}{suffix}$", flags=re.IGNORECASE, ) diff --git a/src/virtualenv/info.py b/src/virtualenv/info.py index cdc0f725a..8b217d0c3 100644 --- a/src/virtualenv/info.py +++ b/src/virtualenv/info.py @@ -48,6 +48,10 @@ def fs_supports_symlink(): return _CAN_SYMLINK +def fs_path_id(path: str) -> str: + return path.casefold() if fs_is_case_sensitive() else path + + __all__ = ( "IS_CPYTHON", "IS_MAC_ARM64", @@ -56,5 +60,6 @@ def fs_supports_symlink(): "IS_ZIPAPP", "ROOT", "fs_is_case_sensitive", + "fs_path_id", "fs_supports_symlink", ) diff --git a/tests/unit/discovery/test_discovery.py b/tests/unit/discovery/test_discovery.py index a52aad0ec..0e11c2d90 100644 --- a/tests/unit/discovery/test_discovery.py +++ b/tests/unit/discovery/test_discovery.py @@ -16,7 +16,7 @@ @pytest.mark.skipif(not fs_supports_symlink(), reason="symlink not supported") @pytest.mark.parametrize("case", ["mixed", "lower", "upper"]) -@pytest.mark.parametrize("specificity", ["more", "less"]) +@pytest.mark.parametrize("specificity", ["more", "less", "none"]) def test_discovery_via_path(monkeypatch, case, specificity, tmp_path, caplog, session_app_data): # noqa: PLR0913 caplog.set_level(logging.DEBUG) current = PythonInfo.current_system(session_app_data) @@ -33,7 +33,11 @@ def test_discovery_via_path(monkeypatch, case, specificity, tmp_path, caplog, se # e.g. spec: python3.12.1, exe: /bin/python3 core_ver = ".".join(str(i) for i in current.version_info[0:3]) exe_ver = current.version_info.major - core = f"somethingVeryCryptic{core_ver}" + elif specificity == "none": + # e.g. spec: python3.12.1, exe: /bin/python + core_ver = ".".join(str(i) for i in current.version_info[0:3]) + exe_ver = "" + core = "" if specificity == "none" else f"{name}{core_ver}" exe_name = f"{name}{exe_ver}{'.exe' if sys.platform == 'win32' else ''}" target = tmp_path / current.install_path("scripts") target.mkdir(parents=True)