From 04b0a92167dae14f73f7d7f97ef61ccd592fe949 Mon Sep 17 00:00:00 2001 From: dhvcc <1337kwiz@gmail.com> Date: Thu, 18 Aug 2022 16:39:43 +0300 Subject: [PATCH 1/3] Removed hardcoded project names for env testing Added venv_name fixture to tests/utils --- tests/console/commands/env/conftest.py | 5 ++- tests/utils/conftest.py | 21 ++++++++++ tests/utils/test_env.py | 58 ++++++++++++++------------ 3 files changed, 56 insertions(+), 28 deletions(-) create mode 100644 tests/utils/conftest.py diff --git a/tests/console/commands/env/conftest.py b/tests/console/commands/env/conftest.py index d7caccd7d17..32827c33e7a 100644 --- a/tests/console/commands/env/conftest.py +++ b/tests/console/commands/env/conftest.py @@ -18,7 +18,10 @@ @pytest.fixture def venv_name(app: PoetryTestApplication) -> str: - return EnvManager.generate_env_name("simple-project", str(app.poetry.file.parent)) + return EnvManager.generate_env_name( + app.poetry.package.name, + str(app.poetry.file.parent), + ) @pytest.fixture diff --git a/tests/utils/conftest.py b/tests/utils/conftest.py new file mode 100644 index 00000000000..8ad174758c4 --- /dev/null +++ b/tests/utils/conftest.py @@ -0,0 +1,21 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + + +if TYPE_CHECKING: + from poetry.poetry import Poetry + from poetry.utils.env import EnvManager + + +@pytest.fixture +def venv_name( + manager: EnvManager, + poetry: Poetry, +) -> str: + return manager.generate_env_name( + poetry.package.name, + str(poetry.file.parent), + ) diff --git a/tests/utils/test_env.py b/tests/utils/test_env.py index a3a4474b399..d6442d35af3 100644 --- a/tests/utils/test_env.py +++ b/tests/utils/test_env.py @@ -208,6 +208,7 @@ def test_activate_activates_non_existing_virtualenv_no_envs_file( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] @@ -225,7 +226,6 @@ def test_activate_activates_non_existing_virtualenv_no_envs_file( m = mocker.patch("poetry.utils.env.EnvManager.build_venv", side_effect=build_venv) env = manager.activate("python3.7", NullIO()) - venv_name = EnvManager.generate_env_name("simple-project", str(poetry.file.parent)) m.assert_called_with( Path(tmp_dir) / f"{venv_name}-py3.7", @@ -255,12 +255,11 @@ def test_activate_activates_existing_virtualenv_no_envs_file( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) - os.mkdir(os.path.join(tmp_dir, f"{venv_name}-py3.7")) config.merge({"virtualenvs": {"path": str(tmp_dir)}}) @@ -295,12 +294,11 @@ def test_activate_activates_same_virtualenv_with_envs_file( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) - envs_file = TOMLFile(Path(tmp_dir) / "envs.toml") doc = tomlkit.document() doc[venv_name] = {"minor": "3.7", "patch": "3.7.1"} @@ -339,11 +337,11 @@ def test_activate_activates_different_virtualenv_with_envs_file( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) envs_file = TOMLFile(Path(tmp_dir) / "envs.toml") doc = tomlkit.document() doc[venv_name] = {"minor": "3.7", "patch": "3.7.1"} @@ -392,11 +390,11 @@ def test_activate_activates_recreates_for_different_patch( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) envs_file = TOMLFile(Path(tmp_dir) / "envs.toml") doc = tomlkit.document() doc[venv_name] = {"minor": "3.7", "patch": "3.7.0"} @@ -458,11 +456,11 @@ def test_activate_does_not_recreate_when_switching_minor( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) envs_file = TOMLFile(Path(tmp_dir) / "envs.toml") doc = tomlkit.document() doc[venv_name] = {"minor": "3.7", "patch": "3.7.0"} @@ -509,12 +507,11 @@ def test_deactivate_non_activated_but_existing( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) - python = ".".join(str(c) for c in sys.version_info[:2]) (Path(tmp_dir) / f"{venv_name}-py{python}").mkdir() @@ -538,11 +535,11 @@ def test_deactivate_activated( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) version = Version.from_parts(*sys.version_info[:3]) other_version = Version.parse("3.4") if version.major == 2 else version.next_minor() (Path(tmp_dir) / f"{venv_name}-py{version.major}.{version.minor}").mkdir() @@ -581,11 +578,10 @@ def test_get_prefers_explicitly_activated_virtualenvs_over_env_var( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): os.environ["VIRTUAL_ENV"] = "/environment/prefix" - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) - config.merge({"virtualenvs": {"path": str(tmp_dir)}}) (Path(tmp_dir) / f"{venv_name}-py3.7").mkdir() @@ -609,10 +605,15 @@ def test_get_prefers_explicitly_activated_virtualenvs_over_env_var( assert env.base == Path("/prefix") -def test_list(tmp_dir: str, manager: EnvManager, poetry: Poetry, config: Config): +def test_list( + tmp_dir: str, + manager: EnvManager, + poetry: Poetry, + config: Config, + venv_name: str, +): config.merge({"virtualenvs": {"path": str(tmp_dir)}}) - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) (Path(tmp_dir) / f"{venv_name}-py3.7").mkdir() (Path(tmp_dir) / f"{venv_name}-py3.6").mkdir() @@ -629,10 +630,10 @@ def test_remove_by_python_version( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): config.merge({"virtualenvs": {"path": str(tmp_dir)}}) - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) (Path(tmp_dir) / f"{venv_name}-py3.7").mkdir() (Path(tmp_dir) / f"{venv_name}-py3.6").mkdir() @@ -654,10 +655,10 @@ def test_remove_by_name( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): config.merge({"virtualenvs": {"path": str(tmp_dir)}}) - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) (Path(tmp_dir) / f"{venv_name}-py3.7").mkdir() (Path(tmp_dir) / f"{venv_name}-py3.6").mkdir() @@ -679,10 +680,10 @@ def test_remove_also_deactivates( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): config.merge({"virtualenvs": {"path": str(tmp_dir)}}) - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) (Path(tmp_dir) / f"{venv_name}-py3.7").mkdir() (Path(tmp_dir) / f"{venv_name}-py3.6").mkdir() @@ -712,12 +713,12 @@ def test_remove_keeps_dir_if_not_deleteable( poetry: Poetry, config: Config, mocker: MockerFixture, + venv_name: str, ): # Ensure we empty rather than delete folder if its is an active mount point. # See https://github.com/python-poetry/poetry/pull/2064 config.merge({"virtualenvs": {"path": str(tmp_dir)}}) - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) venv_path = Path(tmp_dir) / f"{venv_name}-py3.6" venv_path.mkdir() @@ -848,12 +849,12 @@ def test_create_venv_tries_to_find_a_compatible_python_executable_using_generic_ config: Config, mocker: MockerFixture, config_virtualenvs_path: Path, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] poetry.package.python_versions = "^3.6" - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) mocker.patch("sys.version_info", (2, 7, 16)) mocker.patch( @@ -885,12 +886,12 @@ def test_create_venv_tries_to_find_a_compatible_python_executable_using_specific config: Config, mocker: MockerFixture, config_virtualenvs_path: Path, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] poetry.package.python_versions = "^3.6" - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) mocker.patch("sys.version_info", (2, 7, 16)) mocker.patch("subprocess.check_output", side_effect=["3.5.3", "3.9.0"]) @@ -971,6 +972,7 @@ def test_create_venv_uses_patch_version_to_detect_compatibility( config: Config, mocker: MockerFixture, config_virtualenvs_path: Path, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] @@ -979,7 +981,6 @@ def test_create_venv_uses_patch_version_to_detect_compatibility( poetry.package.python_versions = "^" + ".".join( str(c) for c in sys.version_info[:3] ) - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) mocker.patch("sys.version_info", (version.major, version.minor, version.patch + 1)) check_output = mocker.patch( @@ -1012,13 +1013,13 @@ def test_create_venv_uses_patch_version_to_detect_compatibility_with_executable( config: Config, mocker: MockerFixture, config_virtualenvs_path: Path, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] version = Version.from_parts(*sys.version_info[:3]) poetry.package.python_versions = f"~{version.major}.{version.minor-1}.0" - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) check_output = mocker.patch( "subprocess.check_output", @@ -1320,12 +1321,12 @@ def test_create_venv_accepts_fallback_version_w_nonzero_patchlevel( config: Config, mocker: MockerFixture, config_virtualenvs_path: Path, + venv_name: str, ): if "VIRTUAL_ENV" in os.environ: del os.environ["VIRTUAL_ENV"] poetry.package.python_versions = "~3.5.1" - venv_name = manager.generate_env_name("simple-project", str(poetry.file.parent)) check_output = mocker.patch( "subprocess.check_output", @@ -1353,9 +1354,12 @@ def test_create_venv_accepts_fallback_version_w_nonzero_patchlevel( ) -def test_generate_env_name_ignores_case_for_case_insensitive_fs(tmp_dir: str): - venv_name1 = EnvManager.generate_env_name("simple-project", "MyDiR") - venv_name2 = EnvManager.generate_env_name("simple-project", "mYdIr") +def test_generate_env_name_ignores_case_for_case_insensitive_fs( + poetry: Poetry, + tmp_dir: str, +): + venv_name1 = EnvManager.generate_env_name(poetry.package.name, "MyDiR") + venv_name2 = EnvManager.generate_env_name(poetry.package.name, "mYdIr") if sys.platform == "win32": assert venv_name1 == venv_name2 else: From 4d91dad5af0e62626d4b8702cf55aa2717038113 Mon Sep 17 00:00:00 2001 From: dhvcc <1337kwiz@gmail.com> Date: Thu, 18 Aug 2022 20:35:44 +0300 Subject: [PATCH 2/3] Update full path handling for env remove Add IncorrectEnvError exception Add check_env_is_for_current_project method Add a couple of tests for new and old env remove logic --- src/poetry/utils/env.py | 33 ++++++++++++++++- tests/utils/test_env.py | 80 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/poetry/utils/env.py b/src/poetry/utils/env.py index d9270cf276d..d58157474fb 100644 --- a/src/poetry/utils/env.py +++ b/src/poetry/utils/env.py @@ -175,6 +175,7 @@ def _version_nodot(version): GET_PYTHON_VERSION_ONELINER = ( "\"import sys; print('.'.join([str(s) for s in sys.version_info[:3]]))\"" ) +GET_ENV_PATH_ONELINER = '"import sys; print(sys.prefix)"' GET_SYS_PATH = """\ import json @@ -461,6 +462,12 @@ class EnvError(Exception): pass +class IncorrectEnvError(EnvError): + def __init__(self, env_name: str) -> None: + message = f"Env {env_name} doesn't belong to this project." + super().__init__(message) + + class EnvCommandError(EnvError): def __init__(self, e: CalledProcessError, input: str | None = None) -> None: self.e = e @@ -737,6 +744,14 @@ def list(self, name: str | None = None) -> list[VirtualEnv]: env_list.insert(0, VirtualEnv(venv)) return env_list + def check_env_is_for_current_project(self, env: str) -> bool: + """ + Check if env name starts with projects name. + + This is done to prevent action on other project's envs. + """ + return env.startswith(self._poetry.package.name) + def remove(self, python: str) -> Env: venv_path = self._poetry.config.virtualenvs_path @@ -744,7 +759,23 @@ def remove(self, python: str) -> Env: envs_file = TOMLFile(venv_path / self.ENVS_FILE) base_env_name = self.generate_env_name(self._poetry.package.name, str(cwd)) - if python.startswith(base_env_name): + python_path = Path(python) + if python_path.is_file(): + # Validate env name if provided env is a full path to python + try: + env_dir = decode( + subprocess.check_output( + list_to_shell_command([python, "-c", GET_ENV_PATH_ONELINER]), + shell=True, + ) + ).strip("\n") + env_name = Path(env_dir).name + if not self.check_env_is_for_current_project(env_name): + raise IncorrectEnvError(env_name) + except CalledProcessError as e: + raise EnvCommandError(e) + + if self.check_env_is_for_current_project(python): venvs = self.list() for venv in venvs: if venv.path.name == python: diff --git a/tests/utils/test_env.py b/tests/utils/test_env.py index d6442d35af3..73c017c04be 100644 --- a/tests/utils/test_env.py +++ b/tests/utils/test_env.py @@ -22,6 +22,7 @@ from poetry.utils.env import EnvCommandError from poetry.utils.env import EnvManager from poetry.utils.env import GenericEnv +from poetry.utils.env import IncorrectEnvError from poetry.utils.env import InvalidCurrentPythonVersionError from poetry.utils.env import MockEnv from poetry.utils.env import NoCompatiblePythonVersionFound @@ -674,6 +675,85 @@ def test_remove_by_name( assert not expected_venv_path.exists() +def test_remove_by_string_with_python_and_version( + tmp_dir: str, + manager: EnvManager, + poetry: Poetry, + config: Config, + mocker: MockerFixture, + venv_name: str, +): + config.merge({"virtualenvs": {"path": str(tmp_dir)}}) + + (Path(tmp_dir) / f"{venv_name}-py3.7").mkdir() + (Path(tmp_dir) / f"{venv_name}-py3.6").mkdir() + + mocker.patch( + "subprocess.check_output", + side_effect=check_output_wrapper(Version.parse("3.6.6")), + ) + + venv = manager.remove("python3.6") + + expected_venv_path = Path(tmp_dir) / f"{venv_name}-py3.6" + assert venv.path == expected_venv_path + assert not expected_venv_path.exists() + + +def test_remove_by_full_path_to_python( + tmp_dir: str, + manager: EnvManager, + poetry: Poetry, + config: Config, + mocker: MockerFixture, + venv_name: str, +): + config.merge({"virtualenvs": {"path": str(tmp_dir)}}) + + (Path(tmp_dir) / f"{venv_name}-py3.7").mkdir() + (Path(tmp_dir) / f"{venv_name}-py3.6").mkdir() + + mocker.patch( + "subprocess.check_output", + side_effect=check_output_wrapper(Version.parse("3.6.6")), + ) + + expected_venv_path = Path(tmp_dir) / f"{venv_name}-py3.6" + python_path = expected_venv_path / "bin" / "python" + + venv = manager.remove(str(python_path)) + + assert venv.path == expected_venv_path + assert not expected_venv_path.exists() + + +def test_doesnt_act_on_different_projects_env_when_passing_full_path( + tmp_dir: str, + manager: EnvManager, + poetry: Poetry, + config: Config, + mocker: MockerFixture, +): + config.merge({"virtualenvs": {"path": str(tmp_dir)}}) + + different_venv_name = "different-project" + different_venv_path = Path(tmp_dir) / f"{different_venv_name}-py3.6" + different_venv_bin_path = different_venv_path / "bin" + different_venv_bin_path.mkdir(parents=True) + + python_path = different_venv_bin_path / "python" + python_path.touch(exist_ok=True) + + # Patch initial call where python env path is extracted + mocker.patch( + "subprocess.check_output", + side_effect=lambda *args, **kwargs: str(different_venv_path), + ) + + with pytest.raises(IncorrectEnvError): + manager.remove(str(python_path)) + + def test_remove_also_deactivates( tmp_dir: str, manager: EnvManager, From 2700195cec1fb18684ef74ec847e40bf610ba6b0 Mon Sep 17 00:00:00 2001 From: dhvcc <1337kwiz@gmail.com> Date: Thu, 18 Aug 2022 22:01:32 +0300 Subject: [PATCH 3/3] Update env remove logic Update remove handling, so that if other project's venv name is passed, raise IncorrectEnvError for better error message --- src/poetry/utils/env.py | 15 ++++++++--- tests/utils/test_env.py | 56 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/src/poetry/utils/env.py b/src/poetry/utils/env.py index d58157474fb..2d201e4dc96 100644 --- a/src/poetry/utils/env.py +++ b/src/poetry/utils/env.py @@ -744,13 +744,14 @@ def list(self, name: str | None = None) -> list[VirtualEnv]: env_list.insert(0, VirtualEnv(venv)) return env_list - def check_env_is_for_current_project(self, env: str) -> bool: + @staticmethod + def check_env_is_for_current_project(env: str, base_env_name: str) -> bool: """ Check if env name starts with projects name. This is done to prevent action on other project's envs. """ - return env.startswith(self._poetry.package.name) + return env.startswith(base_env_name) def remove(self, python: str) -> Env: venv_path = self._poetry.config.virtualenvs_path @@ -770,12 +771,12 @@ def remove(self, python: str) -> Env: ) ).strip("\n") env_name = Path(env_dir).name - if not self.check_env_is_for_current_project(env_name): + if not self.check_env_is_for_current_project(env_name, base_env_name): raise IncorrectEnvError(env_name) except CalledProcessError as e: raise EnvCommandError(e) - if self.check_env_is_for_current_project(python): + if self.check_env_is_for_current_project(python, base_env_name): venvs = self.list() for venv in venvs: if venv.path.name == python: @@ -806,6 +807,12 @@ def remove(self, python: str) -> Env: raise ValueError( f'Environment "{python}" does not exist.' ) + else: + venv_path = self._poetry.config.virtualenvs_path + # Get all the poetry envs, even for other projects + env_names = [Path(p).name for p in sorted(venv_path.glob("*-*-py*"))] + if python in env_names: + raise IncorrectEnvError(python) try: python_version = Version.parse(python) diff --git a/tests/utils/test_env.py b/tests/utils/test_env.py index 73c017c04be..7e062368933 100644 --- a/tests/utils/test_env.py +++ b/tests/utils/test_env.py @@ -727,7 +727,7 @@ def test_remove_by_full_path_to_python( assert not expected_venv_path.exists() -def test_doesnt_act_on_different_projects_env_when_passing_full_path( +def test_raises_if_acting_on_different_project_by_full_path( tmp_dir: str, manager: EnvManager, poetry: Poetry, @@ -754,6 +754,60 @@ def test_doesnt_act_on_different_projects_env_when_passing_full_path( manager.remove(str(python_path)) +def test_raises_if_acting_on_different_project_by_name( + tmp_dir: str, + manager: EnvManager, + poetry: Poetry, + config: Config, +): + config.merge({"virtualenvs": {"path": str(tmp_dir)}}) + + different_venv_name = ( + EnvManager.generate_env_name( + "different-project", + str(poetry.file.parent), + ) + + "-py3.6" + ) + different_venv_path = Path(tmp_dir) / different_venv_name + different_venv_bin_path = different_venv_path / "bin" + different_venv_bin_path.mkdir(parents=True) + + python_path = different_venv_bin_path / "python" + python_path.touch(exist_ok=True) + + with pytest.raises(IncorrectEnvError): + manager.remove(different_venv_name) + + +def test_raises_when_passing_old_env_after_dir_rename( + tmp_dir: str, + manager: EnvManager, + poetry: Poetry, + config: Config, + venv_name: str, +): + # Make sure that poetry raises when trying to remove old venv after you've renamed + # root directory of the project, which will create another venv with new name. + # This is not ideal as you still "can't" remove it by name, but it at least doesn't + # cause any unwanted side effects + config.merge({"virtualenvs": {"path": str(tmp_dir)}}) + + previous_venv_name = EnvManager.generate_env_name( + poetry.package.name, + "previous_dir_name", + ) + venv_path = Path(tmp_dir) / f"{venv_name}-py3.6" + venv_path.mkdir() + + previous_venv_name = f"{previous_venv_name}-py3.6" + previous_venv_path = Path(tmp_dir) / previous_venv_name + previous_venv_path.mkdir() + + with pytest.raises(IncorrectEnvError): + manager.remove(previous_venv_name) + + def test_remove_also_deactivates( tmp_dir: str, manager: EnvManager,