Skip to content

BLD: move cibuildwheel configuration to pyproject.toml - #21128

Merged
mattip merged 2 commits into
numpy:mainfrom
mayeut:cibuildwheel-config
Mar 5, 2022
Merged

mattip merged 2 commits into
numpy:mainfrom
mayeut:cibuildwheel-config

Conversation

@mayeut

@mayeut mayeut commented Feb 27, 2022 •

Copy link
Copy Markdown
Contributor

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

mayeut commented Feb 27, 2022

Copy link
Copy Markdown
Contributor Author

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

mattip commented Mar 4, 2022

Copy link
Copy Markdown
Member

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

mayeut commented Mar 5, 2022

Copy link
Copy Markdown
Contributor Author

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

@mayeut

mayeut commented Mar 5, 2022

Copy link
Copy Markdown
Contributor Author

@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

mayeut commented Mar 5, 2022

Copy link
Copy Markdown
Contributor Author

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

mattip commented Mar 5, 2022

Copy link
Copy Markdown
Member

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?

Comment thread pyproject.toml Resolved
@mayeut

mayeut commented Mar 5, 2022

Copy link
Copy Markdown
Contributor Author

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 <[email protected]>
@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

mattip commented Mar 5, 2022

Copy link
Copy Markdown
Member

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