diff --git a/Addon.py b/Addon.py index 3daad4d1..786c118a 100644 --- a/Addon.py +++ b/Addon.py @@ -29,7 +29,12 @@ from typing import Dict, Set, List, Optional from threading import Lock from enum import IntEnum, auto -from xml.etree.ElementTree import ParseError as XmlParseError + +# 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 + +from defusedxml import DefusedXmlException try: import importlib.metadata as importlib_metadata @@ -78,6 +83,12 @@ "web": "Web", } +# The package metadata content types that have a dedicated category in the Addon Manager's +# filter list. Every other content type, whether it is the standard "other" type or a type +# introduced after this version of the Addon Manager was released, is shown in the "Other" +# category. +CATEGORIZED_CONTENT_TYPES = frozenset(["workbench", "macro", "preferencepack", "bundle"]) + class Addon: """Encapsulates information about a FreeCAD addon""" @@ -179,6 +190,16 @@ 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 = "" + + # 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 @@ -325,11 +346,11 @@ def load_metadata_file(self, file: str) -> None: if os.path.exists(file): try: metadata = MetadataReader.from_file(file) - except XmlParseError: + except (XmlParseError, DefusedXmlException): 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() @@ -344,11 +365,11 @@ def _load_installed_metadata(self) -> None: if os.path.isfile(installed_metadata_path): try: self.installed_metadata = MetadataReader.from_file(installed_metadata_path) - except XmlParseError: + except (XmlParseError, DefusedXmlException): 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: @@ -484,17 +505,21 @@ def contains_macro(self) -> bool: return True return self.contains_packaged_content("macro") + def packaged_content_types(self) -> Set[str]: + """The content types declared by this package's metadata. Empty for anything that is + not a package.""" + if self.repo_type != Addon.Kind.PACKAGE: + return set() + if self.metadata is None: + fci.Console.PrintLog( + f"Addon Manager internal error: lost metadata for package {self.name}\n" + ) + return set() + return set(self.metadata.content) + def contains_packaged_content(self, content_type: str): """Determine if the package contains content_type""" - if self.repo_type == Addon.Kind.PACKAGE: - if self.metadata is None: - fci.Console.PrintLog( - f"Addon Manager internal error: lost metadata for package {self.name}\n" - ) - return False - content = self.metadata.content - return content_type in content - return False + return content_type in self.packaged_content_types() def contains_preference_pack(self) -> bool: """Determine if this package contains a preference pack""" @@ -505,8 +530,9 @@ def contains_bundle(self) -> bool: return self.contains_packaged_content("bundle") def contains_other(self) -> bool: - """Determine if this package contains an "other" content item""" - return self.contains_packaged_content("other") + """Determine if this package contains an "other" content item, or any content type that + this version of the Addon Manager does not have a category for.""" + return bool(self.packaged_content_types() - CATEGORIZED_CONTENT_TYPES) def walk_dependency_tree(self, all_repos: Dict[str, "Addon"], deps: Dependencies): """Compute the total dependency tree for this repo (recursive) @@ -688,7 +714,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: @@ -835,7 +863,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/AddonCatalog.py b/AddonCatalog.py index ab930e97..305433fa 100644 --- a/AddonCatalog.py +++ b/AddonCatalog.py @@ -25,7 +25,12 @@ import base64 import datetime import os -from xml.etree.ElementTree import ParseError as XmlParseError + +# 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 + +from defusedxml import DefusedXmlException from dataclasses import dataclass import json from hashlib import sha256 @@ -81,7 +86,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 @@ -138,26 +143,27 @@ 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 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: self._load_addon_metadata(addon, self.metadata) - except XmlParseError: + except (XmlParseError, DefusedXmlException): fci.Console.PrintWarning( "An invalid or corrupted package.xml file was installed " f"for {addon.display_name}\n" @@ -171,7 +177,7 @@ def instantiate_addon(self, addon_id: str) -> Addon: try: package_file = os.path.join(fci.DataPaths().mod_dir, addon_id, "package.xml") addon.installed_metadata = MetadataReader.from_file(package_file) - except (FileNotFoundError, XmlParseError, RuntimeError): + except (FileNotFoundError, XmlParseError, DefusedXmlException, RuntimeError): pass # If there was an error, just ignore it, no metadata is not fatal most_recent_mtime = AddonCatalogEntry.most_recent_mtime(addon_id) 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..36e0bed2 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -22,6 +22,7 @@ """Classes and utility functions to generate a remotely hosted cache of all addon catalog entries. 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 @@ -36,8 +37,18 @@ import os import re import requests -import subprocess -from xml.etree.ElementTree import ParseError as XmlParseError + +# 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 + +# 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 + +from defusedxml import DefusedXmlException import zipfile import AddonCatalog @@ -55,9 +66,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 +213,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 +234,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]: @@ -249,7 +265,7 @@ def get_git_info( results = [] for cmd in (hash_cmd, tag_cmd): try: - result = subprocess.run( + result = subprocess.run( # nosec B603 cmd, capture_output=True, text=True, @@ -312,7 +328,7 @@ def generate_cache_entry_from_package_xml( metadata = addonmanager_metadata.MetadataReader.from_bytes( cache_entry.package_xml.encode("utf-8") ) - except XmlParseError: + except (XmlParseError, DefusedXmlException): print(f"ERROR: Failed to parse XML from {path_to_package_xml}") return None except RuntimeError: @@ -496,7 +512,7 @@ def clone_or_update(self, name: str, url: str, branch: str) -> None: name, ] try: - completed_process = subprocess.run(command, timeout=CLONE_TIMEOUT) + 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.") @@ -537,18 +553,22 @@ def sparse_clone(self, name: str, url: str, branch: str, files: List[str]) -> No os.makedirs(clone_path) os.chdir(clone_path) try: - subprocess.run(["git", "init", "--quiet"], check=True) - subprocess.run(["git", "remote", "add", "origin", url], check=True) - subprocess.run(["git", "config", "core.sparsecheckout", "true"], check=True) + subprocess.run(["git", "init", "--quiet"], check=True) # nosec B603 B607 + subprocess.run( + ["git", "remote", "add", "origin", url], check=True + ) # nosec B603 B607 + subprocess.run( # nosec B603 B607 + ["git", "config", "core.sparsecheckout", "true"], check=True + ) with open(".git/info/sparse-checkout", "w") as f: f.write("\n".join(files)) f.write("\n") # So we are safe appending later - subprocess.run( + subprocess.run( # nosec B603 B607 ["git", "fetch", "--depth=1", "origin", branch], check=True, timeout=CLONE_TIMEOUT, ) - subprocess.run(["git", "checkout", branch], check=True) + subprocess.run(["git", "checkout", branch], check=True) # nosec B603 B607 except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: self.clone_errors[name] = str(e) print(f"ERROR: {e}") @@ -558,13 +578,17 @@ def sparse_clone(self, name: str, url: str, branch: str, files: List[str]) -> No cwd = os.getcwd() os.chdir(os.path.join(cwd, name)) try: - subprocess.run( + subprocess.run( # nosec B603 B607 ["git", "fetch", "--force", "--depth=1", "origin", branch], check=True, timeout=CLONE_TIMEOUT, ) - subprocess.run(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], check=True) - subprocess.run(["git", "clean", "-x", "-f", "-d", "--quiet"], check=True) + subprocess.run( # nosec B603 B607 + ["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], check=True + ) + subprocess.run( # nosec B603 B607 + ["git", "clean", "-x", "-f", "-d", "--quiet"], check=True + ) except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: self.clone_errors[name] = str(e) print(f"ERROR: {e}") @@ -580,7 +604,7 @@ def add_to_sparse_clone(self, name: str, files: List[str]) -> None: f.write("\n".join(files)) f.write("\n") # So we are safe appending later try: - subprocess.run(["git", "read-tree", "-m", "-u", "HEAD"], check=True) + subprocess.run(["git", "read-tree", "-m", "-u", "HEAD"], check=True) # nosec B603 B607 except subprocess.CalledProcessError as e: self.clone_errors[name] = str(e) print(f"ERROR: {e}") @@ -615,7 +639,9 @@ def fetch_and_reset(name: str, url: str, branch: str) -> None: if any of the git calls fails.""" try: - completed_process = subprocess.run(["git", "fetch", "--force"], timeout=CLONE_TIMEOUT) + completed_process = subprocess.run( # nosec B603 B607 + ["git", "fetch", "--force"], timeout=CLONE_TIMEOUT + ) except subprocess.TimeoutExpired: raise RuntimeError(f"git fetch for {name} timed out after {CLONE_TIMEOUT} seconds") if completed_process.returncode != 0: @@ -624,11 +650,15 @@ def fetch_and_reset(name: str, url: str, branch: str) -> None: 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(["git", "reset", "--hard", reset_target, "--quiet"]) + completed_process = subprocess.run( # nosec B603 B607 + ["git", "reset", "--hard", reset_target, "--quiet"] + ) if completed_process.returncode != 0: raise RuntimeError(f"git reset failed for {name} ref {reset_target}") - completed_process = subprocess.run(["git", "clean", "-x", "-f", "-d", "--quiet"]) + completed_process = subprocess.run( # nosec B603 B607 + ["git", "clean", "-x", "-f", "-d", "--quiet"] + ) if completed_process.returncode != 0: raise RuntimeError(f"git clean failed for {name}") @@ -637,16 +667,16 @@ def determine_git_ref_type(name: str, _url: str, branch: str) -> GitRefType: """Determine if the given branch, tag, or hash is a tag, branch, or hash. Returns the type if determinable, otherwise raises a RuntimeError.""" command = ["git", "show-ref", "--verify", f"refs/remotes/origin/{branch}"] - completed_process = subprocess.run(command, capture_output=True) + completed_process = subprocess.run(command, capture_output=True) # nosec B603 if completed_process.returncode == 0: return GitRefType.BRANCH command = ["git", "show-ref", "--tags"] - completed_process = subprocess.run(command, capture_output=True) + completed_process = subprocess.run(command, capture_output=True) # nosec B603 completed_process_output = completed_process.stdout.decode("utf-8") if branch in completed_process_output: return GitRefType.TAG command = ["git", "rev-parse", branch] - completed_process = subprocess.run(command) + completed_process = subprocess.run(command) # nosec B603 if completed_process.returncode == 0: return GitRefType.HASH raise RuntimeError( @@ -658,7 +688,7 @@ def determine_git_ref_type(name: str, _url: str, branch: str) -> GitRefType: def determine_last_commit_time() -> datetime.datetime: """Executed on the current working directory. Returns the time of the last commit.""" command = ["git", "log", "-1", "--format=%cd", "--date=iso-strict"] - completed_process = subprocess.run(command, capture_output=True) + completed_process = subprocess.run(command, capture_output=True) # nosec B603 completed_process_output = completed_process.stdout.decode("utf-8").strip() try: dt = datetime.datetime.fromisoformat(completed_process_output) diff --git a/AddonManager.py b/AddonManager.py index 245d503b..a34e2497 100644 --- a/AddonManager.py +++ b/AddonManager.py @@ -49,7 +49,6 @@ from composite_view import CompositeView from Widgets.addonmanager_widget_global_buttons import WidgetGlobalButtonBar from Widgets.addonmanager_widget_progress_bar import Progress -from Widgets.addonmanager_utility_dialogs import MessageDialog from package_list import PackageListItemModel from Addon import Addon, cycle_to_sub_addon, MissingDependencies from addonmanager_python_deps_gui import ( @@ -648,7 +647,7 @@ def post_startup(self) -> None: proceed = False all_deps = set() all_deps.update(deps.wbs) - all_deps.update(deps.external_addons) + all_deps.update([addon.name for addon in deps.external_addons]) all_deps.update(deps.python_requires) for dep in all_deps: if dep not in ignored_deps: @@ -673,7 +672,7 @@ def ignore_missing_dependencies(self): old_deps = set(old_deps_string.split(";") if old_deps_string else []) deps = self.check_missing_dependencies_worker.missing_dependencies new_deps = old_deps.union(deps.wbs) - new_deps = new_deps.union(deps.external_addons) + new_deps = new_deps.union([addon.name for addon in deps.external_addons]) new_deps = new_deps.union(deps.python_requires) new_deps_string = ";".join(new_deps) fci.Preferences().set("ignored_missing_deps", new_deps_string) diff --git a/AddonManagerTest/app/mocks.py b/AddonManagerTest/app/mocks.py index 0da8e255..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,26 +236,25 @@ 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 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 +267,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_addon.py b/AddonManagerTest/app/test_addon.py index f7abc78e..7ccc1f87 100644 --- a/AddonManagerTest/app/test_addon.py +++ b/AddonManagerTest/app/test_addon.py @@ -47,6 +47,25 @@ def test_display_name(self): self.assertEqual(addon.name, "FreeCAD") self.assertEqual(addon.display_name, "Test Workbench") + def test_load_metadata_file_ignores_xml_with_entity_declaration(self): + addon = Addon( + "FreeCAD", + "https://github.com/FreeCAD/FreeCAD", + Addon.Status.NOT_INSTALLED, + "master", + ) + xml_with_entity = ( + '\n' + ']>\n' + '&payload;\n' + ) + with tempfile.TemporaryDirectory() as temp_dir: + file_path = os.path.join(temp_dir, "package.xml") + with open(file_path, "w", encoding="utf-8") as f: + f.write(xml_with_entity) + addon.load_metadata_file(file_path) + self.assertIsNone(addon.metadata) + def test_git_url_cleanup(self): base_url = "https://github.com/FreeCAD/FreeCAD" test_urls = [f" {base_url} ", f"{base_url}.git", f" {base_url}.git "] @@ -158,6 +177,20 @@ def test_contains_functions(self): # There is no equivalent for preference packs, they are always accompanied by a # metadata file + def test_contains_other_includes_unrecognized_content(self): + addon = Addon( + "FreeCAD", + "https://github.com/FreeCAD/FreeCAD", + Addon.Status.NOT_INSTALLED, + "master", + ) + addon.load_metadata_file(os.path.join(self.test_dir, "unrecognized_content_only.xml")) + self.assertFalse(addon.contains_workbench()) + self.assertFalse(addon.contains_macro()) + self.assertFalse(addon.contains_preference_pack()) + self.assertFalse(addon.contains_bundle()) + self.assertTrue(addon.contains_other()) + def test_create_from_macro(self): macro_file = os.path.join(self.test_dir, "DoNothing.FCMacro") macro = Macro("DoNothing") 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, + } + ), ], } diff --git a/AddonManagerTest/app/test_addoncatalog.py b/AddonManagerTest/app/test_addoncatalog.py index 41cc2405..f14fd745 100644 --- a/AddonManagerTest/app/test_addoncatalog.py +++ b/AddonManagerTest/app/test_addoncatalog.py @@ -90,6 +90,68 @@ 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) + 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 + 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() + ) + 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.""" + 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.""" diff --git a/AddonManagerTest/app/test_cmake_file_lists.py b/AddonManagerTest/app/test_cmake_file_lists.py index 22add35e..4fa18ce6 100644 --- a/AddonManagerTest/app/test_cmake_file_lists.py +++ b/AddonManagerTest/app/test_cmake_file_lists.py @@ -28,7 +28,9 @@ import os import re -import subprocess + +# Audited: only runs a fixed git command against this repository (added nosec B404) +import subprocess # nosec B404 import unittest REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..")) @@ -71,16 +73,16 @@ def files_listed_in_cmake(cmake_path): """Return the set of file names referenced inside any SET() block. - Tokens are file names when they contain a dot or are the literal LICENSE; + Words are file names when they contain a dot or are the literal LICENSE; the SET variable names (for example AddonManager_SRCS) have neither and are skipped.""" with open(cmake_path, "r", encoding="utf-8") as cmake_file: contents = cmake_file.read() listed = set() for block in re.findall(r"SET\s*\((.*?)\)", contents, re.DOTALL | re.IGNORECASE): - for token in block.split(): - if "." in token or token == "LICENSE": - listed.add(token) + for word in block.split(): + if "." in word or word == "LICENSE": + listed.add(word) return listed @@ -91,7 +93,8 @@ def tracked_files_in(relative_directory): artifacts (cache archives, the CatalogCache and FreeCAD-macros trees, build output) from masquerading as un-registered source files.""" prefix = "" if relative_directory == "." else relative_directory.replace(os.sep, "/") + "/" - output = subprocess.run( + # Audited: fixed git command, no shell, local path prefix (added nosec B603, B607) + output = subprocess.run( # nosec B603 B607 ["git", "ls-files", "-z", f"{prefix}*"], cwd=REPO_ROOT, capture_output=True, diff --git a/AddonManagerTest/app/test_dependency_installer.py b/AddonManagerTest/app/test_dependency_installer.py index 9786d601..0d7db259 100644 --- a/AddonManagerTest/app/test_dependency_installer.py +++ b/AddonManagerTest/app/test_dependency_installer.py @@ -21,7 +21,9 @@ import functools import os -import subprocess + +# Audited: used only for its types in test mocks; nothing is executed (added nosec B404) +import subprocess # nosec B404 import tempfile from time import sleep import unittest diff --git a/AddonManagerTest/app/test_freecad_interface.py b/AddonManagerTest/app/test_freecad_interface.py index a8646062..6406abc1 100644 --- a/AddonManagerTest/app/test_freecad_interface.py +++ b/AddonManagerTest/app/test_freecad_interface.py @@ -26,7 +26,7 @@ import sys import tempfile import unittest -from unittest.mock import patch, MagicMock +import unittest.mock # pylint: disable=protected-access,import-outside-toplevel diff --git a/AddonManagerTest/app/test_git.py b/AddonManagerTest/app/test_git.py index 008c541c..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() @@ -78,6 +79,43 @@ 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_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 1108cade..53f69b06 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,72 @@ 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_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.""" + 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.""" + + 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_metadata.py b/AddonManagerTest/app/test_metadata.py index 911e770d..c739f01d 100644 --- a/AddonManagerTest/app/test_metadata.py +++ b/AddonManagerTest/app/test_metadata.py @@ -404,11 +404,22 @@ def test_parse_content_valid(self, mock_create_node): mock_create_node.reset_mock() @patch("addonmanager_metadata.MetadataReader._create_node") - def test_parse_content_invalid(self, mock_create_node): - content_item = "no_such_content_type" - tree_mock = [self.given_mock_tree_node(content_item, None)] + def test_parse_content_unknown_type(self, mock_create_node): + """Content types that this version of the Addon Manager does not know about are + still parsed, so that the metadata standard can be extended.""" + tree_mock = [self.given_mock_tree_node("no_such_content_type", None)] metadata_mock = MagicMock() amm.MetadataReader._parse_content("", metadata_mock, tree_mock) + mock_create_node.assert_called_once() + + @patch("addonmanager_metadata.MetadataReader._create_node") + def test_parse_content_foreign_namespace(self, mock_create_node): + """Elements from some other namespace are not content types and are skipped.""" + tree_mock = [self.given_mock_tree_node("{http://example.com/}workbench", None)] + metadata_mock = MagicMock() + amm.MetadataReader._parse_content( + "{https://wiki.freecad.org/Package_Metadata}", metadata_mock, tree_mock + ) mock_create_node.assert_not_called() @@ -502,6 +513,15 @@ def test_other(self): self.assertIn("other", metadata.content) self.assertEqual(len(metadata.content["other"]), 1) + def test_unrecognized_content_type_is_retained(self): + filename = os.path.join(self.test_data_dir, "unrecognized_content_only.xml") + metadata = amm.MetadataReader.from_file(filename) + self.assertIn("contenttypefromthefuture", metadata.content) + self.assertEqual(len(metadata.content["contenttypefromthefuture"]), 1) + self.assertEqual( + "Some Future Content", metadata.content["contenttypefromthefuture"][0].name + ) + def test_content_combination(self): filename = os.path.join(self.test_data_dir, "combination.xml") metadata = amm.MetadataReader.from_file(filename) diff --git a/AddonManagerTest/app/test_python_deps.py b/AddonManagerTest/app/test_python_deps.py index 391ad4fd..993c216f 100644 --- a/AddonManagerTest/app/test_python_deps.py +++ b/AddonManagerTest/app/test_python_deps.py @@ -20,23 +20,30 @@ ################################################################################ import os -import subprocess + +# Audited: used only for its types in test mocks; nothing is executed (added nosec B404) +import subprocess # nosec B404 +import tempfile import unittest from unittest.mock import MagicMock, patch from AddonManagerTest.app.mocks import SignalCatcher +from addonmanager_utilities import ProcessInterrupted from addonmanager_python_deps import ( + AsynchronousPipWorker, PackageInfo, + PipCommand, PythonPackageListModel, parse_pip_list_output, call_pip, PipFailed, + PipInterrupted, ) class TestPythonDepsStandaloneFunctions(unittest.TestCase): - @patch("addonmanager_python_deps.run_interruptable_subprocess") + @patch("addonmanager_python_deps.run_monitored_subprocess") def test_call_pip(self, mock_run_subprocess: MagicMock): mock_run_subprocess.return_value = MagicMock() mock_run_subprocess.return_value.returncode = 0 @@ -51,7 +58,7 @@ def test_call_pip_no_python(self, mock_get_python_exe: MagicMock): with self.assertRaises(PipFailed): call_pip(["arg1", "arg2", "arg3"]) - @patch("addonmanager_python_deps.run_interruptable_subprocess") + @patch("addonmanager_python_deps.run_monitored_subprocess") def test_call_pip_exception_raised(self, mock_run_subprocess: MagicMock): mock_run_subprocess.side_effect = subprocess.CalledProcessError( -1, "dummy_command", "Fake contents of stdout", "Fake contents of stderr" @@ -59,7 +66,24 @@ def test_call_pip_exception_raised(self, mock_run_subprocess: MagicMock): with self.assertRaises(PipFailed): call_pip(["arg1", "arg2", "arg3"]) - @patch("addonmanager_python_deps.run_interruptable_subprocess") + @patch("addonmanager_python_deps.run_monitored_subprocess") + def test_call_pip_interrupted(self, mock_run_subprocess: MagicMock): + """An interrupted pip call is reported as a cancellation, not a generic failure.""" + mock_run_subprocess.side_effect = ProcessInterrupted() + with self.assertRaises(PipInterrupted): + call_pip(["arg1", "arg2", "arg3"]) + + @patch("addonmanager_python_deps.run_monitored_subprocess") + def test_call_pip_passes_line_callback(self, mock_run_subprocess: MagicMock): + """The caller's line callback is handed to the subprocess runner so that pip output can + be displayed as it is produced.""" + mock_run_subprocess.return_value = MagicMock() + mock_run_subprocess.return_value.stdout = "" + callback = MagicMock() + call_pip(["list"], line_callback=callback) + self.assertIs(callback, mock_run_subprocess.call_args[1]["line_callback"]) + + @patch("addonmanager_python_deps.run_monitored_subprocess") def test_call_pip_splits_results(self, mock_run_subprocess: MagicMock): result_mock = MagicMock() result_mock.stdout = "\n".join(["Value 1", "Value 2", "Value 3"]) @@ -85,6 +109,22 @@ def test_parse_pip_list_output_all_packages_no_updates(self): self.assertEqual("41.2.0", results_list[1].installed_version) self.assertEqual("", results_list[1].available_version) + def test_parse_pip_list_output_ignores_pip_log_lines(self): + """Because pip's error output is merged into its standard output, log lines can appear + alongside the package table and must not be mistaken for packages.""" + results_list = parse_pip_list_output( + [ + "WARNING: Ignoring invalid distribution ~umpy", + "Package Version", + "---------- -------", + "gitdb 4.0.9", + "ERROR: something went wrong", + "setuptools 41.2.0", + ], + {}, + ) + self.assertEqual(["gitdb", "setuptools"], [package.name for package in results_list]) + def test_parse_pip_list_output_update_available_when_constrained_version_differs(self): """An update is available when the constrained version differs from what is installed; a package without a constraint, or already at its constrained version, shows no update.""" @@ -203,205 +243,388 @@ def test_determine_new_python_dependencies_single_addon_given(self): python_deps, ) - class TestUpdateMultiplePackages(unittest.TestCase): - @patch("addonmanager_python_deps.call_pip") - @patch("addonmanager_python_deps.fci.Console.PrintLog") - @patch("addonmanager_python_deps.fci.Console.PrintError") - def test_update_all_packages(self, mock_print_error, mock_print_log, mock_call_pip): - model = PythonPackageListModel([]) - model.vendor_path = "/vendor/path" - model.package_list = [ - PackageInfo("pkg1", "1", "2", []), - PackageInfo("pkg2", "1", "2", []), - ] - - model.update_all_packages() - - mock_call_pip.assert_called_once_with( - ["install", "--upgrade", "--target", "/vendor/path", "pkg1", "pkg2"] - ) - mock_print_log.assert_called_once() - mock_print_error.assert_not_called() - - @patch("addonmanager_python_deps.call_pip", side_effect=PipFailed("upgrade failed")) - @patch("addonmanager_python_deps.fci.Console.PrintLog") - @patch("addonmanager_python_deps.fci.Console.PrintError") - def test_update_packages_pip_failure(self, mock_print_error, mock_print_log, mock_call_pip): - model = PythonPackageListModel([]) - model.vendor_path = "/vendor/path" - model.package_list = [PackageInfo("pkg1", "1", "2", [])] - model.update_all_packages() +@patch("addonmanager_python_deps.get_pip_target_directory", return_value="/vendor/path") +@patch("addonmanager_python_deps.get_constraints") +@patch("addonmanager_python_deps.using_system_pip_installation_location", return_value=True) +class TestUpdateMultiplePackages(unittest.TestCase): + """Tests of the pip call used to install and update packages. The system installation + location is simulated, so no backup of the package directory is involved.""" + + @patch("addonmanager_python_deps.call_pip", return_value=[]) + @patch("addonmanager_python_deps.fci.Console.PrintLog") + @patch("addonmanager_python_deps.fci.Console.PrintError") + def test_update_all_packages(self, mock_print_error, mock_print_log, mock_call_pip, *_): + model = PythonPackageListModel([]) + model.vendor_path = "/vendor/path" + model.package_list = [ + PackageInfo("pkg1", "1", "2", []), + PackageInfo("pkg2", "1", "2", []), + ] - mock_call_pip.assert_called_once() - mock_print_error.assert_called_once_with("upgrade failed\n") + model.update_all_packages() - class TestCleanupOldPackageVersions(unittest.TestCase): - """Tests for the _cleanup_old_package_versions method""" + self.assertEqual( + [ + "install", + "--progress-bar", + "off", + "--upgrade", + "--target", + "/vendor/path", + "pkg1", + "pkg2", + ], + mock_call_pip.call_args_list[0][0][0], + ) + mock_print_log.assert_called_once() + mock_print_error.assert_not_called() - @patch("addonmanager_python_deps.os.path.exists") - @patch("addonmanager_python_deps.os.listdir") - @patch("addonmanager_python_deps.shutil.rmtree") - @patch("addonmanager_python_deps.fci.Console.PrintLog") - def test_cleanup_removes_old_versions_keeps_newest( - self, mock_print_log, mock_rmtree, mock_listdir, mock_exists - ): - """Test that old package versions are removed and newest is kept""" - mock_exists.return_value = True - mock_listdir.return_value = [ - "requests-2.28.0.dist-info", - "requests-2.31.0.dist-info", - "numpy-1.24.0.dist-info", - "numpy-1.26.0.dist-info", - "numpy-1.25.2.dist-info", - "other_file.txt", - ] + @patch("addonmanager_python_deps.call_pip", side_effect=PipFailed("upgrade failed")) + @patch("addonmanager_python_deps.fci.Console.PrintLog") + @patch("addonmanager_python_deps.fci.Console.PrintError") + def test_update_packages_pip_failure(self, mock_print_error, mock_print_log, mock_call_pip, *_): + model = PythonPackageListModel([]) + model.vendor_path = "/vendor/path" + model.package_list = [PackageInfo("pkg1", "1", "2", [])] - model = PythonPackageListModel([]) - model.vendor_path = "/fake/path" - model._cleanup_old_package_versions() - - # Should remove old versions but keep newest - self.assertEqual(mock_rmtree.call_count, 3) - removed_paths = [call[0][0] for call in mock_rmtree.call_args_list] - - # Check old versions were removed (works on all platforms) - self.assertIn(os.path.join("/fake/path", "requests-2.28.0.dist-info"), removed_paths) - self.assertIn(os.path.join("/fake/path", "numpy-1.24.0.dist-info"), removed_paths) - self.assertIn(os.path.join("/fake/path", "numpy-1.25.2.dist-info"), removed_paths) - - @patch("addonmanager_python_deps.os.path.exists") - @patch("addonmanager_python_deps.os.listdir") - @patch("addonmanager_python_deps.shutil.rmtree") - def test_cleanup_single_version_no_removal(self, mock_rmtree, mock_listdir, mock_exists): - """Test that packages with only one version are not touched""" - mock_exists.return_value = True - mock_listdir.return_value = [ - "requests-2.31.0.dist-info", - "numpy-1.26.0.dist-info", - "pandas-2.1.0.dist-info", - ] + model.update_all_packages() + + mock_call_pip.assert_called() + mock_print_error.assert_called_once_with("upgrade failed\n") - model = PythonPackageListModel([]) - model.vendor_path = "/fake/path" - model._cleanup_old_package_versions() - # No removals should happen when only one version exists per package - mock_rmtree.assert_not_called() +class TestAsynchronousPipWorker(unittest.TestCase): + """Tests of the worker that runs pip off the GUI thread.""" - @patch("addonmanager_python_deps.os.path.exists") - def test_cleanup_nonexistent_directory(self, mock_exists): - """Test graceful handling when vendor path doesn't exist""" - mock_exists.return_value = False + @patch("addonmanager_python_deps.fci.Console.PrintError") + @patch("addonmanager_python_deps.call_pip", side_effect=RuntimeError("something broke")) + def test_finished_is_emitted_after_an_unexpected_error(self, _mock_call_pip, _mock_print_error): + """Whatever goes wrong, the caller is told the run is over, so that it can restore the + package directory.""" + worker = AsynchronousPipWorker(PipCommand.Upgrade, ["pkg1"]) + catcher = SignalCatcher() + worker.finished.connect(catcher.catch_signal) - model = PythonPackageListModel([]) - model.vendor_path = "/nonexistent/path" - model._cleanup_old_package_versions() + worker.run() - @patch("addonmanager_python_deps.os.path.exists") - @patch("addonmanager_python_deps.os.listdir") - @patch("addonmanager_python_deps.shutil.rmtree") - def test_cleanup_empty_directory(self, mock_rmtree, mock_listdir, mock_exists): - """Test handling of empty vendor directory""" - mock_exists.return_value = True - mock_listdir.return_value = [] + self.assertTrue(catcher.caught) + self.assertIn("something broke", worker.error) + self.assertFalse(worker.is_running) - model = PythonPackageListModel([]) - model.vendor_path = "/fake/path" - model._cleanup_old_package_versions() - mock_rmtree.assert_not_called() - - @patch("addonmanager_python_deps.os.path.exists") - @patch("addonmanager_python_deps.os.listdir") - @patch("addonmanager_python_deps.os.path.isdir") - @patch("addonmanager_python_deps.shutil.rmtree") - @patch("addonmanager_python_deps.fci.Console.PrintWarning") - def test_cleanup_handles_permission_error( - self, mock_print_warning, mock_rmtree, mock_isdir, mock_listdir, mock_exists - ): - """Test that permission errors are handled gracefully""" - mock_exists.return_value = True - mock_listdir.return_value = [ - "requests-2.28.0.dist-info", - "requests-2.31.0.dist-info", - ] - mock_isdir.return_value = True - mock_rmtree.side_effect = PermissionError("Permission denied") + @patch("addonmanager_python_deps.fci.Console.PrintMessage") + @patch("addonmanager_python_deps.call_pip", side_effect=PipInterrupted("cancelled")) + def test_interrupted_installation_is_recorded_and_listing_skipped( + self, mock_call_pip, _mock_print_message + ): + worker = AsynchronousPipWorker(PipCommand.Install, ["pkg1"]) + catcher = SignalCatcher() + worker.finished.connect(catcher.catch_signal) - model = PythonPackageListModel([]) - model.vendor_path = "/fake/path" - model._cleanup_old_package_versions() - mock_print_warning.assert_called() - - @patch("addonmanager_python_deps.os.path.exists") - @patch("addonmanager_python_deps.os.listdir") - @patch("addonmanager_python_deps.os.path.isdir") - @patch("addonmanager_python_deps.shutil.rmtree") - def test_cleanup_normalizes_package_names( - self, mock_rmtree, mock_isdir, mock_listdir, mock_exists - ): - """Test that package names are normalized per PEP 503 (underscores to dashes)""" - mock_exists.return_value = True - mock_listdir.return_value = [ - "my_package-1.0.0.dist-info", - "my-package-2.0.0.dist-info", - ] - mock_isdir.return_value = True + worker.run() - model = PythonPackageListModel([]) - model.vendor_path = "/fake/path" - model._cleanup_old_package_versions() - - # Should remove old version (they're the same package after normalization) - mock_rmtree.assert_called_once_with( - os.path.join("/fake/path", "my_package-1.0.0.dist-info") - ) - - @patch("addonmanager_python_deps.os.path.exists") - @patch("addonmanager_python_deps.os.listdir") - @patch("addonmanager_python_deps.os.path.isdir") - @patch("addonmanager_python_deps.shutil.rmtree") - def test_cleanup_ignores_non_dist_info_directories( - self, mock_rmtree, mock_isdir, mock_listdir, mock_exists - ): - """Test that only .dist-info directories are processed""" - mock_exists.return_value = True - mock_listdir.return_value = [ - "requests-2.28.0.dist-info", - "requests-2.31.0.dist-info", - "some_package", - "__pycache__", - "random_file.txt", - ] - mock_isdir.return_value = True + self.assertTrue(worker.cancelled) + self.assertTrue(worker.error) + self.assertTrue(catcher.caught) + mock_call_pip.assert_called_once() - model = PythonPackageListModel([]) - model.vendor_path = "/fake/path" - model._cleanup_old_package_versions() - mock_rmtree.assert_called_once_with( - os.path.join("/fake/path", "requests-2.28.0.dist-info") - ) - - @patch("addonmanager_python_deps.os.path.exists") - @patch("addonmanager_python_deps.os.listdir") - @patch("addonmanager_python_deps.os.path.isdir") - @patch("addonmanager_python_deps.shutil.rmtree") - @patch("addonmanager_python_deps.fci.Console.PrintWarning") - def test_cleanup_handles_invalid_version_format( - self, mock_print_warning, mock_rmtree, mock_isdir, mock_listdir, mock_exists - ): - """Test handling of malformed version strings""" - mock_exists.return_value = True - mock_listdir.return_value = [ - "badpackage-invalid.version.dist-info", - "goodpackage-1.0.0.dist-info", - "goodpackage-2.0.0.dist-info", - ] - mock_isdir.return_value = True + @patch("addonmanager_python_deps.get_constraints") + @patch("addonmanager_python_deps.fci.Console.PrintLog") + def test_pip_output_is_reported_as_progress(self, _mock_print_log, _mock_get_constraints): + def fake_call_pip(args, line_callback=None): + if line_callback is not None: + line_callback("Collecting numpy") + line_callback(" ") + return [] - model = PythonPackageListModel([]) - model.vendor_path = "/fake/path" - model._cleanup_old_package_versions() - mock_rmtree.assert_called_once_with( - os.path.join("/fake/path", "goodpackage-1.0.0.dist-info") - ) + messages = [] + worker = AsynchronousPipWorker(PipCommand.Install, ["numpy"]) + worker.progress_message.connect(messages.append) + + with patch("addonmanager_python_deps.call_pip", side_effect=fake_call_pip): + worker.run() + + self.assertIn("Collecting numpy", messages) + self.assertNotIn(" ", messages) + + +@patch("addonmanager_python_deps.using_system_pip_installation_location", return_value=False) +class TestPackageDirectoryBackup(unittest.TestCase): + """Tests of the backup that protects the installed packages while pip runs.""" + + def setUp(self): + self.temp_directory = tempfile.TemporaryDirectory() + self.model = PythonPackageListModel([]) + self.model.vendor_path = os.path.join(self.temp_directory.name, "py311") + self.backup_path = self.model.vendor_path + ".old" + + def tearDown(self): + self.temp_directory.cleanup() + + @staticmethod + def _create_directory_containing(path: str, filename: str) -> None: + os.makedirs(path, exist_ok=True) + with open(os.path.join(path, filename), "w", encoding="utf-8") as marker: + marker.write("marker") + + def test_existing_directory_is_moved_aside(self, _mock_system_location): + self._create_directory_containing(self.model.vendor_path, "installed.txt") + + self.assertTrue(self.model._set_aside_package_directory()) + + self.assertEqual(self.backup_path, self.model.backup_path) + self.assertTrue(os.path.exists(os.path.join(self.backup_path, "installed.txt"))) + self.assertEqual([], os.listdir(self.model.vendor_path)) + + def test_missing_directory_is_created_without_a_backup(self, _mock_system_location): + self.assertTrue(self.model._set_aside_package_directory()) + + self.assertIsNone(self.model.backup_path) + self.assertTrue(os.path.isdir(self.model.vendor_path)) + + @patch("addonmanager_python_deps.fci.Console.PrintWarning") + def test_leftover_backup_is_recovered_when_packages_are_missing( + self, _mock_print_warning, _mock_system_location + ): + """A backup left behind by a run that never completed holds the only copy of the + packages, so it is put back rather than deleted.""" + self._create_directory_containing(self.backup_path, "installed.txt") + os.makedirs(self.model.vendor_path) + + self.assertTrue(self.model._set_aside_package_directory()) + + self.assertTrue(os.path.exists(os.path.join(self.backup_path, "installed.txt"))) + self.assertEqual([], os.listdir(self.model.vendor_path)) + + def test_leftover_backup_is_discarded_when_packages_are_present(self, _mock_system_location): + self._create_directory_containing(self.backup_path, "stale.txt") + self._create_directory_containing(self.model.vendor_path, "installed.txt") + + self.assertTrue(self.model._set_aside_package_directory()) + + self.assertTrue(os.path.exists(os.path.join(self.backup_path, "installed.txt"))) + self.assertFalse(os.path.exists(os.path.join(self.backup_path, "stale.txt"))) + + def test_failed_run_restores_the_backup(self, _mock_system_location): + self._create_directory_containing(self.model.vendor_path, "installed.txt") + self.model._set_aside_package_directory() + self.model.update_worker = MagicMock(error="pip call failed", is_running=False) + + self.model.finalize_package_directory() + + self.assertTrue(os.path.exists(os.path.join(self.model.vendor_path, "installed.txt"))) + self.assertFalse(os.path.exists(self.backup_path)) + self.assertIsNone(self.model.backup_path) + + def test_successful_run_discards_the_backup(self, _mock_system_location): + self._create_directory_containing(self.model.vendor_path, "installed.txt") + self.model._set_aside_package_directory() + self.model.update_worker = MagicMock(error="", is_running=False) + + self.model.finalize_package_directory() + + self.assertFalse(os.path.exists(self.backup_path)) + self.assertFalse(os.path.exists(os.path.join(self.model.vendor_path, "installed.txt"))) + self.assertIsNone(self.model.backup_path) + + @patch("addonmanager_python_deps.fci.Console.PrintError") + def test_backup_is_kept_while_pip_is_still_running( + self, _mock_print_error, _mock_system_location + ): + self._create_directory_containing(self.model.vendor_path, "installed.txt") + self.model._set_aside_package_directory() + self.model.update_worker = MagicMock(error="", is_running=True) + + self.model.finalize_package_directory() + + self.assertTrue(os.path.exists(os.path.join(self.backup_path, "installed.txt"))) + self.assertEqual(self.backup_path, self.model.backup_path) + + @patch("addonmanager_python_deps.fci.Console.PrintError") + @patch("addonmanager_python_deps.call_pip") + def test_installation_is_abandoned_when_the_backup_fails( + self, mock_call_pip, _mock_print_error, _mock_system_location + ): + """If the packages cannot be moved to safety then pip is not run at all, because a + failure would otherwise destroy them.""" + self._create_directory_containing(self.model.vendor_path, "installed.txt") + catcher = SignalCatcher() + self.model.update_complete.connect(catcher.catch_signal) + + with patch("addonmanager_python_deps.os.rename", side_effect=OSError("locked")): + self.model.install_packages(["pkg1"]) + + mock_call_pip.assert_not_called() + self.assertTrue(catcher.caught) + self.assertTrue(os.path.exists(os.path.join(self.model.vendor_path, "installed.txt"))) + + +class TestCleanupOldPackageVersions(unittest.TestCase): + """Tests for the _cleanup_old_package_versions method""" + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + @patch("addonmanager_python_deps.fci.Console.PrintLog") + def test_cleanup_removes_old_versions_keeps_newest( + self, mock_print_log, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test that old package versions are removed and newest is kept""" + mock_exists.return_value = True + mock_isdir.return_value = True + mock_listdir.return_value = [ + "requests-2.28.0.dist-info", + "requests-2.31.0.dist-info", + "numpy-1.24.0.dist-info", + "numpy-1.26.0.dist-info", + "numpy-1.25.2.dist-info", + "other_file.txt", + ] + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + + # Should remove old versions but keep newest + self.assertEqual(mock_rmtree.call_count, 3) + removed_paths = [call[0][0] for call in mock_rmtree.call_args_list] + + # Check old versions were removed (works on all platforms) + self.assertIn(os.path.join("/fake/path", "requests-2.28.0.dist-info"), removed_paths) + self.assertIn(os.path.join("/fake/path", "numpy-1.24.0.dist-info"), removed_paths) + self.assertIn(os.path.join("/fake/path", "numpy-1.25.2.dist-info"), removed_paths) + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_single_version_no_removal(self, mock_rmtree, mock_listdir, mock_exists): + """Test that packages with only one version are not touched""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "requests-2.31.0.dist-info", + "numpy-1.26.0.dist-info", + "pandas-2.1.0.dist-info", + ] + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + + # No removals should happen when only one version exists per package + mock_rmtree.assert_not_called() + + @patch("addonmanager_python_deps.os.path.exists") + def test_cleanup_nonexistent_directory(self, mock_exists): + """Test graceful handling when vendor path doesn't exist""" + mock_exists.return_value = False + + model = PythonPackageListModel([]) + model.vendor_path = "/nonexistent/path" + model._cleanup_old_package_versions() + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_empty_directory(self, mock_rmtree, mock_listdir, mock_exists): + """Test handling of empty vendor directory""" + mock_exists.return_value = True + mock_listdir.return_value = [] + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_rmtree.assert_not_called() + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + @patch("addonmanager_python_deps.fci.Console.PrintWarning") + def test_cleanup_handles_permission_error( + self, mock_print_warning, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test that permission errors are handled gracefully""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "requests-2.28.0.dist-info", + "requests-2.31.0.dist-info", + ] + mock_isdir.return_value = True + mock_rmtree.side_effect = PermissionError("Permission denied") + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_print_warning.assert_called() + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_normalizes_package_names( + self, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test that package names are normalized per PEP 503 (underscores to dashes)""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "my_package-1.0.0.dist-info", + "my-package-2.0.0.dist-info", + ] + mock_isdir.return_value = True + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + + # Should remove old version (they're the same package after normalization) + mock_rmtree.assert_called_once_with( + os.path.join("/fake/path", "my_package-1.0.0.dist-info") + ) + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + def test_cleanup_ignores_non_dist_info_directories( + self, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test that only .dist-info directories are processed""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "requests-2.28.0.dist-info", + "requests-2.31.0.dist-info", + "some_package", + "__pycache__", + "random_file.txt", + ] + mock_isdir.return_value = True + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_rmtree.assert_called_once_with(os.path.join("/fake/path", "requests-2.28.0.dist-info")) + + @patch("addonmanager_python_deps.os.path.exists") + @patch("addonmanager_python_deps.os.listdir") + @patch("addonmanager_python_deps.os.path.isdir") + @patch("addonmanager_python_deps.shutil.rmtree") + @patch("addonmanager_python_deps.fci.Console.PrintWarning") + def test_cleanup_handles_invalid_version_format( + self, mock_print_warning, mock_rmtree, mock_isdir, mock_listdir, mock_exists + ): + """Test handling of malformed version strings""" + mock_exists.return_value = True + mock_listdir.return_value = [ + "badpackage-invalid.version.dist-info", + "goodpackage-1.0.0.dist-info", + "goodpackage-2.0.0.dist-info", + ] + mock_isdir.return_value = True + + model = PythonPackageListModel([]) + model.vendor_path = "/fake/path" + model._cleanup_old_package_versions() + mock_rmtree.assert_called_once_with( + os.path.join("/fake/path", "goodpackage-1.0.0.dist-info") + ) diff --git a/AddonManagerTest/app/test_uninstaller.py b/AddonManagerTest/app/test_uninstaller.py index ae420d93..e32ab47c 100644 --- a/AddonManagerTest/app/test_uninstaller.py +++ b/AddonManagerTest/app/test_uninstaller.py @@ -22,6 +22,7 @@ """Contains the unit test class for addonmanager_uninstaller.py non-GUI functionality.""" import functools +import json import os from stat import S_IREAD, S_IRGRP, S_IROTH, S_IWUSR import tempfile @@ -162,6 +163,20 @@ def func(self, *args): _ = self.test_object.run() self.assertTrue(interceptor.called, "Failed to call uninstall script") + @patch("addonmanager_uninstaller.InstallationManifest") + def test_uninstall_skips_script_when_disabled(self, mock_install_manifest): + """Tests that run() does not call the uninstall.py script when it has been disabled""" + + calls = [] + with tempfile.TemporaryDirectory() as temp_dir: + toplevel_path = self.setup_dummy_installation(temp_dir) + self.test_object.run_uninstall_script = lambda *args: calls.append(args) + self.test_object.should_run_uninstall_script = False + _ = self.test_object.run() + self.assertFalse(calls, "Called uninstall script even though it was disabled") + self.assertFalse(os.path.exists(toplevel_path), "Failed to remove the addon") + self.assertIn("success", self.signals_caught) + def test_remove_extra_files_no_digest(self): """Tests that a lack of digest file is not an error, and nothing gets removed""" with tempfile.TemporaryDirectory() as temp_dir: @@ -364,6 +379,42 @@ def test_remove_macro_with_files(self): self.assertIn("success", self.signals_caught) self.assertIn("finished", self.signals_caught) + def test_remove_macro_removes_generated_toolbar_icon(self): + with tempfile.TemporaryDirectory() as temp_dir: + self.test_object.installation_location = temp_dir + self.mock_addon.macro.icon = "mock_icon_test.svg" + self.mock_addon.macro.install(temp_dir) + toolbar_icon = os.path.join(temp_dir, "MockMacro_icon.svg") + with open(toolbar_icon, "wb") as f: + f.write(b"Fake icon data generated by the toolbar button installer") + self.test_object.run() + self.assertFalse( + os.path.exists(toolbar_icon), + "Expected the generated toolbar icon to be removed, and it was not", + ) + self.assertNotIn("failure", self.signals_caught) + self.assertIn("success", self.signals_caught) + + def test_remove_macro_with_manifest_removes_generated_toolbar_icon(self): + with tempfile.TemporaryDirectory() as temp_dir: + self.test_object.installation_location = temp_dir + self.mock_addon.macro.xpm = "/*Fake XPM data*/" + self.mock_addon.macro.install(temp_dir) + macro_file = os.path.join(temp_dir, self.mock_addon.macro.filename) + manifest_file = macro_file + ".manifest" + with open(manifest_file, "w", encoding="utf-8") as f: + f.write(json.dumps([macro_file])) + toolbar_icon = os.path.join(temp_dir, "MockMacro_icon.xpm") + self.assertTrue(os.path.exists(toolbar_icon)) + self.test_object.run() + self.assertFalse( + os.path.exists(toolbar_icon), + "Expected the generated toolbar icon to be removed, and it was not", + ) + self.assertFalse(os.path.exists(manifest_file)) + self.assertNotIn("failure", self.signals_caught) + self.assertIn("success", self.signals_caught) + def test_remove_nonexistent_macro(self): with tempfile.TemporaryDirectory() as temp_dir: self.test_object.installation_location = temp_dir diff --git a/AddonManagerTest/app/test_utilities.py b/AddonManagerTest/app/test_utilities.py index 5b3c18ce..c4f0c856 100644 --- a/AddonManagerTest/app/test_utilities.py +++ b/AddonManagerTest/app/test_utilities.py @@ -24,7 +24,10 @@ import unittest from unittest.mock import MagicMock, patch, mock_open import os -import subprocess + +# Audited: subprocess is used only for its types; its execution entry points are mocked +# (added nosec B404) +import subprocess # nosec B404 import sys from AddonManagerTest.app.mocks import MockAddon as Addon @@ -47,6 +50,7 @@ git_host_of, identify_git_host, pep503_normalize, + points_at_a_repository, process_date_string_to_python_datetime, recognized_git_location, reload_git_hosts, @@ -54,8 +58,8 @@ resolve_constraints_location, run_interruptable_subprocess, run_monitored_subprocess, + should_use_git, ProcessInterrupted, - SubprocessTimeout, ) @@ -64,10 +68,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.""" @@ -76,6 +84,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 @@ -154,6 +163,61 @@ 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" + ) + 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", @@ -324,14 +388,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/AddonManagerTest/data/unrecognized_content_only.xml b/AddonManagerTest/data/unrecognized_content_only.xml new file mode 100644 index 00000000..aba0c551 --- /dev/null +++ b/AddonManagerTest/data/unrecognized_content_only.xml @@ -0,0 +1,20 @@ + + + Test Unrecognized Content + A package.xml file for unit testing. + 1.0.1 + 2022-01-07 + FreeCAD Developer + LGPL-2.1 + https://github.com/chennes/FreeCAD-Package + https://github.com/chennes/FreeCAD-Package/blob/main/README.md + + + + Some Future Content + A content type that this version of the Addon Manager knows nothing about. + TagFromTheFuture + + + + diff --git a/AddonManagerTest/gui/test_installer_gui.py b/AddonManagerTest/gui/test_installer_gui.py index f3543daa..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, @@ -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,191 @@ 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_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_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.""" + 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_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.""" + 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, gui.cancelling_dialog) + + 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.""" + 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.""" + 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/AddonManagerTest/gui/test_python_deps_gui.py b/AddonManagerTest/gui/test_python_deps_gui.py index 8dfbae59..1be25450 100644 --- a/AddonManagerTest/gui/test_python_deps_gui.py +++ b/AddonManagerTest/gui/test_python_deps_gui.py @@ -3,6 +3,7 @@ import sys import unittest +from unittest.mock import MagicMock from PySideWrapper import QtCore, QtWidgets @@ -15,6 +16,35 @@ class TestPythonPackageManagerGui(unittest.TestCase): def setUp(self) -> None: self.manager = PythonPackageManagerGui([]) + def test_stop_button_is_only_enabled_while_pip_runs(self): + self.manager._working(True) + self.assertTrue(self.manager.dlg.buttonCancel.isEnabled()) + self.manager._working(False) + self.assertFalse(self.manager.dlg.buttonCancel.isEnabled()) + + def test_progress_message_is_displayed(self): + self.manager._working(True) + self.manager._show_progress_message("Collecting numpy") + self.assertNotEqual("", self.manager.dlg.progressDetailsLabel.text()) + + def test_progress_message_is_cleared_when_the_run_ends(self): + self.manager._show_progress_message("Collecting numpy") + self.manager._working(False) + self.assertEqual("", self.manager.dlg.progressDetailsLabel.text()) + + def test_stop_button_cancels_the_run(self): + self.manager.model.cancel_update = MagicMock() + self.manager.dlg.buttonCancel.click() + self.manager.model.cancel_update.assert_called_once() + self.assertFalse(self.manager.dlg.buttonCancel.isEnabled()) + + def test_closing_the_dialog_waits_for_pip_to_stop(self): + """The model is destroyed with the dialog, so a running pip call must be stopped and its + backup dealt with before the dialog goes away.""" + self.manager.model.cancel_update = MagicMock() + self.manager.dlg.reject() + self.manager.model.cancel_update.assert_called_once_with(wait_for_completion=True) + if __name__ == "__main__": app = QtWidgets.QApplication(sys.argv) 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/AddonManagerTest/gui/test_uninstaller_gui.py b/AddonManagerTest/gui/test_uninstaller_gui.py index 8d9e16ce..1721a855 100644 --- a/AddonManagerTest/gui/test_uninstaller_gui.py +++ b/AddonManagerTest/gui/test_uninstaller_gui.py @@ -20,7 +20,10 @@ ################################################################################ import functools +import os +import tempfile import unittest +from unittest.mock import MagicMock, patch try: from PySide import QtCore, QtWidgets @@ -38,9 +41,9 @@ FakeWorker, MockThread, ) -from AddonManagerTest.app.mocks import MockAddon +from AddonManagerTest.app.mocks import MockAddon, MockMacro -from addonmanager_uninstaller_gui import AddonUninstallerGUI +from addonmanager_uninstaller_gui import AddonUninstallerGUI, UninstallScriptDialog translate = fci.translate @@ -132,8 +135,147 @@ def test_failure_dialog(self): self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + def test_toolbar_button_removed_for_macro(self): + macro_addon = MockAddon() + macro_addon.macro = MockMacro() + uninstaller_gui = AddonUninstallerGUI(macro_addon) + with patch("addonmanager_uninstaller_gui.fci.FreeCADGui", MagicMock()): + with patch("addonmanager_uninstaller_gui.ToolbarAdapter") as toolbar_adapter: + uninstaller_gui._remove_toolbar_button() + toolbar_adapter.return_value.remove_custom_toolbar_button.assert_called_once_with( + macro_addon.macro.filename + ) + + def test_toolbar_button_not_removed_for_non_macro(self): + with patch("addonmanager_uninstaller_gui.fci.FreeCADGui", MagicMock()): + with patch("addonmanager_uninstaller_gui.ToolbarAdapter") as toolbar_adapter: + self.uninstaller_gui._remove_toolbar_button() + toolbar_adapter.assert_not_called() + + def test_toolbar_button_not_removed_without_gui(self): + macro_addon = MockAddon() + macro_addon.macro = MockMacro() + uninstaller_gui = AddonUninstallerGUI(macro_addon) + with patch("addonmanager_uninstaller_gui.fci.FreeCADGui", None): + with patch("addonmanager_uninstaller_gui.ToolbarAdapter") as toolbar_adapter: + uninstaller_gui._remove_toolbar_button() + toolbar_adapter.assert_not_called() + + def test_toolbar_button_removal_failure_is_not_fatal(self): + macro_addon = MockAddon() + macro_addon.macro = MockMacro() + uninstaller_gui = AddonUninstallerGUI(macro_addon) + with patch("addonmanager_uninstaller_gui.fci.FreeCADGui", MagicMock()): + with patch( + "addonmanager_uninstaller_gui.ToolbarAdapter", + side_effect=RuntimeError("Unit test failure"), + ): + uninstaller_gui._remove_toolbar_button() # Should not raise + def test_finalize(self): self.uninstaller_gui.finished.connect(functools.partial(self.catch_signal, "finished")) self.uninstaller_gui.worker_thread = MockThread() self.uninstaller_gui._finalize() self.assertIn("finished", self.signals_caught) + + def setup_installation_with_script(self, temp_dir) -> str: + """Create a fake installed addon whose uninstall.py writes a marker file when run. + Returns the path of the marker file.""" + addon_path = os.path.join(temp_dir, self.addon_to_remove.name) + os.makedirs(addon_path) + marker_file = os.path.join(temp_dir, "RAN_UNINSTALL_SCRIPT.txt") + double_escaped = marker_file.replace("\\", "\\\\") + with open(os.path.join(addon_path, "uninstall.py"), "w", encoding="utf-8") as f: + f.write( + f"""# Mock uninstall script +with open('{double_escaped}', "w", encoding="utf-8") as f: + f.write("File created by uninstall.py from unit tests") +""" + ) + self.uninstaller_gui.uninstaller.installation_path = temp_dir + return marker_file + + def test_uninstall_script_no_prompt_without_script(self): + with tempfile.TemporaryDirectory() as temp_dir: + os.makedirs(os.path.join(temp_dir, self.addon_to_remove.name)) + self.uninstaller_gui.uninstaller.installation_path = temp_dir + with patch("addonmanager_uninstaller_gui.UninstallScriptDialog") as mock_dialog: + self.uninstaller_gui._handle_uninstall_script() + mock_dialog.assert_not_called() + self.assertFalse(self.uninstaller_gui.uninstaller.should_run_uninstall_script) + + def test_uninstall_script_no_prompt_for_macro(self): + macro_addon = MockAddon() + macro_addon.macro = MockMacro() + uninstaller_gui = AddonUninstallerGUI(macro_addon) + with patch("addonmanager_uninstaller_gui.UninstallScriptDialog") as mock_dialog: + uninstaller_gui._handle_uninstall_script() + mock_dialog.assert_not_called() + + def test_uninstall_script_prompt_declined(self): + with tempfile.TemporaryDirectory() as temp_dir: + marker_file = self.setup_installation_with_script(temp_dir) + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.No + ) + self.uninstaller_gui._handle_uninstall_script() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertFalse( + os.path.exists(marker_file), "Ran the uninstall script without permission" + ) + self.assertFalse(self.uninstaller_gui.uninstaller.should_run_uninstall_script) + + def test_uninstall_script_prompt_accepted(self): + with tempfile.TemporaryDirectory() as temp_dir: + marker_file = self.setup_installation_with_script(temp_dir) + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.Yes + ) + self.uninstaller_gui._handle_uninstall_script() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertTrue( + os.path.exists(marker_file), "Failed to run the approved uninstall script" + ) + self.assertFalse(self.uninstaller_gui.uninstaller.should_run_uninstall_script) + + +class TestUninstallScriptDialog(unittest.TestCase): + + MODULE = "test_uninstaller_gui" # file name without extension + + def setUp(self): + self.dialog = UninstallScriptDialog("Mock Addon", os.path.join("path", "uninstall.py")) + + def tearDown(self): + self.dialog.close() + del self.dialog # Immediately destroy the widget so no top-level window leaks + + def test_run_button_requests_run(self): + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.Yes + ) + self.dialog.exec() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertTrue(self.dialog.run_requested) + + def test_do_not_run_button_does_not_request_run(self): + dialog_watcher = DialogWatcher( + "AddonManager_RunUninstallScriptDialog", QtWidgets.QDialogButtonBox.No + ) + self.dialog.exec() + self.assertTrue(dialog_watcher.dialog_found, "Failed to find the expected dialog box") + self.assertTrue(dialog_watcher.button_found, "Failed to find the expected button") + self.assertFalse(self.dialog.run_requested) + + def test_open_button_opens_editor_without_closing_dialog(self): + with patch("addonmanager_uninstaller_gui.open_file_in_text_editor") as mock_open: + self.dialog.show() + self.dialog.open_button.click() + self.assertTrue(self.dialog.isVisible(), "Open button should not close the dialog") + self.dialog.skip_button.click() + self.assertFalse(self.dialog.isVisible()) + mock_open.assert_called_once_with(self.dialog.script_path) + self.assertFalse(self.dialog.run_requested) 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/PythonDependencyUpdateDialog.ui b/PythonDependencyUpdateDialog.ui index 46bab399..b7cddec1 100644 --- a/PythonDependencyUpdateDialog.ui +++ b/PythonDependencyUpdateDialog.ui @@ -63,6 +63,22 @@ + + + + + 0 + 0 + + + + + + + false + + + @@ -89,6 +105,16 @@ + + + + Stop + + + Stop the running pip installation or update + + + diff --git a/Resources/translations/run_translation_cycle.py b/Resources/translations/run_translation_cycle.py index ed543623..07ad10be 100644 --- a/Resources/translations/run_translation_cycle.py +++ b/Resources/translations/run_translation_cycle.py @@ -29,7 +29,10 @@ import os import shutil import stat -import subprocess + +# Audited: fixed lrelease/lupdate commands operating on local repository files, no shell +# (added nosec B404) +import subprocess # nosec B404 import sys import tempfile import time @@ -87,8 +90,9 @@ def _make_api_req(self, url, extra_headers=None, method="GET", data=None): parsed_url = urlparse(url) if parsed_url.scheme != "https": raise Exception("API requests must be made over HTTPS") - request = Request(url, headers=headers, method=method, data=data) - request_result = urlopen(request) + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + request = Request(url, headers=headers, method=method, data=data) # nosec B310 + request_result = urlopen(request) # nosec B310 if request_result.getcode() >= 300: print(f"Failed to make API request {url}: return code {request_result.getcode()}") raise Exception("Failed to make API request") @@ -143,7 +147,8 @@ def download(self, build_id): if parsed_url.scheme != "https": raise Exception("API requests must be made over HTTPS") - urlretrieve(response["url"], filename) + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + urlretrieve(response["url"], filename) # nosec B310 print("download of " + filename + " complete") def build(self): @@ -193,7 +198,7 @@ def process_single_translation_file(source_path: str, target_path: str): print("Generating qm file for", basename, "...") try: - subprocess.run( + subprocess.run( # nosec B603 B607 [ "lrelease", new_path, @@ -356,7 +361,7 @@ def run_and_download_build(crowdin_updater: CrowdinUpdater): "-ts", os.path.join(TS_FILE_PATH, CROWDIN_FILE_NAME), ] - result = subprocess.run( + result = subprocess.run( # nosec B603 args, timeout=30, check=True, diff --git a/addonmanager_dependency_installer.py b/addonmanager_dependency_installer.py index 288cdba5..da892fad 100644 --- a/addonmanager_dependency_installer.py +++ b/addonmanager_dependency_installer.py @@ -22,7 +22,10 @@ """Class to manage installation of sets of Python dependencies.""" import os -import subprocess + +# Audited: subprocess is used only for its types; commands run through the audited wrappers +# in addonmanager_utilities (added nosec B404) +import subprocess # nosec B404 from typing import List import addonmanager_freecad_interface as fci diff --git a/addonmanager_git.py b/addonmanager_git.py index 715ae5ff..faeb74d1 100644 --- a/addonmanager_git.py +++ b/addonmanager_git.py @@ -26,8 +26,10 @@ import os import platform import shutil -import subprocess -from typing import List, Dict, Optional + +# Audited: subprocess calls use fixed argument lists and no shell (added nosec B404) +import subprocess # nosec B404 +from typing import Callable, List, Dict, Optional import time import addonmanager_utilities as utils @@ -44,6 +46,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%(" @@ -79,18 +87,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 +147,22 @@ 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 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( @@ -156,7 +177,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 +189,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 +219,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() @@ -450,8 +468,11 @@ def _git_is_real() -> bool: on the Mac actually requires us to check for that installation.""" try: # Get the path to git from xcrun + # Audited: fixed arguments, no shell, no untrusted input (added nosec B603, B607) git_path = ( - subprocess.check_output(["xcrun", "--find", "git"], stderr=subprocess.DEVNULL) + subprocess.check_output( # nosec B603 B607 + ["xcrun", "--find", "git"], stderr=subprocess.DEVNULL + ) .decode() .strip() ) @@ -459,7 +480,8 @@ def _git_is_real() -> bool: return False # Check if running git triggers version output - result = subprocess.run( + # Audited: runs the git that xcrun reported, fixed arguments, no shell (added nosec B603) + result = subprocess.run( # nosec B603 [git_path, "--version"], stdout=subprocess.PIPE, stderr=subprocess.DEVNULL ) return result.returncode == 0 @@ -471,19 +493,30 @@ 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( + 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 5c6944c5..e5ac7597 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 @@ -41,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: @@ -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, @@ -138,8 +146,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 @@ -193,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]: @@ -238,9 +252,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 +279,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) @@ -281,18 +296,35 @@ 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 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 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 @@ -324,7 +356,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): diff --git a/addonmanager_installer_gui.py b/addonmanager_installer_gui.py index d7f73b78..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 @@ -44,6 +45,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 +83,9 @@ 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.installation_method = InstallationMethod.ANY self.worker_thread = None # Set up the installer connections @@ -114,44 +131,125 @@ 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.worker_thread.started.connect(self.installer.run) + self.installer.progress_message.connect(self._progress_message) + self.worker_thread.started.connect(partial(self.installer.run, install_method)) + self.create_installing_dialog() + self.installer.finished.connect(self.installing_dialog.hide) + 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.installing_dialog.label.setText( - 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) - 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: + """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) - - def _cancel_addon_installation(self): - dlg = QtWidgets.QMessageBox( - QtWidgets.QMessageBox.NoIcon, + 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.""" + 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 _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.""" + if self._is_an_update(): + message = translate("AddonsInstaller", "Cancelling the update of '{}'…") + else: + message = translate("AddonsInstaller", "Cancelling the installation of '{}'…") + self.cancelling_dialog = self._create_busy_dialog( + "AddonInstaller_CancellingDialog", translate("AddonsInstaller", "Cancelling"), - translate("AddonsInstaller", "Cancelling installation of '{}'").format( - self.addon_to_install.display_name - ), - QtWidgets.QMessageBox.NoButton, - parent=utils.get_main_am_window(), + message.format(self.addon_to_install.display_name), ) - dlg.setObjectName("AddonInstaller_CancellingDialog") - dlg.show() + + def _cancel_addon_installation(self): + 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. @@ -162,10 +260,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) + self.cancelling_dialog.hide() self.finished.emit() + 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", "Removing the partial download of {} 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( @@ -180,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) diff --git a/addonmanager_metadata.py b/addonmanager_metadata.py index 0b92ff66..52eb53ce 100644 --- a/addonmanager_metadata.py +++ b/addonmanager_metadata.py @@ -379,15 +379,17 @@ def _parse_dependency(child: ET.Element) -> Dependency: @staticmethod def _parse_content(namespace: str, metadata: Metadata, root: ET.Element): - """Given a content node, loop over its children, and if they are a recognized - element type, recurse into each one to parse it.""" - known_content_types = ["workbench", "macro", "preferencepack", "bundle", "other"] + """Given a content node, loop over its children and recurse into each one to parse it. + Every child element is treated as a content type, including types that this version of + the Addon Manager does not know about, so that new types added to the package metadata + standard are still available to callers.""" for child in root: + if not isinstance(child.tag, str) or not child.tag.startswith(namespace): + continue content_type = child.tag[len(namespace) :] - if content_type in known_content_types: - if content_type not in metadata.content: - metadata.content[content_type] = [] - metadata.content[content_type].append(MetadataReader._create_node(namespace, child)) + metadata.content.setdefault(content_type, []).append( + MetadataReader._create_node(namespace, child) + ) @staticmethod def _create_node(namespace, child) -> Metadata: diff --git a/addonmanager_python_deps.py b/addonmanager_python_deps.py index b0296158..d6bf800c 100644 --- a/addonmanager_python_deps.py +++ b/addonmanager_python_deps.py @@ -27,13 +27,17 @@ import os import re import shutil -import subprocess -from typing import Dict, Iterable, List, TypedDict, Optional, Set + +# Audited: subprocess is used only for its CalledProcessError exception type; commands run +# through the audited wrappers in addonmanager_utilities (added nosec B404) +import subprocess # nosec B404 +from typing import Callable, Dict, Iterable, List, TypedDict, Optional, Set from enum import Enum from addonmanager_metadata import Version from addonmanager_utilities import ( + ProcessInterrupted, create_pip_call, - run_interruptable_subprocess, + run_monitored_subprocess, get_pip_target_directory, pep503_normalize, translate, @@ -48,31 +52,38 @@ translate = fci.translate +BACKUP_SUFFIX = ".old" +CANCELLATION_TIMEOUT_MS = 10000 + + class PipFailed(Exception): - """Exception thrown when pip times out or otherwise fails to return valid results""" + """Exception thrown when pip fails to return valid results""" + + +class PipInterrupted(PipFailed): + """Exception thrown when a pip call is stopped by an interruption request.""" -def call_pip(args: List[str]) -> List[str]: +def call_pip(args: List[str], line_callback: Optional[Callable[[str], None]] = None) -> List[str]: """Tries to locate the appropriate Python executable and run pip with version checking - disabled. Fails if Python can't be found or if pip is not installed.""" + disabled. Fails if Python can't be found or if pip is not installed. Each line of output is + passed to line_callback as it is produced, if a callback is provided.""" try: call_args = create_pip_call(args) - fci.Console.PrintLog(f"Running pip with the following command:\n") + fci.Console.PrintLog("Running pip with the following command:\n") fci.Console.PrintLog(" ".join(call_args) + "\n") except RuntimeError as exception: raise PipFailed() from exception try: - proc = run_interruptable_subprocess(call_args, timeout_secs=None) + proc = run_monitored_subprocess(call_args, line_callback=line_callback) + except ProcessInterrupted as exception: + raise PipInterrupted("The pip call was cancelled") from exception except subprocess.CalledProcessError as exception: raise PipFailed(f"pip call failed:\n{exception}") from exception - if proc.returncode != 0: - raise PipFailed(proc.stderr) - - data = proc.stdout - return data.split("\n") + return proc.stdout.split("\n") @dataclasses.dataclass @@ -83,10 +94,14 @@ class PackageInfo: dependencies: List[str] +LOG_LINE_PREFIXES = ("WARNING:", "ERROR:", "DEPRECATION:", "NOTICE:") + + def parse_pip_list_output(all_packages, constrained_versions: Dict[str, str]) -> List[PackageInfo]: """Parse 'pip list --path' output into package information, marking an update as available whenever the vetted (constrained) version differs from the installed one. The pip output - should be an array of lines of text. + should be an array of lines of text. Anything before the underlined header, and any log line + that pip mixed into its output, is ignored. All Packages output looks like this: Package Version @@ -96,10 +111,12 @@ def parse_pip_list_output(all_packages, constrained_versions: Dict[str, str]) -> """ packages: Dict[str, PackageInfo] = {} - skip_counter = 0 + header_seen = False for line in all_packages: - if skip_counter < 2: - skip_counter += 1 + if line.startswith(LOG_LINE_PREFIXES): + continue + if not header_seen: + header_seen = line.startswith("---") continue entries = line.split() if len(entries) > 1: @@ -133,6 +150,7 @@ class AsynchronousPipWorker(QtCore.QObject): """A worker class that runs pip to install/update/list packages.""" finished = QtCore.Signal() + progress_message = QtCore.Signal(str) # A line of pip output, or a status message def __init__( self, @@ -143,18 +161,27 @@ def __init__( super().__init__(parent) self.is_running = False self.error = "" + self.cancelled = False self.vendor_path = get_pip_target_directory() self.package_list = package_list or [] self.command = command def run(self): - """Runs pip: when complete, either self.package_list is populated, or self.error is set.""" + """Runs pip: when complete, either self.package_list is populated, or self.error is set. + The finished signal is emitted no matter how the run ends, so that callers can always + rely on it to restore whatever state they set up before starting the run.""" self.is_running = True self.error = "" + self.cancelled = False - if self.command in (PipCommand.Upgrade, PipCommand.Install): - self._install_or_update() - self._list() + try: + if self.command in (PipCommand.Upgrade, PipCommand.Install): + self._install_or_update() + if not self.cancelled: + self._list() + except Exception as e: + self.error = f"Unexpected failure while running pip: {e}" + fci.Console.PrintError(f"{self.error}\n") self.is_running = False self.finished.emit() @@ -167,23 +194,37 @@ def _install_or_update(self) -> None: action = "install" if self.command == PipCommand.Install else "upgrade" log_message = f"Running pip to {action} the following packages in {self.vendor_path}: {update_string}\n" upgrade = ["--upgrade"] if self.command == PipCommand.Upgrade else [] - command = ["install", *upgrade, "--target", self.vendor_path] + command = ["install", "--progress-bar", "off", *upgrade, "--target", self.vendor_path] command.extend(self.package_list) fci.Console.PrintLog(f"{log_message}\n") + self.progress_message.emit(translate("AddonsInstaller", "Starting pip")) try: - upgrade_stdout = call_pip(command) + upgrade_stdout = call_pip(command, line_callback=self._report_progress) for line in upgrade_stdout: fci.Console.PrintLog(f"{line}\n") + except PipInterrupted as e: + self.cancelled = True + self.error = str(e) + fci.Console.PrintMessage(f"{self.error}\n") except PipFailed as e: self.error = str(e) fci.Console.PrintError(f"{self.error}\n") + def _report_progress(self, line: str) -> None: + """Forward a non-empty line of pip output to anyone displaying progress.""" + stripped_line = line.strip() + if stripped_line: + self.progress_message.emit(stripped_line) + def _list(self) -> None: try: all_packages_stdout = call_pip(["list", "--path", self.vendor_path]) constrained_versions = get_constraints().constrained_versions() self.package_list = parse_pip_list_output(all_packages_stdout, constrained_versions) + except PipInterrupted as e: + self.cancelled = True + self.error = str(e) except PipFailed as e: self.error = str(e) @@ -194,6 +235,7 @@ class PythonPackageListModel(QtCore.QAbstractTableModel): for the Qt view.""" update_complete = QtCore.Signal() + progress_message = QtCore.Signal(str) def __init__(self, addons): super().__init__() @@ -205,6 +247,7 @@ def __init__(self, addons): self.update_worker = None self.reset_worker_thread = None self.update_worker_thread = None + self.backup_path = None def can_use_thread(self) -> bool: threaded = ( @@ -219,6 +262,7 @@ def reset_package_list(self): self.beginResetModel() self.package_list.clear() self.reset_worker = AsynchronousPipWorker(PipCommand.List) + self.reset_worker.progress_message.connect(self.progress_message) if self.can_use_thread(): self.reset_worker_thread = QtCore.QThread() self.reset_worker.moveToThread(self.reset_worker_thread) @@ -319,12 +363,11 @@ def install_packages(self, packages: list[str]) -> None: def _install_or_update_packages(self, packages: list[str], command: PipCommand) -> None: """Installs/Upgrade packages. Uses an asynchronous thread when possible.""" + if not using_system_pip_installation_location() and not self._set_aside_package_directory(): + self.update_complete.emit() + return self.update_worker = AsynchronousPipWorker(command, packages) - if not using_system_pip_installation_location(): - # pip doesn't properly update when using the target directory, so we have to delete - # it and reinstall - os.rename(self.vendor_path, self.vendor_path + ".old") - os.mkdir(self.vendor_path) + self.update_worker.progress_message.connect(self.progress_message) if self.can_use_thread(): self.update_worker_thread = QtCore.QThread() self.update_worker.moveToThread(self.update_worker_thread) @@ -337,25 +380,158 @@ def _install_or_update_packages(self, packages: list[str], command: PipCommand) self.update_call_finished() def update_call_finished(self): + """Put the package directory into its final state, then report that the run is over.""" + self.finalize_package_directory() self.update_complete.emit() - if not using_system_pip_installation_location(): - if self.update_worker.error: - try: - os.rename(self.vendor_path + ".old", self.vendor_path) - except Exception as err: - fci.Console.PrintError(f"Backup restore failed: {self.vendor_path}.old.\n") - fci.Console.PrintError(f"{err}\n") + + def cancel_update(self, wait_for_completion: bool = False) -> None: + """Ask any running pip call to stop. When wait_for_completion is set, the call blocks + until the worker has stopped and the package directory has been dealt with, which is + required when the caller is about to destroy this model.""" + for thread in (self.update_worker_thread, self.reset_worker_thread): + if thread is not None and thread.isRunning(): + thread.requestInterruption() + if not wait_for_completion: + return + for worker, thread in ( + (self.update_worker, self.update_worker_thread), + (self.reset_worker, self.reset_worker_thread), + ): + if thread is None or not thread.isRunning(): + continue + worker.blockSignals(True) + thread.quit() + if not thread.wait(CANCELLATION_TIMEOUT_MS): + fci.Console.PrintWarning( + translate("AddonsInstaller", "A pip call did not stop when asked to") + "\n" + ) + self.finalize_package_directory() + + def finalize_package_directory(self) -> None: + """Restore the backup of the package directory if the run failed or was cancelled, and + discard it if the run succeeded. Does nothing if no backup was made.""" + if self.backup_path is None: + return + if self.update_worker is not None and self.update_worker.is_running: + fci.Console.PrintError( + translate( + "AddonsInstaller", + "pip is still running, so the Python packages were left in {}", + ).format(self.backup_path) + + "\n" + ) + return + if self.update_worker is not None and self.update_worker.error: + self._restore_package_directory_backup() + else: + self._discard_package_directory_backup() + self._cleanup_old_package_versions() + + def _set_aside_package_directory(self) -> bool: + """Move the existing package directory aside so that it can be restored if pip does not + succeed, because pip cannot reliably upgrade in place when installing to a target + directory. Returns True if the installation may proceed.""" + backup_path = self.vendor_path + BACKUP_SUFFIX + self.backup_path = None + if os.path.exists(backup_path): + self._resolve_leftover_backup(backup_path) + if not os.path.exists(self.vendor_path): + try: + os.makedirs(self.vendor_path) + except OSError as err: + fci.Console.PrintError( + translate( + "AddonsInstaller", "Failed to create the Python package directory {}" + ).format(self.vendor_path) + + f"\n{err}\n" + ) + return False + return True + try: + os.rename(self.vendor_path, backup_path) + except OSError as err: + fci.Console.PrintError( + translate( + "AddonsInstaller", + "Failed to back up the Python package directory {}, so no packages were" + " installed or updated", + ).format(self.vendor_path) + + f"\n{err}\n" + ) + return False + try: + os.mkdir(self.vendor_path) + except OSError as err: + fci.Console.PrintError(f"{err}\n") + self.backup_path = backup_path + self._restore_package_directory_backup() + return False + self.backup_path = backup_path + return True + + def _resolve_leftover_backup(self, backup_path: str) -> None: + """Deal with a backup left behind by a run that never completed: it is put back when the + package directory is missing or empty, and discarded otherwise.""" + try: + if not os.path.exists(self.vendor_path): + os.rename(backup_path, self.vendor_path) + elif not os.listdir(self.vendor_path): + os.rmdir(self.vendor_path) + os.rename(backup_path, self.vendor_path) else: - shutil.rmtree(self.vendor_path + ".old") - # Clean up old package versions that may remain after update - self._cleanup_old_package_versions() + shutil.rmtree(backup_path) + return + fci.Console.PrintWarning( + translate( + "AddonsInstaller", + "Recovered the Python packages left in {} by an interrupted update", + ).format(backup_path) + + "\n" + ) + except OSError as err: + fci.Console.PrintError(f"{err}\n") + + def _restore_package_directory_backup(self) -> None: + """Put the backed-up package directory back after a failed or cancelled run.""" + backup_path = self.backup_path + self.backup_path = None + try: + if os.path.exists(self.vendor_path): + shutil.rmtree(self.vendor_path) + os.rename(backup_path, self.vendor_path) + return + except OSError as err: + fci.Console.PrintError(f"{err}\n") + try: + shutil.copytree(backup_path, self.vendor_path, dirs_exist_ok=True) + except OSError as err: + fci.Console.PrintError( + translate( + "AddonsInstaller", + "Failed to restore the Python packages: they remain in {}", + ).format(backup_path) + + f"\n{err}\n" + ) + + def _discard_package_directory_backup(self) -> None: + """Remove the backup of the package directory after a successful run.""" + backup_path = self.backup_path + self.backup_path = None + try: + shutil.rmtree(backup_path) + except OSError as err: + fci.Console.PrintWarning( + translate("AddonsInstaller", "Failed to remove the backup directory {}").format( + backup_path + ) + + f"\n{err}\n" + ) def _cleanup_old_package_versions(self): """Remove old package version metadata directories after an update. - When pip updates packages with --target, it doesn't always remove old - version metadata (.dist-info directories). This can cause version detection - to find the old version instead of the new one, especially in Flatpak + When pip updates packages with --target, it doesn't always remove old version metadata (.dist-info directories). + This can cause version detection to find the old version instead of the new one, especially in Flatpak installations where multiple versions accumulate. """ if not os.path.exists(self.vendor_path): diff --git a/addonmanager_python_deps_gui.py b/addonmanager_python_deps_gui.py index 596cce1c..d68d5600 100644 --- a/addonmanager_python_deps_gui.py +++ b/addonmanager_python_deps_gui.py @@ -25,7 +25,7 @@ import addonmanager_freecad_interface as fci from addonmanager_python_deps import PythonPackageListModel -from PySideWrapper import QtWidgets +from PySideWrapper import QtCore, QtWidgets translate = fci.translate @@ -53,8 +53,11 @@ def __init__(self, addons): self.dlg.buttonInstallPkgs.clicked.connect(self._install_button_clicked) self.dlg.buttonUpdateAll.clicked.connect(self._update_button_clicked) + self.dlg.buttonCancel.clicked.connect(self._cancel_button_clicked) + self.dlg.rejected.connect(self._dialog_rejected) self.model.modelReset.connect(self._model_was_reset) self.model.update_complete.connect(self._update_complete) + self.model.progress_message.connect(self._show_progress_message) def show(self): self._working(True) @@ -63,12 +66,36 @@ def show(self): self.dlg.exec() def _working(self, working: bool) -> None: + """Show or hide the progress display, and enable the buttons that make sense while pip is + running, or while it is not.""" self.dlg.buttonInstallPkgs.setEnabled(not working) self.dlg.buttonUpdateAll.setEnabled(not working and self.model.updates_are_available()) + self.dlg.buttonCancel.setEnabled(working) if working: self.dlg.updateInProgressLabel.show() + self.dlg.progressDetailsLabel.show() else: self.dlg.updateInProgressLabel.hide() + self.dlg.progressDetailsLabel.hide() + self.dlg.progressDetailsLabel.setText("") + + def _show_progress_message(self, message: str) -> None: + """Display the most recent line of pip output, shortened to fit the available width.""" + label = self.dlg.progressDetailsLabel + elided = label.fontMetrics().elidedText( + message, QtCore.Qt.TextElideMode.ElideRight, label.width() + ) + label.setText(elided) + + def _cancel_button_clicked(self): + """Ask the running pip call to stop, without waiting for it to do so.""" + self.dlg.buttonCancel.setEnabled(False) + self._show_progress_message(translate("AddonsInstaller", "Stopping pip…")) + self.model.cancel_update() + + def _dialog_rejected(self): + """Stop any running pip call before this dialog and its model are destroyed.""" + self.model.cancel_update(wait_for_completion=True) def _install_button_clicked(self): title = translate("AddonsInstaller", "Install") 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_uninstaller.py b/addonmanager_uninstaller.py index fbcbfaf5..e302d082 100644 --- a/addonmanager_uninstaller.py +++ b/addonmanager_uninstaller.py @@ -83,6 +83,10 @@ class AddonUninstaller(QtCore.QObject): uninstaller = AddonInstaller(addon_to_remove) uninstaller.run() + If the addon provides an "uninstall.py" script it is executed as part of run() by + default. Set should_run_uninstall_script to False before calling run() to skip the + script (the GUI wrapper does this, asking the user for permission first and running + the script itself only when the user approves). """ # Signals: success and failure Emitted when the installation process is complete. @@ -100,6 +104,7 @@ def __init__(self, addon: Addon): self.addon_to_remove = addon self.installation_path = fci.DataPaths().mod_dir self.macro_installation_path = fci.DataPaths().macro_dir + self.should_run_uninstall_script = True def run(self) -> bool: """Remove an addon. Returns True if the addon was removed cleanly, or False @@ -115,7 +120,8 @@ def run(self) -> bool: path_to_remove, self.installation_path ): try: - self.run_uninstall_script(path_to_remove) + if self.should_run_uninstall_script: + self.run_uninstall_script(path_to_remove) self.remove_extra_files(path_to_remove) success = utils.rmdir(path_to_remove) if ( @@ -148,7 +154,8 @@ def run_uninstall_script(path_to_remove): # pylint: disable=broad-exception-caught try: with open(uninstall_script, encoding="utf-8") as f: - exec(f.read()) + # This use of exec() is behind an explicit user opt-in dialog (added nosec B102) + exec(f.read()) # nosec B102 except Exception: fci.Console.PrintError( translate( @@ -281,7 +288,7 @@ def _get_files_to_remove(self) -> List[str]: manifest_data = f.read() manifest = json.loads(manifest_data) manifest.append(manifest_file) # Remove the manifest itself as well - return manifest + return manifest + self._get_toolbar_icon_files() files_to_remove = [self.addon_to_remove.macro.filename] if self.addon_to_remove.macro.icon: files_to_remove.append(self.addon_to_remove.macro.icon) @@ -289,7 +296,21 @@ def _get_files_to_remove(self) -> List[str]: files_to_remove.append(self.addon_to_remove.macro.name.replace(" ", "_") + "_icon.xpm") for f in self.addon_to_remove.macro.other_files: files_to_remove.append(f) - return files_to_remove + return files_to_remove + self._get_toolbar_icon_files() + + def _get_toolbar_icon_files(self) -> List[str]: + """Get the names of the icon files that the toolbar button installer may have created for + this macro. Those files are created after the installation manifest is written, so they are + not listed in it.""" + macro = self.addon_to_remove.macro + icon_files = [] + if macro.icon: + _, ext = os.path.splitext(macro.icon) + extension = ext[1:].lower() if ext else "png" + icon_files.append(f"{macro.name}_icon.{extension}") + if macro.xpm: + icon_files.append(f"{macro.name}_icon.xpm") + return icon_files @staticmethod def _cleanup_directories(directories): diff --git a/addonmanager_uninstaller_gui.py b/addonmanager_uninstaller_gui.py index e830779e..d41bff75 100644 --- a/addonmanager_uninstaller_gui.py +++ b/addonmanager_uninstaller_gui.py @@ -21,6 +21,12 @@ """GUI functions for uninstalling an Addon or Macro.""" +import os +import platform + +# Audited: subprocess calls use fixed argument lists and no shell (added nosec B404) +import subprocess # nosec B404 + import addonmanager_freecad_interface as fci from Widgets.addonmanager_utility_dialogs import MessageDialog @@ -33,12 +39,75 @@ except ImportError: from PySide2 import QtCore, QtWidgets # Fall back to Qt5 +from addonmanager_toolbar_adapter import ToolbarAdapter from addonmanager_uninstaller import AddonUninstaller, MacroUninstaller import addonmanager_utilities as utils translate = fci.translate +def open_file_in_text_editor(path: str) -> None: + """Open the given file in a text editor chosen by the operating system. Deliberately avoids + QDesktopServices.openUrl, because on some platforms the default action for a Python file is + to execute it rather than to display it.""" + # Audited: fixed editor commands with the file path passed as an argument, never to a shell + # (added nosec B606, B603, B607) + system = platform.system() + if system == "Windows": + try: + os.startfile(path, "edit") # nosec B606 + except OSError: + subprocess.Popen(["notepad.exe", path]) # nosec B603 B607 + elif system == "Darwin": + subprocess.Popen(["open", "-t", path]) # nosec B603 B607 + else: + subprocess.Popen(["xdg-open", path]) # nosec B603 B607 + + +class UninstallScriptDialog(QtWidgets.QDialog): + """Asks the user whether the addon's uninstall script should be run. Offers to open the + script in a text editor so it can be reviewed before deciding. The default action is to not + run the script. After the dialog closes, run_requested is True only if the user explicitly + chose to run the script.""" + + def __init__(self, addon_display_name: str, script_path: str, parent=None): + super().__init__(parent) + self.script_path = script_path + self.run_requested = False + self.setObjectName("AddonManager_RunUninstallScriptDialog") + self.setWindowTitle(translate("AddonsInstaller", "Run Uninstall Script?")) + layout = QtWidgets.QVBoxLayout(self) + message = translate( + "AddonsInstaller", + "{} includes an uninstall script, intended to let the addon clean up after itself " + "when it is removed (for example, by removing its saved preferences). The script is " + "provided by the addon itself, not by the Addon Manager, and can run arbitrary code: " + "you may review it before deciding whether to run it.", + ).format(addon_display_name) + label = QtWidgets.QLabel(message) + label.setWordWrap(True) + layout.addWidget(label) + self.button_box = QtWidgets.QDialogButtonBox() + self.open_button = self.button_box.addButton(QtWidgets.QDialogButtonBox.Open) + self.open_button.setText(translate("AddonsInstaller", "Open Script in Editor…")) + self.run_button = self.button_box.addButton(QtWidgets.QDialogButtonBox.Yes) + self.run_button.setText(translate("AddonsInstaller", "Run Script")) + self.skip_button = self.button_box.addButton(QtWidgets.QDialogButtonBox.No) + self.skip_button.setText(translate("AddonsInstaller", "Do Not Run")) + self.skip_button.setDefault(True) + self.open_button.clicked.connect(self._open_script_in_editor) + self.run_button.clicked.connect(self._run_clicked) + self.skip_button.clicked.connect(self.reject) + layout.addWidget(self.button_box) + + def _run_clicked(self): + self.run_requested = True + self.accept() + + def _open_script_in_editor(self): + open_file_in_text_editor(self.script_path) + + class AddonUninstallerGUI(QtCore.QObject): """User interface for uninstalling an Addon: asks for confirmation, displays a progress dialog, displays completion and/or error dialogs, and emits the finished() signal when all work is @@ -74,6 +143,7 @@ def run(self): self._finalize() return + self._handle_uninstall_script() self.dialog_timer.start() self._run_uninstaller() @@ -91,6 +161,25 @@ def _confirm_uninstallation(self) -> bool: ) return confirm == QtWidgets.QMessageBox.Yes + def _handle_uninstall_script(self): + """If the addon provides an uninstall script, ask the user whether to run it. When the + user approves, the script is run here, on the main GUI thread, so that scripts that show + dialogs of their own work as expected. In either case the core uninstaller is told not to + run the script itself.""" + if not isinstance(self.uninstaller, AddonUninstaller): + return + self.uninstaller.should_run_uninstall_script = False + addon_path = os.path.join(self.uninstaller.installation_path, self.addon_to_remove.name) + script_path = os.path.join(addon_path, "uninstall.py") + if not os.path.isfile(script_path): + return + dialog = UninstallScriptDialog( + self.addon_to_remove.display_name, script_path, parent=utils.get_main_am_window() + ) + dialog.exec() + if dialog.run_requested: + AddonUninstaller.run_uninstall_script(addon_path) + def _show_progress_dialog(self): self.progress_dialog = QtWidgets.QMessageBox( QtWidgets.QMessageBox.NoIcon, @@ -117,6 +206,7 @@ def _succeeded(self, addon): self.dialog_timer.stop() if self.progress_dialog: self.progress_dialog.hide() + self._remove_toolbar_button() MessageDialog.show_modal( MessageDialog.DialogType.INFO, "AddonManager_UninstallCompleteDialog", @@ -126,6 +216,27 @@ def _succeeded(self, addon): ) self._finalize() + def _remove_toolbar_button(self): + """Remove the custom toolbar button that the Addon Manager created for a macro, if there + is one. Does nothing for addons that are not macros, and does nothing when the FreeCAD GUI + is not running.""" + if fci.FreeCADGui is None: + return + macro = getattr(self.addon_to_remove, "macro", None) + if macro is None or not getattr(macro, "filename", ""): + return + # pylint: disable=broad-exception-caught + try: + ToolbarAdapter().remove_custom_toolbar_button(macro.filename) + except Exception as e: + fci.Console.PrintWarning( + translate( + "AddonsInstaller", + "Failed to remove the toolbar button for macro {}", + ).format(self.addon_to_remove.display_name) + + f": {e}\n" + ) + def _failed(self, addon, message): """Callback for failed or partially failed removal""" self.dialog_timer.stop() 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 ac939d0c..e84d1cfc 100644 --- a/addonmanager_utilities.py +++ b/addonmanager_utilities.py @@ -33,7 +33,10 @@ import queue import shutil import stat -import subprocess + +# Audited: the subprocess wrappers below run argument-list commands with no shell; executables +# are resolved locally and caller data appears only as arguments (added nosec B404) +import subprocess # nosec B404 import sys import threading import time @@ -444,15 +447,42 @@ 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 + 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") @@ -645,12 +675,14 @@ def blocking_get(url: str, method=None) -> bytes: if hasattr(p, "data"): p = p.data() elif requests and method is None or method == "requests": - response = requests.get(url, timeout=10.0) + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + response = requests.get(url, timeout=10.0) # nosec B310 if response.status_code == 200: p = response.content else: ctx = ssl.create_default_context() - with urllib.request.urlopen(url, context=ctx) as f: + # Audited: this code does not accept non-HTTPS URL schemes (added nosec B310) + with urllib.request.urlopen(url, context=ctx) as f: # nosec B310 p = f.read() return p @@ -665,7 +697,10 @@ def run_interruptable_subprocess( # Added in Python 3.7 -- only used on Windows creation_flags = subprocess.CREATE_NO_WINDOW try: - p = subprocess.Popen( + # Audited: args is an argument list run with no shell; callers pass locally-resolved + # executables (git, pip) with untrusted data only ever appearing as arguments + # (added nosec B603) + p = subprocess.Popen( # nosec B603 args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -717,7 +752,8 @@ def run_monitored_subprocess( if hasattr(subprocess, "CREATE_NO_WINDOW"): creation_flags = subprocess.CREATE_NO_WINDOW try: - process = subprocess.Popen( + # Audited: args is an argument list run with no shell, as above (added nosec B603) + process = subprocess.Popen( # nosec B603 args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, @@ -735,26 +771,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, "") @@ -773,9 +814,33 @@ 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: + # Audited: fixed system command with a numeric PID argument (added nosec B603, B607) + subprocess.run( # nosec B603 B607 + ["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: diff --git a/addonmanager_workers_startup.py b/addonmanager_workers_startup.py index 14c2a56c..439edb1b 100644 --- a/addonmanager_workers_startup.py +++ b/addonmanager_workers_startup.py @@ -29,7 +29,12 @@ import os from types import SimpleNamespace from typing import List, Optional, Tuple -from xml.etree.ElementTree import ParseError as XmlParseError + +# 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 + +from defusedxml import DefusedXmlException import zipfile from PySideWrapper import QtCore @@ -257,7 +262,7 @@ def _serves_package_xml(self, url: str) -> bool: return False try: MetadataReader.from_bytes(data) - except (XmlParseError, RuntimeError): + except (XmlParseError, DefusedXmlException, RuntimeError): return False return True @@ -282,7 +287,7 @@ def _collect_addon_metadata(cls, name: str, get_file) -> Optional[CatalogEntryMe if package_xml: try: parsed_metadata = MetadataReader.from_bytes(package_xml) - except (XmlParseError, RuntimeError) as e: + except (XmlParseError, DefusedXmlException, RuntimeError) as e: parsed_metadata = None fci.Console.PrintWarning( translate( @@ -868,7 +873,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: @@ -929,7 +934,7 @@ def run(self): f"{addon.display_name} is missing workbenches {', '.join(deps.wbs)}\n" ) if deps.external_addons: - details += f"{addon.display_name} is missing addons {', '.join(deps.external_addons)}\n" + details += f"{addon.display_name} is missing addons {', '.join([x.display_name for x in deps.external_addons])}\n" if deps.python_requires: details += f"{addon.display_name} is missing python packages {', '.join(deps.python_requires)}\n" self.missing_dependencies.join(deps)