diff --git a/AddonManagerTest/app/test_python_deps.py b/AddonManagerTest/app/test_python_deps.py index e5bed28e..0de846c1 100644 --- a/AddonManagerTest/app/test_python_deps.py +++ b/AddonManagerTest/app/test_python_deps.py @@ -20,6 +20,7 @@ # * . * # * * # *************************************************************************** +import os import subprocess import unittest from unittest.mock import MagicMock, patch @@ -268,3 +269,173 @@ def test_update_packages_pip_failure(self, mock_print_error, mock_print_log, moc mock_call_pip.assert_called_once() mock_print_error.assert_called_once_with("upgrade failed\n") + + class TestCleanupOldPackageVersions(unittest.TestCase): + """Tests for the _cleanup_old_package_versions method""" + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.shutil.rmtree") + @patch("addonmanager_python_deps.fci.Console.PrintLog") + def test_cleanup_removes_old_versions_keeps_newest( + self, mock_print_log, mock_rmtree, mock_listdir, mock_exists + ): + """Test that old package versions are removed and newest is kept""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "requests-2.28.0.dist-info", + "requests-2.31.0.dist-info", + "numpy-1.24.0.dist-info", + "numpy-1.26.0.dist-info", + "numpy-1.25.2.dist-info", + "other_file.txt", + ] + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + + # Should remove old versions but keep newest + self.assertEqual(mock_rmtree.call_count, 3) + removed_paths = [call[0][0] for call in mock_rmtree.call_args_list] + + # Check old versions were removed (works on all platforms) + self.assertIn(os.path.join("/fake/path", "requests-2.28.0.dist-info"), removed_paths) + self.assertIn(os.path.join("/fake/path", "numpy-1.24.0.dist-info"), removed_paths) + self.assertIn(os.path.join("/fake/path", "numpy-1.25.2.dist-info"), removed_paths) + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_single_version_no_removal(self, mock_rmtree, mock_listdir, mock_exists): + """Test that packages with only one version are not touched""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "requests-2.31.0.dist-info", + "numpy-1.26.0.dist-info", + "pandas-2.1.0.dist-info", + ] + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + + # No removals should happen when only one version exists per package + mock_rmtree.assert_not_called() + + @patch("addonmanager_python_deps.os.path.exists") + def test_cleanup_nonexistent_directory(self, mock_exists): + """Test graceful handling when vendor path doesn't exist""" + mock_exists.return_value = False + + model = PythonPackageListModel([]) + model.vendor_path = "/nonexistent/path" + model._cleanup_old_package_versions() + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_empty_directory(self, mock_rmtree, mock_listdir, mock_exists): + """Test handling of empty vendor directory""" + mock_exists.return_value = True + mock_listdir.return_value = [] + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_rmtree.assert_not_called() + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + @patch("addonmanager_python_deps.fci.Console.PrintWarning") + def test_cleanup_handles_permission_error( + self, mock_print_warning, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test that permission errors are handled gracefully""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "requests-2.28.0.dist-info", + "requests-2.31.0.dist-info", + ] + mock_isdir.return_value = True + mock_rmtree.side_effect = PermissionError("Permission denied") + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_print_warning.assert_called() + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_normalizes_package_names( + self, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test that package names are normalized per PEP 503 (underscores to dashes)""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "my_package-1.0.0.dist-info", + "my-package-2.0.0.dist-info", + ] + mock_isdir.return_value = True + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + + # Should remove old version (they're the same package after normalization) + mock_rmtree.assert_called_once_with( + os.path.join("/fake/path", "my_package-1.0.0.dist-info") + ) + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_ignores_non_dist_info_directories( + self, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test that only .dist-info directories are processed""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "requests-2.28.0.dist-info", + "requests-2.31.0.dist-info", + "some_package", + "__pycache__", + "random_file.txt", + ] + mock_isdir.return_value = True + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_rmtree.assert_called_once_with( + os.path.join("/fake/path", "requests-2.28.0.dist-info") + ) + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + @patch("addonmanager_python_deps.fci.Console.PrintWarning") + def test_cleanup_handles_invalid_version_format( + self, mock_print_warning, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test handling of malformed version strings""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "badpackage-invalid.version.dist-info", + "goodpackage-1.0.0.dist-info", + "goodpackage-2.0.0.dist-info", + ] + mock_isdir.return_value = True + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_rmtree.assert_called_once_with( + os.path.join("/fake/path", "goodpackage-1.0.0.dist-info") + ) diff --git a/addonmanager_python_deps.py b/addonmanager_python_deps.py index 4bbe091f..5188b4d8 100644 --- a/addonmanager_python_deps.py +++ b/addonmanager_python_deps.py @@ -359,6 +359,55 @@ def update_call_finished(self): self.update_complete.emit() if not using_system_pip_installation_location(): shutil.rmtree(self.vendor_path + ".old") + # Clean up old package versions that may remain after update + self._cleanup_old_package_versions() + + def _cleanup_old_package_versions(self): + """Remove old package version metadata directories after an update. + + When pip updates packages with --target, it doesn't always remove old + version metadata (.dist-info directories). This can cause version detection + to find the old version instead of the new one, especially in Flatpak + installations where multiple versions accumulate. + """ + if not os.path.exists(self.vendor_path): + return + + # Group all dist-info directories by package name + package_versions = {} + for item in os.listdir(self.vendor_path): + item_path = os.path.join(self.vendor_path, item) + if os.path.isdir(item_path) and item.endswith(".dist-info"): + # Extract package name and version from directory name + # Format is typically: package_name-version.dist-info + match = re.match(r"^(.+?)-(\d+.+?)\.dist-info$", item) + if match: + package_name = match.group(1).lower().replace("_", "-") + version_str = match.group(2) + + if package_name not in package_versions: + package_versions[package_name] = [] + package_versions[package_name].append((version_str, item_path)) + + # For each package with multiple versions, keep only the newest + for package_name, versions in package_versions.items(): + if len(versions) > 1: + # Sort by version, newest last + try: + versions.sort(key=lambda x: Version(x[0])) + # Remove all but the newest version + for version_str, path in versions[:-1]: + try: + shutil.rmtree(path) + fci.Console.PrintLog( + f"Removed old version metadata for {package_name}: {version_str}\n" + ) + except (OSError, PermissionError) as e: + fci.Console.PrintWarning( + f"Could not remove old version metadata {path}: {e}\n" + ) + except Exception as e: + fci.Console.PrintWarning(f"Error processing versions for {package_name}: {e}\n") def determine_new_python_dependencies(self, addons) -> Set[str]: """Given a list of Addon objects, finds the Python dependencies for those addons. Also