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

fix: syntax error in setup.py + modernize #236

Closed
wants to merge 3 commits into from

Conversation

william-silversmith
Copy link
Contributor

@william-silversmith william-silversmith commented Jul 3, 2024

Fixes the syntax error, cythonizes dynamically (may make this run on more platforms), sets language level to 3 (Python 2 is EOL for years now) to remove a warning, and adds license and classifiers so PyPI can categorize it better.

Loads numpy dynamically so it won't crash if numpy was installed at the same time as the setup call is run.

Uses numpy for installation, but oldest-supported-numpy for isolated compilation for py38 via pyproject.toml

Adds MANIFEST.in to ensure sdist includes source files, LICENSE, etc.

Resolves #233

@lindstro
Copy link
Member

lindstro commented Jul 3, 2024

@william-silversmith Thanks, this looks great. But no PR should target the master branch, which is the latest official release. Please target develop instead.

Also, there's been a lot of activity lately around NumPy, zfpy in general, and the setup.py bug referenced here. See #234, #233, #231, #227, #210, #201. It would be good to know to what extent there's overlap between these PRs and issues. I'm unfortunately not well versed in Python, so it would be good for those with more expertise to come to a consensus as to what changes are needed. Part of this includes how to best deal with failing AppVeyor tests (some because of lack of NumPy 2.0, some because CMake is too old). I'm too committed with other zfp work at the moment to figure out if now is the time to move our Windows tests to GitHub Actions, which presumably would solve the issues we're having with AppVeyor.

@william-silversmith
Copy link
Contributor Author

Sorry about that, I'll close this and make a new PR on develop. For myself, I can comment on other issues, but for Windows, I don't have regular access to a Windows machine, so that is more difficult.

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.

2 participants