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

pyproject: Use pyproject.toml for building wheels #560

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abelgladstone
Copy link

Remove setup.py and move to pyproject.toml

@pep8speaks
Copy link

pep8speaks commented Oct 7, 2024

Hello @abelgladstone! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 22:80: E501 line too long (87 > 79 characters)
Line 23:80: E501 line too long (96 > 79 characters)
Line 38:80: E501 line too long (101 > 79 characters)
Line 39:80: E501 line too long (82 > 79 characters)
Line 46:80: E501 line too long (80 > 79 characters)
Line 47:80: E501 line too long (89 > 79 characters)
Line 49:80: E501 line too long (99 > 79 characters)
Line 52:80: E501 line too long (123 > 79 characters)
Line 54:80: E501 line too long (86 > 79 characters)

Comment last updated at 2024-10-11 13:58:22 UTC

@mgeier
Copy link
Member

mgeier commented Oct 7, 2024

Thanks for this PR!

I like this approach in general, but it seems that the "cross-platform package creation" doesn't work anymore.
This means that the CI doesn't create platform-specific wheels anymore:

- name: Build binary wheels and a source tarball
run: ./make_dist.sh

@abelgladstone
Copy link
Author

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

@abelgladstone abelgladstone force-pushed the feature-pyproject branch 2 times, most recently from ad2d4ab to 3239885 Compare October 8, 2024 19:36
@abelgladstone
Copy link
Author

abelgladstone commented Oct 8, 2024

Unfortunately my little home PC is stuck on windows the output filename is as expected on a win64_amd64.

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.

@mgeier
Copy link
Member

mgeier commented Oct 8, 2024

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):
Copy link
Member

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 ...

Suggested change
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?

@abelgladstone
Copy link
Author

Agreed.

@mgeier
Copy link
Member

mgeier commented Oct 9, 2024

I've merged #561, can you please rebase this PR?

@abelgladstone
Copy link
Author

Done

@mgeier
Copy link
Member

mgeier commented Oct 11, 2024

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",
Copy link
Member

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.

@abelgladstone
Copy link
Author

Thanks for the review. I'll get back to you soon.

@abelgladstone
Copy link
Author

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.

@mgeier
Copy link
Member

mgeier commented Nov 10, 2024

Thanks a lot for your efforts anyway!

AFAIU, the problem is that package-data cannot be set dynamically depending on the platform, while that is possible in setup.py with package_data.

Maybe future changes to setuptools will make this possible, or maybe we'll find a different build tool that supports this.

If anyone has an idea, please speak up!

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.

3 participants