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

use cibuildwheel to build linux wheels #2800

Merged
merged 12 commits into from Feb 25, 2023

Conversation

njzjz
Copy link
Contributor

@njzjz njzjz commented Jan 11, 2023

Summary

Include a summary of major changes in bullet points:

  • refactor the wheel build workflow using cibuildwheel
  • build manylinux2014 wheels

Checklist

Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request title.

Before a pull request can be merged, the following items must be checked:

  • Doc strings have been added in the Google docstring format. Run pydocstyle on your code.
  • Type annotations are highly encouraged. Run mypy path/to/file.py to type check your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more efficient if you already fix most errors prior to submitting the PR. We highly recommended installing pre-commit hooks. Simply Run

pip install -U pre-commit
pre-commit install

in the repo's root directory. Afterwards linters will run before every commit and abort if any issues pop up.

@coveralls
Copy link

coveralls commented Jan 11, 2023

Coverage Status

Coverage: 78.085% (-0.7%) from 78.8% when pulling 1d01c44 on njzjz:cibuildwheel into 44f24db on materialsproject:master.

@shyuep
Copy link
Member

shyuep commented Jan 13, 2023

Thanks, but there seem to be errors with regard to wheel building. The tests need to pass before we merge.

@njzjz njzjz marked this pull request as ready for review February 22, 2023 22:18
@njzjz
Copy link
Contributor Author

njzjz commented Feb 22, 2023

@shyuep I notice that in the cmd_line directory, some binary files link to external dependencies outside the repository. I don't know if they should be packaged into wheels. I tried to fix these libraries on Linux but skipped those on macos.

The lint failure is not related to this PR, but is caused by a new ruff version.

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

@njzjz Thanks for taking this on! Very excited about this PR. 👍

pyproject.toml Outdated
Comment on lines 18 to 19
[tool.cibuildwheel.macos]
repair-wheel-command = ""
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this section? What's the default for repair-wheel-command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default is below

[tool.cibuildwheel.macos]
repair-wheel-command = "delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v {wheel}"

However, in cmd_line directory, some binary files link to external dependencies outside the repository, and make the repair command fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change to

repair-wheel-command = "delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v {wheel} --ignore-missing-dependencies"

--ignore-missing-dependencies should work for this situation.

Comment on lines 170 to 173
- uses: pypa/gh-action-pypi-publish@master
with:
user: ${{ secrets.TWINE_USERNAME }}
password: ${{ secrets.TWINE_PASSWORD }}
Copy link
Member

Choose a reason for hiding this comment

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

Does this offer advantages over running twine upload manually? I'm wondering if it's worth relying on an external action for sth this simple since it might lock us into waiting for fixes if the action ever breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, twine upload should also work.

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

I have next to 0 experience building Linux wheels but I think this looks good. Let's just take it for a spin and see what happens. :)

@janosh janosh merged commit c03dacb into materialsproject:master Feb 25, 2023
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.

None yet

4 participants