Repository: lektor/lektor
Pull request #1073 (2022-09-20 01:16:00)

Clean up handling of plugin distribution metadata
This fixes and cleans up work done in #1061.

The issue is how to find the distribution which provides a particular Lektor plugin.
`Importlib.metadata` provides the `packages_distribution` function which sort of does the right thing, but,
it turns out that it is not totally reliable[^1].

In any case, under the hood, `packages_distribution` just iterates over all the distributions.  It’s cleaner just to
iterate the distributions to find the plugins — then we *know for sure* which distribution a given plugin comes from.


[^1]: `Packages_distributions` (more-or-less) depends on the distribution(s) providing a `top_level.txt` in its `.pkg-info`.
    It appears that `top_level.txt` is not specified in any PEP and is a remnant from the days of eggs.  Most distributions
    still provide them, but some do not (e.g. python-poetry/poetry#3093.)

<!---
Remember to please check that your code passes tests locally. Test with the Makefile
commands `make test`, or if you only need to test just Python / JS `make test-python`
or `make test-js`.
--->

### Issue(s) Resolved

<!--- List the issue(s) below, in the form "Fixes #1234"; one per line --->

Fixes #

### Related Issues / Links

<!---
Are there any similar or related issues or pull requests?
Did you make a pull request to update the docs?
--->

### Description of Changes

- [x] Wrote at least one-line docstrings (for any new functions)
- [x] Added unit test(s) covering the changes (if testable)


<!--- Explain what you've done and why --->

<!--- Thanks for your help making Lektor better for everyone! --->



Patch:
diff --git a/lektor/compat.py b/lektor/compat.py
index 95c61fe..b73e3c2 100644
--- a/lektor/compat.py
+++ b/lektor/compat.py
@@ -51,11 +51,7 @@ class FixedTemporaryDirectory(tempfile.TemporaryDirectory):
 
 if sys.version_info >= (3, 8):
     TemporaryDirectory = tempfile.TemporaryDirectory
-else:
-    TemporaryDirectory = FixedTemporaryDirectory
-
-if sys.version_info >= (3, 10):
     from importlib import metadata as importlib_metadata
 else:
-    # we use importlib.metadata.packages_distributions() which is new in python 3.10
+    TemporaryDirectory = FixedTemporaryDirectory
     import importlib_metadata
diff --git a/lektor/pluginsystem.py b/lektor/pluginsystem.py
index 090af97..4258870 100644
--- a/lektor/pluginsystem.py
+++ b/lektor/pluginsystem.py
@@ -1,6 +1,9 @@
+from __future__ import annotations
+
 import os
 import sys
 import warnings
+from typing import Type
 from weakref import ref as weakref
 
 from inifile import IniFile
@@ -33,6 +36,8 @@ class Plugin:
     name = "Your Plugin Name"
     description = "Description goes here"
 
+    __dist: metadata.Distribution = None
+
     def __init__(self, env, id):
         self._env = weakref(env)
         self.id = id
@@ -46,10 +51,9 @@ class Plugin:
 
     @property
     def version(self):
-        try:
-            return metadata.version(_dist_name_for_module(self.__module__))
-        except LookupError:
-            return None
+        if self.__dist is not None:
+            return self.__dist.version
+        return None
 
     @property
     def path(self):
@@ -109,32 +113,39 @@ class Plugin:
         }
 
 
-def load_plugins():
-    """Loads all available plugins and returns them."""
-    rv = {}
-    for ep in metadata.entry_points(  # pylint: disable=unexpected-keyword-arg
-        group="lektor.plugins"
-    ):
-        # XXX: do we really need to be so strict about distribution names?
-        match_name = "lektor-" + ep.name.lower()
-        try:
-            dist_name = _dist_name_for_module(ep.module)
-        except LookupError:
-            dist_name = ""
-        if match_name != dist_name.lower():
-            raise RuntimeError(
-                "Disallowed distribution name: distribution name for "
-                f"plugin {ep.name!r} must be {match_name!r} (not {dist_name!r})."
-            )
-        rv[ep.name] = ep.load()
-    return rv
+def _find_plugins():
+    """Find all available plugins.
+
+    Returns an interator of (distribution, entry_point) pairs.
+    """
+    for dist in metadata.distributions():
+        for ep in dist.entry_points:
+            if ep.group == "lektor.plugins":
+                _check_dist_name(dist.metadata["Name"], ep.name)
+                yield dist, ep
+
+
+def _check_dist_name(dist_name, plugin_id):
+    """Check that plugin comes from a validly named distribution.
+
+    Raises RuntimeError if distribution name is not of the form
+    ``lektor-``*<plugin_id>*.
+    """
+    # XXX: do we really need to be so strict about distribution names?
+    match_name = "lektor-" + plugin_id.lower()
+    if match_name != dist_name.lower():
+        raise RuntimeError(
+            "Disallowed distribution name: distribution name for "
+            f"plugin {plugin_id!r} must be {match_name!r} (not {dist_name!r})."
+        )
 
 
 def initialize_plugins(env):
     """Initializes the plugins for the environment."""
-    plugins = load_plugins()
-    for plugin_id, plugin_cls in plugins.items():
-        env.plugin_controller.instanciate_plugin(plugin_id, plugin_cls)
+    for dist, ep in _find_plugins():
+        plugin_id = ep.name
+        plugin_cls = ep.load()
+        env.plugin_controller.instanciate_plugin(plugin_id, plugin_cls, dist)
     env.plugin_controller.emit("setup-env")
 
 
@@ -154,11 +165,22 @@ class PluginController:
             raise RuntimeError("Environment went away")
         return rv
 
-    def instanciate_plugin(self, plugin_id, plugin_cls):
+    def instanciate_plugin(
+        self,
+        plugin_id: str,
+        plugin_cls: Type[Plugin],
+        dist: metadata.Distribution | None = None,
+    ) -> None:
         env = self.env
         if plugin_id in env.plugins:
             raise RuntimeError('Plugin "%s" is already registered' % plugin_id)
-        env.plugins[plugin_id] = plugin_cls(env, plugin_id)
+        plugin = plugin_cls(env, plugin_id)
+        # Plugin.version needs the source distribution to be able to cleanly determine
+        # the plugin version.  For reasons of backward compatibility, we don't want to
+        # change the signature of the constructor, so we stick it in a private attribute
+        # here.
+        plugin._Plugin__dist = dist
+        env.plugins[plugin_id] = plugin
         env.plugin_ids_by_class[plugin_cls] = plugin_id
 
     def iter_plugins(self):
@@ -187,12 +209,3 @@ class PluginController:
                         DeprecationWarning,
                     )
         return rv
-
-
-def _dist_name_for_module(module: str) -> "str | None":
-    """Return the name of the distribution which provides the named module."""
-    top_level = module.partition(".")[0]
-    distributions = metadata.packages_distributions().get(top_level, [])
-    if len(distributions) != 1:
-        raise LookupError(f"Can not find distribution for {module}")
-    return distributions[0]
diff --git a/setup.cfg b/setup.cfg
index 88c74dd..baae718 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -40,7 +40,7 @@ install_requires =
     EXIFRead
     filetype>=1.0.7
     Flask
-    importlib_metadata; python_version<"3.10"
+    importlib_metadata; python_version<"3.8"
     inifile>=0.4.1
     Jinja2>=3.0
     MarkupSafe


Test patch:
diff --git a/tests/test_pluginsystem.py b/tests/test_pluginsystem.py
index a061f98..c0b7f85 100644
--- a/tests/test_pluginsystem.py
+++ b/tests/test_pluginsystem.py
@@ -13,9 +13,9 @@ from lektor.cli import cli
 from lektor.compat import importlib_metadata as metadata
 from lektor.context import Context
 from lektor.packages import add_package_to_project
-from lektor.pluginsystem import _dist_name_for_module
+from lektor.pluginsystem import _check_dist_name
+from lektor.pluginsystem import _find_plugins
 from lektor.pluginsystem import get_plugin
-from lektor.pluginsystem import load_plugins
 from lektor.pluginsystem import Plugin
 from lektor.pluginsystem import PluginController
 
@@ -107,18 +107,11 @@ class DummyPluginFinder(metadata.DistributionFinder):
 
 
 @pytest.fixture
-def dummy_plugin_distribution_name():
-    return "lektor-dummy-plugin"
-
-
-@pytest.fixture
-def dummy_plugin_distribution(
-    dummy_plugin_distribution_name, save_sys_path, monkeypatch
-):
+def dummy_plugin_distribution(save_sys_path, monkeypatch):
     """Add a dummy plugin distribution to the current working_set."""
     dist = DummyDistribution(
         {
-            "Name": dummy_plugin_distribution_name,
+            "Name": "lektor-dummy-plugin",
             "Version": "1.23",
         }
     )
@@ -128,9 +121,11 @@ def dummy_plugin_distribution(
 
 
 @pytest.fixture
-def dummy_plugin(env):
+def dummy_plugin(env, dummy_plugin_distribution):
     """Instantiate and register a dummy plugin in env"""
-    env.plugin_controller.instanciate_plugin("dummy-plugin", DummyPlugin)
+    env.plugin_controller.instanciate_plugin(
+        "dummy-plugin", DummyPlugin, dummy_plugin_distribution
+    )
     return env.plugins["dummy-plugin"]
 
 
@@ -165,9 +160,11 @@ class TestPlugin:
         return scratch_project_data
 
     @pytest.fixture
-    def scratch_plugin(self, scratch_env):
+    def scratch_plugin(self, scratch_env, dummy_plugin_distribution):
         """Instantiate and register a dummy plugin with scratch_env"""
-        scratch_env.plugin_controller.instanciate_plugin("dummy-plugin", DummyPlugin)
+        scratch_env.plugin_controller.instanciate_plugin(
+            "dummy-plugin", DummyPlugin, dummy_plugin_distribution
+        )
         return scratch_env.plugins["dummy-plugin"]
 
     def test_env(self, dummy_plugin, env):
@@ -183,6 +180,12 @@ class TestPlugin:
     def test_version(self, dummy_plugin, dummy_plugin_distribution):
         assert dummy_plugin.version == dummy_plugin_distribution.version
 
+    def test_version_missing(self, env):
+        # Instantiate plugin without specifying distribution
+        env.plugin_controller.instanciate_plugin("dummy-plugin", DummyPlugin)
+        plugin = env.plugins["dummy-plugin"]
+        assert plugin.version is None
+
     def test_path(self, dummy_plugin):
         assert dummy_plugin.path == str(Path(__file__).parent)
 
@@ -252,14 +255,30 @@ class TestPlugin:
         }
 
 
-def test_load_plugins(dummy_plugin_distribution):
-    assert load_plugins() == {"dummy-plugin": DummyPlugin}
+def test_find_plugins(dummy_plugin_distribution):
+    (ep,) = dummy_plugin_distribution.entry_points
+    assert list(_find_plugins()) == [(dummy_plugin_distribution, ep)]
+
 
+@pytest.mark.parametrize(
+    "dist_name, plugin_id",
+    [
+        ("Lektor-FOO", "Foo"),
+    ],
+)
+def test_check_dist_name(dist_name, plugin_id):
+    _check_dist_name(dist_name, plugin_id)
 
-@pytest.mark.parametrize("dummy_plugin_distribution_name", ["evil-dist-name"])
-def test_load_plugins_bad_distname(dummy_plugin_distribution):
-    with pytest.raises(RuntimeError, match=r"Disallowed distribution name"):
-        load_plugins()
+
+@pytest.mark.parametrize(
+    "dist_name, plugin_id",
+    [
+        ("NotLektor-FOO", "Foo"),
+    ],
+)
+def test_check_dist_name_raises(dist_name, plugin_id):
+    with pytest.raises(RuntimeError):
+        _check_dist_name(dist_name, plugin_id)
 
 
 class TestPluginController:
@@ -328,12 +347,3 @@ def test_cli_integration(project, cli_runner, monkeypatch):
     )
     for call in DummyPlugin.calls:
         assert call["extra_flags"] == {"flag1": "flag1", "flag2": "value2"}
-
-
-def test_dist_name_form_module():
-    assert _dist_name_for_module("lektor.db") == "Lektor"
-
-
-def test_dist_name_form_module_raises_exception():
-    with pytest.raises(LookupError):
-        assert _dist_name_for_module("missing.module")