From 78bc1feb4d2dfed937fc971f39289737358d5bb8 Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Wed, 28 Dec 2022 08:24:25 +0100 Subject: [PATCH 01/10] Swap pkgutil.ImpImporter for importlib when loading addon code --- source/addonHandler/__init__.py | 38 ++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/source/addonHandler/__init__.py b/source/addonHandler/__init__.py index cc00c775527..caf6641cbf4 100644 --- a/source/addonHandler/__init__.py +++ b/source/addonHandler/__init__.py @@ -11,7 +11,6 @@ import inspect import itertools import collections -import pkgutil import shutil from io import StringIO import pickle @@ -29,7 +28,10 @@ import addonAPIVersion from . import addonVersionCheck from .addonVersionCheck import isAddonCompatible +import importlib +from types import ModuleType import extensionPoints +from keyword import iskeyword MANIFEST_FILENAME = "manifest.ini" @@ -475,25 +477,31 @@ def _getPathForInclusionInPackage(self, package): extension_path = os.path.join(self.path, package.__name__) return extension_path - def loadModule(self, name): + def loadModule(self, name: str) -> ModuleType: """ loads a python module from the addon directory @param name: the module name - @type name: string - @returns the python module with C{name} - @rtype python module """ - log.debug("Importing module %s from plugin %s", name, self.name) - importer = pkgutil.ImpImporter(self.path) - loader = importer.find_module(name) - if not loader: - return None + path = self.path + moduleName = name + if '.' in name: + splitName = name.split('.') + moduleName = splitName[-1] + path = os.path.join(path, *splitName[:-1]) + if not moduleName.isidentifier() or iskeyword(moduleName): + raise ValueError(f"{name} is an invalid python module name") + log.debug(f"Importing module {name} from plugin {self!r}") # Create a qualified full name to avoid modules with the same name on sys.modules. - fullname = "addons.%s.%s" % (self.name, name) - try: - return loader.load_module(fullname) - except ImportError: - # in this case return None, any other error throw to be handled elsewhere + fullName = f"addons.{self.name}.{name}" + if fullName in sys.modules: + return importlib.import_module(fullName) + spec = importlib.machinery.PathFinder.find_spec(fullName, [path]) + if not spec: return None + mod = importlib.util.module_from_spec(spec) + sys.modules[fullName] = mod + if spec.loader: + spec.loader.exec_module(mod) + return mod def getTranslationsInstance(self, domain='nvda'): """ Gets the gettext translation instance for this add-on. From 60ae795b00ae2e59e6f4791a67ff130f153fe26d Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Wed, 28 Dec 2022 11:44:32 +0100 Subject: [PATCH 02/10] better identifier name checking --- source/addonHandler/__init__.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/source/addonHandler/__init__.py b/source/addonHandler/__init__.py index caf6641cbf4..51a9684641e 100644 --- a/source/addonHandler/__init__.py +++ b/source/addonHandler/__init__.py @@ -481,13 +481,10 @@ def loadModule(self, name: str) -> ModuleType: """ loads a python module from the addon directory @param name: the module name """ - path = self.path - moduleName = name - if '.' in name: - splitName = name.split('.') - moduleName = splitName[-1] - path = os.path.join(path, *splitName[:-1]) - if not moduleName.isidentifier() or iskeyword(moduleName): + splitName = name.split('.') + moduleName = splitName[-1] + path = os.path.join(self.path, *splitName[:-1]) + if any(n for n in splitName if not n.isidentifier() or iskeyword(n)): raise ValueError(f"{name} is an invalid python module name") log.debug(f"Importing module {name} from plugin {self!r}") # Create a qualified full name to avoid modules with the same name on sys.modules. From 2fc8d56b54e02f7483a6362fc60c88375e248540 Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Wed, 28 Dec 2022 14:29:31 +0100 Subject: [PATCH 03/10] Properly support namespace packages --- source/addonHandler/__init__.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/source/addonHandler/__init__.py b/source/addonHandler/__init__.py index 51a9684641e..70316e0b06c 100644 --- a/source/addonHandler/__init__.py +++ b/source/addonHandler/__init__.py @@ -482,23 +482,36 @@ def loadModule(self, name: str) -> ModuleType: @param name: the module name """ splitName = name.split('.') - moduleName = splitName[-1] - path = os.path.join(self.path, *splitName[:-1]) if any(n for n in splitName if not n.isidentifier() or iskeyword(n)): raise ValueError(f"{name} is an invalid python module name") log.debug(f"Importing module {name} from plugin {self!r}") # Create a qualified full name to avoid modules with the same name on sys.modules. fullName = f"addons.{self.name}.{name}" - if fullName in sys.modules: + # If the given name contains dots (i.e. it is a submodule import), ensure the top module is created correctly. + # After that, the import mechanism will be able to resolve the submodule automatically. + fullNameTop = f"addons.{self.name}.{splitName[0]}" + if fullNameTop in sys.modules: + # The module can safely be imported, since the top level module is known. return importlib.import_module(fullName) - spec = importlib.machinery.PathFinder.find_spec(fullName, [path]) + # Ensure the new module is resolvable by the import system. + # For this, all packages in the tree have to be available in sys.modules. + # We add mock modules for the addons package and the addon itself. + # If we don't do this, namespace packages can't be imported correctly. + for parentName in ("addons", f"addons.{self.name}"): + if parentName in sys.modules: + # Parent package already initialized + continue + parentSpec = importlib._bootstrap.ModuleSpec(parentName, None, is_package=True) + parentModule = importlib.util.module_from_spec(parentSpec) + sys.modules[parentModule.__name__] = parentModule + spec = importlib.machinery.PathFinder.find_spec(fullNameTop, [self.path]) if not spec: return None mod = importlib.util.module_from_spec(spec) - sys.modules[fullName] = mod + sys.modules[fullNameTop] = mod if spec.loader: spec.loader.exec_module(mod) - return mod + return mod if fullNameTop == fullName else importlib.import_module(fullName) def getTranslationsInstance(self, domain='nvda'): """ Gets the gettext translation instance for this add-on. From c139c202dd68beec35c1976af957051876de0614 Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Tue, 3 Jan 2023 08:09:29 +0100 Subject: [PATCH 04/10] Linter fix --- source/addonHandler/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/addonHandler/__init__.py b/source/addonHandler/__init__.py index 70316e0b06c..7daa3411fe0 100644 --- a/source/addonHandler/__init__.py +++ b/source/addonHandler/__init__.py @@ -1,5 +1,5 @@ # A part of NonVisual Desktop Access (NVDA) -# Copyright (C) 2012-2022 Rui Batista, NV Access Limited, Noelia Ruiz Martínez, +# Copyright (C) 2012-2023 Rui Batista, NV Access Limited, Noelia Ruiz Martínez, # Joseph Lee, Babbage B.V., Arnold Loubriat, Łukasz Golonka, Leonard de Ruijter # This file is covered by the GNU General Public License. # See the file COPYING for more details. @@ -487,7 +487,8 @@ def loadModule(self, name: str) -> ModuleType: log.debug(f"Importing module {name} from plugin {self!r}") # Create a qualified full name to avoid modules with the same name on sys.modules. fullName = f"addons.{self.name}.{name}" - # If the given name contains dots (i.e. it is a submodule import), ensure the top module is created correctly. + # If the given name contains dots (i.e. it is a submodule import), + # ensure the module at the top of the hierarchy is created correctly. # After that, the import mechanism will be able to resolve the submodule automatically. fullNameTop = f"addons.{self.name}.{splitName[0]}" if fullNameTop in sys.modules: From c3c884a2b90e0ddc49f70f87dd9ee476a1894841 Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Wed, 4 Jan 2023 08:33:44 +0100 Subject: [PATCH 05/10] Never return None, just raise --- include/espeak | 2 +- include/nvda-cldr | 2 +- source/addonHandler/__init__.py | 16 +++++++++++----- source/addonHandler/packaging.py | 16 +++++++++++++++- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/include/espeak b/include/espeak index a51235aaabd..b17ed2d682f 160000 --- a/include/espeak +++ b/include/espeak @@ -1 +1 @@ -Subproject commit a51235aaabdc15f76ed0f7f642f2c1efd562b373 +Subproject commit b17ed2d682fa5cc1bbab3d20089ce781a0579a03 diff --git a/include/nvda-cldr b/include/nvda-cldr index 8e98c77fc3c..091113c8faa 160000 --- a/include/nvda-cldr +++ b/include/nvda-cldr @@ -1 +1 @@ -Subproject commit 8e98c77fc3cdb33cf6d530f27f82c972ef3d93c0 +Subproject commit 091113c8faad01fdc4e6587358773b4bb32749bb diff --git a/source/addonHandler/__init__.py b/source/addonHandler/__init__.py index 7daa3411fe0..f9f0a650ed1 100644 --- a/source/addonHandler/__init__.py +++ b/source/addonHandler/__init__.py @@ -28,10 +28,10 @@ import addonAPIVersion from . import addonVersionCheck from .addonVersionCheck import isAddonCompatible +from .packaging import isModuleName import importlib from types import ModuleType import extensionPoints -from keyword import iskeyword MANIFEST_FILENAME = "manifest.ini" @@ -480,9 +480,10 @@ def _getPathForInclusionInPackage(self, package): def loadModule(self, name: str) -> ModuleType: """ loads a python module from the addon directory @param name: the module name + @raises: Any exception that can be raised when importing a module, such as NameError, AttributeError, ImportError, etc. + a ValueError is raised when the module name is invalid. """ - splitName = name.split('.') - if any(n for n in splitName if not n.isidentifier() or iskeyword(n)): + if not isModuleName(name): raise ValueError(f"{name} is an invalid python module name") log.debug(f"Importing module {name} from plugin {self!r}") # Create a qualified full name to avoid modules with the same name on sys.modules. @@ -490,6 +491,7 @@ def loadModule(self, name: str) -> ModuleType: # If the given name contains dots (i.e. it is a submodule import), # ensure the module at the top of the hierarchy is created correctly. # After that, the import mechanism will be able to resolve the submodule automatically. + splitName = name.split('.') fullNameTop = f"addons.{self.name}.{splitName[0]}" if fullNameTop in sys.modules: # The module can safely be imported, since the top level module is known. @@ -507,7 +509,7 @@ def loadModule(self, name: str) -> ModuleType: sys.modules[parentModule.__name__] = parentModule spec = importlib.machinery.PathFinder.find_spec(fullNameTop, [self.path]) if not spec: - return None + raise ModuleNotFoundError(importlib._bootstrap._ERR_MSG.format(name), name=name) mod = importlib.util.module_from_spec(spec) sys.modules[fullNameTop] = mod if spec.loader: @@ -530,7 +532,11 @@ def runInstallTask(self,taskName,*args,**kwargs): in the add-on's installTasks module if it exists. """ if not hasattr(self,'_installTasksModule'): - self._installTasksModule=self.loadModule('installTasks') + try: + installTasksModule = self.loadModule('installTasks') + except ModuleNotFoundError: + installTasksModule = None + self._installTasksModule = installTasksModule if self._installTasksModule: func=getattr(self._installTasksModule,taskName,None) if func: diff --git a/source/addonHandler/packaging.py b/source/addonHandler/packaging.py index 599108bf7e5..e74447c144e 100644 --- a/source/addonHandler/packaging.py +++ b/source/addonHandler/packaging.py @@ -1,5 +1,5 @@ # A part of NonVisual Desktop Access (NVDA) -# Copyright (C) 2009-2022 NV Access Limited, Rui Batista, Zahari Yurukov, Leonard de Ruijter +# Copyright (C) 2009-2023 NV Access Limited, Rui Batista, Zahari Yurukov, Leonard de Ruijter # This file is covered by the GNU General Public License. # See the file COPYING for more details. @@ -11,6 +11,7 @@ from types import ModuleType import globalVars import config +from keyword import iskeyword def initializeModulePackagePaths(): @@ -60,3 +61,16 @@ def addDirsToPythonPackagePath(module: ModuleType, subdir: Optional[str] = None) pathList = [fullPath] pathList.extend(module.__path__) module.__path__ = pathList + + +def isModuleName(name: str) -> bool: + """When adding a module to sys.modules, it is important to check module name validity. + the L{str.isidentifier} method checks whether a string is a valid python identifier, + however this includes identifiers like 'def' and 'class', which are definitely invalid module names. + Therefore a valid module name should be an identifier but not a keyword. + A valid module name can also contain dots, but a dot is considered invalid in identifiers. + Therefore, use dot as a split separator and check all the name parts independently. + @param moduleName: De module name to check for naming conventions. + @returns: Whether the module name is valid. + """ + return all(n.isidentifier() and not iskeyword(n) for n in name.split(".")) From eefebdb3b15533579cc934d7fa17101bf8159a01 Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Tue, 10 Jan 2023 08:31:44 +0100 Subject: [PATCH 06/10] Lint --- source/addonHandler/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/addonHandler/__init__.py b/source/addonHandler/__init__.py index f9f0a650ed1..2ad6110cdfc 100644 --- a/source/addonHandler/__init__.py +++ b/source/addonHandler/__init__.py @@ -480,7 +480,8 @@ def _getPathForInclusionInPackage(self, package): def loadModule(self, name: str) -> ModuleType: """ loads a python module from the addon directory @param name: the module name - @raises: Any exception that can be raised when importing a module, such as NameError, AttributeError, ImportError, etc. + @raises: Any exception that can be raised when importing a module, + such as NameError, AttributeError, ImportError, etc. a ValueError is raised when the module name is invalid. """ if not isModuleName(name): From 6e4e1046cd5db8dcc18ca31e0f9c3b049d9dea10 Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Tue, 10 Jan 2023 08:32:47 +0100 Subject: [PATCH 07/10] Fix submodule updates --- include/espeak | 2 +- include/nvda-cldr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/espeak b/include/espeak index b17ed2d682f..a51235aaabd 160000 --- a/include/espeak +++ b/include/espeak @@ -1 +1 @@ -Subproject commit b17ed2d682fa5cc1bbab3d20089ce781a0579a03 +Subproject commit a51235aaabdc15f76ed0f7f642f2c1efd562b373 diff --git a/include/nvda-cldr b/include/nvda-cldr index 091113c8faa..8e98c77fc3c 160000 --- a/include/nvda-cldr +++ b/include/nvda-cldr @@ -1 +1 @@ -Subproject commit 091113c8faad01fdc4e6587358773b4bb32749bb +Subproject commit 8e98c77fc3cdb33cf6d530f27f82c972ef3d93c0 From 709507ed216844c653f48f321d5e1d064b42a1f3 Mon Sep 17 00:00:00 2001 From: Leonard de Ruijter Date: Wed, 11 Jan 2023 07:53:59 +0100 Subject: [PATCH 08/10] No longer user private variable on importlib --- source/addonHandler/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/addonHandler/__init__.py b/source/addonHandler/__init__.py index 2ad6110cdfc..d28257281e5 100644 --- a/source/addonHandler/__init__.py +++ b/source/addonHandler/__init__.py @@ -510,7 +510,7 @@ def loadModule(self, name: str) -> ModuleType: sys.modules[parentModule.__name__] = parentModule spec = importlib.machinery.PathFinder.find_spec(fullNameTop, [self.path]) if not spec: - raise ModuleNotFoundError(importlib._bootstrap._ERR_MSG.format(name), name=name) + raise ModuleNotFoundError(f"No module named {name!r}", name=name) mod = importlib.util.module_from_spec(spec) sys.modules[fullNameTop] = mod if spec.loader: From 10c0b4ed0222bd2ce003fea48d308c70df2c8ba9 Mon Sep 17 00:00:00 2001 From: Sean Budd Date: Thu, 12 Jan 2023 09:21:14 +1100 Subject: [PATCH 09/10] update changes --- user_docs/en/changes.t2t | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/user_docs/en/changes.t2t b/user_docs/en/changes.t2t index 91df0d6e79e..ce58b2e61e9 100644 --- a/user_docs/en/changes.t2t +++ b/user_docs/en/changes.t2t @@ -134,6 +134,11 @@ To disable the braille handler programatically, register a handler to ``braille. - It is no longer possible to update the display size of the handler by setting ``braille.handler.displaySize``. To update the displaySize programatically, register a handler to ``braille.handler.filter_displaySize``. Refer to ``brailleViewer`` for an example on how to do this. (#14503) +- There has been changes to the usage of ``addonHandler.Addon.loadModule``. (#14481) + - ``loadModule`` now expects dot as a separator, rather than backslash. + For example "lib.example" instead of "lib\example". + - ``loadModule`` now raises an exception when a module can't be loaded or has errors, instead of silently returning ``None`` without giving information about the cause. + - - From 80345a86cb0d94eb18ac7aa2d2202d9d63902d79 Mon Sep 17 00:00:00 2001 From: Sean Budd Date: Thu, 12 Jan 2023 09:22:22 +1100 Subject: [PATCH 10/10] fix grammar --- user_docs/en/changes.t2t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/user_docs/en/changes.t2t b/user_docs/en/changes.t2t index ce58b2e61e9..59fc69a00d5 100644 --- a/user_docs/en/changes.t2t +++ b/user_docs/en/changes.t2t @@ -134,7 +134,7 @@ To disable the braille handler programatically, register a handler to ``braille. - It is no longer possible to update the display size of the handler by setting ``braille.handler.displaySize``. To update the displaySize programatically, register a handler to ``braille.handler.filter_displaySize``. Refer to ``brailleViewer`` for an example on how to do this. (#14503) -- There has been changes to the usage of ``addonHandler.Addon.loadModule``. (#14481) +- There have been changes to the usage of ``addonHandler.Addon.loadModule``. (#14481) - ``loadModule`` now expects dot as a separator, rather than backslash. For example "lib.example" instead of "lib\example". - ``loadModule`` now raises an exception when a module can't be loaded or has errors, instead of silently returning ``None`` without giving information about the cause.