-
Notifications
You must be signed in to change notification settings - Fork 150
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
pyproject: Use pyproject.toml for building wheels #560
base: master
Are you sure you want to change the base?
Conversation
Hello @abelgladstone! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-10-11 13:58:22 UTC |
Thanks for this PR! I like this approach in general, but it seems that the "cross-platform package creation" doesn't work anymore. python-sounddevice/.github/workflows/publish.yml Lines 18 to 19 in 2748f70
|
@mgeier thank you for the reply. I will try make an effort to get the platform specific builds to work as well. Will get back to you soon. |
ad2d4ab
to
3239885
Compare
The make_dist script works as expected. As a consequence of intercepting the filename. I had to move the version to a separate VERSION file. So that I can read it without any fuss. I hope that this is acceptable. BTW Thank you for a wonderful little library. I cannot tell you how much I use this. |
Thanks for the quick update! I need some more time to review and test this thoroughly, but in the meantime I was wondering whether we should enable building the wheels on PRs: #561. Because otherwise we don't know whether this PR really works, right? |
return f"sounddevice-{version}-py3-none-{oses}.whl" | ||
|
||
|
||
def prepare_meta_for_build_wheel(config_settings=None): |
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.
I guess this should be ...
def prepare_meta_for_build_wheel(config_settings=None): | |
def prepare_metadata_for_build_wheel(config_settings=None): |
... but the whole function seems unnecessary, since it is imported already?
Agreed. |
I've merged #561, can you please rebase this PR? |
3239885
to
c8b2cc7
Compare
Done |
Thanks! The wheels are available here: https://github.com/spatialaudio/python-sounddevice/actions/runs/11293700433?pr=560 Now all of them contain all binaries, but each should only contain the binaries for their own platform. |
[tool.setuptools.package-data] | ||
"_sounddevice_data"= [ | ||
"portaudio-binaries/*.dll", | ||
"portaudio-binaries/*.dylib", |
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.
I guess this causes all wheels to contain all binaries. This should be limited.
Also, the sdist
now also contains the binaries, but it shouldn't.
Thanks for the review. I'll get back to you soon. |
After many tries I am unable to find a way to build the project without the setup.py file in which case what is in the project is already good enough IMO. |
Thanks a lot for your efforts anyway! AFAIU, the problem is that Maybe future changes to If anyone has an idea, please speak up! |
Remove setup.py and move to pyproject.toml