Repository: pipxproject/pipx
Pull request #651 (2021-03-21 06:12:23)

Fix cleanup of app binaries for venvs without pipx_metadata.json
<!-- add an 'x' in the brackets below -->
* [x] I have added an entry to `docs/changelog.md`

## Summary of changes
Found this code bug with `mypy --warn-unreachable`.

This PR fixes a bug with `pipx uninstall` that would leave app binaries leftover in `~/.local/bin` if the venv has no metadata file.

We were testing the wrong object to determine if metadata was present in `uninstall.py`.  `venv.pipx_metadata.main_package` is in fact always not `None`, (it's always `PackageInfo`) so the else (for missing metadata case) was impossible to execute.  Thus the special cleanup code for the non-metadata case to remove app binaries never executed.

I fixed it by changing the test for `None` to the object `venv.pipx_metadata.main_package.package`.

In addition, I updated `venv.get_venv_metadata_for_package` to add its currently-necessary second argument.  (This code must have been unreachable for a while to not get flagged that it is missing an argument.)

## Test plan
<!-- provide evidence of testing, preferably with command(s) that can be copy+pasted by others -->
Tested by running
```shell
pipx install pycowsay
rm ~/.local/pipx/venvs/pycowsay/pipx_metadata.json
pipx uninstall pycowsay
```

This PR:
```shell
> ls -l ~/.local/bin/pycowsay
ls: /Users/mclapp/.local/bin/pycowsay: No such file or directory
```

pipx 0.16.1.0
```shell
> ls -l ~/.local/bin/pycowsay                     
lrwxr-xr-x  1 mclapp  staff  53 Mar 19 22:54 /Users/mclapp/.local/bin/pycowsay@ -> /Users/mclapp/.local/pipx/venvs/pycowsay/bin/pycowsay

```

itsayellow: I suppose I should be a "grown up" and add a test for this large block of code that isn't being tested...

Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 0a16f01..75cf588 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -2,6 +2,7 @@ dev
 
 - Make sure 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)
 
 0.16.1.0
 
diff --git a/src/pipx/commands/uninstall.py b/src/pipx/commands/uninstall.py
index 3e91fee..acf2da5 100644
--- a/src/pipx/commands/uninstall.py
+++ b/src/pipx/commands/uninstall.py
@@ -35,7 +35,7 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode:
 
     venv = Venv(venv_dir, verbose=verbose)
 
-    if venv.pipx_metadata.main_package is not None:
+    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
@@ -46,7 +46,7 @@ def uninstall(venv_dir: Path, local_bin_dir: Path, verbose: bool) -> ExitCode:
         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)
+            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


Test patch:
diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py
index 4662e00..5ebb74b 100644
--- a/tests/test_uninstall.py
+++ b/tests/test_uninstall.py
@@ -1,6 +1,6 @@
 import pytest  # type: ignore
 
-from helpers import mock_legacy_venv, run_pipx_cli
+from helpers import app_name, mock_legacy_venv, run_pipx_cli
 from package_info import PKG
 from pipx import constants, util
 
@@ -22,35 +22,42 @@ def test_uninstall_circular_deps(pipx_temp_env, capsys):
 
 @pytest.mark.parametrize("metadata_version", [None, "0.1"])
 def test_uninstall_legacy_venv(pipx_temp_env, capsys, metadata_version):
+    executable_path = constants.LOCAL_BIN_DIR / app_name("pycowsay")
+
     assert not run_pipx_cli(["install", "pycowsay"])
+    assert executable_path.exists()
+
     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()
 
 
 def test_uninstall_suffix(pipx_temp_env, capsys):
     name = "pbr"
     suffix = "_a"
-    executable = f"{name}{suffix}{'.exe' if constants.WINDOWS else ''}"
+    executable_path = constants.LOCAL_BIN_DIR / app_name(f"{name}{suffix}")
 
     assert not run_pipx_cli(["install", "pbr", f"--suffix={suffix}"])
-    assert (constants.LOCAL_BIN_DIR / executable).exists()
+    assert executable_path.exists()
 
     assert not run_pipx_cli(["uninstall", f"{name}{suffix}"])
-    assert not (constants.LOCAL_BIN_DIR / executable).exists()
+    assert not executable_path.exists()
 
 
 @pytest.mark.parametrize("metadata_version", ["0.1"])
 def test_uninstall_suffix_legacy_venv(pipx_temp_env, capsys, metadata_version):
     name = "pbr"
     suffix = "_a"
-    executable = f"{name}{suffix}{'.exe' if constants.WINDOWS else ''}"
+    executable_path = constants.LOCAL_BIN_DIR / app_name(f"{name}{suffix}")
 
     assert not run_pipx_cli(["install", "pbr", f"--suffix={suffix}"])
     mock_legacy_venv(f"{name}{suffix}", metadata_version=metadata_version)
-    assert (constants.LOCAL_BIN_DIR / executable).exists()
+    assert executable_path.exists()
 
     assert not run_pipx_cli(["uninstall", f"{name}{suffix}"])
-    assert not (constants.LOCAL_BIN_DIR / executable).exists()
+    assert not executable_path.exists()
 
 
 def test_uninstall_with_missing_interpreter(pipx_temp_env, capsys):