From 54870697d921ae6efdbba0bc86f6b54c86d6896a Mon Sep 17 00:00:00 2001 From: Ryan Butler Date: Sat, 21 Mar 2020 18:07:15 -0400 Subject: [PATCH] remove_venv will empty folder contents if it can't delete folder itself --- poetry/utils/env.py | 49 +++++++++---- tests/console/commands/env/test_use.py | 14 ++-- tests/utils/test_env.py | 97 ++++++++++++++++++-------- 3 files changed, 110 insertions(+), 50 deletions(-) diff --git a/poetry/utils/env.py b/poetry/utils/env.py index 0c326880ba1..1bc00f32af0 100644 --- a/poetry/utils/env.py +++ b/poetry/utils/env.py @@ -15,6 +15,7 @@ from typing import List from typing import Optional from typing import Tuple +from typing import Union import tomlkit @@ -403,7 +404,7 @@ def remove(self, python): # type: (str) -> Env if venv.path.name == python: # Exact virtualenv name if not envs_file.exists(): - self.remove_venv(str(venv.path)) + self.remove_venv(venv.path) return venv @@ -413,7 +414,7 @@ def remove(self, python): # type: (str) -> Env current_env = envs.get(base_env_name) if not current_env: - self.remove_venv(str(venv.path)) + self.remove_venv(venv.path) return venv @@ -421,7 +422,7 @@ def remove(self, python): # type: (str) -> Env del envs[base_env_name] envs_file.write(envs) - self.remove_venv(str(venv.path)) + self.remove_venv(venv.path) return venv @@ -475,7 +476,7 @@ def remove(self, python): # type: (str) -> Env del envs[base_env_name] envs_file.write(envs) - self.remove_venv(str(venv)) + self.remove_venv(venv) return VirtualEnv(venv) @@ -621,7 +622,7 @@ def create_venv( "Creating virtualenv {} in {}".format(name, str(venv_path)) ) - self.build_venv(str(venv), executable=executable) + self.build_venv(venv, executable=executable) else: if force: if not env.is_sane(): @@ -633,8 +634,8 @@ def create_venv( io.write_line( "Recreating virtualenv {} in {}".format(name, str(venv)) ) - self.remove_venv(str(venv)) - self.build_venv(str(venv), executable=executable) + self.remove_venv(venv) + self.build_venv(venv, executable=executable) elif io.is_very_verbose(): io.write_line("Virtualenv {} already exists.".format(name)) @@ -657,7 +658,9 @@ def create_venv( return VirtualEnv(venv) @classmethod - def build_venv(cls, path, executable=None): + def build_venv( + cls, path, executable=None + ): # type: (Union[Path,str], Optional[str]) -> () if executable is not None: # Create virtualenv by using an external executable try: @@ -682,21 +685,41 @@ def build_venv(cls, path, executable=None): use_symlinks = True builder = EnvBuilder(with_pip=True, symlinks=use_symlinks) - builder.create(path) + builder.create(str(path)) except ImportError: try: # We fallback on virtualenv for Python 2.7 from virtualenv import create_environment - create_environment(path) + create_environment(str(path)) except ImportError: # since virtualenv>20 we have to use cli_run from virtualenv import cli_run - cli_run([path]) + cli_run([str(path)]) - def remove_venv(self, path): # type: (str) -> None - shutil.rmtree(path) + @classmethod + def remove_venv(cls, path): # type: (Union[Path,str]) -> None + if isinstance(path, str): + path = Path(path) + assert path.is_dir() + try: + shutil.rmtree(str(path)) + return + except OSError as e: + # Continue only if e.errno == 16 + if e.errno != 16: # ERRNO 16: Device or resource busy + raise e + + # Delete all files and folders but the toplevel one. This is because sometimes + # the venv folder is mounted by the OS, such as in a docker volume. In such + # cases, an attempt to delete the folder itself will result in an `OSError`. + # See https://github.com/python-poetry/poetry/pull/2064 + for file_path in path.iterdir(): + if file_path.is_file() or file_path.is_symlink(): + file_path.unlink() + elif file_path.is_dir(): + shutil.rmtree(str(file_path)) def get_base_prefix(self): # type: () -> Path if hasattr(sys, "real_prefix"): diff --git a/tests/console/commands/env/test_use.py b/tests/console/commands/env/test_use.py index 2b1967bf3b1..0029ee00854 100644 --- a/tests/console/commands/env/test_use.py +++ b/tests/console/commands/env/test_use.py @@ -1,7 +1,9 @@ import os -import shutil import sys +from typing import Optional +from typing import Union + import tomlkit from cleo.testers import CommandTester @@ -16,12 +18,8 @@ CWD = Path(__file__).parent.parent / "fixtures" / "simple_project" -def build_venv(path, executable=None): - os.mkdir(path) - - -def remove_venv(path): - shutil.rmtree(path) +def build_venv(path, executable=None): # type: (Union[Path,str], Optional[str]) -> () + os.mkdir(str(path)) def check_output_wrapper(version=Version.parse("3.7.1")): @@ -62,7 +60,7 @@ def test_activate_activates_non_existing_virtualenv_no_envs_file(app, tmp_dir, m ) m.assert_called_with( - os.path.join(tmp_dir, "{}-py3.7".format(venv_name)), executable="python3.7" + Path(tmp_dir) / "{}-py3.7".format(venv_name), executable="python3.7" ) envs_file = TomlFile(Path(tmp_dir) / "envs.toml") diff --git a/tests/utils/test_env.py b/tests/utils/test_env.py index 9ed47c87b56..04bb4f8808e 100644 --- a/tests/utils/test_env.py +++ b/tests/utils/test_env.py @@ -2,6 +2,9 @@ import shutil import sys +from typing import Optional +from typing import Union + import pytest import tomlkit @@ -83,12 +86,8 @@ def test_env_get_in_project_venv(manager, poetry): shutil.rmtree(str(venv.path)) -def build_venv(path, executable=None): - os.mkdir(path) - - -def remove_venv(path): - shutil.rmtree(path) +def build_venv(path, executable=None): # type: (Union[Path,str], Optional[str]) -> () + os.mkdir(str(path)) def check_output_wrapper(version=Version.parse("3.7.1")): @@ -125,7 +124,7 @@ def test_activate_activates_non_existing_virtualenv_no_envs_file( venv_name = EnvManager.generate_env_name("simple-project", str(poetry.file.parent)) m.assert_called_with( - os.path.join(tmp_dir, "{}-py3.7".format(venv_name)), executable="python3.7" + Path(tmp_dir) / "{}-py3.7".format(venv_name), executable="python3.7" ) envs_file = TomlFile(Path(tmp_dir) / "envs.toml") @@ -243,7 +242,7 @@ def test_activate_activates_different_virtualenv_with_envs_file( env = manager.activate("python3.6", NullIO()) m.assert_called_with( - os.path.join(tmp_dir, "{}-py3.6".format(venv_name)), executable="python3.6" + Path(tmp_dir) / "{}-py3.6".format(venv_name), executable="python3.6" ) assert envs_file.exists() @@ -289,17 +288,15 @@ def test_activate_activates_recreates_for_different_patch( "poetry.utils.env.EnvManager.build_venv", side_effect=build_venv ) remove_venv_m = mocker.patch( - "poetry.utils.env.EnvManager.remove_venv", side_effect=remove_venv + "poetry.utils.env.EnvManager.remove_venv", side_effect=EnvManager.remove_venv ) env = manager.activate("python3.7", NullIO()) build_venv_m.assert_called_with( - os.path.join(tmp_dir, "{}-py3.7".format(venv_name)), executable="python3.7" - ) - remove_venv_m.assert_called_with( - os.path.join(tmp_dir, "{}-py3.7".format(venv_name)) + Path(tmp_dir) / "{}-py3.7".format(venv_name), executable="python3.7" ) + remove_venv_m.assert_called_with(Path(tmp_dir) / "{}-py3.7".format(venv_name)) assert envs_file.exists() envs = envs_file.read() @@ -340,7 +337,7 @@ def test_activate_does_not_recreate_when_switching_minor( "poetry.utils.env.EnvManager.build_venv", side_effect=build_venv ) remove_venv_m = mocker.patch( - "poetry.utils.env.EnvManager.remove_venv", side_effect=remove_venv + "poetry.utils.env.EnvManager.remove_venv", side_effect=EnvManager.remove_venv ) env = manager.activate("python3.6", NullIO()) @@ -535,6 +532,54 @@ def test_remove_also_deactivates(tmp_dir, manager, poetry, config, mocker): assert venv_name not in envs +def test_remove_keeps_dir_if_not_deleteable(tmp_dir, manager, poetry, config, mocker): + # 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) / "{}-py3.6".format(venv_name) + venv_path.mkdir() + + folder1_path = venv_path / "folder1" + folder1_path.mkdir() + + file1_path = folder1_path / "file1" + file1_path.touch(exist_ok=False) + + file2_path = venv_path / "file2" + file2_path.touch(exist_ok=False) + + mocker.patch( + "poetry.utils._compat.subprocess.check_output", + side_effect=check_output_wrapper(Version.parse("3.6.6")), + ) + + original_rmtree = shutil.rmtree + + def err_on_rm_venv_only(path, *args, **kwargs): + print(path) + if path == str(venv_path): + raise OSError(16, "Test error") # ERRNO 16: Device or resource busy + else: + original_rmtree(path) + + m = mocker.patch("shutil.rmtree", side_effect=err_on_rm_venv_only) + + venv = manager.remove("{}-py3.6".format(venv_name)) + + m.assert_any_call(str(venv_path)) + + assert venv_path == venv.path + assert venv_path.exists() + + assert not folder1_path.exists() + assert not file1_path.exists() + assert not file2_path.exists() + + m.side_effect = original_rmtree # Avoid teardown using `err_on_rm_venv_only` + + def test_env_has_symlinks_on_nix(tmp_dir, tmp_venv): venv_available = False try: @@ -584,7 +629,7 @@ def test_create_venv_tries_to_find_a_compatible_python_executable_using_generic_ manager.create_venv(NullIO()) m.assert_called_with( - str(Path("/foo/virtualenvs/{}-py3.7".format(venv_name))), executable="python3" + Path("/foo/virtualenvs/{}-py3.7".format(venv_name)), executable="python3" ) @@ -608,7 +653,7 @@ def test_create_venv_tries_to_find_a_compatible_python_executable_using_specific manager.create_venv(NullIO()) m.assert_called_with( - str(Path("/foo/virtualenvs/{}-py3.8".format(venv_name))), executable="python3.8" + Path("/foo/virtualenvs/{}-py3.8".format(venv_name)), executable="python3.8" ) @@ -691,11 +736,9 @@ def test_create_venv_uses_patch_version_to_detect_compatibility( assert not check_output.called m.assert_called_with( - str( - Path( - "/foo/virtualenvs/{}-py{}.{}".format( - venv_name, version.major, version.minor - ) + Path( + "/foo/virtualenvs/{}-py{}.{}".format( + venv_name, version.major, version.minor ) ), executable=None, @@ -730,11 +773,9 @@ def test_create_venv_uses_patch_version_to_detect_compatibility_with_executable( assert check_output.called m.assert_called_with( - str( - Path( - "/foo/virtualenvs/{}-py{}.{}".format( - venv_name, version.major, version.minor - 1 - ) + Path( + "/foo/virtualenvs/{}-py{}.{}".format( + venv_name, version.major, version.minor - 1 ) ), executable="python{}.{}".format(version.major, version.minor - 1), @@ -768,9 +809,7 @@ def test_activate_with_in_project_setting_does_not_fail_if_no_venvs_dir( manager.activate("python3.7", NullIO()) - m.assert_called_with( - os.path.join(str(poetry.file.parent), ".venv"), executable="python3.7" - ) + m.assert_called_with(poetry.file.parent / ".venv", executable="python3.7") envs_file = TomlFile(Path(tmp_dir) / "virtualenvs" / "envs.toml") assert not envs_file.exists()