Make uninstall on Windows not erroneously uninstall apps from depende…

…ncies (pypa#672)

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. (#672)
 
 0.16.1.0
 

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,10 +210,10 @@ 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,
+        [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(
@@ -242,9 +242,16 @@ def get_venv_summary(
     )
 
 
-def _get_exposed_app_paths_for_package(
-    venv_bin_path: Path, package_binary_names: List[str], local_bin_dir: Path
+def get_exposed_app_paths_for_package(
+    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.
+    # It is necessary for non-symlink systems to return valid app_paths.
+    if package_binary_names is None:
+        package_binary_names = []
+
     bin_symlinks = set()
     for b in local_bin_dir.iterdir():
         try:
@@ -254,9 +261,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:

src/pipx/commands/uninstall.py

@@ -1,23 +1,109 @@
 import logging
 from pathlib import Path
 from shutil import which
-from typing import List
+from typing import List, Optional, Set
 
-from pipx import constants
+from pipx.commands.common import (
+    add_suffix,
+    can_symlink,
+    get_exposed_app_paths_for_package,
+)
 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: 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,
+        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
+) -> 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(
+        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) -> Set[Path]:
+    bin_dir_app_paths = set()
+    if venv.pipx_metadata.main_package.package is not None:
+        # Valid metadata for venv
+        for package_info in venv.package_metadata.values():
+            bin_dir_app_paths |= _get_package_bin_dir_app_paths(
+                venv, package_info, local_bin_dir
+            )
+    elif venv.python_path.is_file():
+        # 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,
+        # 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, venv.root.name
+        )
+        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.
+        # 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 set.
+        if not can_symlink(local_bin_dir):
+            return set()
+
+        bin_dir_app_paths = get_exposed_app_paths_for_package(
+            venv.bin_path, local_bin_dir
+        )
+
+    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.
@@ -35,52 +121,15 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode:
 
     venv = Venv(venv_dir, verbose=verbose)
 
-    suffix = ""
+    bin_dir_app_paths = _get_venv_bin_dir_app_paths(venv, local_bin_dir)
 
-    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
-    else:
-        # fallback if not metadata from pipx_metadata.json
-        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:
-                    filepath.unlink()
+    for bin_dir_app_path in bin_dir_app_paths:
+        try:
+            bin_dir_app_path.unlink()
+        except FileNotFoundError:
+            logger.info(f"tried to remove but couldn't find {bin_dir_app_path}")
         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()
+            logger.info(f"removed file {bin_dir_app_path}")
 
     rmdir(venv_dir)
     print(f"uninstalled {venv.name}! {stars}")

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,11 @@ 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.
+    if WIN:
+        monkeypatch.setitem(
+            commands.common._can_symlink_cache, constants.LOCAL_BIN_DIR, False
+        )

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")
 
@@ -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,13 @@ 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
+
+
+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()

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