-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support the metadata directory (as per PEP-517) for build_wheel #4647
Support the metadata directory (as per PEP-517) for build_wheel #4647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks sound to me.
I don't love the complexity this adds, but I also recognize that refactoring the behavior, such as to re-order operations or not generate egg-info might be dangerous.
# Egg info is still generated, so remove it now to avoid it getting | ||
# copied into the wheel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why egg info is still generated? Is it because other commands might have generated it? What happens if the line for rmtree
is removed (will the tests fail)?
While I'm okay with this approach, I'm also somewhat apprehensive, as it is perpetuating and reinforcing the expectation that egg-info is generated, even when it's not needed, making it harder in the future to correct this suboptimal approach. For example, this project also has the goal to someday obviate egg-info altogether, and to generate dist-info directly, and adding this additional handling of egg-info will likely complicate that work (though maybe not too much).
I wonder - how much do other commands from bdist_wheel
depend on the generation of egg-info and could those commands get by without generating it if bdist_wheel
could pass through the instruction to re-use existing dist-info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why egg info is still generated?
Looks like install
calls install_egg_info
(a registered sub-command, which is automatically run), which then calls egg_info
. I wasn't brave enough to try to touch this.
What happens if the line for rmtree is removed (will the tests fail)?
I've added a specific test for this. Previously the test would have passed.
this project also has the goal to someday obviate egg-info altogether
Yes, I appreciate that. IMO, this change does add an additional place where you have to think about egg-info, but I believe it is already necessary to deal with bdist_wheel
wrt egg-info
, and the added code-path now explicitly rejects its use - I think that ultimately makes that easier to remove egg-info in the future.
Also, I just noticed you're a first-time contributor. Thanks for the contribution and nice work! |
d58825b
to
94a3cf1
Compare
94a3cf1
to
04f989b
Compare
I noticed that this PR needed a rebase, so have just done so. Please let me know if there is anything else you need from my side. |
04f989b
to
bd615ee
Compare
Co-authored-by: Anderson Bravalheri <[email protected]>
Co-authored-by: Anderson Bravalheri <[email protected]>
Thank you very much! |
Thanks for your review 👍 🎉 |
This seems to have broken building some projects. https://github.com/yaml/pyyaml I am seeing failures like:
with the main branch of setuptools, but going back to the commit before this was merged (3106af0 which is v75.1.0) works again. I have not had time to debug this further and it is not all packages nor do I know if this is the only one. |
Looks like that project is importing To be honest, that implementation looks to be rather unnecessary - I'm not sure on the backwards compat policy of |
@henryiii already has a proposed fix in yaml/pyyaml#823 (a few weeks before I made this MR 😂). |
Due to upcoming changes to setuptools (pypa/setuptools#4647) using bdist_wheel from wheel will fail.
Due to upcoming changes to setuptools (pypa/setuptools#4647) using bdist_wheel from wheel will fail.
Co-authored-by: Thomas A Caswell <[email protected]>
OK, I hit the PyYAML problem in the integration tests when trying to cut a release. I wonder if there is any workaround for this problem. Trying out #4684. |
Due to upcoming changes to setuptools (pypa/setuptools#4647) using bdist_wheel from wheel will fail.
Summary of changes
Allow
dist-info-dir
to be passed tobdist_wheel
.More importantly, fixes the
build_wheel
PEP-517 interface to treat the given metadata as per the spec.With this change, I was able to verify that I could:
And the resulting wheel contained my desired metadata file. My personal goal is to support this behaviour in PEP-517 build backends, and to write a setuptools build backend wrapper which injects such metadata (I need to do something like
setuptools-ext
does). Presently, the functionality does not exist, you end up having to post-process the built wheel (hence the complexity that exists insetuptools-ext
).Honestly, despite having written many Command subclasses over many years, I am very unfamiliar with the intricacies of
distutils.Command
andsetuptools
in general, and may have implemented a solution which is not quite right due to a lack of that knowledge.Closes #1825. Follows a prototype I did in pypa/wheel#611.
Pull Request Checklist
newsfragments/
.(See documentation for details)