4.2LTS: Extensions: forward any errors setting up wheels to reports

When actions on extensions failed setup extensions, errors weren't forwarded to the user and were only visible in the terminal. --- Back ported from fb3fe458bf Pull Request: https://projects.blender.org/blender/blender/pulls/139017
2025-05-19 10:13:01 +02:00
parent 0269725011
commit 7629abe435
3 changed files with 185 additions and 42 deletions
@@ -13,6 +13,7 @@ __all__ = (
    "apply_action",
 )

+import contextlib
 import os
 import re
 import shutil
@@ -21,6 +22,7 @@ import zipfile

 from collections.abc import (
    Callable,
+    Iterator,
 )

 WheelSource = tuple[
@@ -179,6 +181,9 @@ def _zipfile_extractall_safe(
        zip_fh: zipfile.ZipFile,
        path: str,
        path_restrict: str,
+        *,
+        error_fn: Callable[[Exception], None],
+        remove_error_fn: Callable[[str, Exception], None],
 ) -> None:
    """
    A version of ``ZipFile.extractall`` that wont write to paths outside ``path_restrict``.
@@ -203,20 +208,69 @@ def _zipfile_extractall_safe(
    path_restrict_with_slash = path_restrict + sep
    assert len(path) >= len(path_restrict_with_slash)
    if not path.startswith(path_restrict_with_slash):
-        raise Exception("Expected the restricted directory to start with ")
+        # This is an internal error if it ever happens.
+        raise Exception("Expected the restricted directory to start with \"{:s}\"".format(path_restrict_with_slash))
+
+    has_error = False
+    member_index = 0
+
+    # Use an iterator to avoid duplicating the checks (for the cleanup pass).
+    def zip_iter_filtered(*, verbose: bool) -> Iterator[tuple[zipfile.ZipInfo, str, str]]:
+        for member in zip_fh.infolist():
+            filename_orig = member.filename
+            filename_next = path_prefix + filename_orig
+
+            # This isn't likely to happen so accept a noisy print here.
+            # If this ends up happening more often, it could be suppressed.
+            # (although this hints at bigger problems because we might be excluding necessary files).
+            if os.path.normpath(filename_next).startswith(".." + sep):
+                if verbose:
+                    print("Skipping path:", filename_next, "that escapes:", path_restrict)
+                continue
+            yield member, filename_orig, filename_next
+
+    for member, filename_orig, filename_next in zip_iter_filtered(verbose=True):
+        # Increment before extracting, so a potential cleanup will a file that failed to extract.
+        member_index += 1
+
+        member.filename = filename_next
+
+        # Extraction can fail for many reasons, see: #132924.
+        try:
+            zip_fh.extract(member, path_restrict)
+        except Exception as ex:
+            error_fn(ex)
+
+            filepath_native = path_restrict + sep + filename_next.replace("/", sep)
+            print("Failed to extract path:", filepath_native, "error", str(ex))
+            remove_error_fn(filepath_native, ex)
+            has_error = True

-    for member in zip_fh.infolist():
-        filename_orig = member.filename
-        member.filename = path_prefix + filename_orig
-        # This isn't likely to happen so accept a noisy print here.
-        # If this ends up happening more often, it could be suppressed.
-        # (although this hints at bigger problems because we might be excluding necessary files).
-        if os.path.normpath(member.filename).startswith(".." + sep):
-            print("Skipping path:", member.filename, "that escapes:", path_restrict)
-            continue
-        zip_fh.extract(member, path_restrict)
        member.filename = filename_orig

+        if has_error:
+            break
+
+    # If the zip-file failed to extract, remove all files that were extracted.
+    # This is done so failure to extract a file never results in a partially-working
+    # state which can cause confusing situations for users.
+    if has_error:
+        # NOTE: this currently leaves empty directories which is not ideal.
+        # It's possible to calculate directories created by this extraction but more involved.
+        member_cleanup_len = member_index + 1
+        member_index = 0
+
+        for member, filename_orig, filename_next in zip_iter_filtered(verbose=False):
+            member_index += 1
+            if member_index >= member_cleanup_len:
+                break
+
+            filepath_native = path_restrict + sep + filename_next.replace("/", sep)
+            try:
+                os.unlink(filepath_native)
+            except Exception as ex:
+                remove_error_fn(filepath_native, ex)
+

 WHEEL_VERSION_RE = re.compile(r"(\d+)?(?:\.(\d+))?(?:\.(\d+))")

@@ -311,6 +365,7 @@ def apply_action(
        local_dir: str,
        local_dir_site_packages: str,
        wheel_list: list[WheelSource],
+        error_fn: Callable[[Exception], None],
        remove_error_fn: Callable[[str, Exception], None],
        debug: bool,
 ) -> None:
@@ -419,5 +474,20 @@ def apply_action(
        filepath = wheels_dir_info_to_filepath_map[dir_info]
        # `ZipFile.extractall` is needed because some wheels contain paths that point to parent directories.
        # Handle this *safely* by allowing extracting to parent directories but limit this to the `local_dir`.
-        with zipfile.ZipFile(filepath, mode="r") as zip_fh:
-            _zipfile_extractall_safe(zip_fh, local_dir_site_packages, local_dir)
+
+        try:
+            # pylint: disable-next=consider-using-with
+            zip_fh_context = zipfile.ZipFile(filepath, mode="r")
+        except Exception as ex:
+            print("Error ({:s}) opening zip-file: {:s}".format(str(ex), filepath))
+            error_fn(ex)
+            continue
+
+        with contextlib.closing(zip_fh_context) as zip_fh:
+            _zipfile_extractall_safe(
+                zip_fh,
+                local_dir_site_packages,
+                local_dir,
+                error_fn=error_fn,
+                remove_error_fn=remove_error_fn,
+            )
@@ -13,6 +13,7 @@ __all__ = (
    "reset_all",
    "module_bl_info",
    "extensions_refresh",
+    "stale_pending_remove_paths",
    "stale_pending_stage_paths",
 )

@@ -347,7 +348,10 @@ def enable(module_name, *, default_set=False, persistent=False, refresh_handled=

    if (is_extension := module_name.startswith(_ext_base_pkg_idname_with_dot)):
        if not refresh_handled:
-            extensions_refresh(addon_modules_pending=[module_name])
+            extensions_refresh(
+                addon_modules_pending=[module_name],
+                handle_error=handle_error,
+            )

        # Ensure the extensions are compatible.
        if _extensions_incompatible:
@@ -389,7 +393,7 @@ def enable(module_name, *, default_set=False, persistent=False, refresh_handled=
                print("Exception in module unregister():", (mod_file or module_name))
                handle_error(ex)
                if is_extension and not refresh_handled:
-                    extensions_refresh()
+                    extensions_refresh(handle_error=handle_error)
                return None

        mod.__addon_enabled__ = False
@@ -405,7 +409,7 @@ def enable(module_name, *, default_set=False, persistent=False, refresh_handled=
                del sys.modules[module_name]

                if is_extension and not refresh_handled:
-                    extensions_refresh()
+                    extensions_refresh(handle_error=handle_error)
                return None
            mod.__addon_enabled__ = False

@@ -477,7 +481,7 @@ def enable(module_name, *, default_set=False, persistent=False, refresh_handled=
            if default_set:
                _addon_remove(module_name)
            if is_extension and not refresh_handled:
-                extensions_refresh()
+                extensions_refresh(handle_error=handle_error)
            return None

        if is_extension:
@@ -516,7 +520,7 @@ def enable(module_name, *, default_set=False, persistent=False, refresh_handled=
            if default_set:
                _addon_remove(module_name)
            if is_extension and not refresh_handled:
-                extensions_refresh()
+                extensions_refresh(handle_error=handle_error)
            return None
        finally:
            _bl_owner_id_set(owner_id_prev)
@@ -578,7 +582,7 @@ def disable(module_name, *, default_set=False, refresh_handled=False, handle_err
        _addon_remove(module_name)

    if not refresh_handled:
-        extensions_refresh()
+        extensions_refresh(handle_error=handle_error)

    if _bpy.app.debug_python:
        print("\taddon_utils.disable", module_name)
@@ -1144,12 +1148,13 @@ def _initialize_extensions_compat_ensure_up_to_date(extensions_directory, extens
    return updated, wheel_list


-def _initialize_extensions_compat_ensure_up_to_date_wheels(extensions_directory, wheel_list, debug):
+def _initialize_extensions_compat_ensure_up_to_date_wheels(extensions_directory, wheel_list, debug, error_fn):
    import os
    _extension_sync_wheels(
        local_dir=os.path.join(extensions_directory, ".local"),
        wheel_list=wheel_list,
        debug=debug,
+        error_fn=error_fn,
    )


@@ -1159,6 +1164,7 @@ def _initialize_extensions_compat_data(
        ensure_wheels,  # `bool`
        addon_modules_pending,  # `Optional[Sequence[str]]`
        use_startup_fastpath,  # `bool`
+        error_fn,  # `Callable[[Exception], None] | None`
 ):
    # WARNING: this function must *never* raise an exception because it would interfere with low level initialization.
    # As the function deals with file IO, use what are typically over zealous exception checks so as to rule out
@@ -1204,7 +1210,7 @@ def _initialize_extensions_compat_data(
            extensions_enabled,
            print_debug,
        )
-    except Exception as ex:
+    except Exception:
        print("Extension: unexpected error detecting cache, this is is a bug!")
        import traceback
        traceback.print_exc()
@@ -1212,9 +1218,18 @@ def _initialize_extensions_compat_data(

    if ensure_wheels:
        if updated:
+            if error_fn is None:
+                def error_fn(ex):
+                    print("Error:", str(ex))
+
            try:
-                _initialize_extensions_compat_ensure_up_to_date_wheels(extensions_directory, wheel_list, debug)
-            except Exception as ex:
+                _initialize_extensions_compat_ensure_up_to_date_wheels(
+                    extensions_directory,
+                    wheel_list,
+                    debug,
+                    error_fn=error_fn,
+                )
+            except Exception:
                print("Extension: unexpected error updating wheels, this is is a bug!")
                import traceback
                traceback.print_exc()
@@ -1336,6 +1351,7 @@ def _extension_sync_wheels(
        local_dir,  # `str`
        wheel_list,  # `List[WheelSource]`
        debug,           # `bool`
+        error_fn,  # `Callable[[Exception], None]`
 ):  # `-> None`
    import os
    import sys
@@ -1357,6 +1373,7 @@ def _extension_sync_wheels(
        local_dir=local_dir,
        local_dir_site_packages=local_dir_site_packages,
        wheel_list=wheel_list,
+        error_fn=error_fn,
        remove_error_fn=remove_error_fn,
        debug=debug,
    )
@@ -1804,6 +1821,8 @@ def _initialize_extensions_repos_once():
        ensure_wheels=True,
        addon_modules_pending=None,
        use_startup_fastpath=True,
+        # Runs on startup, fall back to printing.
+        error_fn=None,
    )

    # Setup repositories for the first time.
@@ -1819,7 +1838,23 @@ def _initialize_extensions_repos_once():
 # -----------------------------------------------------------------------------
 # Extension Public API

-def extensions_refresh(ensure_wheels=True, addon_modules_pending=None):
+def extensions_refresh(
+        ensure_wheels=True,
+        addon_modules_pending=None,
+        handle_error=None,
+):
+    """
+    Ensure data relating to extensions is up to date.
+    This should be called after extensions on the file-system have changed.
+
+    :arg ensure_wheels: When true, refresh installed wheels with wheels used by extensions.
+    :type ensure_wheels: bool
+    :arg addon_modules_pending: Refresh these add-ons by listing their package names, as if they are enabled.
+       This is needed so wheels can be setup before the add-on is enabled.
+    :type addon_modules_pending: Sequence[str] | None
+    :arg handle_error: Called in the case of an error, taking an exception argument.
+    :type handle_error: Callable[[Exception], None] | None
+    """

    # Ensure any changes to extensions refresh `_extensions_incompatible`.
    _initialize_extensions_compat_data(
@@ -1827,6 +1862,7 @@ def extensions_refresh(ensure_wheels=True, addon_modules_pending=None):
        ensure_wheels=ensure_wheels,
        addon_modules_pending=addon_modules_pending,
        use_startup_fastpath=False,
+        error_fn=handle_error,
    )