Skip to content

Commit

Permalink
update env remove logic (#6195)
Browse files Browse the repository at this point in the history
Resolves: #6018 

1. Added a check so that if `python` argument is a file (then it should
be a python path) - extract it's venv name and raise `IncorrectEnvError`
if it doesn't belong to this project
    **Before**
    ```
└❯ poetry env remove
~/.cache/pypoetry/virtualenvs/different-project-OKfJHH_5-py3.10/bin/python
    /bin/sh: 1: different-project-OKfJHH_5-py3.10: not found

Deleted virtualenv: ~/.cache/pypoetry/virtualenvs/poetry-4pWfmigs-py3.10
    ```
    Removes current project's env, which is wrong.
    **After**
    ```
└❯ poetry env remove
~/.cache/pypoetry/virtualenvs/different-project-OKfJHH_5-py3.10/bin/python

Env different-project-OKfJHH_5-py3.10 doesn't belong to this project.
    ```
2. Added the exact same check as before ^, but for cases where env name
is passed.
    **Before**
    ```
    └❯ poetry env remove different-project-OKfJHH_5-py3.10      
     /bin/sh: 1: different-project-OKfJHH_5-py3.10: not found

Command different-project-OKfJHH_5-py3.10 -c "import sys;
print('.'.join([str(s) for s in sys.version_info[:3]]))" errored with
the following return code 127, and output:
    ```
Errors while trying to exec env name as an interpreter, error is not
clear.
    **After**
    ```
└❯ poetry env remove different-project-OKfJHH_5-py3.10

Env different-project-OKfJHH_5-py3.10 doesn't belong to this project.
    ```
3. Added a couple of tests for **new** and for **old** scenarios which
weren't tested.
4. Added `venv_name` fixture for `tests/utils` directory to use in
`test_env`. Also replaced some of `"simple-project"` hardcoded value to
use `poetry.package.name`

It's up to maintainers to choose what they want for this project - I'm
happy either way if we at least fix the bug. I can remove/change any of
the stuff I added on top of the fix. But yeah I just decided that if we
fix the bug, we might also make some improvements/changes in this area
of code. Any thoughts on this are welcome, thanks!
  • Loading branch information
dhvcc authored Sep 17, 2022
1 parent b61a4dd commit 2def357
Show file tree
Hide file tree
Showing 4 changed files with 229 additions and 29 deletions.
40 changes: 39 additions & 1 deletion src/poetry/utils/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -740,14 +747,39 @@ def list(self, name: str | None = None) -> list[VirtualEnv]:
env_list.insert(0, VirtualEnv(venv))
return env_list

@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(base_env_name)

def remove(self, python: str) -> Env:
venv_path = self._poetry.config.virtualenvs_path

cwd = self._poetry.file.parent
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, base_env_name):
raise IncorrectEnvError(env_name)
except CalledProcessError as e:
raise EnvCommandError(e)

if self.check_env_is_for_current_project(python, base_env_name):
venvs = self.list()
for venv in venvs:
if venv.path.name == python:
Expand Down Expand Up @@ -778,6 +810,12 @@ def remove(self, python: str) -> Env:
raise ValueError(
f'<warning>Environment "{python}" does not exist.</warning>'
)
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)
Expand Down
5 changes: 4 additions & 1 deletion tests/console/commands/env/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 21 additions & 0 deletions tests/utils/conftest.py
Original file line number Diff line number Diff line change
@@ -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),
)
Loading

0 comments on commit 2def357

Please sign in to comment.