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]

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.

[Exirel]

I approve on the premise that you'll rebase on master and solve the merge conflict. I added a nitpick but it's not blocking anything.

[SnoopJ]

Sorry about the force-push noise, but this should clear the unrelated typing problems in CI now and is ready for final review. Happy to squash it further if desired.

[Exirel]

Lovely!

Reading your commit history, I think it's a good case for a squash into a single commit, because the code & test should go together here, and because the last commits erase/replace the initial commit's changes.

[dgw]

I agree with @Exirel about squashing to a single commit. While I could suggest merging some changes together and rearranging the chronology to make a multi-commit history that tells a coherent story about the feature, this patch isn't big enough to make it worth the time it'd take to carefully rewrite history that way.

Oh, I also played around a little and found that self.entry_point.dist (if it exists) has a version attr. I have not looked at all the edge cases that importlib.metadata.version() should theoretically handle for us, though, so don't mess with the patch any more. This is a curiosity I wanted to mention, not patch feedback!

[SnoopJ]

Squishy squashed again, ready to rock

[dgw]

Realized it's been over a week since this got squashed and readied for re-review… Had to tear myself away from working on updates to a Factorio mod (balance changes for 2.0, etc.) 😅