-
Notifications
You must be signed in to change notification settings - Fork 99
Replace pkg_resources usage with importlib. #747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
c8a82e6 to
b909816
Compare
b95bed1 to
49af39f
Compare
Some APIs used were not introduced until 3.10 so use the backport unless we have it.
49af39f to
00166b3
Compare
The backport module does not implicitly shadow the standard library modules. APIs that we're using in bloom were only introduced in importlib_metadata 3.6 and 3.10 of the standard library. Thus to use this API we need to check our python version and use the backport module explicitly.
This requirement is met by jammy and noble but _not_ focal which includes python 3.8 and importlib-metadata 1.5. Therefore, to release this PR we'll need to drop support for focal.
The backport module for importlib_resources doesn't shadow the standard library import.resources thus we need to import conditionally based on the current Python / standard library version.
cottsay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I'll say that elsewhere we're using a try/except to attempt use of the standard library import and falling back to the standalone package rather than using conditionals based on the version, but I don't see a particular reason that this wouldn't work as well.
In some cases, even when the stdlib library is available (e.g. 3.7+) the specific API functions that we're using were only added later. So library presence is not a sufficient condition to check and we can't rely on the import error alone. To use try-exceptw we'd have to catch the argument error down the line which is pretty nasty. Rather than think about whether I could use try-import-except or had to check the stdlib version I just check the stdlib version everywhere. Edit: specific context in 00166b3 |
|
I just cherry-picked this PR for Debian. FYI, you should probably drop |
| catkin_pkg_v=catkin_pkg.__version__, | ||
| # Until https://github.com/ros-infrastructure/rosdistro/issues/16 | ||
| rosdistro_v=pkg_resources.require("rosdistro")[0].version, | ||
| rosdistro_v=importlib_metadata.metadata("rosdistro").version, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be importlib_metadata.metadata("rosdistro").get("version") btw
Cleaning up bloom for a release and getting pelted with these deprecation warnings. Apparently pkg_resources will be removed in setuptools 81:
Different APIs that we use are only available as of certain stdlib versions so to fill in for Python < 3.10, we use the backport modules from pypi. These modules are also packaged in Debian / Ubuntu and the version combinations that we need are available in Jammy, Noble, Bookwork, and Trixie, however not Focal. Thus focal will need to be dropped from Bloom's supported distros before this can merge.