From 1912420bb9e9b32849e7997d70dec23c5190bb37 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Mon, 17 Jul 2023 07:32:25 +0200 Subject: [PATCH 01/11] Run CI tests with Python 3.12.0-rc.1 --- .github/workflows/pypi-release.yml | 2 +- .github/workflows/run-tests.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pypi-release.yml b/.github/workflows/pypi-release.yml index 35bebda807..bfc4007ebb 100644 --- a/.github/workflows/pypi-release.yml +++ b/.github/workflows/pypi-release.yml @@ -81,7 +81,7 @@ jobs: fail-fast: false matrix: os: [macos-11, windows-2019] - python-version: ['3.7', '3.8', '3.9', '3.10', '3.11'] + python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12.0-rc.1'] steps: - uses: actions/checkout@v3 - name: Set up Python ${{ matrix.python-version }} diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 015c889433..8f72c83f40 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -9,7 +9,7 @@ jobs: strategy: matrix: os: [macos-latest, ubuntu-latest, windows-2019] - python-version: ['3.7', '3.8', '3.9', '3.10', '3.11'] + python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12.0-rc.1'] include: - os: macos-11 python-version: '3.7' From 0a56f408546fdc467571e1ce2373adef4b241f3b Mon Sep 17 00:00:00 2001 From: skelly37 Date: Fri, 22 Jul 2022 10:49:23 +0200 Subject: [PATCH 02/11] PICARD-2354: Replaced deprecated imp module in plugin manager --- picard/pluginmanager.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index 492d2b3dc3..3b769156d8 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -25,7 +25,6 @@ from functools import partial -import imp import importlib import json import os.path @@ -158,10 +157,8 @@ def _plugin_name_from_path(path): def load_manifest(archive_path): archive = zipfile.ZipFile(archive_path) - manifest_data = None with archive.open('MANIFEST.json') as f: - manifest_data = json.loads(str(f.read().decode())) - return manifest_data + return json.loads(str(f.read().decode())) def zip_import(path): @@ -274,6 +271,7 @@ def _get_plugin_index_by_name(self, name): def _load_plugin_from_directory(self, name, plugindir): module_file = None + info = None zipfilename = os.path.join(plugindir, name + '.zip') (zip_importer, module_name, manifest_data) = zip_import(zipfilename) if zip_importer: @@ -287,11 +285,15 @@ def _load_plugin_from_directory(self, name, plugindir): return None module_pathname = zip_importer.get_filename(name) else: - try: - info = imp.find_module(name, [plugindir]) - module_file = info[0] - module_pathname = info[1] - except ImportError: + # instead of looking for a module in the whole directory, we have to provide an explicit path to it + # e.g. for Picard in /home/user/picard it'd be: + # importlib.util.spec_from_file_location("picard", "/home/user/picard/picard/__init__.py") + # alternatively we could use importlib.find_loader, which works pretty much the same but is unfortunately + # deprecated since 3.4 + # + # comment to be deleted, just for the PR purposes + info = importlib.machinery.PathFinder().find_spec(name, [plugindir]) + if not info.loader: errorfmt = _('Failed loading plugin "%(plugin)s" in "%(dirname)s"') self.plugin_error(name, errorfmt, params={ 'plugin': name, @@ -299,6 +301,10 @@ def _load_plugin_from_directory(self, name, plugindir): }) return None + module_pathname = info.origin + if module_pathname.endswith("__init__.py"): + module_pathname = os.path.dirname(module_pathname) + plugin = None try: existing_plugin, existing_plugin_index = self._get_plugin_index_by_name(name) @@ -314,7 +320,7 @@ def _load_plugin_from_directory(self, name, plugindir): if zip_importer: plugin_module = zip_importer.load_module(full_module_name) else: - plugin_module = imp.load_module(full_module_name, *info) + plugin_module = importlib.util.module_from_spec(info) plugin = PluginWrapper(plugin_module, plugindir, file=module_pathname, manifest_data=manifest_data) compatible_versions = _compatible_api_versions(plugin.api_versions) @@ -345,8 +351,6 @@ def _load_plugin_from_directory(self, name, plugindir): errorfmt = _('Plugin "%(plugin)s"') self.plugin_error(name, errorfmt, log_func=log.exception, params={'plugin': name}) - if module_file is not None: - module_file.close() return plugin def _get_existing_paths(self, plugin_name, fileexts): From 136e2b7dd323348b7df28da3cfbe6a527b0c86be Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Fri, 9 Dec 2022 18:15:01 +0100 Subject: [PATCH 03/11] PICARD-2354: Exec plugin module when loading --- picard/pluginmanager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index 3b769156d8..d2915c72e7 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -270,7 +270,6 @@ def _get_plugin_index_by_name(self, name): return (None, None) def _load_plugin_from_directory(self, name, plugindir): - module_file = None info = None zipfilename = os.path.join(plugindir, name + '.zip') (zip_importer, module_name, manifest_data) = zip_import(zipfilename) @@ -321,6 +320,8 @@ def _load_plugin_from_directory(self, name, plugindir): plugin_module = zip_importer.load_module(full_module_name) else: plugin_module = importlib.util.module_from_spec(info) + info.loader.exec_module(plugin_module) + plugin = PluginWrapper(plugin_module, plugindir, file=module_pathname, manifest_data=manifest_data) compatible_versions = _compatible_api_versions(plugin.api_versions) From b1afa8e1216ce13f4ca501663dfd06284cac54e7 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sat, 9 Sep 2023 14:23:23 +0200 Subject: [PATCH 04/11] PICARD-2354: Use MetaPathFinder to find Picard plugins --- picard/pluginmanager.py | 50 +++++++++++++++++++++++++++++++++++++++-- picard/tagger.py | 26 +++++---------------- test/test_plugins.py | 46 +++++++++++++++++++++---------------- 3 files changed, 80 insertions(+), 42 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index d2915c72e7..4ab64efc6d 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -26,9 +26,11 @@ from functools import partial import importlib +from importlib.abc import MetaPathFinder import json import os.path import shutil +import sys import tempfile import zipfile import zipimport @@ -45,6 +47,7 @@ PLUGINS_API, USER_PLUGIN_DIR, ) +from picard.const.sys import IS_FROZEN from picard.plugin import ( _PLUGIN_MODULE_PREFIX, PluginData, @@ -184,6 +187,35 @@ def _compatible_api_versions(api_versions): return set(versions) & set(picard.api_versions_tuple) +_plugin_dirs = [] + + +def plugin_dirs(): + yield from _plugin_dirs + + +def init_default_plugin_dirs(): + # Add user plugin dir first + if not os.path.exists(USER_PLUGIN_DIR): + os.makedirs(USER_PLUGIN_DIR) + register_plugin_dir(USER_PLUGIN_DIR) + + # Register system wide plugin dir + if IS_FROZEN: + toppath = sys.argv[0] + else: + toppath = os.path.abspath(__file__) + + topdir = os.path.dirname(toppath) + plugin_dir = os.path.join(topdir, "plugins") + register_plugin_dir(plugin_dir) + + +def register_plugin_dir(path): + if path not in _plugin_dirs: + _plugin_dirs.append(path) + + class PluginManager(QtCore.QObject): plugin_installed = QtCore.pyqtSignal(PluginWrapper, bool) @@ -198,6 +230,7 @@ def __init__(self, plugins_directory=None): if plugins_directory is None: plugins_directory = USER_PLUGIN_DIR self.plugins_directory = os.path.normpath(plugins_directory) + init_default_plugin_dirs() @property def available_plugins(self): @@ -291,8 +324,9 @@ def _load_plugin_from_directory(self, name, plugindir): # deprecated since 3.4 # # comment to be deleted, just for the PR purposes - info = importlib.machinery.PathFinder().find_spec(name, [plugindir]) - if not info.loader: + # info = importlib.machinery.PathFinder().find_spec(name, [plugindir]) + info = PluginMetaPathFinder().find_spec(_PLUGIN_MODULE_PREFIX + name, [plugindir]) + if not info or not info.loader: errorfmt = _('Failed loading plugin "%(plugin)s" in "%(dirname)s"') self.plugin_error(name, errorfmt, params={ 'plugin': name, @@ -558,3 +592,15 @@ def _display_update(): self.query_available_plugins(_display_update) else: _display_update() + + +class PluginMetaPathFinder(MetaPathFinder): + def find_spec(self, fullname, path, target=None): + if not fullname.startswith(_PLUGIN_MODULE_PREFIX): + return None + info = importlib.machinery.PathFinder().find_spec(fullname, _plugin_dirs) + if info and info.loader: + return info + + +sys.meta_path.append(PluginMetaPathFinder()) diff --git a/picard/tagger.py b/picard/tagger.py index f363446a94..9e0ae56a33 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -94,12 +94,8 @@ setup_config, ) from picard.config_upgrade import upgrade_config -from picard.const import ( - USER_DIR, - USER_PLUGIN_DIR, -) +from picard.const import USER_DIR from picard.const.sys import ( - IS_FROZEN, IS_HAIKU, IS_MACOS, IS_WIN, @@ -114,7 +110,10 @@ from picard.file import File from picard.formats import open_ as open_file from picard.i18n import setup_gettext -from picard.pluginmanager import PluginManager +from picard.pluginmanager import ( + PluginManager, + plugin_dirs, +) from picard.releasegroup import ReleaseGroup from picard.track import ( NonAlbumTrack, @@ -175,21 +174,6 @@ def _patched_shutil_copystat(src, dst, *, follow_symlinks=True): shutil.copystat = _patched_shutil_copystat -def plugin_dirs(): - if IS_FROZEN: - toppath = sys.argv[0] - else: - toppath = os.path.abspath(__file__) - - topdir = os.path.dirname(toppath) - plugin_dir = os.path.join(topdir, "plugins") - yield plugin_dir - - if not os.path.exists(USER_PLUGIN_DIR): - os.makedirs(USER_PLUGIN_DIR) - yield USER_PLUGIN_DIR - - class ParseItemsToLoad: WINDOWS_DRIVE_TEST = re.compile(r"^[a-z]\:", re.IGNORECASE) diff --git a/test/test_plugins.py b/test/test_plugins.py index 56c9244205..f56bfb7aba 100644 --- a/test/test_plugins.py +++ b/test/test_plugins.py @@ -3,7 +3,7 @@ # Picard, the next-generation MusicBrainz tagger # # Copyright (C) 2019-2021 Laurent Monin -# Copyright (C) 2019-2021 Philipp Wolfer +# Copyright (C) 2019-2021, 2023 Philipp Wolfer # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License @@ -30,14 +30,15 @@ import picard from picard.const import USER_PLUGIN_DIR from picard.plugin import ( - _PLUGIN_MODULE_PREFIX, PluginWrapper, _unregister_module_extensions, ) from picard.pluginmanager import ( PluginManager, _compatible_api_versions, + _plugin_dirs, _plugin_name_from_path, + register_plugin_dir, ) from picard.version import ( Version, @@ -92,9 +93,8 @@ def unload_plugin(plugin_name): _unregister_module_extensions(plugin_name) if hasattr(picard.plugins, plugin_name): delattr(picard.plugins, plugin_name) - key = _PLUGIN_MODULE_PREFIX + plugin_name - if key in sys.modules: - del sys.modules[key] + if plugin_name in sys.modules: + del sys.modules[plugin_name] class TestPicardPluginsCommon(PicardTestCase): @@ -112,6 +112,7 @@ class TestPicardPluginsCommonTmpDir(TestPicardPluginsCommon): def setUp(self): super().setUp() self.tmp_directory = self.mktmpdir() + register_plugin_dir(self.tmp_directory) class TestPicardPluginManager(TestPicardPluginsCommon): @@ -146,10 +147,6 @@ def test_plugin_name_from_path(self): class TestPicardPluginsInstall(TestPicardPluginsCommonTmpDir): def _test_plugin_install(self, name): - unload_plugin('dummyplugin') - with self.assertRaises(ImportError): - from picard.plugins.dummyplugin import DummyPlugin - plugin_path = _testplugins[name] pm = PluginManager(plugins_directory=self.tmp_directory) @@ -159,14 +156,18 @@ def _test_plugin_install(self, name): self.assertEqual(pm.plugins[0].name, 'Dummy plugin', msg) # if module is properly loaded, this should work - from picard.plugins.dummyplugin import DummyPlugin # noqa: F811 + from picard.plugins.dummyplugin import DummyPlugin DummyPlugin() - def _test_plugin_install_data(self, name): - unload_plugin('dummyplugin') + # Remove plugin again + pm.remove_plugin('dummyplugin') + unload_plugin('picard.plugins.dummyplugin') with self.assertRaises(ImportError): - from picard.plugins.dummyplugin import DummyPlugin + from picard.plugins.dummyplugin import ( # noqa: F811 # pylint: disable=reimported + DummyPlugin, + ) + def _test_plugin_install_data(self, name): # simulate installation from UI using data from picard plugins api web service with open(_testplugins[name], 'rb') as f: data = f.read() @@ -179,9 +180,17 @@ def _test_plugin_install_data(self, name): self.assertEqual(pm.plugins[0].name, 'Dummy plugin', msg) # if module is properly loaded, this should work - from picard.plugins.dummyplugin import DummyPlugin # noqa: F811 + from picard.plugins.dummyplugin import DummyPlugin DummyPlugin() + # Remove plugin again + pm.remove_plugin('dummyplugin') + unload_plugin('picard.plugins.dummyplugin') + with self.assertRaises(ImportError): + from picard.plugins.dummyplugin import ( # noqa: F811 # pylint: disable=reimported + DummyPlugin, + ) + # module def test_plugin_install_module(self): self._test_plugin_install('module') @@ -221,13 +230,10 @@ def test_plugin_install_no_path_no_plugin_name(self): class TestPicardPluginsLoad(TestPicardPluginsCommonTmpDir): def _test_plugin_load_from_directory(self, name): - unload_plugin('dummyplugin') - with self.assertRaises(ImportError): - from picard.plugins.dummyplugin import DummyPlugin - pm = PluginManager(plugins_directory=self.tmp_directory) src_dir = os.path.dirname(_testplugins[name]) + register_plugin_dir(src_dir) msg = "plugins_load_from_directory: %s %r" % (name, src_dir) pm.load_plugins_from_directory(src_dir) @@ -235,9 +241,11 @@ def _test_plugin_load_from_directory(self, name): self.assertEqual(pm.plugins[0].name, 'Dummy plugin', msg) # if module is properly loaded, this should work - from picard.plugins.dummyplugin import DummyPlugin # noqa: F811 + from picard.plugins.dummyplugin import DummyPlugin DummyPlugin() + _plugin_dirs.remove(src_dir) + # singlefile def test_plugin_load_from_directory_singlefile(self): self._test_plugin_load_from_directory('singlefile') From 1944a0e2cb343524e1d181c2ad9c352fcc57c186 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sat, 9 Sep 2023 15:47:51 +0200 Subject: [PATCH 05/11] PICARD-2355: Use zipimporter.find_spec if available Avoids use of deprecated and in Python 3.12 removed API --- picard/pluginmanager.py | 100 ++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 46 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index 4ab64efc6d..c979c782f2 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -158,28 +158,25 @@ def _plugin_name_from_path(path): return None -def load_manifest(archive_path): - archive = zipfile.ZipFile(archive_path) - with archive.open('MANIFEST.json') as f: - return json.loads(str(f.read().decode())) +def load_zip_manifest(archive_path): + if is_zipped_package(archive_path): + try: + archive = zipfile.ZipFile(archive_path) + with archive.open('MANIFEST.json') as f: + return json.loads(str(f.read().decode())) + except Exception as why: + log.warning("Failed to load manifest data from json: %s", why) + return None def zip_import(path): if (not is_zip(path) or not os.path.isfile(path)): - return (None, None, None) + return None try: - zip_importer = zipimport.zipimporter(path) - plugin_name = _plugin_name_from_path(path) - manifest_data = None - if is_zipped_package(path): - try: - manifest_data = load_manifest(path) - except Exception as why: - log.warning("Failed to load manifest data from json: %s", why) - return (zip_importer, plugin_name, manifest_data) + return zipimport.zipimporter(path) except zipimport.ZipImportError as why: log.error("ZIP import error: %s", why) - return (None, None, None) + return None def _compatible_api_versions(api_versions): @@ -303,30 +300,31 @@ def _get_plugin_index_by_name(self, name): return (None, None) def _load_plugin_from_directory(self, name, plugindir): - info = None - zipfilename = os.path.join(plugindir, name + '.zip') - (zip_importer, module_name, manifest_data) = zip_import(zipfilename) - if zip_importer: - name = module_name - if not zip_importer.find_module(name): - errorfmt = _('Failed loading zipped plugin "%(plugin)s" from "%(filename)s"') - self.plugin_error(name, errorfmt, params={ - 'plugin': name, - 'filename': zipfilename, - }) - return None - module_pathname = zip_importer.get_filename(name) - else: - # instead of looking for a module in the whole directory, we have to provide an explicit path to it - # e.g. for Picard in /home/user/picard it'd be: - # importlib.util.spec_from_file_location("picard", "/home/user/picard/picard/__init__.py") - # alternatively we could use importlib.find_loader, which works pretty much the same but is unfortunately - # deprecated since 3.4 - # - # comment to be deleted, just for the PR purposes - # info = importlib.machinery.PathFinder().find_spec(name, [plugindir]) - info = PluginMetaPathFinder().find_spec(_PLUGIN_MODULE_PREFIX + name, [plugindir]) - if not info or not info.loader: + spec = None + module_pathname = None + zip_importer = None + manifest_data = None + full_module_name = _PLUGIN_MODULE_PREFIX + name + + # Legacy loading of ZIP plugins. In Python >= 3.10 this is all handled + # by PluginMetaPathFinder. Remove once Python 3.9 is no longer supported. + if not hasattr(zipimport.zipimporter, 'find_spec'): + zipfilename = os.path.join(plugindir, name + '.zip') + zip_importer = zip_import(zipfilename) + if zip_importer: + if not zip_importer.find_module(name): + errorfmt = _('Failed loading zipped plugin "%(plugin)s" from "%(filename)s"') + self.plugin_error(name, errorfmt, params={ + 'plugin': name, + 'filename': zipfilename, + }) + return None + module_pathname = zip_importer.get_filename(name) + manifest_data = load_zip_manifest(zip_importer.archive) + + if not module_pathname: + spec = PluginMetaPathFinder().find_spec(full_module_name, [plugindir]) + if not spec or not spec.loader: errorfmt = _('Failed loading plugin "%(plugin)s" in "%(dirname)s"') self.plugin_error(name, errorfmt, params={ 'plugin': name, @@ -334,7 +332,9 @@ def _load_plugin_from_directory(self, name, plugindir): }) return None - module_pathname = info.origin + module_pathname = spec.origin + if isinstance(spec.loader, zipimport.zipimporter): + manifest_data = load_zip_manifest(spec.loader.archive) if module_pathname.endswith("__init__.py"): module_pathname = os.path.dirname(module_pathname) @@ -349,12 +349,11 @@ def _load_plugin_from_directory(self, name, plugindir): existing_plugin.version, existing_plugin.file) _unregister_module_extensions(name) - full_module_name = _PLUGIN_MODULE_PREFIX + name if zip_importer: plugin_module = zip_importer.load_module(full_module_name) else: - plugin_module = importlib.util.module_from_spec(info) - info.loader.exec_module(plugin_module) + plugin_module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(plugin_module) plugin = PluginWrapper(plugin_module, plugindir, file=module_pathname, manifest_data=manifest_data) @@ -598,9 +597,18 @@ class PluginMetaPathFinder(MetaPathFinder): def find_spec(self, fullname, path, target=None): if not fullname.startswith(_PLUGIN_MODULE_PREFIX): return None - info = importlib.machinery.PathFinder().find_spec(fullname, _plugin_dirs) - if info and info.loader: - return info + plugin_name = fullname[len(_PLUGIN_MODULE_PREFIX):] + for plugin_dir in _plugin_dirs: + spec = None + if hasattr(zipimport.zipimporter, 'find_spec'): # Python >= 3.10 + zipfilename = os.path.join(plugin_dir, plugin_name + '.zip') + zip_importer = zip_import(zipfilename) + if zip_importer: + spec = zip_importer.find_spec(fullname) + if not spec: + spec = importlib.machinery.PathFinder().find_spec(fullname, [plugin_dir]) + if spec and spec.loader: + return spec sys.meta_path.append(PluginMetaPathFinder()) From bcda101daec81f64685b11bd398c4af853f4f040 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sat, 9 Sep 2023 17:17:52 +0200 Subject: [PATCH 06/11] pluginmanager: Avoid multiple calls to PathFinder().find_spec() --- picard/pluginmanager.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index c979c782f2..70c0566927 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -598,17 +598,17 @@ def find_spec(self, fullname, path, target=None): if not fullname.startswith(_PLUGIN_MODULE_PREFIX): return None plugin_name = fullname[len(_PLUGIN_MODULE_PREFIX):] - for plugin_dir in _plugin_dirs: - spec = None - if hasattr(zipimport.zipimporter, 'find_spec'): # Python >= 3.10 + spec = None + if hasattr(zipimport.zipimporter, 'find_spec'): # Python >= 3.10 + for plugin_dir in _plugin_dirs: zipfilename = os.path.join(plugin_dir, plugin_name + '.zip') zip_importer = zip_import(zipfilename) if zip_importer: spec = zip_importer.find_spec(fullname) - if not spec: - spec = importlib.machinery.PathFinder().find_spec(fullname, [plugin_dir]) - if spec and spec.loader: - return spec + break + if not spec: + spec = importlib.machinery.PathFinder().find_spec(fullname, _plugin_dirs) + return spec if spec and spec.loader else None sys.meta_path.append(PluginMetaPathFinder()) From 70904c11e035c2ad850c2eb3d09509808e25aea9 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sun, 10 Sep 2023 11:47:32 +0200 Subject: [PATCH 07/11] Refactor PluginMetaPathFinder --- picard/pluginmanager.py | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index 70c0566927..6fdf00e8e0 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -598,17 +598,30 @@ def find_spec(self, fullname, path, target=None): if not fullname.startswith(_PLUGIN_MODULE_PREFIX): return None plugin_name = fullname[len(_PLUGIN_MODULE_PREFIX):] - spec = None + for plugin_dir in plugin_dirs(): + for file_path in self._plugin_file_paths(plugin_dir, plugin_name): + if os.path.exists(file_path): + spec = self._spec_from_path(fullname, file_path) + if spec and spec.loader: + return spec + + def _spec_from_path(self, fullname, file_path): + if file_path.endswith('.zip'): + return self._spec_from_zip(fullname, file_path) + else: + return importlib.util.spec_from_file_location(fullname, file_path) + + def _spec_from_zip(self, fullname, file_path): + zip_importer = zip_import(file_path) + if zip_importer: + return zip_importer.find_spec(fullname) + + @staticmethod + def _plugin_file_paths(plugin_dir, plugin_name): + yield os.path.join(plugin_dir, plugin_name, '__init__.py') + yield os.path.join(plugin_dir, plugin_name + '.py') if hasattr(zipimport.zipimporter, 'find_spec'): # Python >= 3.10 - for plugin_dir in _plugin_dirs: - zipfilename = os.path.join(plugin_dir, plugin_name + '.zip') - zip_importer = zip_import(zipfilename) - if zip_importer: - spec = zip_importer.find_spec(fullname) - break - if not spec: - spec = importlib.machinery.PathFinder().find_spec(fullname, _plugin_dirs) - return spec if spec and spec.loader else None + yield os.path.join(plugin_dir, plugin_name + '.zip') sys.meta_path.append(PluginMetaPathFinder()) From 4db506c49f8c4e8dcf73ec9bfdba40010b626173 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sun, 10 Sep 2023 12:28:48 +0200 Subject: [PATCH 08/11] PICARD-2751: Avoid plugins with relative imports executing plugin body twice --- picard/pluginmanager.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index 6fdf00e8e0..da33d9ccf1 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -353,6 +353,12 @@ def _load_plugin_from_directory(self, name, plugindir): plugin_module = zip_importer.load_module(full_module_name) else: plugin_module = importlib.util.module_from_spec(spec) + # This is kind of a hack. The module will be in sys.modules + # after exec_module has run. But if inside of the loaded plugin + # there are relative imports it would load the same plugin + # module twice. This executes the plugins code twice and leads + # to potential side effects. + sys.modules[full_module_name] = plugin_module spec.loader.exec_module(plugin_module) plugin = PluginWrapper(plugin_module, plugindir, From a88975249f2a0f7ff4d5f5779f1c8f326e11a393 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sun, 10 Sep 2023 13:01:09 +0200 Subject: [PATCH 09/11] PICARD-2751: Load plugins in plugin dir priority order If a plugin is already loaded avoid running its code again. This avoids double execution in case the plugin is in both the user and system plugin directory. --- picard/pluginmanager.py | 64 +++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index da33d9ccf1..0541d624f0 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -213,6 +213,13 @@ def register_plugin_dir(path): _plugin_dirs.append(path) +def plugin_dir_for_path(path): + for plugin_dir in plugin_dirs(): + if path.startswith(plugin_dir): + return plugin_dir + return path + + class PluginManager(QtCore.QObject): plugin_installed = QtCore.pyqtSignal(PluginWrapper, bool) @@ -289,7 +296,7 @@ def load_plugins_from_directory(self, plugindir): len(names)) for name in sorted(names): try: - self._load_plugin_from_directory(name, plugindir) + self._load_plugin(name) except Exception: self.plugin_error(name, _("Unable to load plugin '%s'"), name, log_func=log.exception) @@ -299,7 +306,15 @@ def _get_plugin_index_by_name(self, name): return (plugin, index) return (None, None) - def _load_plugin_from_directory(self, name, plugindir): + def _load_plugin(self, name): + existing_plugin, existing_plugin_index = self._get_plugin_index_by_name(name) + if existing_plugin: + log.debug("Ignoring already loaded plugin %r (version %r at %r)", + existing_plugin.module_name, + existing_plugin.version, + existing_plugin.file) + return + spec = None module_pathname = None zip_importer = None @@ -309,26 +324,27 @@ def _load_plugin_from_directory(self, name, plugindir): # Legacy loading of ZIP plugins. In Python >= 3.10 this is all handled # by PluginMetaPathFinder. Remove once Python 3.9 is no longer supported. if not hasattr(zipimport.zipimporter, 'find_spec'): - zipfilename = os.path.join(plugindir, name + '.zip') - zip_importer = zip_import(zipfilename) - if zip_importer: - if not zip_importer.find_module(name): - errorfmt = _('Failed loading zipped plugin "%(plugin)s" from "%(filename)s"') - self.plugin_error(name, errorfmt, params={ - 'plugin': name, - 'filename': zipfilename, - }) - return None - module_pathname = zip_importer.get_filename(name) - manifest_data = load_zip_manifest(zip_importer.archive) + for plugin_dir in plugin_dirs(): + zipfilename = os.path.join(plugin_dir, name + '.zip') + zip_importer = zip_import(zipfilename) + if zip_importer: + if not zip_importer.find_module(name): + errorfmt = _('Failed loading zipped plugin "%(plugin)s" from "%(filename)s"') + self.plugin_error(name, errorfmt, params={ + 'plugin': name, + 'filename': zipfilename, + }) + return None + module_pathname = zip_importer.get_filename(name) + manifest_data = load_zip_manifest(zip_importer.archive) + break if not module_pathname: - spec = PluginMetaPathFinder().find_spec(full_module_name, [plugindir]) + spec = PluginMetaPathFinder().find_spec(full_module_name, []) if not spec or not spec.loader: - errorfmt = _('Failed loading plugin "%(plugin)s" in "%(dirname)s"') + errorfmt = _('Failed loading plugin "%(plugin)s"') self.plugin_error(name, errorfmt, params={ 'plugin': name, - 'dirname': plugindir, }) return None @@ -337,18 +353,10 @@ def _load_plugin_from_directory(self, name, plugindir): manifest_data = load_zip_manifest(spec.loader.archive) if module_pathname.endswith("__init__.py"): module_pathname = os.path.dirname(module_pathname) + plugin_dir = plugin_dir_for_path(module_pathname) plugin = None try: - existing_plugin, existing_plugin_index = self._get_plugin_index_by_name(name) - if existing_plugin: - log.warning("Module %r conflict: unregistering previously" - " loaded %r version %s from %r", - existing_plugin.module_name, - existing_plugin.name, - existing_plugin.version, - existing_plugin.file) - _unregister_module_extensions(name) if zip_importer: plugin_module = zip_importer.load_module(full_module_name) else: @@ -361,7 +369,7 @@ def _load_plugin_from_directory(self, name, plugindir): sys.modules[full_module_name] = plugin_module spec.loader.exec_module(plugin_module) - plugin = PluginWrapper(plugin_module, plugindir, + plugin = PluginWrapper(plugin_module, plugin_dir, file=module_pathname, manifest_data=manifest_data) compatible_versions = _compatible_api_versions(plugin.api_versions) if compatible_versions: @@ -498,7 +506,7 @@ def install_plugin(self, path, update=False, plugin_name=None, plugin_data=None) if not update: try: - installed_plugin = self._load_plugin_from_directory(plugin_name, self.plugins_directory) + installed_plugin = self._load_plugin(plugin_name) if not installed_plugin: raise RuntimeError("Failed loading newly installed plugin %s" % plugin_name) except Exception as e: From 83ea460c5738366bcbb736229e3f978cfbbeba4e Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sun, 10 Sep 2023 13:28:42 +0200 Subject: [PATCH 10/11] Moved legacy code out of PluginManager._load_plugin --- picard/pluginmanager.py | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index 0541d624f0..cc89b717e8 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -324,20 +324,7 @@ def _load_plugin(self, name): # Legacy loading of ZIP plugins. In Python >= 3.10 this is all handled # by PluginMetaPathFinder. Remove once Python 3.9 is no longer supported. if not hasattr(zipimport.zipimporter, 'find_spec'): - for plugin_dir in plugin_dirs(): - zipfilename = os.path.join(plugin_dir, name + '.zip') - zip_importer = zip_import(zipfilename) - if zip_importer: - if not zip_importer.find_module(name): - errorfmt = _('Failed loading zipped plugin "%(plugin)s" from "%(filename)s"') - self.plugin_error(name, errorfmt, params={ - 'plugin': name, - 'filename': zipfilename, - }) - return None - module_pathname = zip_importer.get_filename(name) - manifest_data = load_zip_manifest(zip_importer.archive) - break + (zip_importer, plugin_dir, module_pathname, manifest_data) = self._legacy_load_zip_plugin(name) if not module_pathname: spec = PluginMetaPathFinder().find_spec(full_module_name, []) @@ -357,7 +344,7 @@ def _load_plugin(self, name): plugin = None try: - if zip_importer: + if zip_importer: # Legacy ZIP import for Python < 3.10 plugin_module = zip_importer.load_module(full_module_name) else: plugin_module = importlib.util.module_from_spec(spec) @@ -401,6 +388,23 @@ def _load_plugin(self, name): params={'plugin': name}) return plugin + def _legacy_load_zip_plugin(self, name): + for plugin_dir in plugin_dirs(): + zipfilename = os.path.join(plugin_dir, name + '.zip') + zip_importer = zip_import(zipfilename) + if zip_importer: + if not zip_importer.find_module(name): + errorfmt = _('Failed loading zipped plugin "%(plugin)s" from "%(filename)s"') + self.plugin_error(name, errorfmt, params={ + 'plugin': name, + 'filename': zipfilename, + }) + return (None, None, None, None) + module_pathname = zip_importer.get_filename(name) + manifest_data = load_zip_manifest(zip_importer.archive) + return (zip_importer, plugin_dir, module_pathname, manifest_data) + return (None, None, None, None) + def _get_existing_paths(self, plugin_name, fileexts): dirpath = os.path.join(self.plugins_directory, plugin_name) if not os.path.isdir(dirpath): From 7d84330c5f1020a97ac4ab04e39246779dd66c87 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Sun, 10 Sep 2023 16:31:19 +0200 Subject: [PATCH 11/11] PluginManager: Minor code cleanup --- picard/pluginmanager.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/picard/pluginmanager.py b/picard/pluginmanager.py index cc89b717e8..3ffbcb9f39 100644 --- a/picard/pluginmanager.py +++ b/picard/pluginmanager.py @@ -215,8 +215,11 @@ def register_plugin_dir(path): def plugin_dir_for_path(path): for plugin_dir in plugin_dirs(): - if path.startswith(plugin_dir): - return plugin_dir + try: + if os.path.commonpath((path, plugin_dir)) == plugin_dir: + return plugin_dir + except ValueError: + pass return path @@ -338,7 +341,7 @@ def _load_plugin(self, name): module_pathname = spec.origin if isinstance(spec.loader, zipimport.zipimporter): manifest_data = load_zip_manifest(spec.loader.archive) - if module_pathname.endswith("__init__.py"): + if os.path.basename(module_pathname) == '__init__.py': module_pathname = os.path.dirname(module_pathname) plugin_dir = plugin_dir_for_path(module_pathname)