Fix PATH-based Python discovery on Windows (#2712)

Author: ofek

.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: "[email protected]" }
           - { os: macos-latest, py: "[email protected]" }
           - { os: macos-latest, py: "[email protected]" }
+          - { 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

docs/changelog/2712.bugfix.rst

@@ -0,0 +1 @@
+fix PATH-based Python discovery on Windows - by :user:`ofek`.

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,22 +84,28 @@ 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,
     env: Mapping[str, str] | None = None,
 ) -> 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:
             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, 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)

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>{impl})(?P<v>{version}){suffix}$",
+            rf"(?P<impl>{impl})(?P<v>{version}){version_conditional}{suffix}$",
             flags=re.IGNORECASE,
         )
 

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",
 )

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)