From 96800d7c1905fcfc2796ab9ea70ef77eb26d3085 Mon Sep 17 00:00:00 2001 From: Kapale Om Shripad Date: Tue, 30 Dec 2025 18:33:25 +0530 Subject: [PATCH 1/6] Fix: Clean up old Python package versions after update in Flatpak When updating Python dependencies with pip using --target directory, old package version metadata (.dist-info directories) are not always removed. This causes FreeCAD to detect the old version instead of the newly installed version, particularly in Flatpak installations. This fix adds cleanup logic to remove old package version metadata after a successful update, keeping only the newest version of each package. Fixes #26510 --- addonmanager_python_deps.py | 51 +++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/addonmanager_python_deps.py b/addonmanager_python_deps.py index e74ff7a8..9d589247 100644 --- a/addonmanager_python_deps.py +++ b/addonmanager_python_deps.py @@ -357,6 +357,57 @@ 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 From 3ab0ce7fd5aa8116cbbcc3ce676d11a895163816 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 31 Dec 2025 03:18:46 +0000 Subject: [PATCH 2/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- addonmanager_python_deps.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/addonmanager_python_deps.py b/addonmanager_python_deps.py index 9d589247..424bf939 100644 --- a/addonmanager_python_deps.py +++ b/addonmanager_python_deps.py @@ -362,7 +362,7 @@ def update_call_finished(self): 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 @@ -370,23 +370,23 @@ def _cleanup_old_package_versions(self): """ 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'): + 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) + match = re.match(r"^(.+?)-(\d+.+?)\.dist-info$", item) if match: - package_name = match.group(1).lower().replace('_', '-') + 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: @@ -405,9 +405,7 @@ def _cleanup_old_package_versions(self): 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" - ) + 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 From 7079ecb583d225ea3ab5d7ba3eb4bc589b584409 Mon Sep 17 00:00:00 2001 From: Kapale Om Shripad Date: Mon, 5 Jan 2026 11:23:28 +0530 Subject: [PATCH 3/6] Add comprehensive tests for _cleanup_old_package_versions method Added unit tests covering: - Removal of old versions while keeping newest - Single version packages (no removal) - Empty and nonexistent directories - Permission error handling - PEP 503 package name normalization - Non-.dist-info directory filtering - Invalid version format handling Tests use proper mocking to avoid filesystem operations and follow existing test patterns in the codebase. These tests verify the cleanup logic works correctly across various scenarios including edge cases like permission errors and malformed directory names. All tests use mocks to ensure fast, reliable execution without touching the actual filesystem. --- AddonManagerTest/app/test_python_deps.py | 167 +++++++++++++++++++++++ 1 file changed, 167 insertions(+) diff --git a/AddonManagerTest/app/test_python_deps.py b/AddonManagerTest/app/test_python_deps.py index e5bed28e..78b77b4d 100644 --- a/AddonManagerTest/app/test_python_deps.py +++ b/AddonManagerTest/app/test_python_deps.py @@ -268,3 +268,170 @@ 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 + self.assertIn("/fake/path/requests-2.28.0.dist-info", removed_paths) + self.assertIn("/fake/path/numpy-1.24.0.dist-info", removed_paths) + self.assertIn("/fake/path/numpy-1.25.2.dist-info", removed_paths) + + # Verify logging happened + self.assertEqual(mock_print_log.call_count, 3) + + @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("/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("/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("/fake/path/goodpackage-1.0.0.dist-info") From bd1b6ac52370aa99301c328ed6eec763469d9e67 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 5 Jan 2026 05:54:13 +0000 Subject: [PATCH 4/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- AddonManagerTest/app/test_python_deps.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/AddonManagerTest/app/test_python_deps.py b/AddonManagerTest/app/test_python_deps.py index 78b77b4d..6fd3f910 100644 --- a/AddonManagerTest/app/test_python_deps.py +++ b/AddonManagerTest/app/test_python_deps.py @@ -297,12 +297,12 @@ def test_cleanup_removes_old_versions_keeps_newest( # 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 self.assertIn("/fake/path/requests-2.28.0.dist-info", removed_paths) self.assertIn("/fake/path/numpy-1.24.0.dist-info", removed_paths) self.assertIn("/fake/path/numpy-1.25.2.dist-info", removed_paths) - + # Verify logging happened self.assertEqual(mock_print_log.call_count, 3) @@ -341,7 +341,7 @@ 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() @@ -363,7 +363,7 @@ def test_cleanup_handles_permission_error( ] mock_isdir.return_value = True mock_rmtree.side_effect = PermissionError("Permission denied") - + model = PythonPackageListModel([]) model.vendor_path = "/fake/path" model._cleanup_old_package_versions() @@ -380,7 +380,7 @@ def test_cleanup_normalizes_package_names( mock_exists.return_value = True mock_listdir.return_value = [ "my_package-1.0.0.dist-info", - "my-package-2.0.0.dist-info", + "my-package-2.0.0.dist-info", ] mock_isdir.return_value = True @@ -403,7 +403,7 @@ def test_cleanup_ignores_non_dist_info_directories( mock_listdir.return_value = [ "requests-2.28.0.dist-info", "requests-2.31.0.dist-info", - "some_package", + "some_package", "__pycache__", "random_file.txt", ] From b6cf067939d54ec36590a544c431c616554f0016 Mon Sep 17 00:00:00 2001 From: Kapale Om Shripad Date: Tue, 6 Jan 2026 09:43:05 +0530 Subject: [PATCH 5/6] Fix tests for Windows compatibility and remove logging assertion - Use os.path.join() for all path construction to ensure Windows compatibility - Remove logging count assertion as suggested in review - Tests now work across all platforms (Windows, Linux, macOS) --- AddonManagerTest/app/test_python_deps.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/AddonManagerTest/app/test_python_deps.py b/AddonManagerTest/app/test_python_deps.py index 6fd3f910..ba499cb8 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 @@ -298,13 +299,10 @@ def test_cleanup_removes_old_versions_keeps_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 - self.assertIn("/fake/path/requests-2.28.0.dist-info", removed_paths) - self.assertIn("/fake/path/numpy-1.24.0.dist-info", removed_paths) - self.assertIn("/fake/path/numpy-1.25.2.dist-info", removed_paths) - - # Verify logging happened - self.assertEqual(mock_print_log.call_count, 3) + # 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") @@ -389,7 +387,7 @@ def test_cleanup_normalizes_package_names( model._cleanup_old_package_versions() # Should remove old version (they're the same package after normalization) - mock_rmtree.assert_called_once_with("/fake/path/my_package-1.0.0.dist-info") + 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") @@ -412,7 +410,7 @@ def test_cleanup_ignores_non_dist_info_directories( model = PythonPackageListModel([]) model.vendor_path = "/fake/path" model._cleanup_old_package_versions() - mock_rmtree.assert_called_once_with("/fake/path/requests-2.28.0.dist-info") + 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") @@ -434,4 +432,4 @@ def test_cleanup_handles_invalid_version_format( model = PythonPackageListModel([]) model.vendor_path = "/fake/path" model._cleanup_old_package_versions() - mock_rmtree.assert_called_once_with("/fake/path/goodpackage-1.0.0.dist-info") + mock_rmtree.assert_called_once_with(os.path.join("/fake/path", "goodpackage-1.0.0.dist-info")) From f3cd818f79945d1ed207e45ff892a3c7c46a2504 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 6 Jan 2026 04:13:16 +0000 Subject: [PATCH 6/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- AddonManagerTest/app/test_python_deps.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/AddonManagerTest/app/test_python_deps.py b/AddonManagerTest/app/test_python_deps.py index ba499cb8..0de846c1 100644 --- a/AddonManagerTest/app/test_python_deps.py +++ b/AddonManagerTest/app/test_python_deps.py @@ -387,7 +387,9 @@ def test_cleanup_normalizes_package_names( 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")) + 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") @@ -410,7 +412,9 @@ def test_cleanup_ignores_non_dist_info_directories( 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")) + 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") @@ -432,4 +436,6 @@ def test_cleanup_handles_invalid_version_format( 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")) + mock_rmtree.assert_called_once_with( + os.path.join("/fake/path", "goodpackage-1.0.0.dist-info") + )