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

make_wheels: implement PEP 517 #1

Closed
wants to merge 1 commit into from

Conversation

FFY00
Copy link

@FFY00 FFY00 commented Jul 12, 2021

Make make_wheels.py a Python build backend, as defined in PEP 517,
allowing this project to be pip-installable from source.

Make make_wheels.py a Python build backend, as defined in PEP 517,
allowing this project to be pip-installable from source.
@FFY00
Copy link
Author

FFY00 commented Jul 12, 2021

I'd probably wait for pypa/packaging#446.

Copy link
Collaborator

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! I didn't realize something like this is possible.

I have to ask though--is this something we want? I'm not sure what the use case of installing from source here would be. An obvious one is to let end users to install e.g. nightly builds, or for platforms that don't have wheels uploaded on PyPI, but this isn't actually implemented, and it would complicate things quite a lot to do so.

Ultimately make_wheels.py is a bit of a hack with a singular purpose: to get reproducible binary wheels on PyPI. I appreciate the work you've put into this PR but I ultimately feel like it's out of scope.

@FFY00
Copy link
Author

FFY00 commented Jul 13, 2021

I understand your sentiment. The main benefit would be that the project would be installable via pip when the --no-binary option is passed, it would also allow people to pip install . with this checked out without having to build all the wheels. It's good practice to release a source distribution for packages published on PyPI instead of only binary ones.
Nightly builds would be nice, and this would make that much easier. It's an improvement that could be made in the future, but that we don't really need to do.

Ultimately, it's up to you. I don't really think the code this PR introduces is that much, it's mostly build_sdist, and some lines in build_wheel, the rest is just minor refactoring. I could split the refactoring to a separate commit if you want. FWIW, I would be happy to help out with maintenance of this if that is a concern to you.
Anyway, please let me know what you think. If this is out of scope, that is perfectly fine 😊

@whitequark
Copy link
Collaborator

whitequark commented Jul 15, 2021

It's good practice to release a source distribution for packages published on PyPI instead of only binary ones.

One reason this might not be a good idea is that we're using wheel's API (which is explicitly not public) and we override some of its internals. This is doesn't matter one bit if only final artifacts that are uploaded to PyPI, but it does matter for sdists.

Also--unless I'm missing something, does the sdist not explicitly specify the dependency on either wheel or libarchive-c? The former I think will be always installed with PEP517, the latter I don't think so?

@whitequark
Copy link
Collaborator

whitequark commented Jul 15, 2021

Overall, I feel like building wheels through PEP517 is a nice idea (even if I personally may not see much use of it), but I'm not convinced at all that I want to upload sdists that rely on internal wheel API and download artifacts via HTTP, since that's potentially a liability for people who use them in the future. The wheels aren't going to break, the sdists very well might.

@FFY00
Copy link
Author

FFY00 commented Jul 16, 2021

One reason this might not be a good idea is that we're using wheel's API (which is explicitly not public) and we override some of its internals. This is doesn't matter one bit if only final artifacts that are uploaded to PyPI, but it does matter for sdists.

Oh, I didn't notice.

Also--unless I'm missing something, does the sdist not explicitly specify the dependency on either wheel or libarchive-c? The former I think will be always installed with PEP517, the latter I don't think so?

I forgot to commit the pyproject.toml, which specifies those dependencies.

Overall, I feel like building wheels through PEP517 is a nice idea (even if I personally may not see much use of it), but I'm not convinced at all that I want to upload sdists that rely on internal wheel API and download artifacts via HTTP, since that's potentially a liability for people who use them in the future. The wheels aren't going to break, the sdists very well might.

Makes sense. We could ship a sdist with all the required artifacts, but that doesn't seem great.

Anyway, I totally forgot to generate the metadata for the sdist anyway, so this PR would require a bit more complexity.

Closing this as out-of scope for now, if in the future we want to make this work with nightly builds or similar, it could be revisited if it makes sense.

@FFY00 FFY00 closed this Jul 16, 2021
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