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

Closes #262.

Oct 24 '22 10:10 agronholm

Maybe try this change in test.yml:

-coverage run -m pytest -W always
+python -m coverage run -m pytest -W always

https://stackoverflow.com/questions/7063709/coverage-py-python-module-import-errors-when-running-script

Oct 24 '22 11:10 HexDecimal

That didn't help. Maybe there is something wrong with wheel itself that is causing this.

Oct 24 '22 11:10 agronholm

That's too bad. I've had similar issues with pytest which were solved like this before.

The issue isn't locally reproducible for me either. So I guess I'll try to messing with the CI myself.

Oct 24 '22 11:10 HexDecimal

I was right – bdist_wheel didn't add any actual modules to the wheel file. So it's just a bug in the new branch.

Oct 24 '22 11:10 agronholm

Sounds like that'd do it. It'd be easier to detect these kinds of regressions if the tests CI ran on all branches instead of only main. You would've seen the failing commit right away.

Oct 24 '22 12:10 HexDecimal

Sounds like that'd do it. It'd be easier to detect these kinds of regressions if the tests CI ran on all branches instead of only main. You would've seen the failing commit right away.

If I enabled CI to run on all branches, it would run the same test suite twice every time a changeset was pushed to a PR branch. If I then disabled the trigger for pull requests, it would not show test results if somebody made a PR from their own fork. So what do you suggest?

Oct 24 '22 12:10 agronholm

Codecov Report

Base: 68.35% // Head: 72.46% // Increases project coverage by +4.10% :tada:

Coverage data is based on head (59fd006) compared to base (c483df5). Patch coverage: 78.81% of modified lines in pull request are covered.

@@            Coverage Diff             @@
##             main     #472      +/-   ##
==========================================
+ Coverage   68.35%   72.46%   +4.10%     
==========================================
  Files          12       11       -1     
  Lines         929     1064     +135     
==========================================
+ Hits          635      771     +136     
+ Misses        294      293       -1     

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Oct 24 '22 12:10 codecov[bot]

If I enabled CI to run on all branches, it would run the same test suite twice every time a changeset was pushed to a PR branch. If I then disabled the trigger for pull requests, it would not show test results if somebody made a PR from their own fork. So what do you suggest?

For CI? Run test workflows for all branches and PR's, even if tests get run twice sometimes. If the tests are small and fast then it isn't a issue, and in my experience even very long running workflows have never caused me any trouble. The upside is that your topic branches are tested immediately instead of having to make a PR first, and you have a status record of all pushed commits.

Oct 24 '22 12:10 HexDecimal

For CI? Run test workflows for all branches and PR's, even if tests get run twice sometimes.

Sorry but it's not that straightforward.

GA is a shared resource for the organisation (capacity is provided on a per-or basis by Github) which means that those unnecessary runs have a human cost -- of the contributors and maintainers of other projects needing to wait.

I don't want to digress down this discussion further, but I'll state my request for now, and leave it at that: Please don't run CI unnecessarily in the pypa org.

Oct 24 '22 13:10 pradyunsg

@agronholm do you have a different version of pip locally vs in CI?

Oct 24 '22 13:10 pradyunsg

This was not a pip issue, but a simple bug in the new bdist_wheel code which I've fixed now.

Oct 24 '22 13:10 agronholm

I've made some changes to the wheel reader/writer APIs. Let me know if you object.

Oct 24 '22 13:10 agronholm

Additionally I dropped the bdist_wininst converter, as it had never been covered by any tests, and the format was dropped back in Python 3.9.

Oct 24 '22 13:10 agronholm

src/wheel/__main__.py has import wheel.cli but that module was renamed.

Are you using Mypy? Maybe I should make a PR to fix the type issues I see here.

Oct 24 '22 14:10 HexDecimal

This project is pretty far from being mypy clean, especially with the trainwreck that is the setuptools command system. But if you want to fix typing issues, feel free to submit a PR against this branch.

Oct 24 '22 14:10 agronholm

I can see that. Trying to fix some issues I run into problems that stuff from other packages are being used magically. So at most I can only suppress or ignore the errors rather than make it type safe.

Oct 24 '22 14:10 HexDecimal

Apart from bdist_wheel, there are fixable issues within the code base. Would you still like to fix those issues in a PR?

Oct 24 '22 15:10 agronholm

Yes.

Oct 24 '22 15:10 HexDecimal

Shouldn't there be a compatibly shim for wheel.wheelfile to ease the transition? This would will break a few dozen packages (77 results - including things like scikit-build, meson-python, trampolim, auditwheel (vendored, though), and multibuild). A shim for a release or two would be nice. I'm happy to report that scikit-build-core will not be broken by this, though. :)

Also, as I've mentioned before, wheel.bdist_wheel is heavily used, and renaming it without warning will break at least 1.2K packages. Again, having a warning for a couple of releases is the Pythonic thing to do. This would be a really easy shim to write, I think, since it's a simple rename. Python provides deprecation warnings for a couple of releases before removing anything. Also, due to the command-based design of setuptools, being able to customize commands is important, and I don't know how a user is supposed to customize bdist_wheel if they can't import it.

PS: I really love the idea of wheel gaining a public interface, just wanting to avoid mass breakage.

Nov 03 '22 03:11 henryiii

An example of something that requires modifying the bdist_wheel command, as far as I know: adding support for limited ABI wheels (at least without calling setup.py commands). An example from psutil: https://github.com/giampaolo/psutil/blob/8e4099d9f063ceb4ee3da5845562c5b934f83544/setup.py#L369-L375

Without a temporary compatibility shim that produces a helpful warning, all ABI3 packages will likely instantly break. I'd like to support ABI3 in the rewritten scikit_build_core, but I'm not sure how to do so correctly without importing bdist_wheel from wheel, which I don't want to do due to it not being public.

Nov 07 '22 17:11 henryiii

The long term plan for bdist_wheel is to move it to setuptools itself. Thus the plan could be as follows:

1. Rename wheel._bdist_wheel back to wheel.bdist_wheel
2. Add

An example of something that requires modifying the bdist_wheel command, as far as I know: adding support for limited ABI wheels (at least without calling setup.py commands). An example from psutil:

Strictly speaking, that doesn't require modifying bdist_wheel. See here for an example. What the psutil override does is dynamically set the Python tag to the current tag (for reasons beyond my understanding, bdist_wheel requires statically setting the Python tag in order to make ABI3 wheels).

All that said, it does look like making wheel.bdist_wheel private is premature at this point, so I'll undo that.

Nov 07 '22 21:11 agronholm

@pradyunsg I'd like to hear your thoughts on the current state of the branch. Are my API changes acceptable?

Nov 08 '22 08:11 agronholm

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?

The reason why I am asking this is because PEP 517 requires the .dist-info generated by the prepare_metadata_for_build_wheel hook to contain identical metadata in the final wheel (except from the RECORD signatures). It is easier and less error proof to guarantee they are identical, if the same logic used in bdist_wheel is also used in that hook...

I understand that in the long term, if bdist_wheel moves to setuptools, then setuptools will either implement the logic for the conversion of .dist-info directories (and be able to reuse it in all PEP 517 hooks) or implement the latest version of the core metadata directly[^1]. But for the time being it would be very handy to count on that.

Alternatively, we could also think about an arrangement where wheel explicitly allows only setuptools to use a set of functions (something like _egg2info with explicitly permission for setuptools to use.)

[^1]: My hopes here is that in the near future packaging will expose a core metadata API that can then be used by setuptools to create both PKG-INFO and METADATA files (I have done some contributions in the past in that direction but packaging' maintainers opted for a different API that is currently under development).

Nov 25 '22 10:11 abravalheri

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.

Nov 26 '22 23:11 pradyunsg

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.

Nov 26 '22 23:11 agronholm

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.

Nov 26 '22 23:11 agronholm

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

Dec 11 '22 20:12 pradyunsg

@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?

Dec 11 '22 22:12 agronholm

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.

Dec 11 '22 22:12 agronholm

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.

Dec 12 '22 08:12 pradyunsg