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

Add a public API #472

Draft
wants to merge 65 commits into
base: main
Choose a base branch
from
Draft

Add a public API #472

wants to merge 65 commits into from

Conversation

agronholm
Copy link
Contributor

@agronholm agronholm commented Oct 24, 2022

This enables official support for using wheel as a library, rather than just a setuptools extension and CLI utility.

Closes #262.

It was always triggered twice on PRs, so I hope it will work as intended now.
The tmp_path and tmp_path_factory fixtures are based on pathlib while tmpdir/tmpdir_factory are based on the third party py.path.local library.
This lets MyPy use the type definitions in this library from other projects too.
@pradyunsg
Copy link
Member

Are my API changes acceptable?

Thanks for the mentioned @agronholm! I took a cursory look through the PR, on the GitHub UI and only spotted one thing (which you've already addressed :P). I don't wanna commit to spending time reviewing code on a Sunday -- but I may be able to take a more proper look tomorrow.

@agronholm
Copy link
Contributor Author

agronholm commented Nov 26, 2022

Hi @agronholm, would you consider keeping the egg2dist function in bdist_wheel or at least adding the pkginfo_to_metadata function as part of wheel's public API?

I really want to get this tool chain to a state where this ridiculous notion of converting .egg-info to .dist-info goes away (save for converting egg files to wheels), even if I have to get involved in that personally. Setuptools should be able to generate dist-info natively, and right now it cannot.

So, here are steps I want to take (not entirely sure about the order):

  • Give setuptools the ability to generate .dist-info natively
  • Move bdist_wheel to setuptools
  • Publish wheel 1.0

Only if there's no "proper" way of doing things, would I want to re-add egg2dist to this branch.

@agronholm
Copy link
Contributor Author

Are my API changes acceptable?

Thanks for the mentioned @agronholm! I took a cursory look through the PR, on the GitHub UI and only spotted one thing (which you've already addressed :P). I don't wanna commit to spending time reviewing code on a Sunday -- but I may be able to take a more proper look tomorrow.

Thanks. If there are no problems, then we can maybe get the ball rolling with the changes on the setuptools side too.

@pradyunsg
Copy link
Member

pradyunsg commented Dec 11, 2022

@agronholm One last change for installer: Can you add a validation_error = WheelError to WheelFile, as a class attribute? This is related to pypa/installer#147 (which was mentioned earlier in the thread).

@agronholm
Copy link
Contributor Author

@agronholm One last change for installer: Can you add a validation_error = WheelError to WheelFile, as a class attribute? This is related to pypa/installer#147 (which was mentioned earlier in the thread).

Why? This seems a bit icky to me. Normally interfaces have defined exceptions that they raise. Having a magic field in the class that indicates what exception some method would raise seems like less-than-good design to me. I don't want to add anything that is really specific to a third party library that has otherwise no use in the project. Can we have an alternative here?

@agronholm
Copy link
Contributor Author

I've made the following changes recently:

  • Added a compatibility shim for wheel.wheelfile (I checked downstream projects to see what functionality they were using)
  • Added automatic detection of the .dist-info directory in WheelReader, for both cases where the file is opened from an existing open file (potentially io.BytesIO) or generated with an older wheel release that did not properly normalize the wheel names

I've also been working on a local setuptools branch to develop a version that has a native bdist_wheel command and potentially vendors the public-api version of wheel. Something I'm currently stuck on is that wheel completely lacks any support functionality for editable (virtual) wheels. In particular, the code to generate a WHEEL file is currently tied to the WheelWriter class, but setuptools needs it usable without an actual wheel file.

@pradyunsg
Copy link
Member

pradyunsg commented Dec 12, 2022

I'm open to ideas for alternatives. I can't think of any way to allow implementing general logic of the form:

try:
    source.validate_record()
except [exception type]:
    ...

where exception type isn't overly generic like ValueError. Coupling it with the source object to catch source.validation_error is the only design I could come up with that didn't require a package like wheel to add a dependency on installer.

@agronholm
Copy link
Contributor Author

where exception type isn't overly generic like ValueError. Coupling it with the source object to catch source.validation_error is the only design I could come up with that didn't require a package like wheel to add a dependency on installer.

If there's an interface that has a validate_record() method, then it should raise a predefined exception (documented in the method's docstring) when validation fails. If Installer deals with wheel files in Python code, shouldn't it instead have a dependency on wheel?

@pradyunsg
Copy link
Member

pradyunsg commented Dec 14, 2022

Well, installer has a bootstrapping concern since it (along with build) are supposed to help make build-up-from-source easier. More importantly, it was implemented when wheel didn't have a public API. My hope is that I'd be able to cleanly migrate to using wheel under the hood; especially once bdist_wheel moves to setuptools.

None the less, it's more of a suggestion rather than a demand to be clear. As implemented, this works. :)

PS: it does have a predefined error type.

@nanonyme
Copy link

@pradyunsg well, flit-core itself is capable of creating wheels without using wheel. So as long as installer can install generated wheels without wheel, that should not break the bootstrap chain especially now that wheel is built with flit-core. The important bit is that flit-core must not ever use wheel, that will result in cycles.

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.

Provide a public API
7 participants