From 9a848a8233d7923bb8f7ba1d8a827741c4f5685a Mon Sep 17 00:00:00 2001 From: darrenhuai <60621295+darrenhuai@users.noreply.github.com> Date: Wed, 28 Oct 2026 21:06:47 -0700 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix(discovery):=20let=20-p?= =?UTF-8?q?=20replace=20the=20python=20set=20in=20config=20or=20env?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --python and --try-first-with were plain append actions, so a value from virtualenv.ini or VIRTUALENV_PYTHON became the starting list and the command-line values were added after it. Discovery takes the first spec that resolves, so with python = 3.13 in virtualenv.ini, virtualenv -p 3.14 env built a 3.13 environment, and -p 3.12 on a machine without 3.12 succeeded with 3.13 instead of reporting that no interpreter was found. Builtin worked around this for the env var alone by rotating the list by one whenever VIRTUALENV_PYTHON was set. That covered a single env value, but with several values it moved the second one to the front, with or without -p, and it never helped the config file. Both options now use the ExtendAction from #3440, which already does this for --extra-search-dir: the first command-line value replaces the configured list and later ones are added in order. nargs=1 hands the action a one-item list per flag, since it extends with whatever it receives. A configured list is still tried in order when the command line gives none, and several -p are still all tried in order. The rotation in Builtin is gone because the parser now gets the order right. One visible change follows from the precedence: a -p that matches nothing now fails the same way it does without any configuration, instead of falling back to VIRTUALENV_PYTHON. --- .../+cli-python-overrides-config.bugfix.rst | 4 +++ src/virtualenv/discovery/builtin.py | 10 +++--- tests/unit/discovery/test_discovery.py | 35 +++++++++++++++++-- 3 files changed, 42 insertions(+), 7 deletions(-) create mode 100644 docs/changelog/+cli-python-overrides-config.bugfix.rst diff --git a/docs/changelog/+cli-python-overrides-config.bugfix.rst b/docs/changelog/+cli-python-overrides-config.bugfix.rst new file mode 100644 index 000000000..155bf95ed --- /dev/null +++ b/docs/changelog/+cli-python-overrides-config.bugfix.rst @@ -0,0 +1,4 @@ +Make ``-p``/``--python`` and ``--try-first-with`` replace the interpreters set by ``VIRTUALENV_PYTHON``, +``VIRTUALENV_TRY_FIRST_WITH`` or the configuration file instead of being merged with them, so the configured interpreter +no longer wins over the one asked for and a ``-p`` that finds nothing fails instead of falling back to it. Try several +``VIRTUALENV_PYTHON`` values in the order given rather than starting with the second. diff --git a/src/virtualenv/discovery/builtin.py b/src/virtualenv/discovery/builtin.py index 395e77116..04ff08ac6 100644 --- a/src/virtualenv/discovery/builtin.py +++ b/src/virtualenv/discovery/builtin.py @@ -7,6 +7,8 @@ from python_discovery import get_interpreter as _get_interpreter +from virtualenv.config.cli.parser import ExtendAction + from .discover import Discover if TYPE_CHECKING: @@ -36,8 +38,6 @@ class Builtin(Discover): def __init__(self, options: VirtualEnvOptions) -> None: super().__init__(options) self.python_spec = options.python or [sys.executable] - if self._env.get("VIRTUALENV_PYTHON"): - self.python_spec = self.python_spec[1:] + self.python_spec[:1] self.app_data = options.app_data self.try_first_with = options.try_first_with @@ -49,7 +49,8 @@ def add_parser_arguments(cls, parser: ArgumentParser) -> None: dest="python", metavar="py", type=str, - action="append", + nargs=1, + action=ExtendAction, default=[], help="interpreter based on what to create environment (path/identifier/version-specifier) " "- by default use the interpreter where the tool is installed - first found wins. " @@ -60,7 +61,8 @@ def add_parser_arguments(cls, parser: ArgumentParser) -> None: dest="try_first_with", metavar="py_exe", type=str, - action="append", + nargs=1, + action=ExtendAction, default=[], help="try first these interpreters before starting the discovery", ) diff --git a/tests/unit/discovery/test_discovery.py b/tests/unit/discovery/test_discovery.py index a5aba7e8f..3d6e698d8 100644 --- a/tests/unit/discovery/test_discovery.py +++ b/tests/unit/discovery/test_discovery.py @@ -5,6 +5,7 @@ import sys from argparse import Namespace from pathlib import Path +from typing import TYPE_CHECKING import pytest from python_discovery import PythonInfo @@ -13,6 +14,9 @@ from virtualenv.info import IS_WIN from virtualenv.run import cli_run, session_via_cli +if TYPE_CHECKING: + from pytest_mock import MockerFixture + def test_relative_path(session_app_data, monkeypatch) -> None: sys_executable = Path(PythonInfo.current_system(session_app_data).system_executable) @@ -61,9 +65,7 @@ def mock_get_interpreter(mocker): ("env_var", "python", "expected"), [ pytest.param("python_from_env_var", ["python_from_env_var"], "python_from_env_var", id="env-var-only"), - pytest.param( - "python_from_env_var", ["python_from_env_var", "python_from_cli"], "python_from_cli", id="env-var-and-cli" - ), + pytest.param("first,second", ["first", "second"], "first", id="env-var-multiple"), pytest.param(None, ["python_from_cli"], "python_from_cli", id="cli-only"), ], ) @@ -79,6 +81,33 @@ def test_returns_python_specified( # ruff:ignore[too-many-arguments] assert builtin.run() == getattr(mocker.sentinel, expected) +@pytest.mark.parametrize("source", ["env", "ini"]) +@pytest.mark.parametrize("cli", [(), ("first",), ("first", "second")]) +@pytest.mark.parametrize("dest", ["python", "try_first_with"]) +def test_cli_replaces_configured_interpreters( # ruff:ignore[too-many-arguments] + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, mocker: MockerFixture, source: str, cli: tuple[str, ...], dest: str +) -> None: + if source == "env": + monkeypatch.setenv(f"VIRTUALENV_{dest.upper()}", "configured-1,configured-2") + else: + config = tmp_path / "virtualenv.ini" + config.write_text(f"[virtualenv]\n{dest} =\n configured-1\n configured-2\n", encoding="utf-8") + monkeypatch.setenv("VIRTUALENV_CONFIG_FILE", str(config)) + tried: list[tuple[str, list[str]]] = [] + mocker.patch( + "virtualenv.discovery.builtin.get_interpreter", + lambda key, try_first_with, **_: tried.append((key, list(try_first_with))), + ) + option = f"--{dest.replace('_', '-')}" + args = [arg for value in cli for arg in (option, value)] + + with pytest.raises(RuntimeError, match="failed to find interpreter"): + session_via_cli([*args, str(tmp_path / "env")]) + + expected = list(cli) or ["configured-1", "configured-2"] + assert tried == ([(key, []) for key in expected] if dest == "python" else [(sys.executable, expected)]) + + def test_discovery_absolute_path_with_try_first(tmp_path, session_app_data) -> None: good_env = tmp_path / "good" bad_env = tmp_path / "bad" From 734d4a01a7933daae4415379b1948471c0cde6ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bern=C3=A1t=20G=C3=A1bor?= Date: Fri, 30 Oct 2026 11:23:52 -0700 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=93=9D=20docs(config):=20say=20a=20CL?= =?UTF-8?q?I=20value=20replaces=20the=20list?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document that -p and the other multi-value options replace the configured list, and why --python needs nargs=1 with ExtendAction. The precedence test becomes a table of expected discovery calls with an autospec mock, so it has no branches. --- docs/how-to/usage.rst | 3 ++ src/virtualenv/discovery/builtin.py | 1 + tests/unit/discovery/test_discovery.py | 64 ++++++++++++++++++-------- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/docs/how-to/usage.rst b/docs/how-to/usage.rst index d979b5873..d11fb7ac7 100644 --- a/docs/how-to/usage.rst +++ b/docs/how-to/usage.rst @@ -252,6 +252,9 @@ Specify multiple values on separate lines: /path/to/dists /path/to/other/dists +A value given on the command line replaces the configured list instead of adding to it. Repeat the option to pass +several, for example ``-p 3.14 -p 3.13``. + Environment variables ===================== diff --git a/src/virtualenv/discovery/builtin.py b/src/virtualenv/discovery/builtin.py index 04ff08ac6..07c41c009 100644 --- a/src/virtualenv/discovery/builtin.py +++ b/src/virtualenv/discovery/builtin.py @@ -49,6 +49,7 @@ def add_parser_arguments(cls, parser: ArgumentParser) -> None: dest="python", metavar="py", type=str, + # ExtendAction extends the list with the values it gets, so a lone string would split into characters nargs=1, action=ExtendAction, default=[], diff --git a/tests/unit/discovery/test_discovery.py b/tests/unit/discovery/test_discovery.py index 3d6e698d8..e5343278d 100644 --- a/tests/unit/discovery/test_discovery.py +++ b/tests/unit/discovery/test_discovery.py @@ -5,7 +5,7 @@ import sys from argparse import Namespace from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Final import pytest from python_discovery import PythonInfo @@ -81,31 +81,55 @@ def test_returns_python_specified( # ruff:ignore[too-many-arguments] assert builtin.run() == getattr(mocker.sentinel, expected) -@pytest.mark.parametrize("source", ["env", "ini"]) -@pytest.mark.parametrize("cli", [(), ("first",), ("first", "second")]) -@pytest.mark.parametrize("dest", ["python", "try_first_with"]) +@pytest.mark.parametrize( + ("variable", "value"), + [ + pytest.param("VIRTUALENV_{dest}", "configured-1,configured-2", id="env"), + pytest.param("VIRTUALENV_CONFIG_FILE", "{config}", id="ini"), + ], +) +@pytest.mark.parametrize( + ("dest", "cli", "tried"), + [ + pytest.param("python", [], [("configured-1", []), ("configured-2", [])], id="python-configured"), + pytest.param("python", ["-p", "first"], [("first", [])], id="python-cli-replaces"), + pytest.param("python", ["-p", "first", "-p", "second"], [("first", []), ("second", [])], id="python-cli-adds"), + pytest.param( + "try_first_with", [], [(sys.executable, ["configured-1", "configured-2"])], id="try-first-configured" + ), + pytest.param( + "try_first_with", ["--try-first-with", "first"], [(sys.executable, ["first"])], id="try-first-cli-replaces" + ), + pytest.param( + "try_first_with", + ["--try-first-with", "first", "--try-first-with", "second"], + [(sys.executable, ["first", "second"])], + id="try-first-cli-adds", + ), + ], +) def test_cli_replaces_configured_interpreters( # ruff:ignore[too-many-arguments] - tmp_path: Path, monkeypatch: pytest.MonkeyPatch, mocker: MockerFixture, source: str, cli: tuple[str, ...], dest: str + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + mocker: MockerFixture, + variable: str, + value: str, + dest: str, + cli: list[str], + tried: list[tuple[str, list[str]]], ) -> None: - if source == "env": - monkeypatch.setenv(f"VIRTUALENV_{dest.upper()}", "configured-1,configured-2") - else: - config = tmp_path / "virtualenv.ini" - config.write_text(f"[virtualenv]\n{dest} =\n configured-1\n configured-2\n", encoding="utf-8") - monkeypatch.setenv("VIRTUALENV_CONFIG_FILE", str(config)) - tried: list[tuple[str, list[str]]] = [] - mocker.patch( - "virtualenv.discovery.builtin.get_interpreter", - lambda key, try_first_with, **_: tried.append((key, list(try_first_with))), + (config := tmp_path / "virtualenv.ini").write_text( + f"[virtualenv]\n{dest} =\n configured-1\n configured-2\n", encoding="utf-8" + ) + monkeypatch.setenv(variable.format(dest=dest.upper()), value.format(config=config)) + get_interpreter: Final = mocker.patch( + "virtualenv.discovery.builtin.get_interpreter", autospec=True, return_value=None ) - option = f"--{dest.replace('_', '-')}" - args = [arg for value in cli for arg in (option, value)] with pytest.raises(RuntimeError, match="failed to find interpreter"): - session_via_cli([*args, str(tmp_path / "env")]) + session_via_cli([*cli, str(tmp_path / "env")]) - expected = list(cli) or ["configured-1", "configured-2"] - assert tried == ([(key, []) for key in expected] if dest == "python" else [(sys.executable, expected)]) + assert [call.args for call in get_interpreter.call_args_list] == tried def test_discovery_absolute_path_with_try_first(tmp_path, session_app_data) -> None: