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

Simplify wheel building #520

Merged
merged 7 commits into from
Feb 13, 2021

Conversation

psavery
Copy link
Collaborator

@psavery psavery commented Feb 12, 2021

This moves wheel building over to use cibuildwheel and github actions. This simplifies the wheel building logic with a lot of shared script and code paths between platforms.

On new tag pushes, the built wheels will automatically be pushed to PyPI.

The following wheels were tested:

  • Linux: 3.8 - 3.9
  • Mac: 3.5 - 3.9
  • Windows: 3.8 - 3.9

When running programs like cibuildwheel, this allows us to more
easily append extra cmake arguments to the list via an environment
variable. The cmake arguments are semicolon delimited.

Example:

EXTRA_CMAKE_ARGS=-DEIGEN3_INCLUDE_DIR=/c/eigen;-DBUILD_TYPE=Release

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
This checks if pybind11 was installed via pip. If it was,
then it will automatically include the pybind11 directory in
the cmake arguments.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
The previous wheel-building infrastructure used appveyor, travis-ci,
and circleci. Remove these as we are doing all of the wheel building
on github actions now.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
@ghutchis
Copy link
Member

I'll take a look at the other build failures - I've been busy with deadlines, but want to make sure we get regular builds / releases again.

The test only imports avogadro, creates a molecule, and loads xyz
data from a string, and verifies that the data was loaded properly.
But this indicates that the wheels are at least functional.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
@ghutchis
Copy link
Member

I think if you push again, it should fix the build_cmake failures - it was a problem with the openchemistry cmake files.

@psavery
Copy link
Collaborator Author

psavery commented Feb 12, 2021

@ghutchis Sounds good, I am simplifying one or two things. But it should be ready soon.

This allows us to specify dependencies to install before setup.py is
ran.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
I don't know why auditwheel can't see the libraries since they are
already inside the wheel. If we can figure out how to get it to see
the libraries, we can skip the whole script.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
@psavery psavery marked this pull request as ready for review February 13, 2021 01:28
@psavery
Copy link
Collaborator Author

psavery commented Feb 13, 2021

I think this is ready to go!

@ghutchis
Copy link
Member

I'll check on the Windows build later - I don't think that's a serious problem.

Do you know if we still need the Travis build or can we disable that after merging this? (I know it was to build wheels..)

@ghutchis ghutchis merged commit 337e28f into OpenChemistry:master Feb 13, 2021
@ghutchis
Copy link
Member

Looks good - the Windows break is on my side - should be fixed soon.

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