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

Merged
merged 6 commits into from
Sep 29, 2024

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.

Closes #192

@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).

@ofek
Copy link

ofek commented Jun 26, 2024

What are the next steps for this to be merged?

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Aug 15, 2024
- Bump PORTREVISION for package change

PR:		280646
Obtained from:	pypa/pyproject-hooks#199
nanorkyo pushed a commit to nanorkyo/freebsd-ports that referenced this pull request Aug 21, 2024
- Bump PORTREVISION for package change

PR:		280646
Obtained from:	pypa/pyproject-hooks#199
@abravalheri
Copy link
Contributor Author

abravalheri commented Sep 11, 2024

Hi @takluyver, @jaraco (and/or any other maintainers) is this approach (or the one in #195 which includes importlib_metadata), a direction that the project is interested in pursuing? Or is the project interested in pursuing a different approach?

@jaraco
Copy link
Member

jaraco commented Sep 11, 2024

I unfortunately have paged out the rich context, but I agree that it sounds as if this narrow and surgical fix may be a very solid improvement. Let's go with it for now. Thanks for the diligent investigation and proposal.

@nanonyme
Copy link

Would it be possible to make a release with this soon? The bug this aims to fix is basically update blocker for pyproject-hooks for any distro-like entities.

@icp1994
Copy link

icp1994 commented Sep 26, 2024

And for any distro which has upgraded pyproject-hooks unaware of this cannot update affected packages - pex for example.

@jaraco
Copy link
Member

jaraco commented Sep 27, 2024

@pradyunsg or @takluyver ^

@takluyver takluyver added this to the 1.2 milestone Sep 29, 2024
@takluyver
Copy link
Member

Sorry for the delay.

@takluyver takluyver merged commit 6b57973 into pypa:main Sep 29, 2024
20 checks passed
@takluyver
Copy link
Member

v1.2 is tagged and on PyPI now - thanks for your patience and for the periodic reminders.

@abravalheri abravalheri deleted the issue-192-take2 branch September 29, 2024 11:15
@henryiii
Copy link
Contributor

henryiii commented Oct 6, 2024

1.2 has broken build's test suite when using backend-path on Python 3.8 and 3.9 (and check-sdist's setuptools test, though I just dropped the setuptools test there). See pypa/build#820, python/importlib_metadata#508, henryiii/check-sdist#55. I've finally narrowed it down to this package's 1.2 release.

Backend path is ".", and setuptools is making test_no_prepare.egg-info, which is then getting picked up as a nameless distribution, which causes a confusing error when trying to get the name and falling back on None, then not handling the None case.

@takluyver
Copy link
Member

😮‍💨 Sorry.

I have no idea what's the right thing to do here now. #165 was meant to improve how an in-tree backend was loaded, but broke setuptools in some scenarios (#192). This PR was meant to fix that, but it seems that it's broken something in another scenario. 😞 Maybe we should have used importlib_metadata in some scenarios? But IIUC setuptools also has its own bundled copy, so that doesn't necessarily solve it.

I'm sorely tempted to declare that pyproject-hooks now requires Python 3.10 or above, and you should just pin whichever version works for you on older Python versions. I'm normally pretty keen on backwards compatibility, so I'd welcome a better solution, but I don't have the time or energy to wade through importlib stuff to figure this out. And I don't know how we can trust that the next solution won't just lead to another of these bugs popping up somewhere.

@pradyunsg
Copy link
Member

Let's have the discussion on that in #206.

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.

Release 1.1: Regression breaks setuptools builds
8 participants