Skip to content
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

use setuptools.parse_version in _version_check #3488

Closed
wants to merge 1 commit into from
Closed

use setuptools.parse_version in _version_check #3488

wants to merge 1 commit into from

Conversation

craigahobbs
Copy link

This eliminates the dependency on packaging.

@alex
Copy link
Member

alex commented Apr 4, 2017

Jenkins, ok to test.

@alex
Copy link
Member

alex commented Apr 4, 2017

@dstufft can you please tell us what to do here :-)

@alex alex assigned alex and unassigned alex Apr 4, 2017
@alex alex requested a review from dstufft April 4, 2017 12:21
@alex
Copy link
Member

alex commented Apr 4, 2017

@craigahobbs what's the motivation here? Modern setuptools depends on packaging so it's not like this adds a new dependency.

@dstufft
Copy link
Member

dstufft commented Apr 4, 2017

I think that the only benefit to this is old setuptools and possibly future setuptools if we convince jaraco to re-bundle his deps. Whether that's worth it or not I dunno, but besides that the change looks fine on the surface.

@alex
Copy link
Member

alex commented Apr 4, 2017

Oh. I just realized, a downside of this is pkg_resources is super slow to import.

@dstufft
Copy link
Member

dstufft commented Apr 4, 2017

We already use pkg_resources to iterate the list of installed backends though right?

@alex
Copy link
Member

alex commented Apr 4, 2017

We moved that import into the function, so it's only imported when you call default_backends.

@alex
Copy link
Member

alex commented Apr 4, 2017

_version_check is called at module scope, so there's no equivilant solution.

@dstufft
Copy link
Member

dstufft commented Apr 4, 2017

Ah right.

@craigahobbs
Copy link
Author

Regarding motivation, it is solely to eliminate the dependency on packaging. Not a huge deal. We're using Python 3.6 and use the setuptools included with Python. Interestingly, it seems to include packaging - it imports it like this "from pkg_resources.extern import packaging" - not that I'd recommend that.

@reaperhulk
Copy link
Member

Yeah we moved to depend directly on packaging to avoid the import time side effects of pkg_resources. So we can't switch back without causing a regression. Sorry, but thanks for the contribution anyway! It's definitely non-obvious why we're doing it this way.

refs #3347 and #3190

@reaperhulk reaperhulk closed this Apr 5, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants