From 2cf589f1539ea135c7310be7088082d1cf77a7cb Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Wed, 14 Apr 2021 19:11:42 -0700 Subject: [PATCH 01/30] Fix uninstall with metadata. --- src/pipx/commands/uninstall.py | 48 ++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index 00db1c6d96..65eb9e070b 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -4,6 +4,7 @@ from typing import List from pipx import constants +from pipx.commands.common import _get_exposed_app_paths_for_package, add_suffix from pipx.constants import ( EXIT_CODE_OK, EXIT_CODE_UNINSTALL_ERROR, @@ -35,17 +36,22 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode: venv = Venv(venv_dir, verbose=verbose) - suffix = "" - + bin_dir_app_paths: List[Path] = [] if venv.pipx_metadata.main_package.package is not None: - app_paths: List[Path] = [] for viewed_package in venv.package_metadata.values(): - app_paths += viewed_package.app_paths - for dep_paths in viewed_package.app_paths_of_dependencies.values(): - app_paths += dep_paths - suffix = venv.pipx_metadata.main_package.suffix + suffix = viewed_package.suffix + apps = viewed_package.apps + if viewed_package.include_dependencies: + apps += viewed_package.apps_of_dependencies + bin_dir_app_paths += _get_exposed_app_paths_for_package( + venv.bin_path, [add_suffix(app, suffix) for app in apps], local_bin_dir + ) else: # fallback if not metadata from pipx_metadata.json + + # TODO: determine suffix for Windows + suffix = "" + if venv.python_path.is_file(): # has a valid python interpreter and can get metadata about the package # In pre-metadata-pipx venv_dir.name is name of main package @@ -54,6 +60,7 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode: for dep_paths in metadata.app_paths_of_dependencies.values(): app_paths += dep_paths else: + # Doesn't have a valid python interpreter. We'll take our best guess on what to uninstall # here based on symlink location. pipx doesn't use symlinks on windows, so this is for # non-windows only. @@ -69,18 +76,21 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode: ] app_paths = apps_linking_to_venv_bin_dir - for filepath in local_bin_dir.iterdir(): - if WINDOWS: - for b in app_paths: - bin_name = b.stem + suffix + b.suffix - if filepath.exists() and filepath.name == bin_name: - filepath.unlink() - else: - symlink = filepath - for b in app_paths: - if symlink.exists() and b.exists() and symlink.samefile(b): - logger.info(f"removing symlink {str(symlink)}") - symlink.unlink() + for filepath in local_bin_dir.iterdir(): + if WINDOWS: + for b in app_paths: + bin_name = b.stem + suffix + b.suffix + if filepath.exists() and filepath.name == bin_name: + bin_dir_app_paths.append(filepath) + else: + symlink = filepath + for b in app_paths: + if symlink.exists() and b.exists() and symlink.samefile(b): + logger.info(f"removing symlink {str(symlink)}") + bin_dir_app_paths.append(symlink) + + for bin_dir_app_path in bin_dir_app_paths: + bin_dir_app_path.unlink() rmdir(venv_dir) print(f"uninstalled {venv.name}! {stars}") From 47d953d18bf02af230c9e25124890f367aa5082b Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 15:29:58 -0700 Subject: [PATCH 02/30] Refactor for clarity. --- src/pipx/commands/uninstall.py | 155 +++++++++++++++++++++------------ 1 file changed, 101 insertions(+), 54 deletions(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index 65eb9e070b..c7615d5e45 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -4,21 +4,119 @@ from typing import List from pipx import constants -from pipx.commands.common import _get_exposed_app_paths_for_package, add_suffix +from pipx.commands.common import ( + _can_symlink, + _get_exposed_app_paths_for_package, + add_suffix, +) from pipx.constants import ( EXIT_CODE_OK, EXIT_CODE_UNINSTALL_ERROR, EXIT_CODE_UNINSTALL_VENV_NONEXISTENT, - WINDOWS, ExitCode, ) from pipx.emojis import hazard, sleep, stars +from pipx.pipx_metadata_file import PackageInfo from pipx.util import rmdir from pipx.venv import Venv, VenvContainer +from pipx.venv_inspect import VenvMetadata logger = logging.getLogger(__name__) +def venv_metadata_to_package_info( + venv_metadata: VenvMetadata, + package_name: str, + package_or_url: str, + pip_args: List[str], + include_apps: bool, + include_dependencies: bool, + suffix: str, +) -> PackageInfo: + return PackageInfo( + package=package_name, + package_or_url=package_or_url, + pip_args=pip_args, + include_apps=include_apps, + include_dependencies=include_dependencies, + apps=venv_metadata.apps, + app_paths=venv_metadata.app_paths, + apps_of_dependencies=venv_metadata.apps_of_dependencies, + app_paths_of_dependencies=venv_metadata.app_paths_of_dependencies, + package_version=venv_metadata.package_version, + suffix=suffix, + ) + + +def _get_package_bin_dir_app_paths( + venv: Venv, package_info: PackageInfo, local_bin_dir: Path +) -> List[Path]: + bin_dir_package_app_paths: List[Path] = [] + suffix = package_info.suffix + apps = package_info.apps + if package_info.include_dependencies: + apps += package_info.apps_of_dependencies + bin_dir_package_app_paths += _get_exposed_app_paths_for_package( + venv.bin_path, [add_suffix(app, suffix) for app in apps], local_bin_dir + ) + return bin_dir_package_app_paths + + +def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: + bin_dir_app_paths: List[Path] = [] + if venv.pipx_metadata.main_package.package is not None: + for viewed_package in venv.package_metadata.values(): + bin_dir_app_paths += _get_package_bin_dir_app_paths( + venv, viewed_package, local_bin_dir + ) + elif venv.python_path.is_file(): + # fallback if not metadata from pipx_metadata.json + # has a valid python interpreter and can get metadata about the package + # In pre-metadata-pipx venv.root.name is name of main package + # In pre-metadata-pipx there is no suffix + # We make the conservative assumptions: no injected packages, + # not include_dependencies. Other PackageInfo fields are irrelevant + # here. + venv_metadata = venv.get_venv_metadata_for_package(venv.root.name, set()) + main_package_info = venv_metadata_to_package_info( + venv_metadata, + package_name=venv.root.name, + package_or_url="", + pip_args=[], + include_apps=True, + include_dependencies=False, + suffix="", + ) + bin_dir_app_paths += _get_package_bin_dir_app_paths( + venv, main_package_info, local_bin_dir + ) + else: + # No metadata and no valid python interpreter. + # We'll take our best guess on what to uninstall here based on symlink + # location for symlink-capable systems. For non-symlink systems we + # give up and return and empty list. + # The heuristic here is any symlink in ~/.local/bin pointing to + # .local/pipx/venvs/VENV_NAME/bin should be uninstalled. + if not _can_symlink(local_bin_dir): + return [] + + apps_linking_to_venv_bin_dir = [ + f + for f in constants.LOCAL_BIN_DIR.iterdir() + if str(f.resolve()).startswith(str(venv.bin_path)) + ] + app_paths = apps_linking_to_venv_bin_dir + + for filepath in local_bin_dir.iterdir(): + symlink = filepath + for b in app_paths: + if symlink.exists() and b.exists() and symlink.samefile(b): + logger.info(f"removing symlink {str(symlink)}") + bin_dir_app_paths.append(symlink) + + return bin_dir_app_paths + + def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode: """Uninstall entire venv_dir, including main package and all injected packages. @@ -36,58 +134,7 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode: venv = Venv(venv_dir, verbose=verbose) - bin_dir_app_paths: List[Path] = [] - if venv.pipx_metadata.main_package.package is not None: - for viewed_package in venv.package_metadata.values(): - suffix = viewed_package.suffix - apps = viewed_package.apps - if viewed_package.include_dependencies: - apps += viewed_package.apps_of_dependencies - bin_dir_app_paths += _get_exposed_app_paths_for_package( - venv.bin_path, [add_suffix(app, suffix) for app in apps], local_bin_dir - ) - else: - # fallback if not metadata from pipx_metadata.json - - # TODO: determine suffix for Windows - suffix = "" - - if venv.python_path.is_file(): - # has a valid python interpreter and can get metadata about the package - # In pre-metadata-pipx venv_dir.name is name of main package - metadata = venv.get_venv_metadata_for_package(venv_dir.name, set()) - app_paths = metadata.app_paths - for dep_paths in metadata.app_paths_of_dependencies.values(): - app_paths += dep_paths - else: - - # Doesn't have a valid python interpreter. We'll take our best guess on what to uninstall - # here based on symlink location. pipx doesn't use symlinks on windows, so this is for - # non-windows only. - # The heuristic here is any symlink in ~/.local/bin pointing to .local/pipx/venvs/VENV_NAME/bin - # should be uninstalled. - if WINDOWS: - app_paths = [] - else: - apps_linking_to_venv_bin_dir = [ - f - for f in constants.LOCAL_BIN_DIR.iterdir() - if str(f.resolve()).startswith(str(venv.bin_path)) - ] - app_paths = apps_linking_to_venv_bin_dir - - for filepath in local_bin_dir.iterdir(): - if WINDOWS: - for b in app_paths: - bin_name = b.stem + suffix + b.suffix - if filepath.exists() and filepath.name == bin_name: - bin_dir_app_paths.append(filepath) - else: - symlink = filepath - for b in app_paths: - if symlink.exists() and b.exists() and symlink.samefile(b): - logger.info(f"removing symlink {str(symlink)}") - bin_dir_app_paths.append(symlink) + bin_dir_app_paths = _get_venv_bin_dir_app_paths(venv, local_bin_dir) for bin_dir_app_path in bin_dir_app_paths: bin_dir_app_path.unlink() From 1882de25c689d9ba396cc5eb81bb735dc9f7b2ef Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 15:31:50 -0700 Subject: [PATCH 03/30] Clarify comment. --- src/pipx/commands/uninstall.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index c7615d5e45..e962bce1f1 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -70,8 +70,7 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: venv, viewed_package, local_bin_dir ) elif venv.python_path.is_file(): - # fallback if not metadata from pipx_metadata.json - # has a valid python interpreter and can get metadata about the package + # No metadata from pipx_metadata.json, but valid python interpreter. # In pre-metadata-pipx venv.root.name is name of main package # In pre-metadata-pipx there is no suffix # We make the conservative assumptions: no injected packages, From 0d1f8bccd62c62a6800f616ed1ae4081705ad3a2 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 15:32:37 -0700 Subject: [PATCH 04/30] Clarify comment. --- src/pipx/commands/uninstall.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index e962bce1f1..788439d558 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -65,6 +65,7 @@ def _get_package_bin_dir_app_paths( def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: bin_dir_app_paths: List[Path] = [] if venv.pipx_metadata.main_package.package is not None: + # Valid metadata for venv for viewed_package in venv.package_metadata.values(): bin_dir_app_paths += _get_package_bin_dir_app_paths( venv, viewed_package, local_bin_dir From 35f6d6f2f46ce424dd6698598694479df82b96c5 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 15:47:15 -0700 Subject: [PATCH 05/30] Make functions not private-named. --- src/pipx/commands/common.py | 12 ++++++------ src/pipx/commands/uninstall.py | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/pipx/commands/common.py b/src/pipx/commands/common.py index c53cb4c84b..c8078b426c 100644 --- a/src/pipx/commands/common.py +++ b/src/pipx/commands/common.py @@ -52,7 +52,7 @@ def or_(self, venv_problems: "VenvProblems") -> None: def expose_apps_globally( local_bin_dir: Path, app_paths: List[Path], *, force: bool, suffix: str = "" ) -> None: - if not _can_symlink(local_bin_dir): + if not can_symlink(local_bin_dir): _copy_package_apps(local_bin_dir, app_paths, suffix=suffix) else: _symlink_package_apps(local_bin_dir, app_paths, force=force, suffix=suffix) @@ -61,7 +61,7 @@ def expose_apps_globally( _can_symlink_cache: Dict[Path, bool] = {} -def _can_symlink(local_bin_dir: Path) -> bool: +def can_symlink(local_bin_dir: Path) -> bool: if not WINDOWS: # Technically, even on Unix this depends on the filesystem @@ -210,7 +210,7 @@ def get_venv_summary( if package_metadata.include_dependencies: apps += package_metadata.apps_of_dependencies - exposed_app_paths = _get_exposed_app_paths_for_package( + exposed_app_paths = get_exposed_app_paths_for_package( venv.bin_path, [add_suffix(app, package_metadata.suffix) for app in apps], constants.LOCAL_BIN_DIR, @@ -242,7 +242,7 @@ def get_venv_summary( ) -def _get_exposed_app_paths_for_package( +def get_exposed_app_paths_for_package( venv_bin_path: Path, package_binary_names: List[str], local_bin_dir: Path ) -> Set[Path]: bin_symlinks = set() @@ -254,9 +254,9 @@ def _get_exposed_app_paths_for_package( # We always use the stricter check on non-Windows systems. On # Windows, we use a less strict check if we don't have a symlink. is_same_file = False - if _can_symlink(local_bin_dir) and b.is_symlink(): + if can_symlink(local_bin_dir) and b.is_symlink(): is_same_file = b.resolve().parent.samefile(venv_bin_path) - elif not _can_symlink(local_bin_dir): + elif not can_symlink(local_bin_dir): is_same_file = b.name in package_binary_names if is_same_file: diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index 788439d558..3bc01db373 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -5,9 +5,9 @@ from pipx import constants from pipx.commands.common import ( - _can_symlink, - _get_exposed_app_paths_for_package, add_suffix, + can_symlink, + get_exposed_app_paths_for_package, ) from pipx.constants import ( EXIT_CODE_OK, @@ -56,7 +56,7 @@ def _get_package_bin_dir_app_paths( apps = package_info.apps if package_info.include_dependencies: apps += package_info.apps_of_dependencies - bin_dir_package_app_paths += _get_exposed_app_paths_for_package( + bin_dir_package_app_paths += get_exposed_app_paths_for_package( venv.bin_path, [add_suffix(app, suffix) for app in apps], local_bin_dir ) return bin_dir_package_app_paths @@ -97,7 +97,7 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: # give up and return and empty list. # The heuristic here is any symlink in ~/.local/bin pointing to # .local/pipx/venvs/VENV_NAME/bin should be uninstalled. - if not _can_symlink(local_bin_dir): + if not can_symlink(local_bin_dir): return [] apps_linking_to_venv_bin_dir = [ From 734942aa3cbaac578a4afb09e9e44c63c1a1ab4b Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 16:56:36 -0700 Subject: [PATCH 06/30] Give default arguments to _venv_metadata_to_package_info. --- src/pipx/commands/uninstall.py | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index 3bc01db373..d1e0e350bb 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -1,7 +1,7 @@ import logging from pathlib import Path from shutil import which -from typing import List +from typing import List, Optional from pipx import constants from pipx.commands.common import ( @@ -24,15 +24,18 @@ logger = logging.getLogger(__name__) -def venv_metadata_to_package_info( +def _venv_metadata_to_package_info( venv_metadata: VenvMetadata, package_name: str, - package_or_url: str, - pip_args: List[str], - include_apps: bool, - include_dependencies: bool, - suffix: str, + package_or_url: str = "", + pip_args: Optional[List[str]] = None, + include_apps: bool = True, + include_dependencies: bool = False, + suffix: str = "", ) -> PackageInfo: + if pip_args is None: + pip_args = [] + return PackageInfo( package=package_name, package_or_url=package_or_url, @@ -78,14 +81,8 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: # not include_dependencies. Other PackageInfo fields are irrelevant # here. venv_metadata = venv.get_venv_metadata_for_package(venv.root.name, set()) - main_package_info = venv_metadata_to_package_info( - venv_metadata, - package_name=venv.root.name, - package_or_url="", - pip_args=[], - include_apps=True, - include_dependencies=False, - suffix="", + main_package_info = _venv_metadata_to_package_info( + venv_metadata, venv.root.name ) bin_dir_app_paths += _get_package_bin_dir_app_paths( venv, main_package_info, local_bin_dir From be5d69048202e82571cea53557f9b1529aa4c6d3 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 20:28:00 -0700 Subject: [PATCH 07/30] Fix log messages. --- src/pipx/commands/uninstall.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index d1e0e350bb..0056052fad 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -108,7 +108,6 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: symlink = filepath for b in app_paths: if symlink.exists() and b.exists() and symlink.samefile(b): - logger.info(f"removing symlink {str(symlink)}") bin_dir_app_paths.append(symlink) return bin_dir_app_paths @@ -134,6 +133,7 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode: bin_dir_app_paths = _get_venv_bin_dir_app_paths(venv, local_bin_dir) for bin_dir_app_path in bin_dir_app_paths: + logger.info(f"removing file {bin_dir_app_path}") bin_dir_app_path.unlink() rmdir(venv_dir) From 3a91eb41c9c8686d1f783d3a0d8a0ee541f82268 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 21:25:34 -0700 Subject: [PATCH 08/30] More refactoring, simplifying. --- src/pipx/commands/common.py | 11 +++++++++-- src/pipx/commands/uninstall.py | 25 ++++++++----------------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/pipx/commands/common.py b/src/pipx/commands/common.py index c8078b426c..a712dbb81b 100644 --- a/src/pipx/commands/common.py +++ b/src/pipx/commands/common.py @@ -212,8 +212,8 @@ def get_venv_summary( exposed_app_paths = get_exposed_app_paths_for_package( venv.bin_path, - [add_suffix(app, package_metadata.suffix) for app in apps], constants.LOCAL_BIN_DIR, + [add_suffix(app, package_metadata.suffix) for app in apps], ) exposed_binary_names = sorted(p.name for p in exposed_app_paths) unavailable_binary_names = sorted( @@ -243,8 +243,15 @@ def get_venv_summary( def get_exposed_app_paths_for_package( - venv_bin_path: Path, package_binary_names: List[str], local_bin_dir: Path + venv_bin_path: Path, + local_bin_dir: Path, + package_binary_names: Optional[List[str]] = None, ) -> Set[Path]: + # package_binary_names is used only if local_bin_path cannot use symlinks + # to look for files with the same name + if package_binary_names is None: + package_binary_names = [] + bin_symlinks = set() for b in local_bin_dir.iterdir(): try: diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index 0056052fad..b4bf8753d0 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -3,7 +3,6 @@ from shutil import which from typing import List, Optional -from pipx import constants from pipx.commands.common import ( add_suffix, can_symlink, @@ -60,7 +59,7 @@ def _get_package_bin_dir_app_paths( if package_info.include_dependencies: apps += package_info.apps_of_dependencies bin_dir_package_app_paths += get_exposed_app_paths_for_package( - venv.bin_path, [add_suffix(app, suffix) for app in apps], local_bin_dir + venv.bin_path, local_bin_dir, [add_suffix(app, suffix) for app in apps] ) return bin_dir_package_app_paths @@ -90,25 +89,17 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: else: # No metadata and no valid python interpreter. # We'll take our best guess on what to uninstall here based on symlink - # location for symlink-capable systems. For non-symlink systems we - # give up and return and empty list. + # location for symlink-capable systems. # The heuristic here is any symlink in ~/.local/bin pointing to - # .local/pipx/venvs/VENV_NAME/bin should be uninstalled. + # .local/pipx/venvs/VENV_NAME/{bin,Scripts} should be uninstalled. + + # For non-symlink systems we give up and return and empty list. if not can_symlink(local_bin_dir): return [] - apps_linking_to_venv_bin_dir = [ - f - for f in constants.LOCAL_BIN_DIR.iterdir() - if str(f.resolve()).startswith(str(venv.bin_path)) - ] - app_paths = apps_linking_to_venv_bin_dir - - for filepath in local_bin_dir.iterdir(): - symlink = filepath - for b in app_paths: - if symlink.exists() and b.exists() and symlink.samefile(b): - bin_dir_app_paths.append(symlink) + bin_dir_app_paths = list( + get_exposed_app_paths_for_package(venv.bin_path, local_bin_dir) + ) return bin_dir_app_paths From 45809bf9fa2231c16b8b3991329ca26220641d6d Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 21:37:15 -0700 Subject: [PATCH 09/30] Use sets for bin_dir_app_paths for consistency and safety. --- src/pipx/commands/uninstall.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index b4bf8753d0..cef1cad7a8 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -1,7 +1,7 @@ import logging from pathlib import Path from shutil import which -from typing import List, Optional +from typing import List, Optional, Set from pipx.commands.common import ( add_suffix, @@ -52,24 +52,24 @@ def _venv_metadata_to_package_info( def _get_package_bin_dir_app_paths( venv: Venv, package_info: PackageInfo, local_bin_dir: Path -) -> List[Path]: - bin_dir_package_app_paths: List[Path] = [] +) -> Set[Path]: + bin_dir_package_app_paths = set() suffix = package_info.suffix apps = package_info.apps if package_info.include_dependencies: apps += package_info.apps_of_dependencies - bin_dir_package_app_paths += get_exposed_app_paths_for_package( + bin_dir_package_app_paths |= get_exposed_app_paths_for_package( venv.bin_path, local_bin_dir, [add_suffix(app, suffix) for app in apps] ) return bin_dir_package_app_paths -def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: - bin_dir_app_paths: List[Path] = [] +def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> Set[Path]: + bin_dir_app_paths = set() if venv.pipx_metadata.main_package.package is not None: # Valid metadata for venv for viewed_package in venv.package_metadata.values(): - bin_dir_app_paths += _get_package_bin_dir_app_paths( + bin_dir_app_paths |= _get_package_bin_dir_app_paths( venv, viewed_package, local_bin_dir ) elif venv.python_path.is_file(): @@ -83,7 +83,7 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: main_package_info = _venv_metadata_to_package_info( venv_metadata, venv.root.name ) - bin_dir_app_paths += _get_package_bin_dir_app_paths( + bin_dir_app_paths = _get_package_bin_dir_app_paths( venv, main_package_info, local_bin_dir ) else: @@ -93,12 +93,12 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> List[Path]: # The heuristic here is any symlink in ~/.local/bin pointing to # .local/pipx/venvs/VENV_NAME/{bin,Scripts} should be uninstalled. - # For non-symlink systems we give up and return and empty list. + # For non-symlink systems we give up and return and empty set. if not can_symlink(local_bin_dir): - return [] + return set() - bin_dir_app_paths = list( - get_exposed_app_paths_for_package(venv.bin_path, local_bin_dir) + bin_dir_app_paths = get_exposed_app_paths_for_package( + venv.bin_path, local_bin_dir ) return bin_dir_app_paths From b2fd3d17cb89013e2e29e57f3d4cb726c0214fe7 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 21:42:19 -0700 Subject: [PATCH 10/30] Clarify comment. --- src/pipx/commands/common.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pipx/commands/common.py b/src/pipx/commands/common.py index a712dbb81b..f825584c25 100644 --- a/src/pipx/commands/common.py +++ b/src/pipx/commands/common.py @@ -247,8 +247,8 @@ def get_exposed_app_paths_for_package( local_bin_dir: Path, package_binary_names: Optional[List[str]] = None, ) -> Set[Path]: - # package_binary_names is used only if local_bin_path cannot use symlinks - # to look for files with the same name + # package_binary_names is used only if local_bin_path cannot use symlinks. + # It is necessary for those systems to return app_paths. if package_binary_names is None: package_binary_names = [] From 9c38a53116d9943da63e4795f560f7e174709afe Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Thu, 15 Apr 2021 22:01:48 -0700 Subject: [PATCH 11/30] Change variable name. --- src/pipx/commands/uninstall.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index cef1cad7a8..0c18e6b45a 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -68,9 +68,9 @@ def _get_venv_bin_dir_app_paths(venv: Venv, local_bin_dir: Path) -> Set[Path]: bin_dir_app_paths = set() if venv.pipx_metadata.main_package.package is not None: # Valid metadata for venv - for viewed_package in venv.package_metadata.values(): + for package_info in venv.package_metadata.values(): bin_dir_app_paths |= _get_package_bin_dir_app_paths( - venv, viewed_package, local_bin_dir + venv, package_info, local_bin_dir ) elif venv.python_path.is_file(): # No metadata from pipx_metadata.json, but valid python interpreter. From a87ffb982797c797350df9b11fc5e65529bf2c4d Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 16:28:58 -0700 Subject: [PATCH 12/30] Add test for uninstall dep problem. --- tests/test_uninstall.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 7ea07755c7..6e089179e3 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -98,3 +98,25 @@ def test_uninstall_with_missing_interpreter_legacy_venv( # Also use is_symlink to check for broken symlink. # exists() returns False if symlink exists but target doesn't exist assert not executable_path.exists() and not executable_path.is_symlink() + + +def test_uninstall_proper_dep_behavior(pipx_temp_env): + isort_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["isort"]["apps"]] + pylint_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["pylint"]["apps"]] + + assert not run_pipx_cli(["install", PKG["pylint"]["spec"]]) + assert not run_pipx_cli(["install", PKG["isort"]["spec"]]) + for pylint_app_path in pylint_app_paths: + assert pylint_app_path.exists() + for isort_app_path in isort_app_paths: + assert isort_app_path.exists() + + assert not run_pipx_cli(["uninstall", "pylint"]) + + for pylint_app_path in pylint_app_paths: + # Also use is_symlink to check for broken symlink. + # exists() returns False if symlink exists but target doesn't exist + assert not pylint_app_path.exists() and not pylint_app_path.is_symlink() + # THIS is what we're making sure is true: + for isort_app_path in isort_app_paths: + assert isort_app_path.exists() From 648e13cdb783b7a20199e021a1e42cf6a792b3d5 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 18:07:29 -0700 Subject: [PATCH 13/30] Allow mock_legacy_venv to accept current metadata version. --- tests/helpers.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index 48e86a4d7e..5c37904cb1 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -78,19 +78,22 @@ def mock_legacy_venv(venv_name: str, metadata_version: Optional[str] = None) -> """ venv_dir = Path(constants.PIPX_LOCAL_VENVS) / canonicalize_name(venv_name) - if metadata_version is None: - os.remove(venv_dir / "pipx_metadata.json") + if metadata_version == "0.2": + # Current metadata version, do nothing return - - modern_metadata = pipx_metadata_file.PipxMetadata(venv_dir).to_dict() - - if metadata_version == "0.1": + elif metadata_version == "0.1": mock_pipx_metadata_template = MOCK_PIPXMETADATA_0_1 + elif metadata_version is None: + # No metadata + os.remove(venv_dir / "pipx_metadata.json") + return else: raise Exception( f"Internal Test Error: Unknown metadata_version={metadata_version}" ) + modern_metadata = pipx_metadata_file.PipxMetadata(venv_dir).to_dict() + # Convert to mock old metadata mock_pipx_metadata = {} for key in mock_pipx_metadata_template: @@ -159,6 +162,6 @@ def assert_package_metadata(test_metadata, ref_metadata): apps=sorted(test_metadata.apps), app_paths=sorted(test_metadata.app_paths) ) ref_metadata_replaced = ref_metadata._replace( - apps=sorted(ref_metadata.apps), app_paths=sorted(ref_metadata.app_paths), + apps=sorted(ref_metadata.apps), app_paths=sorted(ref_metadata.app_paths) ) assert test_metadata_replaced == ref_metadata_replaced From 5acab2a546d92ead6a71351eb088bb5a8260af47 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 18:08:12 -0700 Subject: [PATCH 14/30] Remove unused capsys fixture. --- tests/test_uninstall.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 6e089179e3..6a54d8993c 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -7,23 +7,23 @@ from pipx import constants, util -def test_uninstall(pipx_temp_env, capsys): +def test_uninstall(pipx_temp_env): assert not run_pipx_cli(["install", "pycowsay"]) assert not run_pipx_cli(["uninstall", "pycowsay"]) -def test_uninstall_multiple_same_app(pipx_temp_env, capsys): +def test_uninstall_multiple_same_app(pipx_temp_env): assert not run_pipx_cli(["install", "kaggle==1.5.9", "--include-deps"]) assert not run_pipx_cli(["uninstall", "kaggle"]) -def test_uninstall_circular_deps(pipx_temp_env, capsys): +def test_uninstall_circular_deps(pipx_temp_env): assert not run_pipx_cli(["install", PKG["cloudtoken"]["spec"]]) assert not run_pipx_cli(["uninstall", "cloudtoken"]) @pytest.mark.parametrize("metadata_version", [None, "0.1"]) -def test_uninstall_legacy_venv(pipx_temp_env, capsys, metadata_version): +def test_uninstall_legacy_venv(pipx_temp_env, metadata_version): executable_path = constants.LOCAL_BIN_DIR / app_name("pycowsay") assert not run_pipx_cli(["install", "pycowsay"]) @@ -36,7 +36,7 @@ def test_uninstall_legacy_venv(pipx_temp_env, capsys, metadata_version): assert not executable_path.exists() and not executable_path.is_symlink() -def test_uninstall_suffix(pipx_temp_env, capsys): +def test_uninstall_suffix(pipx_temp_env): name = "pbr" suffix = "_a" executable_path = constants.LOCAL_BIN_DIR / app_name(f"{name}{suffix}") @@ -51,7 +51,7 @@ def test_uninstall_suffix(pipx_temp_env, capsys): @pytest.mark.parametrize("metadata_version", ["0.1"]) -def test_uninstall_suffix_legacy_venv(pipx_temp_env, capsys, metadata_version): +def test_uninstall_suffix_legacy_venv(pipx_temp_env, metadata_version): name = "pbr" # legacy uninstall on Windows only works with "canonical name characters" # in suffix @@ -68,7 +68,7 @@ def test_uninstall_suffix_legacy_venv(pipx_temp_env, capsys, metadata_version): assert not executable_path.exists() and not executable_path.is_symlink() -def test_uninstall_with_missing_interpreter(pipx_temp_env, capsys): +def test_uninstall_with_missing_interpreter(pipx_temp_env): assert not run_pipx_cli(["install", "pycowsay"]) _, python_path = util.get_venv_paths(constants.PIPX_LOCAL_VENVS / "pycowsay") @@ -81,7 +81,7 @@ def test_uninstall_with_missing_interpreter(pipx_temp_env, capsys): @pytest.mark.parametrize("metadata_version", [None, "0.1"]) def test_uninstall_with_missing_interpreter_legacy_venv( - pipx_temp_env, capsys, metadata_version + pipx_temp_env, metadata_version ): executable_path = constants.LOCAL_BIN_DIR / app_name("pycowsay") From 6b79df8ac22f3ca0a8b43aad96c19484246006da Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 18:08:40 -0700 Subject: [PATCH 15/30] Modify test_uninstall_proper_dep_behavior to test all metadata versions. --- tests/test_uninstall.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 6a54d8993c..957f100935 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -100,12 +100,15 @@ def test_uninstall_with_missing_interpreter_legacy_venv( assert not executable_path.exists() and not executable_path.is_symlink() -def test_uninstall_proper_dep_behavior(pipx_temp_env): +@pytest.mark.parametrize("metadata_version", [None, "0.1", "0.2"]) +def test_uninstall_proper_dep_behavior(pipx_temp_env, metadata_version): isort_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["isort"]["apps"]] pylint_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["pylint"]["apps"]] assert not run_pipx_cli(["install", PKG["pylint"]["spec"]]) assert not run_pipx_cli(["install", PKG["isort"]["spec"]]) + mock_legacy_venv("pylint", metadata_version=metadata_version) + mock_legacy_venv("isort", metadata_version=metadata_version) for pylint_app_path in pylint_app_paths: assert pylint_app_path.exists() for isort_app_path in isort_app_paths: From b99f46527f5838a8e316884b148e7b018decf0b7 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 18:18:30 -0700 Subject: [PATCH 16/30] Add helper remove_venv_interpreter(). --- tests/helpers.py | 9 ++++++++- tests/test_list.py | 9 ++++----- tests/test_uninstall.py | 14 ++++---------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index 5c37904cb1..7e374e4410 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -9,7 +9,7 @@ from packaging.utils import canonicalize_name from package_info import PKG -from pipx import constants, main, pipx_metadata_file +from pipx import constants, main, pipx_metadata_file, util WIN = sys.platform.startswith("win") @@ -165,3 +165,10 @@ def assert_package_metadata(test_metadata, ref_metadata): apps=sorted(ref_metadata.apps), app_paths=sorted(ref_metadata.app_paths) ) assert test_metadata_replaced == ref_metadata_replaced + + +def remove_venv_interpreter(venv_name): + _, venv_python_path = util.get_venv_paths(constants.PIPX_LOCAL_VENVS / venv_name) + assert venv_python_path.is_file() + venv_python_path.unlink() + assert not venv_python_path.is_file() diff --git a/tests/test_list.py b/tests/test_list.py index b93f94526b..97772c92fe 100644 --- a/tests/test_list.py +++ b/tests/test_list.py @@ -8,10 +8,11 @@ assert_package_metadata, create_package_info_ref, mock_legacy_venv, + remove_venv_interpreter, run_pipx_cli, ) from package_info import PKG -from pipx import constants, util +from pipx import constants from pipx.pipx_metadata_file import PackageInfo, _json_decoder_object_hook @@ -24,14 +25,12 @@ def test_cli(pipx_temp_env, monkeypatch, capsys): def test_missing_interpreter(pipx_temp_env, monkeypatch, capsys): assert not run_pipx_cli(["install", "pycowsay"]) - _, python_path = util.get_venv_paths(constants.PIPX_LOCAL_VENVS / "pycowsay") - assert (python_path).is_file() - assert not run_pipx_cli(["list"]) captured = capsys.readouterr() assert "package pycowsay has invalid interpreter" not in captured.out - python_path.unlink() + remove_venv_interpreter("pycowsay") + assert run_pipx_cli(["list"]) captured = capsys.readouterr() assert "package pycowsay has invalid interpreter" in captured.out diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 957f100935..c7e3773a9a 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -2,9 +2,9 @@ import pytest # type: ignore -from helpers import app_name, mock_legacy_venv, run_pipx_cli +from helpers import app_name, mock_legacy_venv, remove_venv_interpreter, run_pipx_cli from package_info import PKG -from pipx import constants, util +from pipx import constants def test_uninstall(pipx_temp_env): @@ -70,12 +70,7 @@ def test_uninstall_suffix_legacy_venv(pipx_temp_env, metadata_version): def test_uninstall_with_missing_interpreter(pipx_temp_env): assert not run_pipx_cli(["install", "pycowsay"]) - - _, python_path = util.get_venv_paths(constants.PIPX_LOCAL_VENVS / "pycowsay") - assert python_path.is_file() - python_path.unlink() - assert not python_path.is_file() - + remove_venv_interpreter("pycowsay") assert not run_pipx_cli(["uninstall", "pycowsay"]) @@ -89,8 +84,7 @@ def test_uninstall_with_missing_interpreter_legacy_venv( assert executable_path.exists() mock_legacy_venv("pycowsay", metadata_version=metadata_version) - _, venv_python_path = util.get_venv_paths(constants.PIPX_LOCAL_VENVS / "pycowsay") - venv_python_path.unlink() + remove_venv_interpreter("pycowsay") assert not run_pipx_cli(["uninstall", "pycowsay"]) # On Windows we cannot remove app binaries if no metadata and no python From beaf7a283a513713a07b6f84346255de8ef18f73 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 18:18:47 -0700 Subject: [PATCH 17/30] Add uninstall test for missing interpreter. --- tests/test_uninstall.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index c7e3773a9a..3a36cada06 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -117,3 +117,32 @@ def test_uninstall_proper_dep_behavior(pipx_temp_env, metadata_version): # THIS is what we're making sure is true: for isort_app_path in isort_app_paths: assert isort_app_path.exists() + + +@pytest.mark.parametrize("metadata_version", [None, "0.1", "0.2"]) +def test_uninstall_proper_dep_behavior_missing_interpreter( + pipx_temp_env, metadata_version +): + isort_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["isort"]["apps"]] + pylint_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["pylint"]["apps"]] + + assert not run_pipx_cli(["install", PKG["pylint"]["spec"]]) + assert not run_pipx_cli(["install", PKG["isort"]["spec"]]) + mock_legacy_venv("pylint", metadata_version=metadata_version) + mock_legacy_venv("isort", metadata_version=metadata_version) + remove_venv_interpreter("pylint") + remove_venv_interpreter("isort") + for pylint_app_path in pylint_app_paths: + assert pylint_app_path.exists() + for isort_app_path in isort_app_paths: + assert isort_app_path.exists() + + assert not run_pipx_cli(["uninstall", "pylint"]) + + for pylint_app_path in pylint_app_paths: + # Also use is_symlink to check for broken symlink. + # exists() returns False if symlink exists but target doesn't exist + assert not pylint_app_path.exists() and not pylint_app_path.is_symlink() + # THIS is what we're making sure is true: + for isort_app_path in isort_app_paths: + assert isort_app_path.exists() From dc4b4c3846f1246f8a3fb37f1a118bd69e61caaf Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 19:44:56 -0700 Subject: [PATCH 18/30] Add comments. --- tests/test_uninstall.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 3a36cada06..6fe40f6065 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -96,6 +96,8 @@ def test_uninstall_with_missing_interpreter_legacy_venv( @pytest.mark.parametrize("metadata_version", [None, "0.1", "0.2"]) def test_uninstall_proper_dep_behavior(pipx_temp_env, metadata_version): + # isort is a dependency of pylint. Make sure that uninstalling pylint + # does not also uninstall isort app in LOCAL_BIN_DIR isort_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["isort"]["apps"]] pylint_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["pylint"]["apps"]] @@ -123,6 +125,8 @@ def test_uninstall_proper_dep_behavior(pipx_temp_env, metadata_version): def test_uninstall_proper_dep_behavior_missing_interpreter( pipx_temp_env, metadata_version ): + # isort is a dependency of pylint. Make sure that uninstalling pylint + # does not also uninstall isort app in LOCAL_BIN_DIR isort_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["isort"]["apps"]] pylint_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["pylint"]["apps"]] From 6ab0cdd3fb89b70fa304f7551aaf6ec94bbd28b0 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 22:09:27 -0700 Subject: [PATCH 19/30] Clarify comment. --- src/pipx/commands/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pipx/commands/common.py b/src/pipx/commands/common.py index f825584c25..46a778f26c 100644 --- a/src/pipx/commands/common.py +++ b/src/pipx/commands/common.py @@ -248,7 +248,7 @@ def get_exposed_app_paths_for_package( package_binary_names: Optional[List[str]] = None, ) -> Set[Path]: # package_binary_names is used only if local_bin_path cannot use symlinks. - # It is necessary for those systems to return app_paths. + # It is necessary for non-symlink systems to return valid app_paths. if package_binary_names is None: package_binary_names = [] From 7ab4d021c16367e2e69e11b52e63eb50af6862db Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 22:11:46 -0700 Subject: [PATCH 20/30] Add changelog entry, make other changelog entries past tense. --- docs/changelog.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index cde1f582b7..780a91be5e 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,11 +1,12 @@ dev -- Make sure log files are utf-8 encoded to preven Unicode encoding errors from occurring with emojis. (#646) +- Ensured log files are utf-8 encoded to preven Unicode encoding errors from occurring with emojis. (#646) - Fixed issue which made pipx incorrectly list apps as part of a venv when they were not installed by pipx. (#650) - Fixed old regression that would prevent pipx uninstall from cleaning up linked binaries if the venv was old and did not have pipx metadata. (#651) - Fixed bugs with suffixed-venvs on Windows. Now properly summarizes install, and actually uninstalls associated binaries for suffixed-venvs. (#653) -- Add `--json` switch to `pipx list` to output rich json-metadata for all venvs. -- Change venv minimum python version to 3.6, removing python 3.5 which is End of Life. (#666) +- Added `--json` switch to `pipx list` to output rich json-metadata for all venvs. +- Changed venv minimum python version to 3.6, removing python 3.5 which is End of Life. (#666) +- Fixed bug (#670) where uninstalling a venv could erroneously uninstall other apps from the local binary directory. 0.16.1.0 From 02e47efe8f55b3a74b6f92bacb8f1236eb0a4e99 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 22:29:02 -0700 Subject: [PATCH 21/30] Wrap unlink in try-except to handle FileNotFoundError. --- src/pipx/commands/uninstall.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py index 0c18e6b45a..09d22b18e2 100644 --- a/src/pipx/commands/uninstall.py +++ b/src/pipx/commands/uninstall.py @@ -124,8 +124,12 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode: bin_dir_app_paths = _get_venv_bin_dir_app_paths(venv, local_bin_dir) for bin_dir_app_path in bin_dir_app_paths: - logger.info(f"removing file {bin_dir_app_path}") - bin_dir_app_path.unlink() + try: + bin_dir_app_path.unlink() + except FileNotFoundError: + logger.info(f"tried to remove but couldn't find {bin_dir_app_path}") + else: + logger.info(f"removed file {bin_dir_app_path}") rmdir(venv_dir) print(f"uninstalled {venv.name}! {stars}") From c3ca9fbd6baa95ada2e8f30bfc11f100bad0294c Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Fri, 16 Apr 2021 22:46:22 -0700 Subject: [PATCH 22/30] Update changelog entry with PR #. --- docs/changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.md b/docs/changelog.md index 780a91be5e..425c388f7a 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -6,7 +6,7 @@ dev - Fixed bugs with suffixed-venvs on Windows. Now properly summarizes install, and actually uninstalls associated binaries for suffixed-venvs. (#653) - Added `--json` switch to `pipx list` to output rich json-metadata for all venvs. - Changed venv minimum python version to 3.6, removing python 3.5 which is End of Life. (#666) -- Fixed bug (#670) where uninstalling a venv could erroneously uninstall other apps from the local binary directory. +- Fixed bug #670 where uninstalling a venv could erroneously uninstall other apps from the local binary directory. (#672) 0.16.1.0 From 1fe7c3d93269214dcd258db084f7c3ca02e18d2b Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Sat, 17 Apr 2021 12:24:22 -0700 Subject: [PATCH 23/30] Add clause to skip assertions on Windows with no metadata, no interpreter. --- tests/test_uninstall.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 6fe40f6065..ea9cc1247d 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -143,10 +143,13 @@ def test_uninstall_proper_dep_behavior_missing_interpreter( assert not run_pipx_cli(["uninstall", "pylint"]) - for pylint_app_path in pylint_app_paths: - # Also use is_symlink to check for broken symlink. - # exists() returns False if symlink exists but target doesn't exist - assert not pylint_app_path.exists() and not pylint_app_path.is_symlink() + # Do not check the following on Windows without metadata, we do not + # remove bin dir links by design for missing interpreter in that case + if not (sys.platform.startswith("win") and metadata_version is None): + for pylint_app_path in pylint_app_paths: + # Also use is_symlink to check for broken symlink. + # exists() returns False if symlink exists but target doesn't exist + assert not pylint_app_path.exists() and not pylint_app_path.is_symlink() # THIS is what we're making sure is true: for isort_app_path in isort_app_paths: assert isort_app_path.exists() From 17cfd21dd84c19936439e76573c3f25a4c0c30ee Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Sat, 17 Apr 2021 12:49:52 -0700 Subject: [PATCH 24/30] Add helper function to tests. --- tests/test_uninstall.py | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index ea9cc1247d..4e9afd22e1 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -7,6 +7,16 @@ from pipx import constants +def file_or_symlink(filepath): + # Returns True for file or broken symlink or non-broken symlink + # Returns False for no file and no symlink + + # filepath.exists() returns True for file or non-broken symlink + # filepath.exists() returns False for broken symlink + # filepath.is_symlink() returns True for broken or non-broken symlink + return filepath.exists() or filepath.is_symlink() + + def test_uninstall(pipx_temp_env): assert not run_pipx_cli(["install", "pycowsay"]) assert not run_pipx_cli(["uninstall", "pycowsay"]) @@ -31,9 +41,7 @@ def test_uninstall_legacy_venv(pipx_temp_env, metadata_version): mock_legacy_venv("pycowsay", metadata_version=metadata_version) assert not run_pipx_cli(["uninstall", "pycowsay"]) - # Also use is_symlink to check for broken symlink. - # exists() returns False if symlink exists but target doesn't exist - assert not executable_path.exists() and not executable_path.is_symlink() + assert not file_or_symlink(executable_path) def test_uninstall_suffix(pipx_temp_env): @@ -45,9 +53,7 @@ def test_uninstall_suffix(pipx_temp_env): assert executable_path.exists() assert not run_pipx_cli(["uninstall", f"{name}{suffix}"]) - # Also use is_symlink to check for broken symlink. - # exists() returns False if symlink exists but target doesn't exist - assert not executable_path.exists() and not executable_path.is_symlink() + assert not file_or_symlink(executable_path) @pytest.mark.parametrize("metadata_version", ["0.1"]) @@ -63,9 +69,7 @@ def test_uninstall_suffix_legacy_venv(pipx_temp_env, metadata_version): assert executable_path.exists() assert not run_pipx_cli(["uninstall", f"{name}{suffix}"]) - # Also use is_symlink to check for broken symlink. - # exists() returns False if symlink exists but target doesn't exist - assert not executable_path.exists() and not executable_path.is_symlink() + assert not file_or_symlink(executable_path) def test_uninstall_with_missing_interpreter(pipx_temp_env): @@ -89,9 +93,7 @@ def test_uninstall_with_missing_interpreter_legacy_venv( assert not run_pipx_cli(["uninstall", "pycowsay"]) # On Windows we cannot remove app binaries if no metadata and no python if not sys.platform.startswith("win"): - # Also use is_symlink to check for broken symlink. - # exists() returns False if symlink exists but target doesn't exist - assert not executable_path.exists() and not executable_path.is_symlink() + assert not file_or_symlink(executable_path) @pytest.mark.parametrize("metadata_version", [None, "0.1", "0.2"]) @@ -113,9 +115,7 @@ def test_uninstall_proper_dep_behavior(pipx_temp_env, metadata_version): assert not run_pipx_cli(["uninstall", "pylint"]) for pylint_app_path in pylint_app_paths: - # Also use is_symlink to check for broken symlink. - # exists() returns False if symlink exists but target doesn't exist - assert not pylint_app_path.exists() and not pylint_app_path.is_symlink() + assert not file_or_symlink(pylint_app_path) # THIS is what we're making sure is true: for isort_app_path in isort_app_paths: assert isort_app_path.exists() @@ -147,9 +147,7 @@ def test_uninstall_proper_dep_behavior_missing_interpreter( # remove bin dir links by design for missing interpreter in that case if not (sys.platform.startswith("win") and metadata_version is None): for pylint_app_path in pylint_app_paths: - # Also use is_symlink to check for broken symlink. - # exists() returns False if symlink exists but target doesn't exist - assert not pylint_app_path.exists() and not pylint_app_path.is_symlink() + assert not file_or_symlink(pylint_app_path) # THIS is what we're making sure is true: for isort_app_path in isort_app_paths: assert isort_app_path.exists() From 79368a19b68d49925a97a599f5f1da15a63976af Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Sat, 17 Apr 2021 19:32:11 -0700 Subject: [PATCH 25/30] Clarify comments. --- tests/test_uninstall.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 4e9afd22e1..2ad7bd913c 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -11,8 +11,8 @@ def file_or_symlink(filepath): # Returns True for file or broken symlink or non-broken symlink # Returns False for no file and no symlink - # filepath.exists() returns True for file or non-broken symlink - # filepath.exists() returns False for broken symlink + # filepath.exists() returns True for regular file or non-broken symlink + # filepath.exists() returns False for no regular file or broken symlink # filepath.is_symlink() returns True for broken or non-broken symlink return filepath.exists() or filepath.is_symlink() From 2a92bef6ae547cf4e3df67817e12a5fbcaa5b8ea Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Mon, 19 Apr 2021 14:12:10 -0700 Subject: [PATCH 26/30] Combine tests using parameterization, fix if check for win + no metadata. --- tests/test_uninstall.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 2ad7bd913c..8796670e23 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -72,16 +72,8 @@ def test_uninstall_suffix_legacy_venv(pipx_temp_env, metadata_version): assert not file_or_symlink(executable_path) -def test_uninstall_with_missing_interpreter(pipx_temp_env): - assert not run_pipx_cli(["install", "pycowsay"]) - remove_venv_interpreter("pycowsay") - assert not run_pipx_cli(["uninstall", "pycowsay"]) - - -@pytest.mark.parametrize("metadata_version", [None, "0.1"]) -def test_uninstall_with_missing_interpreter_legacy_venv( - pipx_temp_env, metadata_version -): +@pytest.mark.parametrize("metadata_version", [None, "0.1", "0.2"]) +def test_uninstall_with_missing_interpreter(pipx_temp_env, metadata_version): executable_path = constants.LOCAL_BIN_DIR / app_name("pycowsay") assert not run_pipx_cli(["install", "pycowsay"]) @@ -92,7 +84,7 @@ def test_uninstall_with_missing_interpreter_legacy_venv( assert not run_pipx_cli(["uninstall", "pycowsay"]) # On Windows we cannot remove app binaries if no metadata and no python - if not sys.platform.startswith("win"): + if not (sys.platform.startswith("win") and metadata_version is None): assert not file_or_symlink(executable_path) From 31f3ba100e41662c83b1055fd6bc11d48a438bb4 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Sat, 24 Apr 2021 23:36:28 -0700 Subject: [PATCH 27/30] Add test for uninstalling injected apps. --- tests/test_uninstall.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 8796670e23..bbba626a15 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -56,6 +56,27 @@ def test_uninstall_suffix(pipx_temp_env): assert not file_or_symlink(executable_path) +def test_uninstall_injected(pipx_temp_env): + pycowsay_app_paths = [ + constants.LOCAL_BIN_DIR / app for app in PKG["pycowsay"]["apps"] + ] + pylint_app_paths = [constants.LOCAL_BIN_DIR / app for app in PKG["pylint"]["apps"]] + app_paths = pycowsay_app_paths + pylint_app_paths + + assert not run_pipx_cli(["install", PKG["pycowsay"]["spec"]]) + assert not run_pipx_cli( + ["inject", "--include-apps", "pycowsay", PKG["pylint"]["spec"]] + ) + + for app_path in app_paths: + assert app_path.exists() + + assert not run_pipx_cli(["uninstall", "pycowsay"]) + + for app_path in app_paths: + assert not file_or_symlink(app_path) + + @pytest.mark.parametrize("metadata_version", ["0.1"]) def test_uninstall_suffix_legacy_venv(pipx_temp_env, metadata_version): name = "pbr" From 99ba5f5a0fe99361238d37a29ee58595430949fe Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Sun, 25 Apr 2021 21:28:40 -0700 Subject: [PATCH 28/30] Add fixture in test_uninstall: no symlinks always on Windows. --- tests/test_uninstall.py | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index bbba626a15..f22b236751 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -2,9 +2,29 @@ import pytest # type: ignore -from helpers import app_name, mock_legacy_venv, remove_venv_interpreter, run_pipx_cli +from helpers import ( + WIN, + app_name, + mock_legacy_venv, + remove_venv_interpreter, + run_pipx_cli, +) from package_info import PKG -from pipx import constants +from pipx import commands, constants + + +@pytest.fixture(autouse=True) +def windows_no_symlinks(monkeypatch): + """On Windows, monkeypatch pipx.commands.common._can_symlink_cache + to indicate that constants.LOCAL_BIN_DIR cannot use symlinks, even + if we're running as administrator and symlinks are actually possible. + + On all other platforms than Windows this fixture has no effect. + """ + if WIN: + monkeypatch.setitem( + commands.common._can_symlink_cache, constants.LOCAL_BIN_DIR, False + ) def file_or_symlink(filepath): From 7a317e63434815fee72afbc93705aed562935802 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Sun, 25 Apr 2021 22:16:47 -0700 Subject: [PATCH 29/30] Move _can_symlink_cache monkeypath to pipx_temp_env. --- tests/conftest.py | 12 +++++++++++- tests/test_uninstall.py | 24 ++---------------------- 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 315e4be7d3..d846a2164b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,7 +3,8 @@ import pytest # type: ignore -from pipx import constants, shared_libs, venv +from helpers import WIN +from pipx import commands, constants, shared_libs, venv def pytest_addoption(parser): @@ -54,3 +55,12 @@ def pipx_temp_env(tmp_path, monkeypatch): monkeypatch.setenv("PATH_ORIG", str(bin_dir) + os.pathsep + os.getenv("PATH")) monkeypatch.setenv("PATH_TEST", str(bin_dir)) monkeypatch.setenv("PATH", str(bin_dir)) + + # On Windows, monkeypatch pipx.commands.common._can_symlink_cache to + # indicate that constants.LOCAL_BIN_DIR cannot use symlinks, even if + # we're running as administrator and symlinks are actually possible. + # On all other platforms than Windows this has no effect. + if WIN: + monkeypatch.setitem( + commands.common._can_symlink_cache, constants.LOCAL_BIN_DIR, False + ) diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index f22b236751..bbba626a15 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -2,29 +2,9 @@ import pytest # type: ignore -from helpers import ( - WIN, - app_name, - mock_legacy_venv, - remove_venv_interpreter, - run_pipx_cli, -) +from helpers import app_name, mock_legacy_venv, remove_venv_interpreter, run_pipx_cli from package_info import PKG -from pipx import commands, constants - - -@pytest.fixture(autouse=True) -def windows_no_symlinks(monkeypatch): - """On Windows, monkeypatch pipx.commands.common._can_symlink_cache - to indicate that constants.LOCAL_BIN_DIR cannot use symlinks, even - if we're running as administrator and symlinks are actually possible. - - On all other platforms than Windows this fixture has no effect. - """ - if WIN: - monkeypatch.setitem( - commands.common._can_symlink_cache, constants.LOCAL_BIN_DIR, False - ) +from pipx import constants def file_or_symlink(filepath): From 58d18549e0d6c8389f506393c14e331b8f82c7e6 Mon Sep 17 00:00:00 2001 From: Matthew Clapp Date: Sun, 25 Apr 2021 23:09:48 -0700 Subject: [PATCH 30/30] Remove redundant comment. --- tests/conftest.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index d846a2164b..0f42a2a6e1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -59,7 +59,6 @@ def pipx_temp_env(tmp_path, monkeypatch): # On Windows, monkeypatch pipx.commands.common._can_symlink_cache to # indicate that constants.LOCAL_BIN_DIR cannot use symlinks, even if # we're running as administrator and symlinks are actually possible. - # On all other platforms than Windows this has no effect. if WIN: monkeypatch.setitem( commands.common._can_symlink_cache, constants.LOCAL_BIN_DIR, False