Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
65 changes: 54 additions & 11 deletions src/poetry/utils/env/python/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import os
import sys

from subprocess import CalledProcessError

from functools import cached_property
from pathlib import Path
from typing import TYPE_CHECKING
Expand Down Expand Up @@ -88,6 +90,13 @@ def find_all(cls) -> Iterator[Python]:
Path(venv) if (venv := os.environ.get("VIRTUAL_ENV")) else None
)
for python in findpython.find_all():
try:
# Ensure the python interpreter is valid and can be queried.
# Accessing interpreter property runs the check.
_ = python.interpreter
except (CalledProcessError, ValueError):
continue

if venv_path and is_relative_to(python.executable, venv_path):
continue
yield cls(python=python)
Expand Down Expand Up @@ -177,31 +186,52 @@ def name(self) -> str:

@property
def executable(self) -> Path:
return cast("Path", self._python.interpreter)
try:
return cast("Path", self._python.interpreter)
except CalledProcessError as e:
raise ValueError(f"Invalid Python executable: {self._python.executable}") from e

@property
def implementation(self) -> str:
return cast("str", self._python.implementation.lower())
try:
return cast("str", self._python.implementation.lower())
except CalledProcessError as e:
raise ValueError(f"Invalid Python executable: {self._python.executable}") from e

@property
def free_threaded(self) -> bool:
return cast("bool", self._python.freethreaded)
try:
return cast("bool", self._python.freethreaded)
except CalledProcessError as e:
raise ValueError(f"Invalid Python executable: {self._python.executable}") from e

@property
def major(self) -> int:
return cast("int", self._python.major)
try:
return cast("int", self._python.major)
except CalledProcessError as e:
raise ValueError(f"Invalid Python executable: {self._python.executable}") from e

@property
def minor(self) -> int:
return cast("int", self._python.minor)
try:
return cast("int", self._python.minor)
except CalledProcessError as e:
raise ValueError(f"Invalid Python executable: {self._python.executable}") from e

@property
def patch(self) -> int:
return cast("int", self._python.patch)
try:
return cast("int", self._python.patch)
except CalledProcessError as e:
raise ValueError(f"Invalid Python executable: {self._python.executable}") from e

@property
def version(self) -> Version:
return Version.parse(str(self._python.version))
try:
return Version.parse(str(self._python.version))
except CalledProcessError as e:
raise ValueError(f"Invalid Python executable: {self._python.executable}") from e

@cached_property
def patch_version(self) -> Version:
Expand Down Expand Up @@ -233,12 +263,20 @@ def get_active_python(cls) -> Python | None:
or None if no valid environment is found.
"""
for python in ShutilWhichPythonProvider().find_pythons():
return cls(python=python)
try:
_ = python.interpreter
return cls(python=python)
except (CalledProcessError, ValueError):
pass

# fallback to findpython, restrict to finding only executables
# named "python" as the intention here is just that, nothing more
if python := findpython.find("python"):
return cls(python=python)
try:
_ = python.interpreter
return cls(python=python)
except (CalledProcessError, ValueError):
pass

return None

Expand All @@ -259,12 +297,17 @@ def get_system_python(cls) -> Python:
@classmethod
def get_by_name(cls, python_name: str) -> Python | None:
# Ignore broken installations.
with contextlib.suppress(ValueError):
with contextlib.suppress(ValueError, CalledProcessError):
if python := ShutilWhichPythonProvider.find_python_by_name(python_name):
_ = python.interpreter
return cls(python=python)

if python := findpython.find(python_name):
return cls(python=python)
try:
_ = python.interpreter
return cls(python=python)
except (CalledProcessError, ValueError):
pass

return None

Expand Down
74 changes: 74 additions & 0 deletions tests/utils/test_python_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,77 @@ def test_python_find_compatible(
python = Python.get_compatible_python(poetry)

assert Version.from_parts(3, 4) <= python.version <= Version.from_parts(4, 0)


def test_python_ignores_broken_installations(mocker: MockerFixture) -> None:
from subprocess import CalledProcessError

mock_good_pv = mocker.MagicMock(spec=findpython.PythonVersion)
Comment on lines +146 to +149

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Add a positive-path test for get_by_name where a broken primary result is ignored but a valid fallback from findpython.find is used.

The existing test only covers the case where both ShutilWhichPythonProvider.find_python_by_name and findpython.find fail and get_by_name returns None. Please also add a test where find_python_by_name yields a broken PythonVersion (raising CalledProcessError/ValueError on interpreter), but findpython.find(python_name) returns a valid PythonVersion, and assert that Python.get_by_name(python_name) returns a non-None Python to cover the new fallback behavior.

Suggested change
def test_python_ignores_broken_installations(mocker: MockerFixture) -> None:
from subprocess import CalledProcessError
mock_good_pv = mocker.MagicMock(spec=findpython.PythonVersion)
def test_get_by_name_uses_fallback_for_broken_installations(
mocker: MockerFixture,
) -> None:
from subprocess import CalledProcessError
python_name = "python"
mock_bad_pv = mocker.MagicMock(spec=findpython.PythonVersion)
mock_bad_pv.executable = Path("/usr/bin/broken-python")
type(mock_bad_pv).interpreter = mocker.PropertyMock(
side_effect=CalledProcessError(1, [python_name])
)
mock_good_pv = mocker.MagicMock(spec=findpython.PythonVersion)
mock_good_pv.executable = Path("/usr/bin/python3.12")
mock_good_pv.interpreter = Path("/usr/bin/python3.12")
mock_good_pv.version = packaging.version.Version("3.12.0")
mocker.patch(
"poetry.utils.env.python.providers.ShutilWhichPythonProvider."
"find_python_by_name",
return_value=mock_bad_pv,
)
mocker.patch("findpython.find", return_value=mock_good_pv)
python = Python.get_by_name(python_name)
assert python is not None
def test_python_ignores_broken_installations(mocker: MockerFixture) -> None:
from subprocess import CalledProcessError
mock_good_pv = mocker.MagicMock(spec=findpython.PythonVersion)

mock_good_pv.executable = Path("/usr/bin/python3.12")
mock_good_pv.interpreter = Path("/usr/bin/python3.12")
mock_good_pv.version = packaging.version.Version("3.12.0")

mock_bad_pv = mocker.MagicMock(spec=findpython.PythonVersion)
mock_bad_pv.executable = Path("/usr/bin/python2.7")
type(mock_bad_pv).interpreter = mocker.PropertyMock(
side_effect=CalledProcessError(2, ["/usr/bin/python2.7", "-Ic", "..."])
)

mocker.patch("findpython.find_all", return_value=[mock_bad_pv, mock_good_pv])

pythons = list(Python.find_all())
assert len(pythons) == 1
assert pythons[0].executable == Path("/usr/bin/python3.12")


def test_get_active_python_ignores_broken_installations(mocker: MockerFixture) -> None:
from subprocess import CalledProcessError

mock_bad_pv = mocker.MagicMock(spec=findpython.PythonVersion)
mock_bad_pv.executable = Path("/usr/bin/python2.7")
type(mock_bad_pv).interpreter = mocker.PropertyMock(
side_effect=CalledProcessError(2, ["/usr/bin/python2.7", "-Ic", "..."])
)

mocker.patch(
"poetry.utils.env.python.providers.ShutilWhichPythonProvider.find_pythons",
return_value=[mock_bad_pv],
)
mocker.patch("findpython.find", return_value=None)

assert Python.get_active_python() is None


def test_get_by_name_ignores_broken_installations(mocker: MockerFixture) -> None:
from subprocess import CalledProcessError

mock_bad_pv = mocker.MagicMock(spec=findpython.PythonVersion)
mock_bad_pv.executable = Path("/usr/bin/python2.7")
type(mock_bad_pv).interpreter = mocker.PropertyMock(
side_effect=CalledProcessError(2, ["/usr/bin/python2.7", "-Ic", "..."])
)

mocker.patch(
"poetry.utils.env.python.providers.ShutilWhichPythonProvider.find_python_by_name",
return_value=mock_bad_pv,
)
mocker.patch("findpython.find", return_value=None)

assert Python.get_by_name("python") is None


def test_python_properties_raise_value_error_on_subprocess_failure(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Expand this test (or add another) to cover ValueError from interpreter and multiple properties (version, implementation, etc.), not just executable.

Since the implementation now wraps CalledProcessError for multiple properties (implementation, free_threaded, major, minor, patch, version) and also catches ValueError in other places, this test should exercise those paths as well. Consider parametrizing over these properties to assert they all raise ValueError when the underlying PythonVersion raises CalledProcessError, and add a test where interpreter (or another attribute) raises ValueError directly to confirm it is surfaced unchanged and with a consistent message.

mocker: MockerFixture,
) -> None:
from subprocess import CalledProcessError

mock_bad_pv = mocker.MagicMock(spec=findpython.PythonVersion)
mock_bad_pv.executable = Path("/usr/bin/python2.7")
type(mock_bad_pv).interpreter = mocker.PropertyMock(
side_effect=CalledProcessError(2, ["/usr/bin/python2.7", "-Ic", "..."])
)

python = Python(python=mock_bad_pv)
with pytest.raises(ValueError, match="Invalid Python executable"):
_ = python.executable

Loading