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

Improve interop with importlib.metadata: variation without importlib_metadata #199

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented May 2, 2024

This is basically the same as #195 but without using importlib_metadata.

Feel free to close if #195 is preferred.

This might be worthy considering:

In general, I find that the following works relatively fine:

  • using importlib_metadata.DistributionFinder, MetadataPathFinder to implement find_distrubitions, but then using importlib.metadata high-level APIs

And the following does not work:

  • using importlib.metadata.DistributionFinder, MetadataPathFinder to implement find_distrubitions, but then using importlib_metadata high-level APIs

i.e. while this approach decreases the "spooky action at distance" of a transitive dependency installing importlib_metadata, it may also be more error prone if some dependency is indeed using importlib_metadata.

@abravalheri abravalheri marked this pull request as ready for review May 2, 2024 11:17
@takluyver
Copy link
Member

Thanks! Does this somehow avoid the exception you found in setuptools tests in this comment?

@abravalheri
Copy link
Contributor Author

For setuptools, it should be fine abravalheri/setuptools#14 (tests should OK, docs failing but that is unrelated).

(I went ahead and already added fixed the importlib.metadata compatibility in setuptools: pypa/setuptools#4339)

@takluyver
Copy link
Member

Aha, so this is functionally similar to the original version of #195 (before involving importlib_metadata), but setuptools has changed so that it works?

I prefer this approach. I'll give it a day or so in case anyone else wants to weigh in, but if not then I'd like to merge this and cut another release.

@abravalheri
Copy link
Contributor Author

(before involving importlib_metadata), but setuptools has changed so that it works?

It is very nuanced 😅

The reason for introducing importlib_metadata was mainly motivated by #195 (comment).

Trying to import importlib_metadata alone does not fix setuptools (because of the vendoring drama).

If setuptools did not have to use vendoring, then yes using importlib_metadata would be enough and no change in setuptools would be necessary. That is related to the drawbacks and considerations discussed in #199 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants