Fix version check for entrypoint plugins with unequal project/package names

[SnoopJ]

Description

Fixes #2593.

This changeset swallows all exceptions when trying to determine the version of an EntryPointPlugin, as there are more failure cases than what catching ValueError specifically covers, and allowing those exceptions to propagate breaks plugin reloading.

Checklist

• I have read CONTRIBUTING.md
• I can and do license this contribution under the EFLv2
• No issues are reported by make qa (runs make lint and make test)
  • 1388 passed, 8 xfailed, 1 warning in 45.44s
• I have tested the functionality of the things this change touches

[dgw]

I dug into the original issue just a bit (literally, this time) and found that we probably could differentiate the EntryPointPlugin type from its parent class PyModulePlugin a bit more.

This PR works as a quick fix for the exact issue you ran into, but I do think the real solution should be actually handling entry points correctly (i.e. not just as "spicy modules" like the current impl).

For checking the version in particular, an EntryPoint object has a dist attribute, which itself has a name that is most likely what would be expected by the importlib.metadata.version() call whose failure in this mismatched-name case spawned #2593.

[dgw]

Bonked on the underlying issue here with my newest plugin project, which bundles multiple related plugins into a single package as the_collection.thing1.plugin and so on. Sopel of course fails to get the version of such plugins (PackageNotFoundError for the_collection.thing1) because of trying to use the plugin module's __package__ attr.

My patch below is not tested thoroughly, but if you want to come back to this branch it already has a test ready and you probably have Tox set up locally to run said test suite across all supported Python versions. 😸

GitHub still has that annoying limitation on how far away from the actual change you can start a line note, so unfortunately I can't make this into a one-click suggestion:

        # inside `EntryPointPlugin.get_version()`; you already know where,
        # since this PR already touches the same area 😉
        if (
            version is None
            and hasattr(self.entry_point, "dist")
            and hasattr(self.entry_point.dist, "name")
        ):
            try:
                version = importlib.metadata.version(self.entry_point.dist.name)
            except Exception:
                # fine, just give up
                pass

[Exirel]

This PR works as a quick fix for the exact issue you ran into, but I do think the real solution should be actually handling entry points correctly (i.e. not just as "spicy modules" like the current impl).

For checking the version in particular, an EntryPoint object has a dist attribute, which itself has a name that is most likely what would be expected by the importlib.metadata.version() call whose failure in this mismatched-name case spawned #2593.

I was looking for that solution, and yes, this is exactly the right solution here, as demonstrated by this snippet of code (running with Python 3.10):

>>> from importlib.metadata import entry_points, version
>>> for entry_point in entry_points(group='console_scripts'):
...     print(entry_point.name, entry_point.dist.name, version(entry_point.dist.name))
...
pip pip 23.0.1
pip3 pip 23.0.1
pip3.9 pip 23.0.1

Once you have an EntryPoint object, you can ask its dist, which has a name, and that's the name you want when calling version(name).

Alternatively, I don't mind if we separate the plugin version from the package version, i.e. allow a Plugin author to use __version__ in the plugin file (as exposed by the entry point).

[dgw]

Alternatively, I don't mind if we separate the plugin version from the package version, i.e. allow a Plugin author to use __version__ in the plugin file (as exposed by the entry point).

I think the intent is already there in the code, by checking whether the version is None after calling super().get_version() before EntryPointPlugin tries its own logic. But we can test it when fixing this dist-name bug!

[SnoopJ]

After discussing on IRC, what's left here is for me to integrate @dgw's suggested tweak, and also to see about adding a logger instance here to strike a balance between more broadly catching exceptions and falling back on the None base case for get_version() and recording what went wrong.

[dgw]

We've been talking about this one on IRC this evening, and I believe @SnoopJ and I have found a good middle ground:

If logging works here (we haven't tested how logging interacts with the CLI interfaces like sopel-plugins), it would be good to catch ValueError and PackageNotFoundError which the importlib.metadata.version() call might raise, and just give up. Maybe log a warning, but that's it. As a fallback, catch Exception and log the full exception traceback for investigation. We'd like to know about other failure cases if they exist, but they shouldn't crash the CLI or the .version command called from IRC.

[dgw]

I had a partial implementation of what's discussed in #2594 (comment) and committed it on top of this branch to clean out uncommitted changes in my Gitpod workspace (they are apparently discontinuing their turnkey cloud IDE product, which sucks). CI failures appear unrelated; the error is in url:

mypy --check-untyped-defs sopel
sopel/builtins/url.py:440: error: Argument 1 to "ip_address" has incompatible type "Rdata"; expected "Union[int, str, bytes, IPv4Address, IPv6Address]"  [arg-type]

Must have been an update somewhere since the last type-checking cleanup in #2614.