Safer handling of the initial installation of add-ons

source/addonHandler/__init__.py

@@ -427,31 +427,45 @@ def getAvailableAddons(
 def installAddonBundle(bundle: AddonBundle) -> Addon | None:
 	""" Extracts an Addon bundle in to a unique subdirectory of the user addons directory,
 	marking the addon as needing 'install completion' on NVDA restart.
+
+	:param bundle: The add-on bundle to install.
+	The bundle._installExceptions property is modified to store any raised exceptions
+	during the installation process.
+
+	:return: The installed add-on object, or None if the installation failed.
+	Regardless if installation failed or not, the created add-on object should be returned
+	to give caller a chance to clean-up modules imported as part of install tasks.
+	This clean-up cannot be done here, as install tasks are blocking,
+	and this function returns as soon as they're started,
+	so removing modules before they're done may cause unpredictable effects.
 	"""
-	addon: Addon | None = None
 	try:
 		bundle.extract()
 		addon = Addon(bundle.pendingInstallPath)
-		# #2715: The add-on must be added to _availableAddons here so that
-		# translations can be used in installTasks module.
-		_availableAddons[addon.path] = addon
+	except Exception as extractException:
+		bundle._installExceptions.append(extractException)
+		log.error(f"Error extracting add-on bundle {bundle}", exc_info=True)
+		return None
+
+	# #2715: The add-on must be added to _availableAddons here so that
+	# translations can be used in installTasks module.
+	_availableAddons[addon.path] = addon
+	try:
+		addon.runInstallTask("onInstall")
+	except Exception as onInstallException:
+		bundle._installExceptions.append(onInstallException)
+		# Broad except used, since we can not know what exceptions might be thrown by the install tasks.
+		log.error(f"task 'onInstall' on addon '{addon.name}' failed", exc_info=True)
+		del _availableAddons[addon.path]
 		try:
-			addon.runInstallTask("onInstall")
-			# Broad except used, since we can not know what exceptions might be thrown by the install tasks.
-		except Exception:
-			log.error(f"task 'onInstall' on addon '{addon.name}' failed", exc_info=True)
-			del _availableAddons[addon.path]
 			addon.completeRemove(runUninstallTask=False)
-			raise AddonError("Installation failed")
-		state[AddonStateCategory.PENDING_INSTALL].add(bundle.manifest['name'])
+		except Exception as removeException:
+			log.error(f"Failed to remove add-on {addon.name}", exc_info=True)
+			bundle._installExceptions.append(removeException)
+	else:
+		state[AddonStateCategory.PENDING_INSTALL].add(bundle.manifest["name"])
 		state.save()
-	finally:
-		# Regardless if installation failed or not, the created add-on object should be returned
-		# to give caller a hance to clean-up modules imported as part of install tasks.
-		# This clean-up cannot be done here, as install tasks are blocking,
-		# and this function returns as soon as they're started,
-		# so removing modules before they're done may cause unpredictable effects.
-		return addon
+	return addon
 
 
 class AddonError(Exception):
@@ -877,6 +891,9 @@ def __init__(self, bundlePath: str):
 		""" Constructs an L{AddonBundle} from a filename.
 		@param bundlePath: The path for the bundle file.
 		"""
+		self._installExceptions: list[Exception] = []
+		"""Exceptions thrown during the installation process."""
+
 		self._path = bundlePath
 		# Read manifest:
 		translatedInput=None

source/addonStore/install.py

@@ -1,5 +1,5 @@
 # A part of NonVisual Desktop Access (NVDA)
-# Copyright (C) 2022-2023 NV Access Limited
+# Copyright (C) 2022-2024 NV Access Limited
 # This file is covered by the GNU General Public License.
 # See the file COPYING for more details.
 
@@ -83,12 +83,11 @@ def installAddon(addonPath: PathLike) -> None:
 	bundle = _getAddonBundleToInstallIfValid(addonPath)
 	prevAddon = _getPreviouslyInstalledAddonById(bundle)
 
-	try:
-		addonObj = systemUtils.ExecAndPump[addonHandler.Addon](addonHandler.installAddonBundle, bundle).funcRes
-		if prevAddon:
-			prevAddon.requestRemove()
-	except addonHandler.AddonError:  # Handle other exceptions as they are known
-		log.error("Error installing addon bundle from %s" % addonPath, exc_info=True)
+	addonObj = systemUtils.ExecAndPump[addonHandler.Addon](addonHandler.installAddonBundle, bundle).funcRes
+	if bundle._installExceptions:
+		log.error(f"Error(s) installing addon bundle from {addonPath}")
+		for e in bundle._installExceptions:
+			log.error(e, exc_info=True)
 		raise DisplayableError(
 			displayMessage=pgettext(
 				"addonStore",
@@ -97,6 +96,8 @@ def installAddon(addonPath: PathLike) -> None:
 				"Failed to install add-on from %s"
 			) % addonPath
 		)
-	finally:
-		if addonObj is not None:
-			addonObj._cleanupAddonImports()
+	else:
+		if prevAddon:
+			prevAddon.requestRemove()
+	if addonObj is not None:
+		addonObj._cleanupAddonImports()

source/gui/addonGui.py

@@ -1,7 +1,7 @@
 # A part of NonVisual Desktop Access (NVDA)
 # This file is covered by the GNU General Public License.
 # See the file COPYING for more details.
-# Copyright (C) 2012-2023 NV Access Limited, Beqa Gozalishvili, Joseph Lee,
+# Copyright (C) 2012-2024 NV Access Limited, Beqa Gozalishvili, Joseph Lee,
 # Babbage B.V., Ethan Holliger, Arnold Loubriat, Thomas Stivers
 
 import weakref
@@ -10,6 +10,7 @@
 import wx
 import core
 import config
+from contextlib import contextmanager
 import gui
 from addonHandler import Addon
 from logHandler import log
@@ -117,9 +118,6 @@ def _addButtons(self, buttonHelper):
 		))
 
 
-# C901 'installAddon' is too complex (16)
-# Note: when working on installAddon, look for opportunities to simplify
-# and move logic out into smaller helper functions.
 def installAddon(parentWindow: wx.Window, addonPath: str) -> bool:  # noqa: C901
 	""" Installs the addon bundle at path.
 	Only used for installing external add-on bundles.
@@ -193,20 +191,34 @@ def installAddon(parentWindow: wx.Window, addonPath: str) -> bool:  # noqa: C901
 		) != wx.YES:
 			return False
 
-	from contextlib import contextmanager
+	return _performExternalAddonBundleInstall(parentWindow, bundle, prevAddon)
 
-	@contextmanager
-	def doneAndDestroy(window):
-		try:
-			yield window
-		except:
-			# pass on any exceptions
-			raise
-		finally:
-			# but ensure that done and Destroy are called.
-			window.done()
-			window.Destroy()
 
+@contextmanager
+def _doneAndDestroy(window: gui.IndeterminateProgressDialog):
+	try:
+		yield window
+	except Exception as e:
+		# pass on any exceptions
+		raise e
+	finally:
+		# but ensure that done and Destroy are called.
+		window.done()
+		window.Destroy()
+
+
+def _performExternalAddonBundleInstall(
+		parentWindow: wx.Window,
+		bundle: addonHandler.AddonBundle,
+		prevAddon: addonHandler.Addon | None,
+) -> bool:
+	"""
+	Perform the installation of an add-on bundle.
+	:param parentWindow: The parent window for the progress dialog.
+	:param bundle: The add-on bundle to install.
+	:param prevAddon: The previously installed add-on, if any.
+	:return: True if the installation was successful, False otherwise.
+	"""
 	#  use a progress dialog so users know that something is happening.
 	progressDialog = gui.IndeterminateProgressDialog(
 		parentWindow,
@@ -216,30 +228,30 @@ def doneAndDestroy(window):
 		_("Please wait while the add-on is being installed.")
 	)
 
-	try:
-		# Use context manager to ensure that `done` and `Destroy` are called on the progress dialog afterwards
-		with doneAndDestroy(progressDialog):
-			addonObj = systemUtils.ExecAndPump[addonHandler.Addon](addonHandler.installAddonBundle, bundle).funcRes
-			if prevAddon:
-				from addonStore.dataManager import addonDataManager
-				assert addonDataManager
-				# External install should remove cached add-on
-				addonDataManager._deleteCacheInstalledAddon(prevAddon.name)
-				prevAddon.requestRemove()
-			return True
-	except:  # noqa: E722
-		log.error("Error installing  addon bundle from %s" % addonPath, exc_info=True)
+	# Use context manager to ensure that `done` and `Destroy` are called on the progress dialog afterwards
+	with _doneAndDestroy(progressDialog):
+		addonObj = systemUtils.ExecAndPump[addonHandler.Addon](addonHandler.installAddonBundle, bundle).funcRes
+	if not bundle._installExceptions:
+		if prevAddon:
+			from addonStore.dataManager import addonDataManager
+			assert addonDataManager
+			# External install should remove cached add-on store data
+			addonDataManager._deleteCacheInstalledAddon(prevAddon.name)
+			prevAddon.requestRemove()
+	else:
+		log.error(f"Error(s) installing addon bundle from {bundle}")
+		for e in bundle._installExceptions:
+			log.error(e, exc_info=True)
 		gui.messageBox(
 			# Translators: The message displayed when an error occurs when installing an add-on package.
-			_("Failed to install add-on from %s") % addonPath,
+			_("Failed to install add-on from %s") % bundle._path,
 			# Translators: The title of a dialog presented when an error occurs.
 			_("Error"),
 			wx.OK | wx.ICON_ERROR
 		)
-	finally:
-		if addonObj is not None:
-			addonObj._cleanupAddonImports()
-	return False
+	if addonObj is not None:
+		addonObj._cleanupAddonImports()
+	return not bool(bundle._installExceptions)
 
 
 def handleRemoteAddonInstall(addonPath: str):