From e5166f5fabddc6fdcd934c7da511db3f6d899658 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 15:12:32 -0500 Subject: [PATCH 01/11] Keep the repo URL for sparse checkout in cache (cherry picked from commit e5b46d8173dbcb2523f999412effed355b69e630) --- Addon.py | 10 +++- AddonCatalog.py | 17 +++---- AddonManagerTest/app/test_addoncatalog.py | 60 +++++++++++++++++++++++ 3 files changed, 76 insertions(+), 11 deletions(-) diff --git a/Addon.py b/Addon.py index 3daad4d1..50cfcbaa 100644 --- a/Addon.py +++ b/Addon.py @@ -179,6 +179,12 @@ def __init__( self.display_name = self.name self.url = url.strip() self.relative_cache_path = "" + + # A remote location for a zip of this Addon's contents. This is used for Addons that are + # not cached in their entirety (typically due to their size). The canonical example here is + # the Parts Library. + self.zip_url = "" + self.branch = branch.strip() self.branch_display_name = branch.strip() self.repo_type = Addon.Kind.WORKBENCH @@ -688,7 +694,9 @@ def _find_classname_in_file(current_file) -> str: return "" def get_zip_url(self) -> str: - if self.url.endswith(".zip"): + if self.zip_url: + zip_url = self.zip_url + elif self.url.endswith(".zip"): zip_url = self.url else: # The ZIP url is based on the location of the main cache file: diff --git a/AddonCatalog.py b/AddonCatalog.py index ab930e97..03b7a90d 100644 --- a/AddonCatalog.py +++ b/AddonCatalog.py @@ -138,21 +138,18 @@ def instantiate_addon(self, addon_id: str) -> Addon: state = Addon.Status.UNCHECKED else: state = Addon.Status.NOT_INSTALLED - if self.sparse_cache: - if self.zip_url: - url = self.zip_url - else: - # Technically, this should never happen, but just in case... - raise RuntimeError(f"Sparse cache entry {addon_id} has no zip_url") - elif self.repository: - url = self.repository - else: - url = self.zip_url + if self.sparse_cache and not self.zip_url: + # Technically, this should never happen, but just in case... + raise RuntimeError(f"Sparse cache entry {addon_id} has no zip_url") + url = self.repository or self.zip_url or "" if self.git_ref: addon = Addon(addon_id, url, state, branch=self.git_ref) else: addon = Addon(addon_id, url, state) addon.relative_cache_path = self.relative_cache_path + if self.sparse_cache or not self.repository: + # If the cache is sparse, we need a "real" location to get the thing from when installing + addon.zip_url = self.zip_url or "" if self.metadata: try: diff --git a/AddonManagerTest/app/test_addoncatalog.py b/AddonManagerTest/app/test_addoncatalog.py index 41cc2405..d7e2eb0a 100644 --- a/AddonManagerTest/app/test_addoncatalog.py +++ b/AddonManagerTest/app/test_addoncatalog.py @@ -90,6 +90,66 @@ def test_version_match_with_min_and_max_bad_match_low(self): ) self.assertFalse(ac.is_compatible()) + def test_instantiate_addon_with_repository(self): + """An Addon that is cached by the Addon Manager keeps its repository as its URL, and has no + zip URL of its own: the cached copy is downloaded instead.""" + ac = self.AddonCatalogEntry( + { + "repository": "https://github.com/FreeCAD/FreeCAD", + "git_ref": "main", + "zip_url": "https://github.com/FreeCAD/FreeCAD/archive/main.zip", + "relative_cache_path": "AddonManager/AnAddon.zip", + } + ) + + addon = ac.instantiate_addon("AnAddon") + + self.assertEqual("https://github.com/FreeCAD/FreeCAD", addon.url) + self.assertEqual("", addon.zip_url) + + def test_instantiate_addon_with_sparse_cache(self): + """A sparsely-cached Addon must be downloaded from the catalog's zip, because only a + fraction of it is in the cache, but its URL is still the repository it came from, so that + file locations such as its README can be constructed from it.""" + ac = self.AddonCatalogEntry( + { + "repository": "https://github.com/FreeCAD/FreeCAD-library", + "git_ref": "master", + "zip_url": "https://github.com/FreeCAD/FreeCAD-library/archive/master.zip", + "sparse_cache": True, + "relative_cache_path": "AddonManager/parts_library.zip", + } + ) + + addon = ac.instantiate_addon("parts_library") + + self.assertEqual("https://github.com/FreeCAD/FreeCAD-library", addon.url) + self.assertEqual( + "https://github.com/FreeCAD/FreeCAD-library/archive/master.zip", addon.get_zip_url() + ) + + def test_instantiate_addon_with_sparse_cache_and_no_zip(self): + """A sparsely-cached Addon with no zip URL cannot be downloaded at all.""" + ac = self.AddonCatalogEntry( + { + "repository": "https://github.com/FreeCAD/FreeCAD-library", + "git_ref": "master", + "sparse_cache": True, + } + ) + + with self.assertRaises(RuntimeError): + ac.instantiate_addon("parts_library") + + def test_instantiate_addon_with_zip_only(self): + """An Addon with no repository is downloaded from the catalog's zip.""" + ac = self.AddonCatalogEntry({"zip_url": "https://example.com/an_addon.zip"}) + + addon = ac.instantiate_addon("AnAddon") + + self.assertEqual("https://example.com/an_addon.zip", addon.url) + self.assertEqual("https://example.com/an_addon.zip", addon.get_zip_url()) + class TestAddonCatalog(TestCase): """Tests for the AddonCatalog class.""" From 2349ee4b471c32a10ad482ec25611d6bdff06512 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 15:19:36 -0500 Subject: [PATCH 02/11] Don't use archive URL to construct file locations (cherry picked from commit f541b015e4eeda17d9a7ad557e2e677c38fd708d) --- AddonManagerTest/app/test_utilities.py | 28 ++++++ .../gui/test_readme_controller.py | 91 +++++++++++++++++++ addonmanager_readme_controller.py | 22 +++++ addonmanager_utilities.py | 18 +++- 4 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 AddonManagerTest/gui/test_readme_controller.py diff --git a/AddonManagerTest/app/test_utilities.py b/AddonManagerTest/app/test_utilities.py index 5b3c18ce..e087331d 100644 --- a/AddonManagerTest/app/test_utilities.py +++ b/AddonManagerTest/app/test_utilities.py @@ -46,6 +46,7 @@ get_zip_url, git_host_of, identify_git_host, + points_at_a_repository, pep503_normalize, process_date_string_to_python_datetime, recognized_git_location, @@ -154,6 +155,33 @@ def test_get_readme_html_url(self): repo = Addon("Test Repo", url, "Addon.Status.NOT_INSTALLED", "main") self.assertEqual(expected_result, get_readme_html_url(repo)) + def test_points_at_a_repository(self): + repository = Addon( + "Test Repo", "https://github.com/FreeCAD/FreeCAD", "Addon.Status.NOT_INSTALLED", "main" + ) + archive = Addon( + "Test Repo", + "https://github.com/FreeCAD/FreeCAD/archive/refs/heads/main.zip", + "Addon.Status.NOT_INSTALLED", + "main", + ) + + self.assertTrue(points_at_a_repository(repository)) + self.assertFalse(points_at_a_repository(archive)) + + def test_get_readme_url_of_an_archive(self): + """An Addon that is only distributed as a zip file has no repository to read a README + from, so no location is constructed for it.""" + repo = Addon( + "Test Repo", + "https://github.com/FreeCAD/FreeCAD/archive/refs/heads/main.zip", + "Addon.Status.NOT_INSTALLED", + "main", + ) + + self.assertEqual("", get_readme_url(repo)) + self.assertEqual("", get_readme_html_url(repo)) + def test_get_zip_url(self): expected_urls = { "https://github.com/FreeCAD/FreeCAD": "https://github.com/FreeCAD/FreeCAD/archive/main.zip", diff --git a/AddonManagerTest/gui/test_readme_controller.py b/AddonManagerTest/gui/test_readme_controller.py new file mode 100644 index 00000000..07e40931 --- /dev/null +++ b/AddonManagerTest/gui/test_readme_controller.py @@ -0,0 +1,91 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# SPDX-FileCopyrightText: 2026 FreeCAD Project Association +# SPDX-FileNotice: Part of the AddonManager. + +################################################################################ +# # +# This addon is free software: you can redistribute it and/or modify # +# it under the terms of the GNU Lesser General Public License as # +# published by the Free Software Foundation, either version 2.1 # +# of the License, or (at your option) any later version. # +# # +# This addon is distributed in the hope that it will be useful, # +# but WITHOUT ANY WARRANTY; without even the implied warranty # +# of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. # +# See the GNU Lesser General Public License for more details. # +# # +# You should have received a copy of the GNU Lesser General Public # +# License along with this addon. If not, see https://www.gnu.org/licenses # +# # +################################################################################ + +"""Tests for the ReadmeController class.""" + +import unittest +from unittest.mock import MagicMock, patch + +from Addon import Addon +from Widgets.addonmanager_widget_readme_browser import WidgetReadmeBrowser + + +class TestReadmeController(unittest.TestCase): + + def setUp(self): + self.network_patch = patch("NetworkManager.AM_NETWORK_MANAGER", MagicMock()) + self.mock_network_manager = self.network_patch.start() + self.initialize_patch = patch("NetworkManager.InitializeNetworkManager") + self.initialize_patch.start() + + from addonmanager_readme_controller import ReadmeController + + self.widget = WidgetReadmeBrowser() + self.controller = ReadmeController(self.widget) + + def tearDown(self): + self.widget.close() + del self.widget + self.initialize_patch.stop() + self.network_patch.stop() + + def test_addon_with_repository_downloads_its_readme(self): + """An Addon whose URL is a repository has its README located within that repository.""" + addon = Addon("TestAddon", "https://github.com/FreeCAD/FreeCAD", Addon.Status.NOT_INSTALLED) + addon.branch = "main" + + self.controller.set_addon(addon) + + self.mock_network_manager.submit_unmonitored_get.assert_called_once_with( + "https://github.com/FreeCAD/FreeCAD/raw/main/README.md" + ) + + def test_addon_without_repository_shows_what_is_known(self): + """An Addon that is only distributed as a zip file has no README location to download, so + the information that is available is displayed instead of a failed download.""" + addon = Addon("TestAddon", "https://example.com/test_addon.zip", Addon.Status.NOT_INSTALLED) + addon.description = "A description of the addon" + + self.controller.set_addon(addon) + + self.mock_network_manager.submit_unmonitored_get.assert_not_called() + self.assertIn("TestAddon", self.widget.toPlainText()) + self.assertIn("A description of the addon", self.widget.toPlainText()) + + def test_addon_without_repository_uses_readme_from_metadata(self): + """Even without a repository, a README location given in the Addon's metadata is used.""" + from addonmanager_metadata import Url, UrlType + + addon = Addon("TestAddon", "https://example.com/test_addon.zip", Addon.Status.NOT_INSTALLED) + addon.metadata = MagicMock() + addon.metadata.url = [ + Url(location="https://example.com/test_addon/README.md", type=UrlType.readme) + ] + + self.controller.set_addon(addon) + + self.mock_network_manager.submit_unmonitored_get.assert_called_once_with( + "https://example.com/test_addon/README.md" + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/addonmanager_readme_controller.py b/addonmanager_readme_controller.py index 7ea29572..f327fbec 100644 --- a/addonmanager_readme_controller.py +++ b/addonmanager_readme_controller.py @@ -143,6 +143,8 @@ def _create_full_url(self, url: str) -> str: def _create_markdown_url(self, file: str) -> str: base_url = utils.get_readme_html_url(self.addon) + if not base_url: + return file lhs, slash, _ = base_url.rpartition("/") return lhs + slash + file @@ -180,6 +182,22 @@ def _create_wiki_display(self): self.readme_data_type = ReadmeDataType.Markdown self.widget.setMarkdown(markdown) + def _create_missing_readme_display(self): + """Display what is known about an Addon whose README cannot be located: this happens when + the catalog provides a download for the Addon, but no repository to read files from, and + the Addon's metadata does not give a README location either.""" + + markdown = f"# {self.addon.display_name}\n\n" + if self.addon.description: + markdown += f"{self.addon.description}\n\n" + markdown += translate( + "AddonsInstaller", "No README information is available for this addon." + ) + self.widget.setUrl("") + self.readme_data = markdown + self.readme_data_type = ReadmeDataType.Markdown + self.widget.setMarkdown(markdown) + def _create_non_wiki_display(self): self.url = utils.get_readme_url(self.addon) if self.addon.metadata and self.addon.metadata.url: @@ -204,6 +222,10 @@ def _create_non_wiki_display(self): ) self.url = self.url.replace("/src/", "/raw/") + if not self.url: + self._create_missing_readme_display() + return + self.widget.setUrl(self.url) self.widget.setText( diff --git a/addonmanager_utilities.py b/addonmanager_utilities.py index ac939d0c..fbc3f437 100644 --- a/addonmanager_utilities.py +++ b/addonmanager_utilities.py @@ -444,15 +444,29 @@ def construct_git_url(repo, filename): return _format_url(_host_or_default(repo).raw_file, repo, filename) +def points_at_a_repository(repo) -> bool: + """Returns whether this repo's URL is the location of a git repository, rather than of a + downloadable archive of its contents. A catalog entry that only provides a zip file has no + repository for file locations to be constructed from.""" + + return not urlparse(repo.url).path.lower().endswith(".zip") + + def get_readme_url(repo): - """Returns the location of a readme file""" + """Returns the location of a readme file, or an empty string if there is no repository to + construct that location from""" + if not points_at_a_repository(repo): + return "" return construct_git_url(repo, "README.md") def get_readme_html_url(repo): - """Returns the location of a html file containing readme""" + """Returns the location of a html file containing readme, or an empty string if there is no + repository to construct that location from""" + if not points_at_a_repository(repo): + return "" return _format_url(_host_or_default(repo).blob, repo, "README.md") From c795a1f644c6e43fb948d4e416b20da27363739b Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 16:48:33 -0500 Subject: [PATCH 03/11] Move sparse cache setting to the Addon Index (cherry picked from commit f7d947f43d7630bad165ab839ab3a6a4563a230e) --- AddonCatalog.py | 2 +- AddonCatalog.schema.json | 3 + AddonCatalogCacheCreator.py | 35 ++++--- .../app/test_addon_catalog_cache_creator.py | 95 ++++++++++++++++++- 4 files changed, 116 insertions(+), 19 deletions(-) diff --git a/AddonCatalog.py b/AddonCatalog.py index 03b7a90d..3b21b0ac 100644 --- a/AddonCatalog.py +++ b/AddonCatalog.py @@ -81,7 +81,7 @@ class AddonCatalogEntry: metadata: Optional[CatalogEntryMetadata] = None # Generated by the cache system last_update_time: str = "" # Generated by the cache system curated: bool = True # Generated by the cache system - sparse_cache: bool = False # Generated by the cache system + sparse_cache: bool = False # Set by the catalog for Addons too large to cache in full relative_cache_path: str = "" # Generated by the cache system git_hash: Optional[str] = None # Generated by the cache system git_tag: Optional[str] = None # Generated by the cache system diff --git a/AddonCatalog.schema.json b/AddonCatalog.schema.json index 749fb282..76d31419 100644 --- a/AddonCatalog.schema.json +++ b/AddonCatalog.schema.json @@ -56,6 +56,9 @@ }, "curated": { "type": "boolean" + }, + "sparse_cache": { + "type": "boolean" } }, "anyOf": [ diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index b3df92e8..f6aa6f1f 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -55,9 +55,6 @@ 300 # Seconds: repos that take longer than this are assumed to be too large to index ) -# Repos that are too large, or that should for some reason not be fully cloned here -FORCE_SPARSE_CLONE = ["parts_library", "offline-documentation", "FreeCAD-Documentation-html"] - def recursive_serialize(obj: Any): """Recursively serialize an object, supporting non-dataclasses that themselves contain @@ -205,18 +202,8 @@ def create_local_copy_of_single_addon( self, addon_id: str, catalog_entries: List[AddonCatalog.AddonCatalogEntry] ): for index, catalog_entry in enumerate(catalog_entries): - if addon_id in FORCE_SPARSE_CLONE: - if catalog_entry.repository is None: - print( - f"ERROR: Cannot use sparse clone for {addon_id} because it has no git repo." - ) - continue - if catalog_entry.zip_url is None: - print( - f"ERROR: Cannot use sparse clone for {addon_id} because it has no zip URL." - ) - continue - catalog_entry.sparse_cache = True + catalog_entry.sparse_cache = self.should_use_sparse_clone(addon_id, catalog_entry) + if catalog_entry.sparse_cache: self.create_local_copy_of_single_addon_with_git_sparse( addon_id, index, catalog_entry ) @@ -236,6 +223,24 @@ def create_local_copy_of_single_addon( self.catalog.add_git_info_to_entry(addon_id, index, git_hash, git_tag) self.create_zip_of_entry(addon_id, index, catalog_entry) + def should_use_sparse_clone( + self, addon_id: str, catalog_entry: AddonCatalog.AddonCatalogEntry + ) -> bool: + """Whether to cache only the metadata files of this Addon, leaving clients to get the rest + of it from its zip URL. The catalog asks for this by setting "sparse_cache" on Addons that + are too large to cache in full, but it takes both a repository to clone the files from and + a zip URL for the clients to use, so an entry without those is cached normally.""" + + if not catalog_entry.sparse_cache: + return False + if catalog_entry.repository is None: + print(f"ERROR: Cannot use sparse clone for {addon_id} because it has no git repo.") + return False + if catalog_entry.zip_url is None: + print(f"ERROR: Cannot use sparse clone for {addon_id} because it has no zip URL.") + return False + return True + def get_git_info( self, addon_id: str, index: int, catalog_entry: AddonCatalog.AddonCatalogEntry ) -> Tuple[str | None, str | None]: diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index f9e07d16..2a1421a0 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py @@ -252,6 +252,89 @@ def test_generate_cache_entry_with_nothing_to_cache(self): def test_generate_cache_entry_with_approval(self): """If the addon appears in the catalog (as opposed to just the index), it gets marked as approved.""" + def test_should_use_sparse_clone_when_the_catalog_asks_for_it(self): + """A catalog entry that asks for a sparse cache and has everything needed for one gets + one, even though its addon is not in the hard-coded list.""" + ace = AddonCatalog.AddonCatalogEntry( + { + "repository": "https://some.url", + "git_ref": "main", + "zip_url": "zip", + "sparse_cache": True, + } + ) + writer = accc.CacheWriter() + self.assertTrue(writer.should_use_sparse_clone("SomeLargeAddon", ace)) + + def test_should_use_sparse_clone_without_a_zip_url(self): + """A sparse cache leaves clients to download the addon from its zip URL, so an entry + without one is cached normally instead.""" + ace = AddonCatalog.AddonCatalogEntry( + {"repository": "https://some.url", "git_ref": "main", "sparse_cache": True} + ) + writer = accc.CacheWriter() + self.assertFalse(writer.should_use_sparse_clone("SomeLargeAddon", ace)) + + def test_should_use_sparse_clone_without_a_repository(self): + """The metadata files of a sparse cache come from a git clone, so an entry without a + repository is cached normally instead.""" + ace = AddonCatalog.AddonCatalogEntry({"zip_url": "zip", "sparse_cache": True}) + writer = accc.CacheWriter() + self.assertFalse(writer.should_use_sparse_clone("SomeLargeAddon", ace)) + + def test_should_use_sparse_clone_for_a_normal_addon(self): + """An addon that does not ask for a sparse cache is cached in full.""" + ace = AddonCatalog.AddonCatalogEntry( + {"repository": "https://some.url", "git_ref": "main", "zip_url": "zip"} + ) + writer = accc.CacheWriter() + self.assertFalse(writer.should_use_sparse_clone("SomeNormalAddon", ace)) + + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse") + def test_create_local_copy_of_single_addon_using_sparse_clone(self, mock_create_with_sparse): + """An entry that asks for a sparse cache is fetched with a sparse clone, and is marked so + that clients know that only part of it is cached.""" + catalog_entries = [ + AddonCatalog.AddonCatalogEntry( + { + "repository": "https://some.url", + "git_ref": "main", + "zip_url": "zip", + "sparse_cache": True, + } + ), + ] + writer = accc.CacheWriter() + writer.catalog = MagicMock() + writer.cwd = os.path.abspath(os.path.join("home", "cache")) + + writer.create_local_copy_of_single_addon("SomeLargeAddon", catalog_entries) + + self.assertEqual(1, mock_create_with_sparse.call_count) + self.assertTrue(catalog_entries[0].sparse_cache) + + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse") + def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( + self, mock_create_with_sparse, mock_create_with_git + ): + """An entry that asks for a sparse cache but cannot have one is cached in full, and is not + marked as sparse: clients must not be told to look for a zip that does not exist.""" + catalog_entries = [ + AddonCatalog.AddonCatalogEntry( + {"repository": "https://some.url", "git_ref": "main", "sparse_cache": True} + ), + ] + writer = accc.CacheWriter() + writer.catalog = MagicMock() + writer.cwd = os.path.abspath(os.path.join("home", "cache")) + + writer.create_local_copy_of_single_addon("SomeLargeAddon", catalog_entries) + + self.assertEqual(0, mock_create_with_sparse.call_count) + self.assertEqual(1, mock_create_with_git.call_count) + self.assertFalse(catalog_entries[0].sparse_cache) + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git): """Given a single addon, each catalog entry is fetched with git if git info is available.""" @@ -312,9 +395,15 @@ def get_catalog(self): AddonCatalog.AddonCatalogEntry({"zip_url": "zip1"}), AddonCatalog.AddonCatalogEntry({"zip_url": "zip2"}), ], - accc.FORCE_SPARSE_CLONE[0]: [ - AddonCatalog.AddonCatalogEntry({"zip_url": "zip1"}), - AddonCatalog.AddonCatalogEntry({"zip_url": "zip2"}), + "TestMod3": [ + AddonCatalog.AddonCatalogEntry( + { + "repository": "https://some.url", + "git_ref": "main", + "zip_url": "zip1", + "sparse_cache": True, + } + ), ], } From a7c1bc4790be22e126511fa41977c3efd9d70a27 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 16:58:49 -0500 Subject: [PATCH 04/11] Enable the use of git when addons are very large (cherry picked from commit a68672f3f280c9097925d644837773fa7702bb20) --- Addon.py | 4 ++++ AddonCatalog.py | 4 ++++ AddonManagerTest/app/test_addoncatalog.py | 2 ++ AddonManagerTest/app/test_installer.py | 14 +++++++++++ AddonManagerTest/app/test_utilities.py | 29 +++++++++++++++++++++++ addonmanager_installer.py | 10 ++++---- addonmanager_update_all_gui.py | 7 +++--- addonmanager_utilities.py | 13 ++++++++++ 8 files changed, 74 insertions(+), 9 deletions(-) diff --git a/Addon.py b/Addon.py index 50cfcbaa..af545928 100644 --- a/Addon.py +++ b/Addon.py @@ -185,6 +185,10 @@ def __init__( # the Parts Library. self.zip_url = "" + # True for Addons that are large enough that downloading all of them for every update is + # expensive, so git is used for them whenever it is available. Set by the Addon Index. + self.prefer_git = False + self.branch = branch.strip() self.branch_display_name = branch.strip() self.repo_type = Addon.Kind.WORKBENCH diff --git a/AddonCatalog.py b/AddonCatalog.py index 3b21b0ac..7ab3901b 100644 --- a/AddonCatalog.py +++ b/AddonCatalog.py @@ -150,6 +150,10 @@ def instantiate_addon(self, addon_id: str) -> Addon: if self.sparse_cache or not self.repository: # If the cache is sparse, we need a "real" location to get the thing from when installing addon.zip_url = self.zip_url or "" + # If it's too big to cache, it's probably too big to want to update by re-downloading the + # whole thing. So if the user's machine has git on it, and we know its git repo, then tell + # the Addon Manager to try to use git when installing/updating. + addon.prefer_git = bool(self.sparse_cache and self.repository) if self.metadata: try: diff --git a/AddonManagerTest/app/test_addoncatalog.py b/AddonManagerTest/app/test_addoncatalog.py index d7e2eb0a..f14fd745 100644 --- a/AddonManagerTest/app/test_addoncatalog.py +++ b/AddonManagerTest/app/test_addoncatalog.py @@ -106,6 +106,7 @@ def test_instantiate_addon_with_repository(self): self.assertEqual("https://github.com/FreeCAD/FreeCAD", addon.url) self.assertEqual("", addon.zip_url) + self.assertFalse(addon.prefer_git) def test_instantiate_addon_with_sparse_cache(self): """A sparsely-cached Addon must be downloaded from the catalog's zip, because only a @@ -127,6 +128,7 @@ def test_instantiate_addon_with_sparse_cache(self): self.assertEqual( "https://github.com/FreeCAD/FreeCAD-library/archive/master.zip", addon.get_zip_url() ) + self.assertTrue(addon.prefer_git) def test_instantiate_addon_with_sparse_cache_and_no_zip(self): """A sparsely-cached Addon with no zip URL cannot be downloaded at all.""" diff --git a/AddonManagerTest/app/test_installer.py b/AddonManagerTest/app/test_installer.py index 1108cade..aa7cb466 100644 --- a/AddonManagerTest/app/test_installer.py +++ b/AddonManagerTest/app/test_installer.py @@ -251,6 +251,20 @@ def test_install_by_copy(self, manifest): readme = os.path.join(addon_name_dir, "README.md") self.assertTrue(os.path.exists(readme)) + def test_determine_install_method_for_a_large_addon(self): + """An Addon that is too large to cache in full is installed with git, when git is + available, so that later updates only have to fetch what changed.""" + + if not initialize_git(): + self.skipTest("git is not available") + self.real_addon.prefer_git = True + + installer = AddonInstaller(self.real_addon, []) + + self.assertIsNotNone(installer.git_manager) + method = installer._determine_install_method(self.real_addon.url, InstallationMethod.ANY) + self.assertEqual(InstallationMethod.GIT, method) + def test_determine_install_method_local_path(self): """Test which install methods are accepted for a local path""" diff --git a/AddonManagerTest/app/test_utilities.py b/AddonManagerTest/app/test_utilities.py index e087331d..432938b4 100644 --- a/AddonManagerTest/app/test_utilities.py +++ b/AddonManagerTest/app/test_utilities.py @@ -55,6 +55,7 @@ resolve_constraints_location, run_interruptable_subprocess, run_monitored_subprocess, + should_use_git, ProcessInterrupted, SubprocessTimeout, ) @@ -155,6 +156,34 @@ def test_get_readme_html_url(self): repo = Addon("Test Repo", url, "Addon.Status.NOT_INSTALLED", "main") self.assertEqual(expected_result, get_readme_html_url(repo)) + def test_should_use_git_for_a_normal_addon(self): + """An Addon that is neither large nor listed in the preference is downloaded as a zip.""" + repo = Addon( + "Test Repo", "https://github.com/FreeCAD/FreeCAD", "Addon.Status.NOT_INSTALLED", "main" + ) + with patch("addonmanager_utilities.fci.Preferences") as mock_preferences: + mock_preferences.return_value.get.return_value = "SomeOtherAddon" + self.assertFalse(should_use_git(repo)) + + def test_should_use_git_for_a_large_addon(self): + """An Addon that is too large to cache in full is updated with git.""" + repo = Addon( + "Test Repo", "https://github.com/FreeCAD/FreeCAD", "Addon.Status.NOT_INSTALLED", "main" + ) + repo.prefer_git = True + with patch("addonmanager_utilities.fci.Preferences") as mock_preferences: + mock_preferences.return_value.get.return_value = "SomeOtherAddon" + self.assertTrue(should_use_git(repo)) + + def test_should_use_git_when_the_user_asks_for_it(self): + """An Addon the user has listed in the preference is updated with git.""" + repo = Addon( + "Test Repo", "https://github.com/FreeCAD/FreeCAD", "Addon.Status.NOT_INSTALLED", "main" + ) + with patch("addonmanager_utilities.fci.Preferences") as mock_preferences: + mock_preferences.return_value.get.return_value = "SomeOtherAddon,Test Repo" + self.assertTrue(should_use_git(repo)) + def test_points_at_a_repository(self): repository = Addon( "Test Repo", "https://github.com/FreeCAD/FreeCAD", "Addon.Status.NOT_INSTALLED", "main" diff --git a/addonmanager_installer.py b/addonmanager_installer.py index 5c6944c5..a91920de 100644 --- a/addonmanager_installer.py +++ b/addonmanager_installer.py @@ -138,8 +138,7 @@ def __init__(self, addon: Addon, allow_list: List[str] = None): super().__init__() self.addon_to_install = addon - forced_repos = fci.Preferences().get("force_git_in_repos").split(",") - if addon and self.addon_to_install.name in forced_repos: + if addon and utils.should_use_git(addon): self.git_manager = initialize_git() else: self.git_manager = None @@ -238,9 +237,8 @@ def _determine_install_method( if not is_remote: return InstallationMethod.COPY - # Use git only if the user specifically requests it, and we have git - forced_repos = fci.Preferences().get("force_git_in_repos").split(",") - if self.git_manager and self.addon_to_install.name in forced_repos: + # Use git only for the Addons that call for it, and only if we have git + if self.git_manager and utils.should_use_git(self.addon_to_install): return InstallationMethod.GIT # Normal case: we aren't locked into any particular method, so use zip downloads from the @@ -266,6 +264,8 @@ def _can_use_update(self) -> bool: install_path = os.path.join(self.installation_path, self.addon_to_install.name) if not os.path.isdir(install_path): return False + if not os.path.isdir(os.path.join(install_path, ".git")): + return False # Installed some other way, most likely from a zip: re-clone it if addon.metadata is None or addon.installed_metadata is None: return True # We can't check if the branch name changed, but the install path exists old_branch = get_branch_from_metadata(self.addon_to_install.installed_metadata) diff --git a/addonmanager_update_all_gui.py b/addonmanager_update_all_gui.py index 8d134808..19c81c6e 100644 --- a/addonmanager_update_all_gui.py +++ b/addonmanager_update_all_gui.py @@ -29,6 +29,7 @@ from PySideWrapper import QtCore, QtWidgets import addonmanager_freecad_interface as fci +import addonmanager_utilities as utils from Addon import Addon, MissingDependencies from addonmanager_installer_gui import AddonDependencyInstallerGUI from addonmanager_installer import AddonInstaller, MacroInstaller @@ -106,9 +107,8 @@ def run(self): def query_sizes(self): """In the background, builds a list of the download sizes for all the addons being updated""" - forced_repos = fci.Preferences().get("force_git_in_repos").split(",") for addon in self.addons: - if addon.name in forced_repos: + if utils.should_use_git(addon): self.sizes_received += 1 continue zip_url = addon.get_zip_url() @@ -312,9 +312,8 @@ def check_for_git_migration(self): ] custom_repos_lines = fci.Preferences().get("CustomRepositories").split("\n") custom_repos = [line.split(" ")[0] for line in custom_repos_lines] - forced_repos = fci.Preferences().get("force_git_in_repos").split(",") for addon in addons_to_update: - if addon.name in custom_repos or addon.name in forced_repos: + if addon.name in custom_repos or utils.should_use_git(addon): continue path_to_addon = str(os.path.join(fci.DataPaths().mod_dir, addon.name)) path_to_git_directory = str(os.path.join(path_to_addon, ".git")) diff --git a/addonmanager_utilities.py b/addonmanager_utilities.py index fbc3f437..b8e09462 100644 --- a/addonmanager_utilities.py +++ b/addonmanager_utilities.py @@ -444,6 +444,19 @@ def construct_git_url(repo, filename): return _format_url(_host_or_default(repo).raw_file, repo, filename) +def should_use_git(repo) -> bool: + """Returns whether this Addon is installed and updated with git, rather than by downloading a + zip of its contents. Addons that the catalog flags as too large to cache in full always are, + because downloading all of a large Addon for every update is expensive; the rest only are if + the user has asked for it. Note that this says nothing about whether git is actually available: + the caller has to check that separately, and fall back to a zip download if it is not.""" + + if getattr(repo, "prefer_git", False): + return True + forced_repos = fci.Preferences().get("force_git_in_repos").split(",") + return repo.name in forced_repos + + def points_at_a_repository(repo) -> bool: """Returns whether this repo's URL is the location of a git repository, rather than of a downloadable archive of its contents. A catalog entry that only provides a zip file has no From 429b06edc725dabe340344f08515b4ab1b7539e8 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 17:00:50 -0500 Subject: [PATCH 05/11] Clean up after a cancelled subprocess (cherry picked from commit 44e987293624660883f774959dd60eafad5139ac) --- AddonManagerTest/app/test_utilities.py | 15 ++++++- addonmanager_utilities.py | 62 +++++++++++++++++++------- 2 files changed, 58 insertions(+), 19 deletions(-) diff --git a/AddonManagerTest/app/test_utilities.py b/AddonManagerTest/app/test_utilities.py index 432938b4..a18d8940 100644 --- a/AddonManagerTest/app/test_utilities.py +++ b/AddonManagerTest/app/test_utilities.py @@ -46,8 +46,8 @@ get_zip_url, git_host_of, identify_git_host, - points_at_a_repository, pep503_normalize, + points_at_a_repository, process_date_string_to_python_datetime, recognized_git_location, reload_git_hosts, @@ -66,10 +66,14 @@ class _FakeStream: def __init__(self, lines): self._lines = list(lines) + self.closed = False def readline(self): return self._lines.pop(0) if self._lines else "" + def close(self): + self.closed = True + class _FakeProcess: """A minimal Popen stand-in for exercising run_monitored_subprocess.""" @@ -78,6 +82,7 @@ def __init__(self, lines, returncode=0): self.stdout = _FakeStream(lines) self.returncode = returncode self.killed = False + self.pid = -1 # A real Popen has one, and the tree-killing code asks for it def wait(self): return self.returncode @@ -381,14 +386,20 @@ def test_run_monitored_subprocess_nonzero_exit_raises(self, mock_popen): with self.assertRaises(subprocess.CalledProcessError): run_monitored_subprocess(["pip", "install", "x"]) + # subprocess.run is patched as well as Popen: killing the process tree shells out to a system + # command, which a unit test must not really run against whatever holds that process ID + @patch("addonmanager_utilities.subprocess.run") @patch("subprocess.Popen") @patch("addonmanager_utilities._interruption_requested", return_value=True) - def test_run_monitored_subprocess_interruption_raises(self, _mock_interrupt, mock_popen): + def test_run_monitored_subprocess_interruption_raises( + self, _mock_interrupt, mock_popen, _mock_run + ): process = _FakeProcess(["Collecting x\n"], 0) mock_popen.return_value = process with self.assertRaises(ProcessInterrupted): run_monitored_subprocess(["pip", "install", "x"]) self.assertTrue(process.killed) + self.assertTrue(process.stdout.closed, "The output pipe was left open") def test_process_date_string_to_python_datetime_non_numeric(self): with self.assertRaises(ValueError): diff --git a/addonmanager_utilities.py b/addonmanager_utilities.py index b8e09462..17949e08 100644 --- a/addonmanager_utilities.py +++ b/addonmanager_utilities.py @@ -762,26 +762,31 @@ def run_monitored_subprocess( collected: List[str] = [] finished_reading = False - while not finished_reading: - try: - line = lines.get(timeout=0.2) - except queue.Empty: + try: + while not finished_reading: + try: + line = lines.get(timeout=0.2) + except queue.Empty: + if _interruption_requested(): + raise ProcessInterrupted() + continue + if line is None: + finished_reading = True + continue + collected.append(line) + if line_callback is not None: + line_callback(line.rstrip()) if _interruption_requested(): - _terminate(process, reader) raise ProcessInterrupted() - continue - if line is None: - finished_reading = True - continue - collected.append(line) - if line_callback is not None: - line_callback(line.rstrip()) - if _interruption_requested(): - _terminate(process, reader) - raise ProcessInterrupted() + except BaseException: + # Whatever went wrong, including a callback that raised, the process must not be left + # running: it holds files open and goes on doing work nobody is waiting for any more + _terminate(process, reader) + raise process.wait() reader.join() + process.stdout.close() output = "".join(collected) if process.returncode != 0: raise subprocess.CalledProcessError(process.returncode, args, output, "") @@ -800,9 +805,32 @@ def _enqueue_lines(stream, lines: "queue.Queue[Optional[str]]") -> None: def _terminate(process: subprocess.Popen, reader: threading.Thread) -> None: """Kill a process and wait for its reader thread to drain, so no output thread is left running after an interruption.""" - process.kill() + _kill_process_tree(process) process.wait() - reader.join() + # The reader is blocked reading the pipe, and it only reaches the end of it once every process + # holding the writing end has gone. A child that outlived its parent can hold it open for a + # long time, so this waits briefly and then closes the pipe itself rather than waiting forever. + reader.join(timeout=2.0) + process.stdout.close() + reader.join(timeout=2.0) + + +def _kill_process_tree(process: subprocess.Popen) -> None: + """Kill a process along with any children it started. Killing only the process itself leaves + its children running, and on Windows they are the ones that do the work for commands such as + git clone: they go on downloading, and they keep its output pipe open.""" + if sys.platform == "win32": + try: + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(process.pid)], + capture_output=True, + check=False, + creationflags=getattr(subprocess, "CREATE_NO_WINDOW", 0), + timeout=10, + ) + except (OSError, subprocess.SubprocessError): + pass # Fall through to killing just the process itself + process.kill() def process_date_string_to_python_datetime(date_string: str) -> datetime: From 4100897361d42225acb6a4067f3a58d94121e9d5 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 17:06:40 -0500 Subject: [PATCH 06/11] Add git progress messages to install dialog (cherry picked from commit e5a43c69356b2ba88e39ef4807ad9d3fa5def560) --- AddonManagerTest/app/mocks.py | 12 +---- AddonManagerTest/app/test_git.py | 19 +++++++ AddonManagerTest/app/test_installer.py | 21 ++++++++ AddonManagerTest/gui/test_installer_gui.py | 30 +++++++++++ addonmanager_git.py | 59 ++++++++++++++-------- addonmanager_installer.py | 26 +++++++++- addonmanager_installer_gui.py | 39 +++++++++++--- 7 files changed, 165 insertions(+), 41 deletions(-) diff --git a/AddonManagerTest/app/mocks.py b/AddonManagerTest/app/mocks.py index 0da8e255..849a54a0 100644 --- a/AddonManagerTest/app/mocks.py +++ b/AddonManagerTest/app/mocks.py @@ -243,19 +243,15 @@ def _check_for_failure(self): self.should_fail = False raise GitFailed("Unit test forced failure") - def clone(self, _remote, _local_path, _args: List[str] = None): + def clone(self, _remote, _local_path, _args: List[str] = None, line_callback=None): self.called_methods.append("clone") self._check_for_failure() - def async_clone(self, _remote, _local_path, _progress_monitor, _args: List[str] = None): - self.called_methods.append("async_clone") - self._check_for_failure() - def checkout(self, _local_path, _spec, _args: List[str] = None): self.called_methods.append("checkout") self._check_for_failure() - def update(self, _local_path): + def update(self, _local_path, line_callback=None): self.called_methods.append("update") self._check_for_failure() @@ -268,10 +264,6 @@ def reset(self, _local_path, _args: List[str] = None): self.called_methods.append("reset") self._check_for_failure() - def async_fetch_and_update(self, _local_path, _progress_monitor, _args=None): - self.called_methods.append("async_fetch_and_update") - self._check_for_failure() - def update_available(self, _local_path) -> bool: self.called_methods.append("update_available") self._check_for_failure() diff --git a/AddonManagerTest/app/test_git.py b/AddonManagerTest/app/test_git.py index 008c541c..5ffdc7ae 100644 --- a/AddonManagerTest/app/test_git.py +++ b/AddonManagerTest/app/test_git.py @@ -78,6 +78,25 @@ def test_clone(self): self.assertTrue(os.path.exists(os.path.join(checkout_dir, ".git"))) self.assertEqual(os.getcwd(), self.cwd, "We should be left in the same CWD we started") + def test_clone_reports_its_progress(self): + """Cloning a large repository takes a long time, so git is asked to report its progress + and each line of that report is handed over as it arrives.""" + checkout_dir = os.path.join(self.test_dir, "test_repo") + reported_lines = [] + + # --no-local stops git from taking the shortcut it takes for a local clone, so that it + # reports its progress the way it does when cloning an Addon from a remote host + self.git.clone( + self.test_repo_remote, checkout_dir, ["--no-local"], line_callback=reported_lines.append + ) + + self.assertTrue(os.path.exists(os.path.join(checkout_dir, ".git"))) + self.assertTrue(reported_lines, "Git did not report any progress at all") + self.assertTrue( + any("Receiving objects" in line for line in reported_lines), + f"Git did not report the progress of its download: {reported_lines}", + ) + def test_checkout(self): """Test git checkout""" checkout_dir = self._clone_test_repo() diff --git a/AddonManagerTest/app/test_installer.py b/AddonManagerTest/app/test_installer.py index aa7cb466..11a6f158 100644 --- a/AddonManagerTest/app/test_installer.py +++ b/AddonManagerTest/app/test_installer.py @@ -251,6 +251,27 @@ def test_install_by_copy(self, manifest): readme = os.path.join(addon_name_dir, "README.md") self.assertTrue(os.path.exists(readme)) + def test_report_git_progress(self): + """Each line of git's progress report is passed on as git worded it, with the percentage + it contains, so that a long clone can drive a progress bar.""" + installer = AddonInstaller(self.real_addon, []) + reported = [] + installer.progress_message.connect( + lambda message, percent: reported.append((message, percent)) + ) + + installer._report_git_progress("Receiving objects: 42% (5218/12345), 120.50 MiB\n") + installer._report_git_progress("Cloning into 'FreeCAD-library'...\n") + installer._report_git_progress(" \n") + + self.assertEqual( + [ + ("Receiving objects: 42% (5218/12345), 120.50 MiB", 42), + ("Cloning into 'FreeCAD-library'...", -1), + ], + reported, + ) + def test_determine_install_method_for_a_large_addon(self): """An Addon that is too large to cache in full is installed with git, when git is available, so that later updates only have to fetch what changed.""" diff --git a/AddonManagerTest/gui/test_installer_gui.py b/AddonManagerTest/gui/test_installer_gui.py index f3543daa..b45e9f6f 100644 --- a/AddonManagerTest/gui/test_installer_gui.py +++ b/AddonManagerTest/gui/test_installer_gui.py @@ -92,6 +92,7 @@ def moveToThread(self, thread): class MockInstaller(QtCore.QObject): progress_update = QtCore.Signal(int, int) + progress_message = QtCore.Signal(str, int) success = QtCore.Signal(object) failure = QtCore.Signal(object, str) finished = QtCore.Signal() @@ -121,6 +122,35 @@ def run_with_delay(self, delay_ms): def moveToThread(self, thread): self.moved_to_thread = True + def _installer_gui_with_dialog(self) -> AddonInstallerGUI: + """An AddonInstallerGUI with its progress dialog set up, as install() leaves it, but + without the installation itself running.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.create_installing_dialog() + self.addCleanup(gui.installing_dialog.close) + return gui + + def test_progress_message_shows_what_git_is_doing(self): + """A git clone reports its progress as text, which is shown as git worded it, with its + percentage driving the bar.""" + gui = self._installer_gui_with_dialog() + + gui._progress_message("Receiving objects: 42% (5218/12345)", 42) + + self.assertIn("Receiving objects", gui.installing_dialog.label.text()) + self.assertEqual(100, gui.installing_dialog.progressBar.maximum()) + self.assertEqual(42, gui.installing_dialog.progressBar.value()) + + def test_progress_message_without_a_percentage(self): + """A git report with no percentage in it leaves the bar alone rather than resetting it.""" + gui = self._installer_gui_with_dialog() + gui._progress_message("Receiving objects: 42% (5218/12345)", 42) + + gui._progress_message("Resolving deltas", -1) + + self.assertIn("Resolving deltas", gui.installing_dialog.label.text()) + self.assertEqual(42, gui.installing_dialog.progressBar.value()) + @patch("addonmanager_installer_gui.AddonDependencyInstallerGUI") @patch("addonmanager_installer_gui.MissingDependencies") def test_dependency_installer_launches( diff --git a/addonmanager_git.py b/addonmanager_git.py index 715ae5ff..3bce7693 100644 --- a/addonmanager_git.py +++ b/addonmanager_git.py @@ -27,7 +27,7 @@ import platform import shutil import subprocess -from typing import List, Dict, Optional +from typing import Callable, List, Dict, Optional import time import addonmanager_utilities as utils @@ -79,18 +79,23 @@ def __init__(self): if not self.git_exe: raise NoGitFound() - def clone(self, remote, local_path, args: List[str] = None): - """Clones the remote to the local path""" + def clone( + self, + remote, + local_path, + args: List[str] = None, + line_callback: Optional[Callable[[str], None]] = None, + ): + """Clones the remote to the local path. Cloning a large repository takes a long time, so if + a line_callback is given, git is asked to report its progress and each line of that report + is handed to the callback as it arrives.""" final_args = ["clone", "--recurse-submodules"] + if line_callback is not None: + final_args.append("--progress") if args: final_args.extend(args) final_args.extend([remote, local_path]) - self._synchronous_call_git(final_args) - - def async_clone(self, remote, local_path, progress_monitor, args: List[str] = None): - """Clones the remote to the local path, sending periodic progress updates - to the passed progress_monitor. Returns a handle that can be used to - cancel the job.""" + self._call_git(final_args, line_callback) def checkout(self, local_path, spec, args: List[str] = None): """Checks out a specific git revision, tag, or branch. Any valid argument to @@ -134,14 +139,18 @@ def detached_head(self, local_path: str) -> bool: os.chdir(old_dir) return result - def update(self, local_path): - """Fetches and pulls the local_path from its remote""" + def update(self, local_path, line_callback: Optional[Callable[[str], None]] = None): + """Fetches and pulls the local_path from its remote. As with clone, a line_callback is + given each line of git's progress report as it arrives.""" old_dir = os.getcwd() os.chdir(local_path) + progress = ["--progress"] if line_callback is not None else [] try: - self._synchronous_call_git(["fetch"]) - self._synchronous_call_git(["pull"]) - self._synchronous_call_git(["submodule", "update", "--init", "--recursive"]) + self._call_git(["fetch"] + progress, line_callback) + self._call_git(["pull"] + progress, line_callback) + self._call_git( + ["submodule", "update", "--init", "--recursive"] + progress, line_callback + ) except GitFailed as e: fci.Console.PrintWarning( translate( @@ -156,7 +165,7 @@ def update(self, local_path): "AddonsInstaller", "Backing up the original directory and re-cloning", ) - + "...\n" + + "…\n" ) remote = self.get_remote(local_path) with open(os.path.join(local_path, "ADDON_DISABLED"), "w", encoding="utf-8") as f: @@ -168,7 +177,7 @@ def update(self, local_path): ) os.chdir("..") os.rename(local_path, local_path + ".backup" + str(time.time())) - self.clone(remote, local_path) + self.clone(remote, local_path, line_callback=line_callback) os.chdir(old_dir) def status(self, local_path) -> str: @@ -198,9 +207,6 @@ def reset(self, local_path, args: List[str] = None): raise e os.chdir(old_dir) - def async_fetch_and_update(self, local_path, progress_monitor, args=None): - """Same as fetch_and_update, but asynchronous""" - def update_available(self, local_path) -> bool: """Returns True if an update is available from the remote, or false if not""" old_dir = os.getcwd() @@ -471,16 +477,27 @@ def _git_is_real() -> bool: def _synchronous_call_git(self, args: List[str]) -> str: """Calls git and returns its output.""" + return self._call_git(args, None) + + def _call_git(self, args: List[str], line_callback: Optional[Callable[[str], None]]) -> str: + """Calls git and returns its output. Without a line_callback the call has to finish within + a fixed timeout, which is fine for the many short-running git commands. With one, each line + of output is sent to the callback as it arrives, and there is no timeout: this is for + operations such as cloning a very large repository, which the user can cancel and wants to + see progress for, but which should not have a timeout.""" final_args = [self.git_exe] final_args.extend(args) try: - proc = utils.run_interruptable_subprocess(final_args) + if line_callback is None: + proc = utils.run_interruptable_subprocess(final_args) + else: + proc = utils.run_monitored_subprocess(final_args, line_callback=line_callback) except subprocess.CalledProcessError as e: raise GitFailed( f"Git returned a non-zero exit status: {e.returncode}\n" + f"Called with: {' '.join(final_args)}\n\n" - + f"Returned stderr:\n{e.stderr}" + + f"Returned stderr:\n{e.stderr if e.stderr else e.output}" ) from e except utils.ProcessInterrupted as e: raise GitFailed( diff --git a/addonmanager_installer.py b/addonmanager_installer.py index a91920de..96938ad5 100644 --- a/addonmanager_installer.py +++ b/addonmanager_installer.py @@ -26,6 +26,7 @@ from datetime import datetime, timezone from enum import IntEnum, auto import os +import re import shutil from typing import List, Optional import tempfile @@ -116,6 +117,13 @@ class AddonInstaller(QtCore.QObject): # number of bytes expected might be set to 0 to indicate an unknown download size. progress_update = QtCore.Signal(int, int) + # Signal: progress_message + # In GUI mode this signal is emitted during an installation whose progress is reported as text + # rather than as a byte count, which is how git reports what it is doing. The string is a + # human-readable description of the work in progress, and the integer is how far through that + # work we are, as a percentage, or -1 when the report did not include one. + progress_message = QtCore.Signal(str, int) + # Signals: success and failure # Emitted when the installation process is complete. The object emitted is the object that the # installation was requested for (usually of class Addon, but any class that provides a name, @@ -281,11 +289,15 @@ def _install_by_git(self) -> bool: install_path = str(os.path.join(self.installation_path, self.addon_to_install.name)) try: if self._can_use_update(): - self.git_manager.update(install_path) + self.git_manager.update(install_path, line_callback=self._report_git_progress) else: if os.path.isdir(install_path): utils.rmdir(install_path) - self.git_manager.clone(self.addon_to_install.url, install_path) + self.git_manager.clone( + self.addon_to_install.url, + install_path, + line_callback=self._report_git_progress, + ) self.git_manager.checkout(install_path, self.addon_to_install.branch) except GitFailed as e: self.failure.emit(self.addon_to_install, str(e)) @@ -293,6 +305,16 @@ def _install_by_git(self) -> bool: self._finalize_successful_installation() return True + def _report_git_progress(self, line: str) -> None: + """Pass a line of git's progress report on to whatever is displaying it. This is basically + all we can do to not appear stalled out when using git to install, there's no way of giving + a "real" progress bar. The percentage is sort of a lie here, but it's all we've got.""" + line = line.strip() + if not line: + return + percentage = re.search(r"(\d{1,3})%", line) + self.progress_message.emit(line, int(percentage.group(1)) if percentage else -1) + def _install_by_zip(self) -> bool: """Installs the specified url by downloading the file (if it is remote) and unzipping it into the appropriate installation location. If the GUI is running, the download is diff --git a/addonmanager_installer_gui.py b/addonmanager_installer_gui.py index d7f73b78..b902b64c 100644 --- a/addonmanager_installer_gui.py +++ b/addonmanager_installer_gui.py @@ -69,6 +69,7 @@ def __init__(self, addon: Addon, addons: List[Addon] = None): self.dependency_dialog = None self.dependency_installation_dialog = None self.installing_dialog = None + self.installation_message = "" self.worker_thread = None # Set up the installer connections @@ -121,25 +122,47 @@ def install(self) -> None: self.installer.moveToThread(self.worker_thread) self.installer.finished.connect(self.worker_thread.quit) self.installer.progress_update.connect(self._progress_update) + self.installer.progress_message.connect(self._progress_message) self.worker_thread.started.connect(self.installer.run) + self.create_installing_dialog() + self.installer.finished.connect(self.installing_dialog.hide) + self.installing_dialog.show() + self.worker_thread.start() # Returns immediately + + def create_installing_dialog(self) -> None: + """Create the dialog that reports the progress of the installation.""" self.installing_dialog = fci.loadUi(os.path.join(os.path.dirname(__file__), "progress.ui")) self.installing_dialog.setObjectName("AddonManager_InstallingDialog") - self.installing_dialog.label.setText( - translate("AddonsInstaller", "Installing '{}'").format( - self.addon_to_install.display_name - ) + self.installation_message = translate("AddonsInstaller", "Installing '{}'").format( + self.addon_to_install.display_name ) - + self.installing_dialog.label.setText(self.installation_message) + # Git's progress reports are long: give the label enough room to show both the activity + # one names and the transfer rate it ends with + self.installing_dialog.label.setMinimumWidth(560) self.installing_dialog.rejected.connect(self._cancel_addon_installation) - self.installer.finished.connect(self.installing_dialog.hide) - self.installing_dialog.show() - self.worker_thread.start() # Returns immediately def _progress_update(self, bytes_read: int, data_size: int) -> None: self.installing_dialog.progressBar.setMaximum(data_size) self.installing_dialog.progressBar.setValue(bytes_read) + def _progress_message(self, message: str, percentage: int) -> None: + """Show what an installation that reports its progress as text, as git does, is doing.""" + if percentage >= 0: + self.installing_dialog.progressBar.setMaximum(100) + self.installing_dialog.progressBar.setValue(percentage) + self._set_installation_detail(message) + + def _set_installation_detail(self, detail: str) -> None: + """Show what the installation is doing right now, on a line of its own below the name of + the Addon being installed. Git's reports in particular are long, so the detail is elided + in the middle, keeping both the activity it names and the numbers it ends with.""" + label = self.installing_dialog.label + available_width = max(label.width(), label.minimumWidth()) + elided = label.fontMetrics().elidedText(detail, QtCore.Qt.ElideMiddle, available_width) + label.setText(f"{self.installation_message}\n{elided}") + def _cancel_addon_installation(self): dlg = QtWidgets.QMessageBox( QtWidgets.QMessageBox.NoIcon, From eb79dd7953e72ace96f9775482a1a58d7fa674bb Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 17:10:19 -0500 Subject: [PATCH 07/11] Fix cancellation messaging (cherry picked from commit bc83c3d6bbf5ff6f0a65accb946a2f9d385be7c8) --- AddonManagerTest/app/mocks.py | 9 ++++++--- AddonManagerTest/app/test_git.py | 21 ++++++++++++++++++++- AddonManagerTest/app/test_installer.py | 19 ++++++++++++++++++- addonmanager_git.py | 12 +++++++++++- addonmanager_installer.py | 9 +++++++-- 5 files changed, 62 insertions(+), 8 deletions(-) diff --git a/AddonManagerTest/app/mocks.py b/AddonManagerTest/app/mocks.py index 849a54a0..2de3a7eb 100644 --- a/AddonManagerTest/app/mocks.py +++ b/AddonManagerTest/app/mocks.py @@ -26,9 +26,9 @@ import os from typing import List - -class GitFailed(RuntimeError): - pass +# The real exception types, so that code under test catches what this mock raises. Importing them +# does not require git to be installed: only constructing a real GitManager does. +from addonmanager_git import GitFailed, GitCancelled class MockConsole: @@ -236,8 +236,11 @@ def __init__(self): self.get_last_authors_response = {"Jane Doe": {"email": "jdoe@freecad.org", "count": 1}} self.should_fail = False self.fail_once = False # Switch back to success after the simulated failure + self.should_be_interrupted = False # Emulate the user cancelling the operation def _check_for_failure(self): + if self.should_be_interrupted: + raise GitCancelled("Unit test forced interruption") if self.should_fail: if self.fail_once: self.should_fail = False diff --git a/AddonManagerTest/app/test_git.py b/AddonManagerTest/app/test_git.py index 5ffdc7ae..da6c7037 100644 --- a/AddonManagerTest/app/test_git.py +++ b/AddonManagerTest/app/test_git.py @@ -28,7 +28,8 @@ import time from zipfile import ZipFile -from addonmanager_git import GitManager, NoGitFound, GitFailed +from addonmanager_git import GitManager, NoGitFound, GitFailed, GitCancelled +import addonmanager_utilities as utils try: git_manager = GitManager() @@ -97,6 +98,24 @@ def test_clone_reports_its_progress(self): f"Git did not report the progress of its download: {reported_lines}", ) + def test_cancelled_update_leaves_the_checkout_alone(self): + """A failed update backs the checkout up and re-clones it, but a cancelled one must not: + the user asked for the work to stop, not for their installed copy to be replaced.""" + checkout_dir = self._clone_test_repo() + + def cancel_the_update(_line): + raise utils.ProcessInterrupted() + + with self.assertRaises(GitCancelled): + self.git.update(checkout_dir, line_callback=cancel_the_update) + + self.assertTrue(os.path.exists(os.path.join(checkout_dir, ".git"))) + self.assertFalse( + os.path.exists(os.path.join(checkout_dir, "ADDON_DISABLED")), + "A cancelled update disabled the addon and re-cloned it", + ) + self.assertEqual(os.getcwd(), self.cwd, "We should be left in the same CWD we started") + def test_checkout(self): """Test git checkout""" checkout_dir = self._clone_test_repo() diff --git a/AddonManagerTest/app/test_installer.py b/AddonManagerTest/app/test_installer.py index 11a6f158..a8ba5075 100644 --- a/AddonManagerTest/app/test_installer.py +++ b/AddonManagerTest/app/test_installer.py @@ -31,8 +31,9 @@ from addonmanager_installer import InstallationMethod, AddonInstaller, MacroInstaller from addonmanager_git import initialize_git from addonmanager_metadata import MetadataReader +from addonmanager_utilities import ProcessInterrupted from Addon import Addon -from AddonManagerTest.app.mocks import MockAddon, MockMacro +from AddonManagerTest.app.mocks import MockAddon, MockGitManager, MockMacro class TestAddonInstaller(unittest.TestCase): @@ -251,6 +252,22 @@ def test_install_by_copy(self, manifest): readme = os.path.join(addon_name_dir, "README.md") self.assertTrue(os.path.exists(readme)) + def test_cancelling_a_git_installation_is_not_a_failure(self): + """Cancelling is something the user asked for, so it is reported as an interruption and + not as a failed installation: the user should not be shown an error for it.""" + installer = AddonInstaller(self.real_addon, []) + installer.git_manager = MockGitManager() + installer.git_manager.should_be_interrupted = True + failures = [] + installer.failure.connect(lambda addon, message: failures.append(message)) + + with tempfile.TemporaryDirectory() as temp_dir: + installer.installation_path = temp_dir + with self.assertRaises(ProcessInterrupted): + installer._install_by_git() + + self.assertEqual([], failures, "Cancelling reported an installation failure") + def test_report_git_progress(self): """Each line of git's progress report is passed on as git worded it, with the percentage it contains, so that a long clone can drive a progress bar.""" diff --git a/addonmanager_git.py b/addonmanager_git.py index 3bce7693..61593cc5 100644 --- a/addonmanager_git.py +++ b/addonmanager_git.py @@ -44,6 +44,12 @@ class GitFailed(RuntimeError): """The call to git returned an error of some kind""" +class GitCancelled(GitFailed): + """The call to git did not finish because the user cancelled it. It is a kind of GitFailed so + that existing handlers still catch it, but nothing that repairs a failed call should try to + repair this one: the user asked for the work to stop, not to be done differently.""" + + def _ref_format_string() -> str: return ( "--format=%(refname:lstrip=2)\t%(upstream:lstrip=2)\t%(authordate:rfc)\t%(" @@ -151,6 +157,10 @@ def update(self, local_path, line_callback: Optional[Callable[[str], None]] = No self._call_git( ["submodule", "update", "--init", "--recursive"] + progress, line_callback ) + except GitCancelled: + # The user cancelled: leave their installed copy exactly as it was found + os.chdir(old_dir) + raise except GitFailed as e: fci.Console.PrintWarning( translate( @@ -500,7 +510,7 @@ def _call_git(self, args: List[str], line_callback: Optional[Callable[[str], Non + f"Returned stderr:\n{e.stderr if e.stderr else e.output}" ) from e except utils.ProcessInterrupted as e: - raise GitFailed( + raise GitCancelled( "The git process was interrupted due to a network timeout (or explicit user cancellation)\n" + f"Called with: {' '.join(final_args)}\n" ) from e diff --git a/addonmanager_installer.py b/addonmanager_installer.py index 96938ad5..46b9a1df 100644 --- a/addonmanager_installer.py +++ b/addonmanager_installer.py @@ -42,7 +42,7 @@ from addonmanager_python_constraints import get_constraints from addonmanager_installation_manifest import InstallationManifest from addonmanager_metadata import get_branch_from_metadata -from addonmanager_git import initialize_git, GitFailed +from addonmanager_git import initialize_git, GitFailed, GitCancelled from addonmanager_icon_utilities import get_icon_for_addon if fci.FreeCADGui: @@ -299,6 +299,9 @@ def _install_by_git(self) -> bool: line_callback=self._report_git_progress, ) self.git_manager.checkout(install_path, self.addon_to_install.branch) + except GitCancelled as e: + # Cancelling is not a failure, so report it like a normal interrupted process + raise utils.ProcessInterrupted() from e except GitFailed as e: self.failure.emit(self.addon_to_install, str(e)) return False @@ -346,7 +349,9 @@ def _run_zip_downloader_in_event_loop(self, zip_url: str): self.zip_download_index = NetworkManager.AM_NETWORK_MANAGER.submit_monitored_get(zip_url) while self.zip_download_index is not None: if QtCore.QThread.currentThread().isInterruptionRequested(): - break + NetworkManager.AM_NETWORK_MANAGER.abort(self.zip_download_index) + self.zip_download_index = None + raise utils.ProcessInterrupted() QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents, 50) def _update_zip_status(self, index: int, bytes_read: int, data_size: int): From ab3c2092acf36a7394eaea6a3f887900aea974a2 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 17:12:35 -0500 Subject: [PATCH 08/11] Fix progress bar startup and labeling (cherry picked from commit ab851804fa77d2f4391e39aa8a6d462e6a81e6f3) --- AddonManagerTest/app/test_installer.py | 15 +++++ AddonManagerTest/gui/test_installer_gui.py | 65 ++++++++++++++++++++++ addonmanager_installer.py | 7 +++ addonmanager_installer_gui.py | 45 ++++++++++++++- 4 files changed, 129 insertions(+), 3 deletions(-) diff --git a/AddonManagerTest/app/test_installer.py b/AddonManagerTest/app/test_installer.py index a8ba5075..53f69b06 100644 --- a/AddonManagerTest/app/test_installer.py +++ b/AddonManagerTest/app/test_installer.py @@ -268,6 +268,21 @@ def test_cancelling_a_git_installation_is_not_a_failure(self): self.assertEqual([], failures, "Cancelling reported an installation failure") + def test_will_use_git(self): + """Callers can ask what the installation is going to do before it starts.""" + if not initialize_git(): + self.skipTest("git is not available") + self.real_addon.prefer_git = True + installer = AddonInstaller(self.real_addon, []) + + self.assertTrue(installer.will_use_git()) + + def test_will_use_git_for_a_normal_addon(self): + """An Addon that is not flagged for git is downloaded as a zip.""" + installer = AddonInstaller(self.real_addon, []) + + self.assertFalse(installer.will_use_git()) + def test_report_git_progress(self): """Each line of git's progress report is passed on as git worded it, with the percentage it contains, so that a long clone can drive a progress bar.""" diff --git a/AddonManagerTest/gui/test_installer_gui.py b/AddonManagerTest/gui/test_installer_gui.py index b45e9f6f..d6b93b22 100644 --- a/AddonManagerTest/gui/test_installer_gui.py +++ b/AddonManagerTest/gui/test_installer_gui.py @@ -130,6 +130,71 @@ def _installer_gui_with_dialog(self) -> AddonInstallerGUI: self.addCleanup(gui.installing_dialog.close) return gui + def test_dialog_titles_say_which_operation_is_happening(self): + """The dialog says whether it is installing or updating, matching the button the user + pressed, rather than the generic title the shared .ui file carries.""" + being_installed = Addon("Test Addon") + being_installed.set_status(Addon.Status.NOT_INSTALLED) + installing = AddonInstallerGUI(being_installed) + installing.create_installing_dialog() + self.addCleanup(installing.installing_dialog.close) + + being_updated = Addon("Test Addon") + being_updated.set_status(Addon.Status.UPDATE_AVAILABLE) + updating = AddonInstallerGUI(being_updated) + updating.create_installing_dialog() + self.addCleanup(updating.installing_dialog.close) + + self.assertIn("Installing", installing.installing_dialog.windowTitle()) + self.assertIn("Installing", installing.installing_dialog.label.text()) + self.assertIn("Updating", updating.installing_dialog.windowTitle()) + self.assertIn("Updating", updating.installing_dialog.label.text()) + + def test_dialog_says_when_git_is_being_used(self): + """Installing with git is slower than downloading a zip, so the dialog explains why it is + worth the wait.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.installer.will_use_git = lambda: True + gui.create_installing_dialog() + self.addCleanup(gui.installing_dialog.close) + + self.assertIn("git", gui.installing_dialog.label.text()) + self.assertIn("Test Addon", gui.installing_dialog.label.text()) + + def test_dialog_does_not_mention_git_for_a_zip_install(self): + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.installer.will_use_git = lambda: False + gui.create_installing_dialog() + self.addCleanup(gui.installing_dialog.close) + + self.assertNotIn("git", gui.installing_dialog.label.text()) + + def test_progress_update_shows_how_much_has_been_downloaded(self): + """A download of an Addon that is gigabytes in size says so, rather than only moving a + bar that gives no sense of how long the wait will be.""" + gui = self._installer_gui_with_dialog() + + gui._progress_update(150_000_000, 2_100_000_000) + + # Qt formats the sizes themselves, in the units and the notation of the user's locale + locale = QtCore.QLocale() + received = locale.formattedDataSize(150_000_000) + total = locale.formattedDataSize(2_100_000_000) + self.assertIn(f"{received} of {total}", gui.installing_dialog.label.text()) + self.assertEqual(2_100_000_000, gui.installing_dialog.progressBar.maximum()) + self.assertEqual(150_000_000, gui.installing_dialog.progressBar.value()) + + def test_progress_update_of_an_unknown_download_size(self): + """When the server does not say how large the download is, the amount received so far is + still shown.""" + gui = self._installer_gui_with_dialog() + + gui._progress_update(150_000_000, 0) + + received = QtCore.QLocale().formattedDataSize(150_000_000) + self.assertIn(received, gui.installing_dialog.label.text()) + self.assertNotIn(" of ", gui.installing_dialog.label.text()) + def test_progress_message_shows_what_git_is_doing(self): """A git clone reports its progress as text, which is shown as git worded it, with its percentage driving the bar.""" diff --git a/addonmanager_installer.py b/addonmanager_installer.py index 46b9a1df..e5ac7597 100644 --- a/addonmanager_installer.py +++ b/addonmanager_installer.py @@ -200,6 +200,13 @@ def run(self, install_method: InstallationMethod = InstallationMethod.ANY) -> bo self.finished.emit() return success + def will_use_git(self, install_method: InstallationMethod = InstallationMethod.ANY) -> bool: + """Whether running this installer will use git, so that callers can say so before the + installation starts.""" + + addon_url = self.addon_to_install.url.replace(os.path.sep, "/") + return self._determine_install_method(addon_url, install_method) == InstallationMethod.GIT + def _determine_install_method( self, addon_url: str, install_method: InstallationMethod ) -> Optional[InstallationMethod]: diff --git a/addonmanager_installer_gui.py b/addonmanager_installer_gui.py index b902b64c..35f06514 100644 --- a/addonmanager_installer_gui.py +++ b/addonmanager_installer_gui.py @@ -130,22 +130,61 @@ def install(self) -> None: self.installing_dialog.show() self.worker_thread.start() # Returns immediately + def _is_an_update(self) -> bool: + """Whether the Addon is already installed, so that the dialogs can say that they are + updating it rather than installing it. This is the same question the details view asks to + decide which button to offer, so the dialog agrees with the button that opened it.""" + return self.addon_to_install.status() != Addon.Status.NOT_INSTALLED + def create_installing_dialog(self) -> None: """Create the dialog that reports the progress of the installation.""" self.installing_dialog = fci.loadUi(os.path.join(os.path.dirname(__file__), "progress.ui")) self.installing_dialog.setObjectName("AddonManager_InstallingDialog") - self.installation_message = translate("AddonsInstaller", "Installing '{}'").format( - self.addon_to_install.display_name - ) + name = self.addon_to_install.display_name + if self._is_an_update(): + self.installing_dialog.setWindowTitle( + translate("AddonsInstaller", "Updating Addon", "Window title") + ) + if self.installer.will_use_git(): + self.installation_message = translate( + "AddonsInstaller", "Updating '{}' with git, so only the changes are downloaded" + ).format(name) + else: + self.installation_message = translate("AddonsInstaller", "Updating '{}'").format( + name + ) + else: + self.installing_dialog.setWindowTitle( + translate("AddonsInstaller", "Installing Addon", "Window title") + ) + if self.installer.will_use_git(): + self.installation_message = translate( + "AddonsInstaller", "Installing '{}' with git (for more efficient updating)" + ).format(name) + else: + self.installation_message = translate("AddonsInstaller", "Installing '{}'").format( + name + ) self.installing_dialog.label.setText(self.installation_message) # Git's progress reports are long: give the label enough room to show both the activity # one names and the transfer rate it ends with self.installing_dialog.label.setMinimumWidth(560) + self.installing_dialog.progressBar.setRange(0, 0) # Start in indeterminate mode self.installing_dialog.rejected.connect(self._cancel_addon_installation) def _progress_update(self, bytes_read: int, data_size: int) -> None: + """Show how much of a download has arrived. A data_size of zero means the server did not + say how large the download is, so only the amount received so far can be shown.""" self.installing_dialog.progressBar.setMaximum(data_size) self.installing_dialog.progressBar.setValue(bytes_read) + locale = QtCore.QLocale() + if data_size > 0: + amount = translate("AddonsInstaller", "{} of {}").format( + locale.formattedDataSize(bytes_read), locale.formattedDataSize(data_size) + ) + else: + amount = locale.formattedDataSize(bytes_read) + self._set_installation_detail(amount) def _progress_message(self, message: str, percentage: int) -> None: """Show what an installation that reports its progress as text, as git does, is doing.""" From e06412e106a3169e93289120806495111bc8d251 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 17:14:14 -0500 Subject: [PATCH 09/11] Explain what's happening after cancelling an install (cherry picked from commit 75a75ff41bfc987f2e357769c043783311ee77bc) --- AddonManagerTest/gui/test_installer_gui.py | 39 ++++++++++++ addonmanager_installer_gui.py | 73 ++++++++++++++++++---- 2 files changed, 99 insertions(+), 13 deletions(-) diff --git a/AddonManagerTest/gui/test_installer_gui.py b/AddonManagerTest/gui/test_installer_gui.py index d6b93b22..28d46139 100644 --- a/AddonManagerTest/gui/test_installer_gui.py +++ b/AddonManagerTest/gui/test_installer_gui.py @@ -150,6 +150,16 @@ def test_dialog_titles_say_which_operation_is_happening(self): self.assertIn("Updating", updating.installing_dialog.windowTitle()) self.assertIn("Updating", updating.installing_dialog.label.text()) + def test_cancelling_dialog_says_which_operation_is_being_stopped(self): + being_updated = Addon("Test Addon") + being_updated.set_status(Addon.Status.UPDATE_AVAILABLE) + gui = AddonInstallerGUI(being_updated) + + gui.create_cancelling_dialog() + self.addCleanup(gui.cancelling_dialog.close) + + self.assertIn("update", gui.cancelling_dialog.label.text()) + def test_dialog_says_when_git_is_being_used(self): """Installing with git is slower than downloading a zip, so the dialog explains why it is worth the wait.""" @@ -169,6 +179,35 @@ def test_dialog_does_not_mention_git_for_a_zip_install(self): self.assertNotIn("git", gui.installing_dialog.label.text()) + def test_cancelling_dialog_shows_that_work_is_going_on(self): + """Stopping a large installation takes time, so the dialog animates and offers no button: + a fixed sentence next to an OK button reads as a hang.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.create_cancelling_dialog() + self.addCleanup(gui.cancelling_dialog.close) + + self.assertIn("Test Addon", gui.cancelling_dialog.label.text()) + self.assertEqual(0, gui.cancelling_dialog.progressBar.minimum()) + self.assertEqual(0, gui.cancelling_dialog.progressBar.maximum()) + self.assertTrue(gui.cancelling_dialog.buttonBox.isHidden()) + + def test_removing_a_partial_installation_keeps_the_interface_alive(self): + """The deletion happens off the GUI thread, so the dialog can say what it is doing and go + on repainting while it happens.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.create_cancelling_dialog() + self.addCleanup(gui.cancelling_dialog.close) + with tempfile.TemporaryDirectory() as temp_dir: + partial_download = os.path.join(temp_dir, "partial") + os.makedirs(os.path.join(partial_download, "subdirectory")) + with open(os.path.join(partial_download, "subdirectory", "file"), "w") as f: + f.write("downloaded so far") + + gui._remove_partial_installation(partial_download) + + self.assertFalse(os.path.exists(partial_download)) + self.assertIn("Removing", gui.cancelling_dialog.label.text()) + def test_progress_update_shows_how_much_has_been_downloaded(self): """A download of an Addon that is gigabytes in size says so, rather than only moving a bar that gives no sense of how long the wait will be.""" diff --git a/addonmanager_installer_gui.py b/addonmanager_installer_gui.py index 35f06514..5b328728 100644 --- a/addonmanager_installer_gui.py +++ b/addonmanager_installer_gui.py @@ -44,6 +44,19 @@ # pylint: disable=c-extension-no-member,too-few-public-methods,too-many-instance-attributes +class DirectoryRemover(QtCore.QThread): + """Deletes a directory and everything in it, off the calling thread. Removing a partly + downloaded Addon can take minutes when the Addon is a large one, which is far too long to + stop the interface from repainting.""" + + def __init__(self, path: str): + super().__init__() + self.path = path + + def run(self): + utils.rmdir(self.path) + + class AddonInstallerGUI(QtCore.QObject): """GUI functions (sequence of dialog boxes) for installing an addon interactively. The actual installation is handled by the AddonInstaller class running in a separate QThread. An instance @@ -69,6 +82,7 @@ def __init__(self, addon: Addon, addons: List[Addon] = None): self.dependency_dialog = None self.dependency_installation_dialog = None self.installing_dialog = None + self.cancelling_dialog = None self.installation_message = "" self.worker_thread = None @@ -202,18 +216,28 @@ def _set_installation_detail(self, detail: str) -> None: elided = label.fontMetrics().elidedText(detail, QtCore.Qt.ElideMiddle, available_width) label.setText(f"{self.installation_message}\n{elided}") + def create_cancelling_dialog(self) -> None: + """Create the dialog shown while an installation is being stopped. Both stopping the work + and clearing up after it can take a long time for a large Addon, so this dialog says which + of the two is happening and animates while it does, rather than presenting a fixed sentence + and a button that does nothing.""" + self.cancelling_dialog = fci.loadUi(os.path.join(os.path.dirname(__file__), "progress.ui")) + self.cancelling_dialog.setObjectName("AddonInstaller_CancellingDialog") + self.cancelling_dialog.setWindowTitle(translate("AddonsInstaller", "Cancelling")) + if self._is_an_update(): + message = translate("AddonsInstaller", "Cancelling the update of '{}'…") + else: + message = translate("AddonsInstaller", "Cancelling the installation of '{}'…") + self.cancelling_dialog.label.setText(message.format(self.addon_to_install.display_name)) + self.cancelling_dialog.label.setMinimumWidth(560) + self.cancelling_dialog.progressBar.setRange(0, 0) # Indeterminate: this has no known length + # There is nothing to offer the user here: the cancellation cannot itself be cancelled + self.cancelling_dialog.buttonBox.hide() + def _cancel_addon_installation(self): - dlg = QtWidgets.QMessageBox( - QtWidgets.QMessageBox.NoIcon, - translate("AddonsInstaller", "Cancelling"), - translate("AddonsInstaller", "Cancelling installation of '{}'").format( - self.addon_to_install.display_name - ), - QtWidgets.QMessageBox.NoButton, - parent=utils.get_main_am_window(), - ) - dlg.setObjectName("AddonInstaller_CancellingDialog") - dlg.show() + self.create_cancelling_dialog() + self.cancelling_dialog.show() + QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents) if self.worker_thread.isRunning(): # Interruption can take a second or more, depending on what was being done. Make sure # we stay responsive and update the dialog with the text above, etc. @@ -224,10 +248,33 @@ def _cancel_addon_installation(self): QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents) path = str(os.path.join(self.installer.installation_path, self.addon_to_install.name)) if os.path.exists(path): - utils.rmdir(path) - dlg.hide() + self._remove_partial_installation(path) + self.cancelling_dialog.hide() self.finished.emit() + def _remove_partial_installation(self, path: str) -> None: + """Delete what had been downloaded when the installation was cancelled. For a large Addon + this takes long enough that it has to happen off this thread: done here it would freeze the + interface, leaving a dialog that cannot even repaint itself to say what it is waiting for. + """ + self.cancelling_dialog.label.setText( + translate( + "AddonsInstaller", "Removing the part of '{}' that was already downloaded…" + ).format(self.addon_to_install.display_name) + ) + fci.Console.PrintMessage( + translate( + "AddonsInstaller", + "Installation of {} was cancelled: removing the partial download at {}", + ).format(self.addon_to_install.display_name, path) + + "\n" + ) + remover = DirectoryRemover(path) + remover.start() + while remover.isRunning(): + remover.wait(50) + QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents) + def _installation_succeeded(self): """Called if the installation was successful.""" MessageDialog.show_modal( From 05c677dca0cc0c2042f00b2beff1d625524ce999 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 17:16:11 -0500 Subject: [PATCH 10/11] Clean up text (cherry picked from commit add39ff5b8963c4320b35aaa580ee611d7763e25) --- Addon.py | 6 +++--- NetworkManager.py | 2 +- addonmanager_workers_startup.py | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Addon.py b/Addon.py index af545928..76c5b369 100644 --- a/Addon.py +++ b/Addon.py @@ -339,7 +339,7 @@ def load_metadata_file(self, file: str) -> None: fci.Console.PrintWarning( "An invalid or corrupted package.xml file was found in the cache for" ) - fci.Console.PrintWarning(f" {self.name}... ignoring the bad data.\n") + fci.Console.PrintWarning(f" {self.name}… ignoring the bad data.\n") return self.set_metadata(metadata) self._clean_url() @@ -358,7 +358,7 @@ def _load_installed_metadata(self) -> None: fci.Console.PrintWarning( "An invalid or corrupted package.xml file was found in installation of" ) - fci.Console.PrintWarning(f" {self.name}... ignoring the bad data.\n") + fci.Console.PrintWarning(f" {self.name}… ignoring the bad data.\n") return def set_metadata(self, metadata: Metadata) -> None: @@ -847,7 +847,7 @@ def package_is_installed(package_name: str) -> bool: # can do the check by PyPI package name: if importlib_metadata is None: fci.Console.PrintMessage( - f"Cannot check for installation of `{package_name}`... marking it for " + f"Cannot check for installation of `{package_name}`… marking it for " "reinstallation to be safe\n" ) return False diff --git a/NetworkManager.py b/NetworkManager.py index 5449d6dd..1f0fd2fd 100644 --- a/NetworkManager.py +++ b/NetworkManager.py @@ -348,7 +348,7 @@ def blocking_get_with_retries( return None if not quiet: fci.Console.PrintWarning( - f"Failed to get {url}, retrying in {delay_ms}ms... (attempt {attempt} of {max_attempts})\n" + f"Failed to get {url}, retrying in {delay_ms}ms… (attempt {attempt} of {max_attempts})\n" ) time.sleep(delay_ms / 1000) diff --git a/addonmanager_workers_startup.py b/addonmanager_workers_startup.py index 14c2a56c..c50ff6e1 100644 --- a/addonmanager_workers_startup.py +++ b/addonmanager_workers_startup.py @@ -868,7 +868,7 @@ def run(self): ).format(self.url) ) else: - fci.Console.PrintWarning("Running score generation in TEST mode...\n") + fci.Console.PrintWarning("Running score generation in TEST mode…\n") json_result = {} for addon in self.addons: if addon.macro: From 5251253796fb21d47fd912dbbb0f477f4d127862 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 14 Aug 2026 17:30:41 -0500 Subject: [PATCH 11/11] Add mechanism to retry if git install fails (cherry picked from commit 3d2c189de3e86243a4bd4bbbc890cd111626b83e) --- AddonManagerTest/gui/test_installer_gui.py | 56 ++++++++- addonmanager_installer_gui.py | 137 ++++++++++++++++----- 2 files changed, 163 insertions(+), 30 deletions(-) diff --git a/AddonManagerTest/gui/test_installer_gui.py b/AddonManagerTest/gui/test_installer_gui.py index 28d46139..12a53573 100644 --- a/AddonManagerTest/gui/test_installer_gui.py +++ b/AddonManagerTest/gui/test_installer_gui.py @@ -30,7 +30,7 @@ from PySideWrapper import QtWidgets, QtCore from Addon import Addon, MissingDependencies -from addonmanager_installer import AddonInstaller +from addonmanager_installer import AddonInstaller, InstallationMethod from addonmanager_installer_gui import ( AddonInstallerGUI, AddonDependencyInstallerGUI, @@ -179,6 +179,58 @@ def test_dialog_does_not_mention_git_for_a_zip_install(self): self.assertNotIn("git", gui.installing_dialog.label.text()) + def test_a_failed_git_installation_can_be_tried_another_way(self): + """Cloning a large Addon fails for reasons a second attempt gets past, so the failure is + not the end of the conversation.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.installer.will_use_git = lambda: True + + gui.installation_method = InstallationMethod.ANY + + self.assertTrue(gui._can_try_another_way()) + + def test_a_failed_zip_installation_is_only_reported(self): + """The zip download is the fallback, so there is nothing left to fall back to.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.installer.will_use_git = lambda: True + gui.installation_method = InstallationMethod.ZIP + + self.assertFalse(gui._can_try_another_way()) + + def test_a_failed_installation_that_did_not_use_git_is_only_reported(self): + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.installer.will_use_git = lambda: False + + self.assertFalse(gui._can_try_another_way()) + + def test_trying_again_starts_a_new_installer(self): + """The installer that failed belongs to a thread that has ended, so the second attempt + gets one of its own, running by whichever method was chosen.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + failed_installer = gui.installer + attempts = [] + gui.install = lambda method=InstallationMethod.ANY: attempts.append(method) + with tempfile.TemporaryDirectory() as temp_dir: + gui.installer.installation_path = temp_dir # Nothing left behind to clean up + + gui._try_again(InstallationMethod.ZIP) + + self.assertEqual([InstallationMethod.ZIP], attempts) + self.assertIsNot(failed_installer, gui.installer) + + def test_trying_again_removes_what_the_failed_attempt_left(self): + """A half-finished checkout is not something the next attempt can build on.""" + gui = AddonInstallerGUI(Addon("Test Addon")) + gui.install = lambda method=InstallationMethod.ANY: None + with tempfile.TemporaryDirectory() as temp_dir: + gui.installer.installation_path = temp_dir + leftovers = os.path.join(temp_dir, "Test Addon") + os.makedirs(os.path.join(leftovers, ".git")) + + gui._try_again(InstallationMethod.ANY) + + self.assertFalse(os.path.exists(leftovers)) + def test_cancelling_dialog_shows_that_work_is_going_on(self): """Stopping a large installation takes time, so the dialog animates and offers no button: a fixed sentence next to an OK button reads as a hang.""" @@ -203,7 +255,7 @@ def test_removing_a_partial_installation_keeps_the_interface_alive(self): with open(os.path.join(partial_download, "subdirectory", "file"), "w") as f: f.write("downloaded so far") - gui._remove_partial_installation(partial_download) + gui._remove_partial_installation(partial_download, gui.cancelling_dialog) self.assertFalse(os.path.exists(partial_download)) self.assertIn("Removing", gui.cancelling_dialog.label.text()) diff --git a/addonmanager_installer_gui.py b/addonmanager_installer_gui.py index 5b328728..e0481f03 100644 --- a/addonmanager_installer_gui.py +++ b/addonmanager_installer_gui.py @@ -24,6 +24,7 @@ classes for details.""" import os import sys +from functools import partial from typing import List import addonmanager_freecad_interface as fci @@ -32,7 +33,7 @@ from PySideWrapper import QtCore, QtWidgets -from addonmanager_installer import AddonInstaller, MacroInstaller +from addonmanager_installer import AddonInstaller, InstallationMethod, MacroInstaller from addonmanager_dependency_installer import DependencyInstaller from addonmanager_metadata import Version from addonmanager_python_constraints import PythonConstraints @@ -84,6 +85,7 @@ def __init__(self, addon: Addon, addons: List[Addon] = None): self.installing_dialog = None self.cancelling_dialog = None self.installation_message = "" + self.installation_method = InstallationMethod.ANY self.worker_thread = None # Set up the installer connections @@ -129,15 +131,16 @@ def run(self): self.dependency_installer.proceed.connect(self.install) self.dependency_installer.run() - def install(self) -> None: + def install(self, install_method: InstallationMethod = InstallationMethod.ANY) -> None: """Installs or updates a workbench, macro, or package""" + self.installation_method = install_method self.worker_thread = QtCore.QThread() self.worker_thread.setObjectName("Addon Installer worker thread") self.installer.moveToThread(self.worker_thread) self.installer.finished.connect(self.worker_thread.quit) self.installer.progress_update.connect(self._progress_update) self.installer.progress_message.connect(self._progress_message) - self.worker_thread.started.connect(self.installer.run) + self.worker_thread.started.connect(partial(self.installer.run, install_method)) self.create_installing_dialog() self.installer.finished.connect(self.installing_dialog.hide) @@ -216,23 +219,32 @@ def _set_installation_detail(self, detail: str) -> None: elided = label.fontMetrics().elidedText(detail, QtCore.Qt.ElideMiddle, available_width) label.setText(f"{self.installation_message}\n{elided}") + def _create_busy_dialog(self, object_name: str, title: str, message: str): + """A dialog reporting work of unknown length that the user cannot interrupt. It animates, + so that a long wait does not look like a hang, and it offers no buttons, because there is + nothing to offer: a dialog with a button that does nothing is worse than one without.""" + dialog = fci.loadUi(os.path.join(os.path.dirname(__file__), "progress.ui")) + dialog.setObjectName(object_name) + dialog.setWindowTitle(title) + dialog.label.setText(message) + dialog.label.setMinimumWidth(560) + dialog.progressBar.setRange(0, 0) # Indeterminate: this has no known length + dialog.buttonBox.hide() + return dialog + def create_cancelling_dialog(self) -> None: """Create the dialog shown while an installation is being stopped. Both stopping the work and clearing up after it can take a long time for a large Addon, so this dialog says which - of the two is happening and animates while it does, rather than presenting a fixed sentence - and a button that does nothing.""" - self.cancelling_dialog = fci.loadUi(os.path.join(os.path.dirname(__file__), "progress.ui")) - self.cancelling_dialog.setObjectName("AddonInstaller_CancellingDialog") - self.cancelling_dialog.setWindowTitle(translate("AddonsInstaller", "Cancelling")) + of the two is happening and animates while it does.""" if self._is_an_update(): message = translate("AddonsInstaller", "Cancelling the update of '{}'…") else: message = translate("AddonsInstaller", "Cancelling the installation of '{}'…") - self.cancelling_dialog.label.setText(message.format(self.addon_to_install.display_name)) - self.cancelling_dialog.label.setMinimumWidth(560) - self.cancelling_dialog.progressBar.setRange(0, 0) # Indeterminate: this has no known length - # There is nothing to offer the user here: the cancellation cannot itself be cancelled - self.cancelling_dialog.buttonBox.hide() + self.cancelling_dialog = self._create_busy_dialog( + "AddonInstaller_CancellingDialog", + translate("AddonsInstaller", "Cancelling"), + message.format(self.addon_to_install.display_name), + ) def _cancel_addon_installation(self): self.create_cancelling_dialog() @@ -248,25 +260,25 @@ def _cancel_addon_installation(self): QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents) path = str(os.path.join(self.installer.installation_path, self.addon_to_install.name)) if os.path.exists(path): - self._remove_partial_installation(path) + self._remove_partial_installation(path, self.cancelling_dialog) self.cancelling_dialog.hide() self.finished.emit() - def _remove_partial_installation(self, path: str) -> None: - """Delete what had been downloaded when the installation was cancelled. For a large Addon - this takes long enough that it has to happen off this thread: done here it would freeze the - interface, leaving a dialog that cannot even repaint itself to say what it is waiting for. - """ - self.cancelling_dialog.label.setText( - translate( - "AddonsInstaller", "Removing the part of '{}' that was already downloaded…" - ).format(self.addon_to_install.display_name) - ) + def _removal_message(self) -> str: + return translate( + "AddonsInstaller", "Removing the part of '{}' that was already downloaded…" + ).format(self.addon_to_install.display_name) + + def _remove_partial_installation(self, path: str, dialog) -> None: + """Delete what an installation left behind when it was stopped, or when it failed. For a + large Addon this takes long enough that it has to happen off this thread: done here it + would freeze the interface, leaving a dialog that cannot even repaint itself to say what + it is waiting for.""" + dialog.label.setText(self._removal_message()) fci.Console.PrintMessage( - translate( - "AddonsInstaller", - "Installation of {} was cancelled: removing the partial download at {}", - ).format(self.addon_to_install.display_name, path) + translate("AddonsInstaller", "Removing the partial download of {} at {}").format( + self.addon_to_install.display_name, path + ) + "\n" ) remover = DirectoryRemover(path) @@ -289,8 +301,77 @@ def _installation_succeeded(self): self.success.emit(self.addon_to_install) self.finished.emit() + def _can_try_another_way(self) -> bool: + """Whether there is anything to offer the user beyond reporting the failure. An + installation that used git can be tried again, or downloaded as a zip instead: cloning a + large Addon has plenty of ways to fail that a second attempt gets past.""" + if self.installation_method == InstallationMethod.ZIP: + return False + return self.installer.will_use_git() + + def _offer_another_attempt(self, message: str) -> None: + """Ask whether to try the installation again, or to fall back to downloading a zip.""" + dialog = QtWidgets.QMessageBox(utils.get_main_am_window()) + dialog.setObjectName("AddonInstaller_RetryDialog") + dialog.setIcon(QtWidgets.QMessageBox.Warning) + dialog.setWindowTitle(translate("AddonsInstaller", "Installation Failed")) + dialog.setText( + translate("AddonsInstaller", "Installing {} with git did not finish.").format( + self.addon_to_install.display_name + ) + ) + dialog.setInformativeText( + translate( + "AddonsInstaller", + "Trying again often gets past whatever interrupted it. This Addon can also be " + "downloaded as a zip file instead, but a download that large is itself easily " + "interrupted, and every later update downloads the whole Addon again.", + ) + ) + dialog.setDetailedText(message) + retry_button = dialog.addButton( + translate("AddonsInstaller", "Try again"), QtWidgets.QMessageBox.AcceptRole + ) + zip_button = dialog.addButton( + translate("AddonsInstaller", "Download a zip instead"), + QtWidgets.QMessageBox.ActionRole, + ) + dialog.addButton(QtWidgets.QMessageBox.Cancel) + dialog.setDefaultButton(retry_button) + dialog.exec() + if dialog.clickedButton() is retry_button: + self._try_again(self.installation_method) + elif dialog.clickedButton() is zip_button: + self._try_again(InstallationMethod.ZIP) + else: + self.finished.emit() + + def _try_again(self, install_method: InstallationMethod) -> None: + """Run the installation again, by the given method. Whatever the failed attempt left on + disk is removed first, because a half-finished checkout is not something the next attempt + can build on. A new installer is used: the old one belongs to a thread that has ended.""" + self._stop_thread(self.worker_thread) + path = str(os.path.join(self.installer.installation_path, self.addon_to_install.name)) + if os.path.exists(path): + dialog = self._create_busy_dialog( + "AddonInstaller_CleaningUpDialog", + translate("AddonsInstaller", "Cleaning up"), + self._removal_message(), + ) + dialog.show() + QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents) + self._remove_partial_installation(path, dialog) + dialog.hide() + self.installer = AddonInstaller(self.addon_to_install) + self.installer.success.connect(self._installation_succeeded) + self.installer.failure.connect(self._installation_failed) + self.install(install_method) + def _installation_failed(self, addon, message): """Called if the installation failed.""" + if self._can_try_another_way(): + self._offer_another_attempt(message) + return error_dialog = QtWidgets.QMessageBox(utils.get_main_am_window()) error_dialog.setObjectName("AddonManager_ErrorDialog") error_dialog.setIcon(QtWidgets.QMessageBox.Critical)