From 579e8d47d14da00c96aaefe1e44cb6038ff7439f Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Thu, 3 Sep 2026 13:19:52 +0200 Subject: [PATCH 01/10] ENH: add retry mecanism to the addon cache builder A crash during the 6-hourly cache rebuild could permanently drop an addon's cached package.xml/icon for that cycle (since the existing good clone was deleted beforehand). Retry git commands a few times before giving up. If an update ultimately fails, keep the existing good clone. Also retry addons that failed on the previous run first, so a rate-limit-driven failure doesn't always strand the same addons at the tail of a fixed processing order. --- AddonCatalog.py | 10 ++ AddonCatalogCacheCreator.py | 229 +++++++++++++++++++++++++++--------- 2 files changed, 184 insertions(+), 55 deletions(-) diff --git a/AddonCatalog.py b/AddonCatalog.py index fe960a1a..c7ceec66 100644 --- a/AddonCatalog.py +++ b/AddonCatalog.py @@ -89,6 +89,7 @@ class AddonCatalogEntry: 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 + cache_error: Optional[str] = None # Generated by the cache system def __init__(self, raw_data: Dict[str, str]) -> None: """Create an AddonDictionaryEntry from the raw JSON data""" @@ -360,6 +361,15 @@ def add_git_info_to_entry( entry.git_hash = commit_hash entry.git_tag = tag + def add_cache_error_to_entry(self, addon_id: str, index: int, error: Optional[str]) -> None: + """Records why this entry's cache generation failed, if it did.""" + entries = self._dictionary.get(addon_id) + if entries is None: + raise RuntimeError(f"Addon {addon_id} does not exist") + if index >= len(entries): + raise RuntimeError(f"Addon {addon_id} index out of range") + entries[index].cache_error = error + def get_available_branches(self, addon_id: str) -> List[str]: """For a given ID, get the list of available branches compatible with this version of FreeCAD. diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index 36e0bed2..bf885ffd 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -27,7 +27,7 @@ import shutil import sys from dataclasses import is_dataclass, fields -from typing import Any, List, Optional, Dict, Tuple +from typing import Any, List, Optional, Dict, Set, Tuple import base64 import enum @@ -37,6 +37,8 @@ import os import re import requests +import time +import traceback # Audited: all subprocess calls in this module are fixed git argument lists run with no shell; # the variable arguments (url, branch, name) come from the addon index this tool exists to @@ -65,6 +67,8 @@ CLONE_TIMEOUT = ( 300 # Seconds: repos that take longer than this are assumed to be too large to index ) +MAX_ATTEMPTS = 3 # Attempts before giving up on a single git or HTTP operation +RETRY_DELAY_SECONDS = 5 # Delay between retry attempts of a failed git or HTTP operation def recursive_serialize(obj: Any): @@ -127,6 +131,22 @@ def __init__(self): self._cache = {} self._sanitize_counter = 0 self._directory_name_cache: Dict[str, str] = {} + self._previously_failed_addon_ids: Set[str] = set() + + def _load_previously_failed_addon_ids(self) -> Set[str]: + """Read the previous run's clone_errors.json, if any, and return the set of addon IDs + that failed to clone/update or download, so this run retries them first. This is + deliberately based only on the immediately preceding run: an addon that succeeds this + time drops out of the priority set next time, and one that fails again stays in it.""" + path = os.path.join(self.cwd, "clone_errors.json") + if not os.path.isfile(path): + return set() + try: + with open(path, "r", encoding="utf-8") as f: + previous_errors = json.load(f) + except (OSError, json.JSONDecodeError): + return set() + return {dirname.split(os.sep, 1)[0] for dirname in previous_errors} def write(self, addon_id: Optional[str] = None) -> None: original_working_directory = os.getcwd() @@ -134,6 +154,7 @@ def write(self, addon_id: Optional[str] = None) -> None: os.chdir(self.cwd) try: + self._previously_failed_addon_ids = self._load_previously_failed_addon_ids() fetcher = CatalogFetcher() self.catalog = fetcher.catalog @@ -202,8 +223,12 @@ def write(self, addon_id: Optional[str] = None) -> None: os.chdir(original_working_directory) def create_local_copy_of_addons(self): + # Addons that failed last run are retried first, so a rate-limit-induced failure doesn't + # always strand the same addons at the tail end of a fixed processing order. + catalog_items = list(self.catalog.get_catalog().items()) + catalog_items.sort(key=lambda item: item[0] not in self._previously_failed_addon_ids) counter = 0 - for addon_id, catalog_entries in self.catalog.get_catalog().items(): + for addon_id, catalog_entries in catalog_items: self.create_local_copy_of_single_addon(addon_id, catalog_entries) counter += 1 if counter >= MAX_COUNT: @@ -228,6 +253,9 @@ def create_local_copy_of_single_addon( "Neither git info nor zip info was specified." ) continue + dirname = self.get_directory_name(addon_id, index, catalog_entry) + if dirname in self.clone_errors: + self.catalog.add_cache_error_to_entry(addon_id, index, self.clone_errors[dirname]) metadata = self.generate_cache_entry(addon_id, index, catalog_entry) self.catalog.add_metadata_to_entry(addon_id, index, metadata) git_hash, git_tag = self.get_git_info(addon_id, index, catalog_entry) @@ -476,65 +504,146 @@ def get_directory_name(self, addon_id, index, catalog_entry): def create_local_copy_of_single_addon_with_zip( self, addon_id: str, index: int, catalog_entry: AddonCatalog.AddonCatalogEntry ): - response = requests.get(catalog_entry.zip_url, timeout=10.0) - if response.status_code != 200: - print(f"ERROR: Failed to fetch zip data for {addon_id} from {catalog_entry.zip_url}.") - return extract_to_dir = self.get_directory_name(addon_id, index, catalog_entry) + response = None + last_error_message = "unknown error" + for attempt in range(1, MAX_ATTEMPTS + 1): + try: + response = requests.get(catalog_entry.zip_url, timeout=10.0) + except requests.exceptions.RequestException as e: + last_error_message = ( + f"Network error fetching {catalog_entry.zip_url}: {e}\n{traceback.format_exc()}" + ) + response = None + else: + if response.status_code == 200: + break + last_error_message = ( + f"Failed to fetch zip data for {addon_id} from {catalog_entry.zip_url}: " + f"HTTP {response.status_code}" + ) + response = None + print(f"WARNING: {last_error_message} (attempt {attempt}/{MAX_ATTEMPTS})", flush=True) + if attempt < MAX_ATTEMPTS: + time.sleep(RETRY_DELAY_SECONDS) + + if response is None: + error_message = f"{last_error_message}\nafter {MAX_ATTEMPTS} attempts" + print(f"ERROR: {error_message}") + self.clone_errors[extract_to_dir] = error_message + return + if os.path.exists(extract_to_dir): utils.rmdir(extract_to_dir) os.makedirs(extract_to_dir, exist_ok=True) - with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: - latest = max( - (info.date_time for info in zip_file.infolist() if not info.is_dir()), default=None + try: + with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file: + latest = max( + (info.date_time for info in zip_file.infolist() if not info.is_dir()), + default=None, + ) + if latest is not None: + catalog_entry.last_update_time = datetime.datetime(*latest).isoformat() + zip_file.extractall(path=extract_to_dir) + except (zipfile.BadZipFile, OSError) as e: + error_message = ( + f"Downloaded zip data for {addon_id} from {catalog_entry.zip_url} is invalid: " + f"{e}\n{traceback.format_exc()}" ) - if latest is not None: - catalog_entry.last_update_time = datetime.datetime(*latest).isoformat() - zip_file.extractall(path=extract_to_dir) + print(f"ERROR: {error_message}") + self.clone_errors[extract_to_dir] = error_message + + @staticmethod + def _tail(text: object, limit: int = 2000) -> str: + """Return the trailing portion of some captured subprocess output, if text actually + holds any (a mock in a test, or a process that produced no output, do not), bounded so + that one addon's error can't bloat the shipped cache. The end of the text is kept + because that's where git and pip put their actual "fatal: ..." error line.""" + if not isinstance(text, str) or not text.strip(): + return "" + stripped = text.strip() + return stripped if len(stripped) <= limit else "…" + stripped[-limit:] + + def clone_with_retries(self, url: str, branch: str, target_dir: str) -> None: + """Attempt a shallow 'git clone' of url/branch into target_dir, retrying up to + MAX_ATTEMPTS times with a short delay in between. A timeout and a non-zero exit code + are treated identically: git's exit code doesn't reliably distinguish a transient + network blip from a permanent error, and retrying a permanent failure a couple of extra + times is cheap for an unattended job. Before every attempt, any pre-existing target_dir + is removed, since git clone refuses to run into a non-empty directory and a partial + checkout can be left behind by a killed or timed-out previous attempt. Raises + RuntimeError (with git's own stderr appended, if any was captured) if every attempt + fails; deliberately does not touch self.clone_errors, since callers use this helper for + two different targets that need different keys.""" + # Shallow, but do include the last commit on each branch and tag + command = ["git", "clone", "--depth", "1", "--branch", branch, url, target_dir] + last_error_message = "unknown error" + for attempt in range(1, MAX_ATTEMPTS + 1): + if os.path.exists(target_dir): + utils.rmdir(target_dir) + print(f"Cloning {url} to {target_dir}", flush=True) + try: + completed_process = subprocess.run( # nosec B603 + command, timeout=CLONE_TIMEOUT, capture_output=True, text=True + ) + except subprocess.TimeoutExpired as e: + summary = f"Clone of {url} timed out after {CLONE_TIMEOUT} seconds" + detail = self._tail(e.stderr) + else: + if completed_process.returncode == 0: + return + summary = f"Failed to clone {url}: git exited with {completed_process.returncode}" + detail = self._tail(completed_process.stderr) + last_error_message = f"{summary}\n{detail}" if detail else summary + print(f"WARNING: {last_error_message} (attempt {attempt}/{MAX_ATTEMPTS})", flush=True) + if attempt < MAX_ATTEMPTS: + time.sleep(RETRY_DELAY_SECONDS) + if os.path.exists(target_dir): + utils.rmdir(target_dir) + raise RuntimeError(f"{last_error_message}\nafter {MAX_ATTEMPTS} attempts") def clone_or_update(self, name: str, url: str, branch: str) -> None: """If a directory called "name" exists, and it contains a subdirectory called .git, then the local copy is fetched and hard reset onto the requested ref; otherwise we use - 'git clone' to make a shallow copy of the repo.""" + 'git clone' to make a shallow copy of the repo. Transient failures are retried before + giving up. If updating an existing copy fails every attempt, it is left untouched (not + deleted), so this cycle's cache generation still finds whatever good data it already + had from a previous run.""" if not os.path.exists(os.path.join(os.getcwd(), name, ".git")): - print(f"Cloning {url} to {name}", flush=True) - # Shallow, but do include the last commit on each branch and tag - command = [ - "git", - "clone", - "--depth", - "1", - "--branch", - branch, - url, - name, - ] - try: - completed_process = subprocess.run(command, timeout=CLONE_TIMEOUT) # nosec B603 - except subprocess.TimeoutExpired: - self.clone_errors[name] = f"Timed out after {CLONE_TIMEOUT} seconds." - raise RuntimeError(f"Clone of {url} timed out.") - # TODO: Automatically fall back to a sparse clone - if completed_process.returncode != 0: - self.clone_errors[name] = f"Failed to clone {url}: {completed_process.returncode}" - raise RuntimeError(f"Clone failed for {url}") - else: - print(f"Updating {name}", flush=True) - old_dir = os.getcwd() - os.chdir(os.path.join(old_dir, name)) try: - CacheWriter.fetch_and_reset(name, url, branch) + self.clone_with_retries(url, branch, name) except RuntimeError as e: - # In the event of basically ANY error, delete the original and re-clone. - print(e) - print("Deleting and re-cloning the original repo") - os.chdir(old_dir) - utils.rmdir(os.path.join(old_dir, name)) - self.clone_or_update(name, url, branch) + self.clone_errors[name] = str(e) + raise + return + + print(f"Updating {name}", flush=True) + old_dir = os.getcwd() + os.chdir(os.path.join(old_dir, name)) + last_error: Optional[RuntimeError] = None + try: + for attempt in range(1, MAX_ATTEMPTS + 1): + try: + CacheWriter.fetch_and_reset(name, url, branch) + last_error = None + break + except RuntimeError as e: + last_error = e + print( + f"WARNING: Update attempt {attempt}/{MAX_ATTEMPTS} failed for {name}: {e}", + flush=True, + ) + if attempt < MAX_ATTEMPTS: + time.sleep(RETRY_DELAY_SECONDS) + finally: os.chdir(old_dir) + if last_error is not None: + self.clone_errors[name] = str(last_error) + raise last_error + def sparse_clone(self, name: str, url: str, branch: str, files: List[str]) -> None: """Perform a sparse clone of a git repo, including only the specified files. Overwrite any existing path.""" @@ -635,32 +744,42 @@ def get_icon_from_metadata(metadata: addonmanager_metadata.Metadata) -> Optional @staticmethod def fetch_and_reset(name: str, url: str, branch: str) -> None: """Update the git clone in the current working directory by fetching from its remote and - hard resetting onto the requested ref, discarding any local state. A RuntimeError is raised - if any of the git calls fails.""" + hard resetting onto the requested ref, discarding any local state. A RuntimeError, with + git's own stderr appended if any was captured, is raised if any of the git calls fails.""" try: completed_process = subprocess.run( # nosec B603 B607 - ["git", "fetch", "--force"], timeout=CLONE_TIMEOUT + ["git", "fetch", "--force"], timeout=CLONE_TIMEOUT, capture_output=True, text=True + ) + except subprocess.TimeoutExpired as e: + raise RuntimeError( + f"git fetch for {name} timed out after {CLONE_TIMEOUT} seconds. " + f"{CacheWriter._tail(e.stderr)}".strip() ) - except subprocess.TimeoutExpired: - raise RuntimeError(f"git fetch for {name} timed out after {CLONE_TIMEOUT} seconds") if completed_process.returncode != 0: - raise RuntimeError(f"git fetch failed for {name}") + raise RuntimeError( + f"git fetch failed for {name}. {CacheWriter._tail(completed_process.stderr)}".strip() + ) git_ref_type = CacheWriter.determine_git_ref_type(name, url, branch) reset_target = f"origin/{branch}" if git_ref_type == GitRefType.BRANCH else branch completed_process = subprocess.run( # nosec B603 B607 - ["git", "reset", "--hard", reset_target, "--quiet"] + ["git", "reset", "--hard", reset_target, "--quiet"], capture_output=True, text=True ) if completed_process.returncode != 0: - raise RuntimeError(f"git reset failed for {name} ref {reset_target}") + raise RuntimeError( + f"git reset failed for {name} ref {reset_target}. " + f"{CacheWriter._tail(completed_process.stderr)}".strip() + ) completed_process = subprocess.run( # nosec B603 B607 - ["git", "clean", "-x", "-f", "-d", "--quiet"] + ["git", "clean", "-x", "-f", "-d", "--quiet"], capture_output=True, text=True ) if completed_process.returncode != 0: - raise RuntimeError(f"git clean failed for {name}") + raise RuntimeError( + f"git clean failed for {name}. {CacheWriter._tail(completed_process.stderr)}".strip() + ) @staticmethod def determine_git_ref_type(name: str, _url: str, branch: str) -> GitRefType: From dc8815c56abf52e4941e398adc16a93427497230 Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Thu, 3 Sep 2026 13:27:44 +0200 Subject: [PATCH 02/10] TST: add unit tests --- .../app/test_addon_catalog_cache_creator.py | 208 +++++++++++++++--- AddonManagerTest/app/test_addoncatalog.py | 29 ++- 2 files changed, 207 insertions(+), 30 deletions(-) diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index 6ea28e41..641667cf 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py @@ -24,20 +24,17 @@ import base64 import dataclasses +import os from unittest import mock +from unittest.mock import MagicMock, patch from pyfakefs.fake_filesystem_unittest import TestCase -from unittest.mock import patch, MagicMock - -import os - -import AddonCatalogCacheCreator as accc import AddonCatalog +import AddonCatalogCacheCreator as accc class TestRecursiveSerialize(TestCase): - def test_simple_object(self): result = accc.recursive_serialize("just a string") self.assertEqual(result, "just a string") @@ -109,7 +106,6 @@ def test_real_catalog(self): class TestCacheWriter(TestCase): - def setUp(self): self.setUpPyfakefs() @@ -291,8 +287,12 @@ def test_should_use_sparse_clone_for_a_normal_addon(self): 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): + @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 = [ @@ -314,8 +314,12 @@ def test_create_local_copy_of_single_addon_using_sparse_clone(self, mock_create_ 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") + @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 ): @@ -323,7 +327,11 @@ def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( 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} + { + "repository": "https://some.url", + "git_ref": "main", + "sparse_cache": True, + } ), ] writer = accc.CacheWriter() @@ -336,7 +344,9 @@ def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( 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") + @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.""" catalog_entries = [ @@ -347,7 +357,11 @@ def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git) {"repository": "https://some.url", "git_ref": "branch-2"} ), AddonCatalog.AddonCatalogEntry( - {"repository": "https://some.url", "git_ref": "branch-3", "zip_url": "zip"} + { + "repository": "https://some.url", + "git_ref": "branch-3", + "zip_url": "zip", + } ), ] writer = accc.CacheWriter() @@ -356,8 +370,12 @@ def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git) writer.create_local_copy_of_single_addon("TestMod", catalog_entries) self.assertEqual(mock_create_with_git.call_count, 3) - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip") + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" + ) + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip" + ) def test_create_local_copy_of_single_addon_using_zip( self, mock_create_with_zip, mock_create_with_git ): @@ -367,7 +385,11 @@ def test_create_local_copy_of_single_addon_using_zip( AddonCatalog.AddonCatalogEntry({"zip_url": "zip1"}), AddonCatalog.AddonCatalogEntry({"zip_url": "zip2"}), AddonCatalog.AddonCatalogEntry( - {"repository": "https://some.url", "git_ref": "branch-3", "zip_url": "zip3"} + { + "repository": "https://some.url", + "git_ref": "branch-3", + "zip_url": "zip3", + } ), ] writer = accc.CacheWriter() @@ -415,6 +437,33 @@ def get_catalog(self): mock_create_single_addon.assert_any_call("TestMod2", mock.ANY) self.assertEqual(3, mock_create_single_addon.call_count) + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon") + def test_create_local_copy_of_addons_processes_previously_failed_addons_first( + self, mock_create_single_addon + ): + """Addons that failed last run are retried before working through the rest, in case the + failure was caused by rate limiting partway through the previous run.""" + + class MockCatalog: + def get_catalog(self): + return { + "TestMod1": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})], + "TestMod2": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})], + "TestMod3": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})], + "TestMod4": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})], + } + + writer = accc.CacheWriter() + writer.catalog = MockCatalog() + writer._previously_failed_addon_ids = {"TestMod3"} + writer.create_local_copy_of_addons() + processed_order = [ + call.args[0] for call in mock_create_single_addon.call_args_list + ] + self.assertEqual( + ["TestMod3", "TestMod1", "TestMod2", "TestMod4"], processed_order + ) + class TestCacheWriterGitUpdate(TestCase): """Tests of the git commands used to bring an existing local clone up to date.""" @@ -445,7 +494,10 @@ def test_fetch_and_reset_with_tag(self, mock_ref_type, mock_run): mock_ref_type.return_value = accc.GitRefType.TAG mock_run.return_value.returncode = 0 accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "v1.0") - self.assertIn(["git", "reset", "--hard", "v1.0", "--quiet"], self.issued_commands(mock_run)) + self.assertIn( + ["git", "reset", "--hard", "v1.0", "--quiet"], + self.issued_commands(mock_run), + ) @patch("AddonCatalogCacheCreator.subprocess.run") @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type") @@ -455,7 +507,8 @@ def test_fetch_and_reset_with_hash(self, mock_ref_type, mock_run): mock_run.return_value.returncode = 0 accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "abc123") self.assertIn( - ["git", "reset", "--hard", "abc123", "--quiet"], self.issued_commands(mock_run) + ["git", "reset", "--hard", "abc123", "--quiet"], + self.issued_commands(mock_run), ) @patch("AddonCatalogCacheCreator.subprocess.run") @@ -476,7 +529,10 @@ def test_fetch_and_reset_removes_untracked_files(self, mock_ref_type, mock_run): mock_ref_type.return_value = accc.GitRefType.BRANCH mock_run.return_value.returncode = 0 accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main") - self.assertIn(["git", "clean", "-x", "-f", "-d", "--quiet"], self.issued_commands(mock_run)) + self.assertIn( + ["git", "clean", "-x", "-f", "-d", "--quiet"], + self.issued_commands(mock_run), + ) @patch("AddonCatalogCacheCreator.subprocess.run") def test_fetch_and_reset_raises_when_fetch_fails(self, mock_run): @@ -501,18 +557,110 @@ def test_fetch_and_reset_raises_when_reset_fails(self, mock_ref_type, mock_run): with self.assertRaises(RuntimeError): accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main") + @patch("AddonCatalogCacheCreator.time.sleep") @patch("AddonCatalogCacheCreator.subprocess.run") - @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset") - def test_clone_or_update_reclones_when_update_fails(self, mock_update, mock_run): - """If the update fails, the local copy is deleted and cloned again.""" - mock_update.side_effect = RuntimeError("Update failed") + def test_clone_or_update_raises_after_exhausting_clone_attempts( + self, mock_run, mock_sleep + ): + """If every attempt fails, the caller is told, with a reason recorded for this addon.""" + mock_run.return_value.returncode = 1 + writer = accc.CacheWriter() + with self.assertRaises(RuntimeError): + writer.clone_or_update("TestMod", "https://some.url", "main") + self.assertEqual(accc.MAX_ATTEMPTS, mock_run.call_count) + self.assertEqual(accc.MAX_ATTEMPTS - 1, mock_sleep.call_count) + self.assertIn("TestMod", writer.clone_errors) + + @patch("AddonCatalogCacheCreator.utils.rmdir") + @patch("AddonCatalogCacheCreator.subprocess.run") + def test_clone_or_update_removes_leftover_directory_before_clone_attempt( + self, mock_run, mock_rmdir + ): + """A partial checkout left by an earlier killed attempt doesn't block a clean retry.""" mock_run.return_value.returncode = 0 + self.fake_fs().create_dir( + os.path.join(os.getcwd(), "TestMod", "partial-checkout") + ) + writer = accc.CacheWriter() + writer.clone_or_update("TestMod", "https://some.url", "main") + mock_rmdir.assert_any_call("TestMod") + + @patch("AddonCatalogCacheCreator.time.sleep") + @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset") + def test_clone_or_update_retries_update_before_reclone( + self, mock_fetch_and_reset, mock_run, mock_sleep + ): + """A single transient update failure is retried and self-heals: no deletion, no reclone.""" + mock_fetch_and_reset.side_effect = [RuntimeError("transient"), None] clone_path = os.path.join(os.getcwd(), "TestMod") self.fake_fs().create_dir(os.path.join(clone_path, ".git")) writer = accc.CacheWriter() writer.clone_or_update("TestMod", "https://some.url", "main") - self.assertFalse(os.path.exists(clone_path)) - self.assertIn("clone", self.issued_commands(mock_run)[0]) + self.assertEqual(2, mock_fetch_and_reset.call_count) + self.assertEqual(1, mock_sleep.call_count) + self.assertEqual(0, mock_run.call_count) + self.assertTrue(os.path.exists(clone_path)) + self.assertEqual({}, writer.clone_errors) + + @patch("AddonCatalogCacheCreator.time.sleep") + @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset") + def test_clone_or_update_reclones_after_exhausting_update_retries( + self, mock_fetch_and_reset, mock_run, mock_sleep + ): + """If every update attempt fails, a fresh clone into a temp dir replaces the original.""" + mock_fetch_and_reset.side_effect = RuntimeError("persistent failure") + + def fake_clone(command, timeout=None): # pylint: disable=unused-argument + self.fake_fs().create_dir(os.path.join(command[-1], ".git")) + return MagicMock(returncode=0) + + mock_run.side_effect = fake_clone + clone_path = os.path.join(os.getcwd(), "TestMod") + self.fake_fs().create_file( + os.path.join(clone_path, ".git", "HEAD"), contents="ref: refs/heads/main\n" + ) + writer = accc.CacheWriter() + writer.clone_or_update("TestMod", "https://some.url", "main") + self.assertEqual(accc.MAX_ATTEMPTS, mock_fetch_and_reset.call_count) + commands = self.issued_commands(mock_run) + self.assertTrue( + any("clone" in cmd and cmd[-1] == "TestMod.reclone-tmp" for cmd in commands) + ) + self.assertTrue(os.path.isdir(os.path.join("TestMod", ".git"))) + self.assertFalse(os.path.exists("TestMod.reclone-tmp")) + self.assertFalse(os.path.exists("TestMod.reclone-old")) + self.assertNotIn("TestMod", writer.clone_errors) + + @patch("AddonCatalogCacheCreator.time.sleep") + @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset") + def test_clone_or_update_reclone_failure_leaves_original_directory_untouched( + self, mock_fetch_and_reset, mock_run, mock_sleep + ): + """If the fallback re-clone also fails, the existing good copy is left exactly as-is.""" + mock_fetch_and_reset.side_effect = RuntimeError("persistent failure") + mock_run.return_value.returncode = ( + 1 # The fallback re-clone fails every attempt too. + ) + clone_path = os.path.join(os.getcwd(), "TestMod") + self.fake_fs().create_file( + os.path.join(clone_path, ".git", "HEAD"), contents="ref: refs/heads/main\n" + ) + self.fake_fs().create_file( + os.path.join(clone_path, "package.xml"), + contents="marker", + ) + writer = accc.CacheWriter() + with self.assertRaises(RuntimeError): + writer.clone_or_update("TestMod", "https://some.url", "main") + self.assertEqual(accc.MAX_ATTEMPTS, mock_fetch_and_reset.call_count) + self.assertTrue(os.path.isdir(clone_path)) + with open(os.path.join(clone_path, "package.xml"), encoding="utf-8") as f: + self.assertEqual("marker", f.read()) + self.assertIn("TestMod", writer.clone_errors) + self.assertFalse(os.path.exists("TestMod.reclone-tmp")) @patch("AddonCatalogCacheCreator.subprocess.run") def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): @@ -522,7 +670,9 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): writer = accc.CacheWriter() writer.sparse_clone("TestMod", "https://some.url", "main", ["package.xml"]) commands = self.issued_commands(mock_run) - self.assertEqual(["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0]) + self.assertEqual( + ["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0] + ) self.assertIn(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], commands) self.assertEqual({}, writer.clone_errors) @@ -530,7 +680,9 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): def test_add_to_sparse_clone_checks_out_without_network_access(self, mock_run): """New sparse checkout entries are taken from the commit that is already local.""" mock_run.return_value.returncode = 0 - sparse_file = os.path.join(os.getcwd(), "TestMod", ".git", "info", "sparse-checkout") + sparse_file = os.path.join( + os.getcwd(), "TestMod", ".git", "info", "sparse-checkout" + ) self.fake_fs().create_file(sparse_file, contents="package.xml\n") writer = accc.CacheWriter() writer.add_to_sparse_clone("TestMod", ["icon.svg"]) diff --git a/AddonManagerTest/app/test_addoncatalog.py b/AddonManagerTest/app/test_addoncatalog.py index 1d0cb020..91c9a140 100644 --- a/AddonManagerTest/app/test_addoncatalog.py +++ b/AddonManagerTest/app/test_addoncatalog.py @@ -6,7 +6,7 @@ """Tests for the AddonCatalog and AddonCatalogEntry classes.""" -from unittest import mock, main, TestCase +from unittest import TestCase, main, mock from unittest.mock import patch AddonCatalogEntry = None @@ -23,7 +23,7 @@ def setUp(self): """Start mock for addonmanager_licenses class.""" self.addon_patch = mock.patch.dict("sys.modules", {"addonmanager_licenses": mock.Mock()}) self.mock_addon_module = self.addon_patch.start() - from AddonCatalog import AddonCatalogEntry, AddonCatalog + from AddonCatalog import AddonCatalog, AddonCatalogEntry from addonmanager_metadata import Version self.AddonCatalogEntry = AddonCatalogEntry @@ -144,6 +144,17 @@ def test_instantiate_addon_with_sparse_cache_and_no_zip(self): with self.assertRaises(RuntimeError): ac.instantiate_addon("parts_library") + def test_cache_error_field_round_trips_from_raw_data(self): + """The cache_error field, generated by the cache system, is accepted like any other + recognized raw-dict key.""" + ac = self.AddonCatalogEntry({"cache_error": "Clone failed for https://example.test"}) + self.assertEqual("Clone failed for https://example.test", ac.cache_error) + + def test_cache_error_defaults_to_none(self): + """An entry with no cache_error key is assumed to have generated cleanly.""" + ac = self.AddonCatalogEntry({}) + self.assertIsNone(ac.cache_error) + 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"}) @@ -417,6 +428,20 @@ def test_get_addon_from_id_with_icon_data(self): addon = self._get_addon_for_test(metadata_dict) self.assertIsNotNone(addon.icon_data) + def test_add_cache_error_to_entry_sets_the_field(self): + """The cache creator can record why an entry's cache generation failed.""" + data = {"AnAddon": [{"git_ref": "main"}]} + catalog = self.AddonCatalog(data) + catalog.add_cache_error_to_entry("AnAddon", 0, "Clone failed after 3 attempts") + entry = catalog.get_catalog()["AnAddon"][0] + self.assertEqual("Clone failed after 3 attempts", entry.cache_error) + + def test_add_cache_error_to_entry_unknown_addon_raises(self): + """Recording a cache error for an addon ID that doesn't exist is a programming error.""" + catalog = self.AddonCatalog({"AnAddon": [{"git_ref": "main"}]}) + with self.assertRaises(RuntimeError): + catalog.add_cache_error_to_entry("NoSuchAddon", 0, "boom") + def _get_addon_for_test(self, metadata_dict) -> Addon: metadata = self.CatalogEntryMetadata.from_dict(metadata_dict) data = { From dcaba5e16b5ca108593c6be7c774af861f2735a2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:30:46 +0000 Subject: [PATCH 03/10] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../app/test_addon_catalog_cache_creator.py | 56 +++++-------------- 1 file changed, 14 insertions(+), 42 deletions(-) diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index 641667cf..f9b60b7c 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py @@ -287,12 +287,8 @@ def test_should_use_sparse_clone_for_a_normal_addon(self): 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 - ): + @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 = [ @@ -314,12 +310,8 @@ def test_create_local_copy_of_single_addon_using_sparse_clone( 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" - ) + @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 ): @@ -344,9 +336,7 @@ def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( 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" - ) + @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.""" catalog_entries = [ @@ -370,12 +360,8 @@ def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git) writer.create_local_copy_of_single_addon("TestMod", catalog_entries) self.assertEqual(mock_create_with_git.call_count, 3) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" - ) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip" - ) + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip") def test_create_local_copy_of_single_addon_using_zip( self, mock_create_with_zip, mock_create_with_git ): @@ -457,12 +443,8 @@ def get_catalog(self): writer.catalog = MockCatalog() writer._previously_failed_addon_ids = {"TestMod3"} writer.create_local_copy_of_addons() - processed_order = [ - call.args[0] for call in mock_create_single_addon.call_args_list - ] - self.assertEqual( - ["TestMod3", "TestMod1", "TestMod2", "TestMod4"], processed_order - ) + processed_order = [call.args[0] for call in mock_create_single_addon.call_args_list] + self.assertEqual(["TestMod3", "TestMod1", "TestMod2", "TestMod4"], processed_order) class TestCacheWriterGitUpdate(TestCase): @@ -559,9 +541,7 @@ def test_fetch_and_reset_raises_when_reset_fails(self, mock_ref_type, mock_run): @patch("AddonCatalogCacheCreator.time.sleep") @patch("AddonCatalogCacheCreator.subprocess.run") - def test_clone_or_update_raises_after_exhausting_clone_attempts( - self, mock_run, mock_sleep - ): + def test_clone_or_update_raises_after_exhausting_clone_attempts(self, mock_run, mock_sleep): """If every attempt fails, the caller is told, with a reason recorded for this addon.""" mock_run.return_value.returncode = 1 writer = accc.CacheWriter() @@ -578,9 +558,7 @@ def test_clone_or_update_removes_leftover_directory_before_clone_attempt( ): """A partial checkout left by an earlier killed attempt doesn't block a clean retry.""" mock_run.return_value.returncode = 0 - self.fake_fs().create_dir( - os.path.join(os.getcwd(), "TestMod", "partial-checkout") - ) + self.fake_fs().create_dir(os.path.join(os.getcwd(), "TestMod", "partial-checkout")) writer = accc.CacheWriter() writer.clone_or_update("TestMod", "https://some.url", "main") mock_rmdir.assert_any_call("TestMod") @@ -641,9 +619,7 @@ def test_clone_or_update_reclone_failure_leaves_original_directory_untouched( ): """If the fallback re-clone also fails, the existing good copy is left exactly as-is.""" mock_fetch_and_reset.side_effect = RuntimeError("persistent failure") - mock_run.return_value.returncode = ( - 1 # The fallback re-clone fails every attempt too. - ) + mock_run.return_value.returncode = 1 # The fallback re-clone fails every attempt too. clone_path = os.path.join(os.getcwd(), "TestMod") self.fake_fs().create_file( os.path.join(clone_path, ".git", "HEAD"), contents="ref: refs/heads/main\n" @@ -670,9 +646,7 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): writer = accc.CacheWriter() writer.sparse_clone("TestMod", "https://some.url", "main", ["package.xml"]) commands = self.issued_commands(mock_run) - self.assertEqual( - ["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0] - ) + self.assertEqual(["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0]) self.assertIn(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], commands) self.assertEqual({}, writer.clone_errors) @@ -680,9 +654,7 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): def test_add_to_sparse_clone_checks_out_without_network_access(self, mock_run): """New sparse checkout entries are taken from the commit that is already local.""" mock_run.return_value.returncode = 0 - sparse_file = os.path.join( - os.getcwd(), "TestMod", ".git", "info", "sparse-checkout" - ) + sparse_file = os.path.join(os.getcwd(), "TestMod", ".git", "info", "sparse-checkout") self.fake_fs().create_file(sparse_file, contents="package.xml\n") writer = accc.CacheWriter() writer.add_to_sparse_clone("TestMod", ["icon.svg"]) From f17822669adfee09a49b89eb2b0c9e09e2a43329 Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Thu, 3 Sep 2026 15:47:45 +0200 Subject: [PATCH 04/10] MAINT: reduce docstring's length since the code speaks for itself --- AddonCatalogCacheCreator.py | 50 +++++++++++++------------------------ 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index bf885ffd..bde9347a 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -23,43 +23,40 @@ Intended to be run by a server-side systemd timer to generate a file that is then loaded by the Addon Manager in each FreeCAD installation.""" -import datetime -import shutil -import sys -from dataclasses import is_dataclass, fields -from typing import Any, List, Optional, Dict, Set, Tuple - import base64 +import datetime import enum import hashlib import io import json import os import re -import requests -import time -import traceback +import shutil # Audited: all subprocess calls in this module are fixed git argument lists run with no shell; # the variable arguments (url, branch, name) come from the addon index this tool exists to # process (added nosec B404, and B603/B607 at the call sites) import subprocess # nosec B404 -from typing import List +import sys +import time +import traceback +import zipfile +from dataclasses import fields, is_dataclass +from types import SimpleNamespace +from typing import Any, Dict, List, Optional, Set, Tuple # Audited: only the exception class is imported, for catching errors raised by defusedxml, # which re-exports this same class. All parsing is done by defusedxml. (added nosec B405) from xml.etree.ElementTree import ParseError as XmlParseError # nosec B405 +import requests from defusedxml import DefusedXmlException -import zipfile +from scour import scour import AddonCatalog +import addonmanager_icon_utilities as icon_utils import addonmanager_metadata import addonmanager_utilities as utils -import addonmanager_icon_utilities as icon_utils - -from scour import scour -from types import SimpleNamespace ADDON_CATALOG_URL = "https://raw.githubusercontent.com/FreeCAD/Addons/main/Data/Index.json" BASE_DIRECTORY = "./CatalogCache" @@ -134,10 +131,8 @@ def __init__(self): self._previously_failed_addon_ids: Set[str] = set() def _load_previously_failed_addon_ids(self) -> Set[str]: - """Read the previous run's clone_errors.json, if any, and return the set of addon IDs - that failed to clone/update or download, so this run retries them first. This is - deliberately based only on the immediately preceding run: an addon that succeeds this - time drops out of the priority set next time, and one that fails again stays in it.""" + """If any previous clone errors are found, return the set of addon IDs + that failed to clone/update or download.""" path = os.path.join(self.cwd, "clone_errors.json") if not os.path.isfile(path): return set() @@ -556,10 +551,9 @@ def create_local_copy_of_single_addon_with_zip( @staticmethod def _tail(text: object, limit: int = 2000) -> str: - """Return the trailing portion of some captured subprocess output, if text actually - holds any (a mock in a test, or a process that produced no output, do not), bounded so - that one addon's error can't bloat the shipped cache. The end of the text is kept - because that's where git and pip put their actual "fatal: ..." error line.""" + """Return the trailing portion of some text so that one addon's error can't + bloat the shipped cache. The end of the text is kept because that's where + git and pip put their actual "fatal: ..." error line.""" if not isinstance(text, str) or not text.strip(): return "" stripped = text.strip() @@ -567,15 +561,7 @@ def _tail(text: object, limit: int = 2000) -> str: def clone_with_retries(self, url: str, branch: str, target_dir: str) -> None: """Attempt a shallow 'git clone' of url/branch into target_dir, retrying up to - MAX_ATTEMPTS times with a short delay in between. A timeout and a non-zero exit code - are treated identically: git's exit code doesn't reliably distinguish a transient - network blip from a permanent error, and retrying a permanent failure a couple of extra - times is cheap for an unattended job. Before every attempt, any pre-existing target_dir - is removed, since git clone refuses to run into a non-empty directory and a partial - checkout can be left behind by a killed or timed-out previous attempt. Raises - RuntimeError (with git's own stderr appended, if any was captured) if every attempt - fails; deliberately does not touch self.clone_errors, since callers use this helper for - two different targets that need different keys.""" + MAX_ATTEMPTS times.""" # Shallow, but do include the last commit on each branch and tag command = ["git", "clone", "--depth", "1", "--branch", branch, url, target_dir] last_error_message = "unknown error" From e7df82c2cdb543366ac9df390af54729fb74642e Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Sat, 12 Sep 2026 15:34:09 +0200 Subject: [PATCH 05/10] TST: remove test for fresh reclone, and replace it with a test for leaving files untouched --- .../app/test_addon_catalog_cache_creator.py | 39 ++----------------- 1 file changed, 4 insertions(+), 35 deletions(-) diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index f9b60b7c..cca98804 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py @@ -566,7 +566,7 @@ def test_clone_or_update_removes_leftover_directory_before_clone_attempt( @patch("AddonCatalogCacheCreator.time.sleep") @patch("AddonCatalogCacheCreator.subprocess.run") @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset") - def test_clone_or_update_retries_update_before_reclone( + def test_clone_or_update_retries_transient_update_failure( self, mock_fetch_and_reset, mock_run, mock_sleep ): """A single transient update failure is retried and self-heals: no deletion, no reclone.""" @@ -584,42 +584,11 @@ def test_clone_or_update_retries_update_before_reclone( @patch("AddonCatalogCacheCreator.time.sleep") @patch("AddonCatalogCacheCreator.subprocess.run") @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset") - def test_clone_or_update_reclones_after_exhausting_update_retries( + def test_clone_or_update_leaves_original_directory_untouched_after_exhausting_update_retries( self, mock_fetch_and_reset, mock_run, mock_sleep ): - """If every update attempt fails, a fresh clone into a temp dir replaces the original.""" + """If every update attempt fails, the existing good copy is left exactly as-is.""" mock_fetch_and_reset.side_effect = RuntimeError("persistent failure") - - def fake_clone(command, timeout=None): # pylint: disable=unused-argument - self.fake_fs().create_dir(os.path.join(command[-1], ".git")) - return MagicMock(returncode=0) - - mock_run.side_effect = fake_clone - clone_path = os.path.join(os.getcwd(), "TestMod") - self.fake_fs().create_file( - os.path.join(clone_path, ".git", "HEAD"), contents="ref: refs/heads/main\n" - ) - writer = accc.CacheWriter() - writer.clone_or_update("TestMod", "https://some.url", "main") - self.assertEqual(accc.MAX_ATTEMPTS, mock_fetch_and_reset.call_count) - commands = self.issued_commands(mock_run) - self.assertTrue( - any("clone" in cmd and cmd[-1] == "TestMod.reclone-tmp" for cmd in commands) - ) - self.assertTrue(os.path.isdir(os.path.join("TestMod", ".git"))) - self.assertFalse(os.path.exists("TestMod.reclone-tmp")) - self.assertFalse(os.path.exists("TestMod.reclone-old")) - self.assertNotIn("TestMod", writer.clone_errors) - - @patch("AddonCatalogCacheCreator.time.sleep") - @patch("AddonCatalogCacheCreator.subprocess.run") - @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset") - def test_clone_or_update_reclone_failure_leaves_original_directory_untouched( - self, mock_fetch_and_reset, mock_run, mock_sleep - ): - """If the fallback re-clone also fails, the existing good copy is left exactly as-is.""" - mock_fetch_and_reset.side_effect = RuntimeError("persistent failure") - mock_run.return_value.returncode = 1 # The fallback re-clone fails every attempt too. clone_path = os.path.join(os.getcwd(), "TestMod") self.fake_fs().create_file( os.path.join(clone_path, ".git", "HEAD"), contents="ref: refs/heads/main\n" @@ -632,11 +601,11 @@ def test_clone_or_update_reclone_failure_leaves_original_directory_untouched( with self.assertRaises(RuntimeError): writer.clone_or_update("TestMod", "https://some.url", "main") self.assertEqual(accc.MAX_ATTEMPTS, mock_fetch_and_reset.call_count) + self.assertEqual(0, mock_run.call_count) self.assertTrue(os.path.isdir(clone_path)) with open(os.path.join(clone_path, "package.xml"), encoding="utf-8") as f: self.assertEqual("marker", f.read()) self.assertIn("TestMod", writer.clone_errors) - self.assertFalse(os.path.exists("TestMod.reclone-tmp")) @patch("AddonCatalogCacheCreator.subprocess.run") def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): From fa6d629777db839a83f874165355f25a5107400b Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Sun, 13 Sep 2026 18:31:23 +0200 Subject: [PATCH 06/10] ENH: sync local copy of repo to the url given in the index this avoids issues in case of a URL change --- AddonCatalogCacheCreator.py | 38 +++++++++++- .../app/test_addon_catalog_cache_creator.py | 62 ++++++++++++++++--- 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index bde9347a..16edd996 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -727,11 +727,45 @@ def get_icon_from_metadata(metadata: addonmanager_metadata.Metadata) -> Optional the cache writer).""" return addonmanager_metadata.get_icon_from_metadata(metadata) + @staticmethod + def sync_remote_url(name: str, url: str) -> None: + """If the "origin" remote of the git clone in the current working directory no longer + matches "url", set the remote to "url".""" + + completed_process = subprocess.run( # nosec B603 B607 + ["git", "remote", "get-url", "origin"], capture_output=True, text=True + ) + if completed_process.returncode != 0: + raise RuntimeError( + f"git remote get-url failed for {name}. " + f"{CacheWriter._tail(completed_process.stderr)}".strip() + ) + previous_url = completed_process.stdout.strip() + if previous_url == url: + return + + print( + f"WARNING: Remote URL for {name} changed from {previous_url} to {url}", + flush=True, + ) + + completed_process = subprocess.run( # nosec B603 B607 + ["git", "remote", "set-url", "origin", url], capture_output=True, text=True + ) + if completed_process.returncode != 0: + raise RuntimeError( + f"git remote set-url failed for {name}. " + f"{CacheWriter._tail(completed_process.stderr)}".strip() + ) + @staticmethod def fetch_and_reset(name: str, url: str, branch: str) -> None: """Update the git clone in the current working directory by fetching from its remote and - hard resetting onto the requested ref, discarding any local state. A RuntimeError, with - git's own stderr appended if any was captured, is raised if any of the git calls fails.""" + hard resetting onto the requested ref, discarding any local state. The origin remote is + first synced to "url" (see sync_remote_url), so a repository that moved since the local + clone was made is kept in sync.""" + + CacheWriter.sync_remote_url(name, url) try: completed_process = subprocess.run( # nosec B603 B607 diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index cca98804..c664796b 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py @@ -459,19 +459,23 @@ def issued_commands(mock_run): return [call.args[0] for call in mock_run.call_args_list] @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type") - def test_fetch_and_reset_with_branch(self, mock_ref_type, mock_run): - """A branch is reset onto the remote tracking branch, not merged.""" + def test_fetch_and_reset_with_branch(self, mock_ref_type, mock_sync_remote_url, mock_run): + """A branch is reset onto the remote tracking branch, not merged, after the origin remote + is synced to the catalog's current url (see the sync_remote_url tests).""" mock_ref_type.return_value = accc.GitRefType.BRANCH mock_run.return_value.returncode = 0 accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main") + mock_sync_remote_url.assert_called_once_with("TestMod", "https://some.url") commands = self.issued_commands(mock_run) self.assertEqual(["git", "fetch", "--force"], commands[0]) self.assertIn(["git", "reset", "--hard", "origin/main", "--quiet"], commands) @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type") - def test_fetch_and_reset_with_tag(self, mock_ref_type, mock_run): + def test_fetch_and_reset_with_tag(self, mock_ref_type, mock_sync_remote_url, mock_run): """A tag is reset onto the tag itself, which has no remote tracking equivalent.""" mock_ref_type.return_value = accc.GitRefType.TAG mock_run.return_value.returncode = 0 @@ -482,8 +486,9 @@ def test_fetch_and_reset_with_tag(self, mock_ref_type, mock_run): ) @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type") - def test_fetch_and_reset_with_hash(self, mock_ref_type, mock_run): + def test_fetch_and_reset_with_hash(self, mock_ref_type, mock_sync_remote_url, mock_run): """A hash is reset onto the hash itself.""" mock_ref_type.return_value = accc.GitRefType.HASH mock_run.return_value.returncode = 0 @@ -494,8 +499,11 @@ def test_fetch_and_reset_with_hash(self, mock_ref_type, mock_run): ) @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type") - def test_fetch_and_reset_does_not_merge_or_pull(self, mock_ref_type, mock_run): + def test_fetch_and_reset_does_not_merge_or_pull( + self, mock_ref_type, mock_sync_remote_url, mock_run + ): """Neither pull nor merge is used, so a force push on the remote cannot fail the update.""" mock_ref_type.return_value = accc.GitRefType.BRANCH mock_run.return_value.returncode = 0 @@ -505,8 +513,11 @@ def test_fetch_and_reset_does_not_merge_or_pull(self, mock_ref_type, mock_run): self.assertNotIn("merge", command) @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type") - def test_fetch_and_reset_removes_untracked_files(self, mock_ref_type, mock_run): + def test_fetch_and_reset_removes_untracked_files( + self, mock_ref_type, mock_sync_remote_url, mock_run + ): """Files left over from a previous run are removed.""" mock_ref_type.return_value = accc.GitRefType.BRANCH mock_run.return_value.returncode = 0 @@ -517,28 +528,61 @@ def test_fetch_and_reset_removes_untracked_files(self, mock_ref_type, mock_run): ) @patch("AddonCatalogCacheCreator.subprocess.run") - def test_fetch_and_reset_raises_when_fetch_fails(self, mock_run): + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") + def test_fetch_and_reset_raises_when_fetch_fails(self, mock_sync_remote_url, mock_run): """A failed fetch is reported as a RuntimeError so that the caller can re-clone.""" mock_run.return_value.returncode = 1 with self.assertRaises(RuntimeError): accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main") @patch("AddonCatalogCacheCreator.subprocess.run") - def test_fetch_and_reset_raises_when_fetch_times_out(self, mock_run): + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") + def test_fetch_and_reset_raises_when_fetch_times_out(self, mock_sync_remote_url, mock_run): """A timed-out fetch is reported as a RuntimeError so that the caller can re-clone.""" mock_run.side_effect = accc.subprocess.TimeoutExpired("git fetch", 1) with self.assertRaises(RuntimeError): accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main") @patch("AddonCatalogCacheCreator.subprocess.run") + @patch("AddonCatalogCacheCreator.CacheWriter.sync_remote_url") @patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type") - def test_fetch_and_reset_raises_when_reset_fails(self, mock_ref_type, mock_run): + def test_fetch_and_reset_raises_when_reset_fails( + self, mock_ref_type, mock_sync_remote_url, mock_run + ): """A failed reset is reported as a RuntimeError so that the caller can re-clone.""" mock_ref_type.return_value = accc.GitRefType.BRANCH mock_run.side_effect = [MagicMock(returncode=0), MagicMock(returncode=1)] with self.assertRaises(RuntimeError): accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main") + @patch("AddonCatalogCacheCreator.subprocess.run") + def test_sync_remote_url_updates_and_warns_when_changed(self, mock_run): + """A repository that moved since the local clone was made (renamed, transferred to a new + owner, etc.) gets its origin remote repointed, with a warning logged so an operator can + tell from the build logs that this happened.""" + mock_run.side_effect = [ + MagicMock(returncode=0, stdout="https://old.url/repo.git\n"), + MagicMock(returncode=0), + ] + with patch("builtins.print") as mock_print: + accc.CacheWriter.sync_remote_url("TestMod", "https://new.url/repo.git") + self.assertEqual( + [ + ["git", "remote", "get-url", "origin"], + ["git", "remote", "set-url", "origin", "https://new.url/repo.git"], + ], + self.issued_commands(mock_run), + ) + warnings = [ + call.args[0] for call in mock_print.call_args_list if "WARNING" in call.args[0] + ] + self.assertTrue( + any( + "https://old.url/repo.git" in w and "https://new.url/repo.git" in w + for w in warnings + ) + ) + @patch("AddonCatalogCacheCreator.time.sleep") @patch("AddonCatalogCacheCreator.subprocess.run") def test_clone_or_update_raises_after_exhausting_clone_attempts(self, mock_run, mock_sleep): From 486bb354fe9e4a6de32247e8d6070200df95e55c Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Sun, 13 Sep 2026 18:41:03 +0200 Subject: [PATCH 07/10] ENH: remove traceback from cache error --- AddonCatalogCacheCreator.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index 16edd996..9ece337a 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -506,9 +506,8 @@ def create_local_copy_of_single_addon_with_zip( try: response = requests.get(catalog_entry.zip_url, timeout=10.0) except requests.exceptions.RequestException as e: - last_error_message = ( - f"Network error fetching {catalog_entry.zip_url}: {e}\n{traceback.format_exc()}" - ) + last_error_message = f"Network error fetching {catalog_entry.zip_url}: {e}" + print(traceback.format_exc(), flush=True) response = None else: if response.status_code == 200: @@ -524,7 +523,7 @@ def create_local_copy_of_single_addon_with_zip( if response is None: error_message = f"{last_error_message}\nafter {MAX_ATTEMPTS} attempts" - print(f"ERROR: {error_message}") + print(f"ERROR: {error_message}", flush=True) self.clone_errors[extract_to_dir] = error_message return @@ -543,11 +542,10 @@ def create_local_copy_of_single_addon_with_zip( zip_file.extractall(path=extract_to_dir) except (zipfile.BadZipFile, OSError) as e: error_message = ( - f"Downloaded zip data for {addon_id} from {catalog_entry.zip_url} is invalid: " - f"{e}\n{traceback.format_exc()}" + f"Downloaded zip data for {addon_id} from {catalog_entry.zip_url} is invalid" ) - print(f"ERROR: {error_message}") - self.clone_errors[extract_to_dir] = error_message + print(f"ERROR: {error_message}: {e}\n{traceback.format_exc()}", flush=True) + self.clone_errors[extract_to_dir] = f"{error_message}: {e}" @staticmethod def _tail(text: object, limit: int = 2000) -> str: From f0e964c4c0766ae3051810782872c266ddf76a22 Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Sun, 13 Sep 2026 18:49:25 +0200 Subject: [PATCH 08/10] MAINT: manually resorting imports as they were before --- AddonCatalogCacheCreator.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index 9ece337a..6a9b6d75 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -23,41 +23,43 @@ Intended to be run by a server-side systemd timer to generate a file that is then loaded by the Addon Manager in each FreeCAD installation.""" -import base64 import datetime +import shutil +import sys +from dataclasses import is_dataclass, fields +from typing import Any, Dict, List, Optional, Set, Tuple + +import base64 import enum import hashlib import io import json import os import re -import shutil +import requests # Audited: all subprocess calls in this module are fixed git argument lists run with no shell; # the variable arguments (url, branch, name) come from the addon index this tool exists to # process (added nosec B404, and B603/B607 at the call sites) import subprocess # nosec B404 -import sys import time import traceback import zipfile -from dataclasses import fields, is_dataclass -from types import SimpleNamespace -from typing import Any, Dict, List, Optional, Set, Tuple # Audited: only the exception class is imported, for catching errors raised by defusedxml, # which re-exports this same class. All parsing is done by defusedxml. (added nosec B405) from xml.etree.ElementTree import ParseError as XmlParseError # nosec B405 -import requests from defusedxml import DefusedXmlException -from scour import scour import AddonCatalog import addonmanager_icon_utilities as icon_utils import addonmanager_metadata import addonmanager_utilities as utils +from scour import scour +from types import SimpleNamespace + ADDON_CATALOG_URL = "https://raw.githubusercontent.com/FreeCAD/Addons/main/Data/Index.json" BASE_DIRECTORY = "./CatalogCache" MAX_COUNT = 10000 # Do at most this many repos (for testing purposes this can be made smaller) From 98d06aa6506f3d6dcd7e2109ba1f3acb9c0589b6 Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Sun, 13 Sep 2026 18:30:29 +0200 Subject: [PATCH 09/10] DEV: add vscode settings to gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 597d516f..f9307acd 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ # SPDX-FileNotice: Part of the AddonManager. .idea +.vscode __pycache* .DS_Store From fb119b0b61711aa13a715b05c01ce1ae0a3fc32d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 13 Sep 2026 16:55:43 +0000 Subject: [PATCH 10/10] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- AddonManagerTest/app/test_addon_catalog_cache_creator.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index c664796b..ffc090cd 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py @@ -573,9 +573,7 @@ def test_sync_remote_url_updates_and_warns_when_changed(self, mock_run): ], self.issued_commands(mock_run), ) - warnings = [ - call.args[0] for call in mock_print.call_args_list if "WARNING" in call.args[0] - ] + warnings = [call.args[0] for call in mock_print.call_args_list if "WARNING" in call.args[0]] self.assertTrue( any( "https://old.url/repo.git" in w and "https://new.url/repo.git" in w