fix(env): prevent crash when invalid Python or Python 2.x is in PATH#10962
fix(env): prevent crash when invalid Python or Python 2.x is in PATH#10962Abiggj wants to merge 4 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The repeated try/except blocks converting CalledProcessError to ValueError in each property (executable, implementation, free_threaded, major, minor, patch, version) could be centralized via a small helper (e.g., a private method or descriptor) to avoid duplication and keep the error handling logic consistent in one place.
- Similarly, the explicit interpreter validation logic (touching python.interpreter and catching CalledProcessError/ValueError) is duplicated across find_all, get_active_python, and get_by_name; consider extracting a shared helper like
_is_valid_python(python)to make the intent clearer and reduce the chance of divergence in future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated try/except blocks converting CalledProcessError to ValueError in each property (executable, implementation, free_threaded, major, minor, patch, version) could be centralized via a small helper (e.g., a private method or descriptor) to avoid duplication and keep the error handling logic consistent in one place.
- Similarly, the explicit interpreter validation logic (touching python.interpreter and catching CalledProcessError/ValueError) is duplicated across find_all, get_active_python, and get_by_name; consider extracting a shared helper like `_is_valid_python(python)` to make the intent clearer and reduce the chance of divergence in future changes.
## Individual Comments
### Comment 1
<location path="tests/utils/test_python_manager.py" line_range="203" />
<code_context>
+ assert Python.get_by_name("python") is None
+
+
+def test_python_properties_raise_value_error_on_subprocess_failure(
+ mocker: MockerFixture,
+) -> None:
</code_context>
<issue_to_address>
**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.
</issue_to_address>
### Comment 2
<location path="tests/utils/test_python_manager.py" line_range="146-149" />
<code_context>
+ assert Python.get_active_python() is None
+
+
+def test_get_by_name_ignores_broken_installations(mocker: MockerFixture) -> None:
+ from subprocess import CalledProcessError
+
</code_context>
<issue_to_address>
**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.
```suggestion
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)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert Python.get_by_name("python") is None | ||
|
|
||
|
|
||
| def test_python_properties_raise_value_error_on_subprocess_failure( |
There was a problem hiding this comment.
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.
| def test_python_ignores_broken_installations(mocker: MockerFixture) -> None: | ||
| from subprocess import CalledProcessError | ||
|
|
||
| mock_good_pv = mocker.MagicMock(spec=findpython.PythonVersion) |
There was a problem hiding this comment.
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.
| 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) |
Documentation Added
Refactoring & CentralizationTo address code duplication and keep error-handling logic consistent:
|
|
|
This should be fixed in findpython. IIRC there already is an open pull request there. |
That makes sense, but having this fix in Poetry will act as a safety net. Even if findpython is patched in future, Poetry supports a range of older findpython versions; so users on older versions would still experience the crash. Adding this within code makes it resilient to legacy Python environments in a user's PATH, ensuring a raw subprocess error doesn't completely block basic commands like |
If and when findpython release a fix, poetry can bump its required findpython version. |
2 participants
Pull Request Check List
Resolves: #10897
Description
With Poetry 2.4, system Python discovery checks candidate binaries using
findpython. Some older or broken Python interpreters (specifically Python 2.7) do not support the isolated-Ioption when probed, resulting in aCalledProcessErrorwith exit status 2 and crashing various Poetry commands.This PR fixes the crash by:
CalledProcessErrorandValueErrorinside python discovery functions (find_all,get_active_python,get_by_name). When encountered, the invalid Python binary is safely skipped.CalledProcessErrorexceptions inside python manager properties (executable,version,implementation, etc.) to a standardValueError.tests/utils/test_python_manager.pythat mock the discovery of broken Python installations (such as Python 2.7) to verify that they are cleanly ignored.The documentation wasn't updates but is prepared, I shall add those once these changes are ensured that they are working and robust.