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

Adding action for building wheels and uploading to PyPI #90

Merged
merged 1 commit into from
Jan 6, 2021
Merged

Adding action for building wheels and uploading to PyPI #90

merged 1 commit into from
Jan 6, 2021

Conversation

mworion
Copy link
Contributor

@mworion mworion commented Jan 5, 2021

Hi Kyle,
attached the PR for the GitHub action. Hopefully we this today working as I need the Wheels to be present before I could make my next release.
Michel

Here is the output automatically generated on test.pypi.org with the clone of your package:

Bildschirmfoto 2021-01-05 um 13 54 21

@kbarbary
Copy link
Owner

kbarbary commented Jan 6, 2021

This looks great! Thanks for all the comments; they made it really easy to understand the workflow. I'm going to go ahead and merge and then make edits on master.

@kbarbary kbarbary merged commit 6b8623a into kbarbary:master Jan 6, 2021
python-version: ${{ matrix.python-version }}

- name: install_deps
run: python -m pip install cibuildwheel wheel cython numpy certifi
Copy link
Owner

Choose a reason for hiding this comment

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

Do we actually need to install wheel cython numpy? These are declared as build-time dependencies in pyproject.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kyle,
I tried it without NumPy and Cython, but it did not work. I put in all packages, the build process demanded.
My guess is the some of the interface headers are used.

Some observations: There is a warning about the interface version to NumPy, which might cause trouble in future. This might lead to set a defined NumPy version for building the wheels or even better to work on the warning.
Second I see a lot of sprint warnings and the hint to replace them with sprintf_s an so on.

Michel

Copy link
Owner

Choose a reason for hiding this comment

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

It is indeed necessary to have numpy and cython installed for "building" the source distribution with python setup.py sdist (it runs setup.py directly). For building wheels via a pip command, I believe pip first reads pyproject.toml and installs the build time dependencies. I removed numpy and cython from the wheel build step and it seems to work.

The pyproject.toml declares oldest-supported-numpy as the build-time dependency, so we do use a fixed numpy version when building, depending on the Python version. Not using the oldest supported numpy when building may have been the cause of the interface version warnings you were seeing, but I'm not sure.

I'm not sure what the cause of the sprint warnings are. Maybe open an issue for that if you think there's something actionable we can fix.

Thanks again for this PR... it's super super cool to be able to just push a tag to do a full release!

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