Skip to content

BLD: move cibuildwheel configuration to pyproject.toml #21128

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

Merged
merged 2 commits into from
Mar 5, 2022

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Feb 27, 2022

This allows to share the configuration between GitHub Actions and Travis-CI.
This also allows to easily run cibuildwheel locally to debug build issues when they arise.

@github-actions github-actions bot added the 36 - Build Build related PR label Feb 27, 2022
@mayeut
Copy link
Contributor Author

mayeut commented Feb 27, 2022

If having the configuration in pyproject.toml is not something that would be considered, it should be easy to move it to tools/wheels/cibuildwheel.toml.

@mayeut mayeut force-pushed the cibuildwheel-config branch 2 times, most recently from 02e5bc9 to ed83d41 Compare March 3, 2022 22:29
@mattip
Copy link
Member

mattip commented Mar 4, 2022

This PR makes sense. How sure are you that the new code is equivalent to the old? Can we somehow compare the number of tests run or some other parameter that would give us confidence that this is correct?

This allows to share the configuration between GitHub Actions and Travis-CI.
This also allows to easily run cibuildwheel locally to debug build issues when they arise.
@mayeut mayeut force-pushed the cibuildwheel-config branch from ed83d41 to 3826415 Compare March 5, 2022 06:56
@mayeut
Copy link
Contributor Author

mayeut commented Mar 5, 2022

I will check number of tests and environment for all wheels.

@mayeut
Copy link
Contributor Author

mayeut commented Mar 5, 2022

@mattip, a deeper look into what you asked for showed some diffs:

  1. On Linux (both Travis-CI & GHA), wheels are now built with NPY_USE_BLAS_ILP64="1". I missed the fact that the environment variable was not passed to the manylinux containers in the current code in main. However, when looking at MacPython/numpy-wheels, official wheels are built with NPY_USE_BLAS_ILP64="1". This raises 2 questions:
    1. Was that an oversight in BLD Uses cibuildwheel for linux + osx wheels [cd build] #20102 ?
    2. If so, shall I open a PR to correct this first ?
  2. There's a difference in test executions for cp3*-win32 (more skipped tests for some reason), I'll look further into this.

@mayeut
Copy link
Contributor Author

mayeut commented Mar 5, 2022

There's a difference in test executions for cp3*-win32 (more skipped tests for some reason), I'll look further into this.

cp39-win32 & cp310-win32 are in fact showing the same tests results. It was only for cp38-win32 because it seems that GHA runners do not all offer the same level of SIMD support and thus, some tests were skipped on cp38-win32 in this PR.

@mattip
Copy link
Member

mattip commented Mar 5, 2022

wheels are now built with NPY_USE_BLAS_ILP64="1"

That was an oversight in the previous cibuildwheel work. Thanks for checking. Since we will merge this, no backport fix is needed, right?

@mayeut
Copy link
Contributor Author

mayeut commented Mar 5, 2022

Since we will merge this, no backport fix is needed, right?

Right, just asking if you wanted to split this in 2 to split the NPY_USE_BLAS_ILP64=1 fix from the configuration refactor.
Since the cibuildwheel workflows are not used to build official wheels yet, it might not be necessary (as you said, no backport needed) and it might just be easier to merge as-is.

Co-authored-by: Matti Picus <matti.picus@gmail.com>
@mayeut mayeut marked this pull request as ready for review March 5, 2022 20:08
@mattip mattip merged commit 64cfb04 into numpy:main Mar 5, 2022
@mattip
Copy link
Member

mattip commented Mar 5, 2022

Thanks @mayeut

@mayeut mayeut deleted the cibuildwheel-config branch March 5, 2022 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
36 - Build Build related PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants