Switch plugin system from pkg_resources to importlib #562
Conversation
There was a problem hiding this comment.
Several comments are mere jubilation. The others, including suggestions are entirely discretionary / informational and don't block approval.
This is a large change but I'm impressed that the majority of the changes are the update of the module name / class names to the new API and more importantly. It does not seem like the new API is as dependent on external libraries. Thanks for tackling this!
| entry_map = dist.get_entry_map() | ||
| for group_name in entry_map.keys(): | ||
| seen = set() | ||
| for dist in distributions(): |
There was a problem hiding this comment.
The iterator for value dist was sorted(working_set) previously. Do we need to sort distributions() as well?
| for dist in distributions(): | |
| for dist in sorted(distributions()): |
There was a problem hiding this comment.
As far as I understand, this no longer works as expected in the current code due to pypa/setuptools#3791, and the importlib-based class isn't inherently sort-able. We'd have to implement our own sort routine, which would involve parsing the version numbers for comparison. It has the potential to be pretty expensive.
From what I've gathered, it's not a well-supported scenario by any tooling to have multiple versions of a distribution installed in the same prefix anyway.
Also worth questioning is whether this was the right thing to do to begin with. Assuming the working set contained all of the distributions across all of the discoverable prefixes, sorting it like that would always prefer the newest version of a distribution, where an import would prefer the one(s) discoverable soonest in the prefix list.
If I'm not mistaken, the existing code wouldn't allow you to (for example) build an older version of a package in a colcon workspace than the one installed on your system and source that workspace, because this code would always prefer the newer version even though an import statement would not.
There was a problem hiding this comment.
I dug into this a little more.
In pkg_resources, it appears that the path ordering was respected and you were never given multiple distributions with the same "key", so if there are multiple dists in the same path directory, it's a matter of luck which one you get.
In importlib.metadata, the dists are not de-duplicated, so you will likely encounter multiple with the same key. If we sorted them, you might get "inferior" or shadowed versions showing up with higher precedence because they have a higher version number.
Since multiple versions in the same path really isn't supported well anywhere, I don't think there's any harm in skipping the sort for performance reasons. I'll say that the one remaining benefit is that it makes the duplicate entry point definition error more deterministic.
cottsay
force-pushed
the
cottsay/extension-point
branch
4 times, most recently
from
1ef4676
to
b3440ff
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #562 +/- ##
==========================================
+ Coverage 81.88% 82.04% +0.16%
==========================================
Files 62 63 +1
Lines 3648 3721 +73
Branches 705 718 +13
==========================================
+ Hits 2987 3053 +66
- Misses 609 613 +4
- Partials 52 55 +3
☔ View full report in Codecov by Sentry. |
cottsay
force-pushed
the
cottsay/extension-point
branch
2 times, most recently
from
966c32d
to
f8ee3a4
Compare
cottsay
force-pushed
the
cottsay/extension-point
branch
from
f8ee3a4
to
aa58f1d
Compare
cottsay
force-pushed
the
cottsay/extension-point
branch
2 times, most recently
from
3acbd4c
to
efc8d8d
Compare
This change deprecates the critical entry_point module in colcon-core in favor of a new module called 'extension_point' which is not coupled to the pkg_resources module of setuptools like its predecessor.
cottsay
force-pushed
the
cottsay/extension-point
branch
from
efc8d8d
to
dab8cad
Compare
This change deprecates the critical entry_point module in colcon-core in favor of a new module called 'extension_point' which is not coupled to the pkg_resources module of setuptools like its predecessor.
There are two commits in this change. The first introduces a replacement API for the existing
entry_pointmodule. The latter is coupled topkg_resourcesby returning classes defined there. Rather than try to mimic that API, I decided to replace it with one that uses only basic types to return the information we need.The second change replaces the implementation behind the new
extension_pointAPI frompkg_resourcestoimportlib.metadata.Downstream consumers of
entry_pointwill start getting a deprecation warning in addition to the existingpkg_resourcesdeprecation warning, and will need to update to use the newextension_pointfunctions. I've been prototyping those changes incolcon-devtools, which has the most complex uses of the API right now.