-
Notifications
You must be signed in to change notification settings - Fork 18
Use cibuildwheel to replace multibuild #217
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored by: [email protected];
Co-authored by: [email protected];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cibuildwheel does seem able to handle this, nice. I am not sure I am ready to transition away from multibuild, since I am the main project maintainer these days I do like to make sure it continues to work. Is there any clear advantage of cibuildwheel over multibuild for this type of package, which anyway invokes its own build/test scripts? The "wheel" is really only a repackaging of the OpenBLAS library with some pkgconfig additions.
A few general questions:
- The macos-x86_64 build seems to be using a arm64 machine, which means that the tests are not run natively. Is there a way to use the macos-15-intel os instead of macos-latest for those builds?
- travis is broken, the build is not running. We still need that for ppc64le and s390x (the s390x build usually fails, but we do have some users of ppc64le)
|
||
- { os: ubuntu-24.04-arm, PLAT: aarch64, INTERFACE64: '0', MB_ML_VER: '_1_2', MB_ML_LIBC: musllinux} | ||
- { os: ubuntu-24.04-arm, PLAT: aarch64, INTERFACE64: '1', MB_ML_VER: '_1_2', MB_ML_LIBC: musllinux} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we no longer use multibuild, the env variables should not have the MB_ prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce the difficulty of review, I have tried to preserve the content of the original script as much as possible in this PR, without making excessive modifications. If you think modifications are needed, I can add a commit now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it would be invasive in other places. Let's leave it for a clean-up PR.
# force the architecture | ||
arch -${PLAT} bash -s << EOF | ||
source ${ROOT_DIR}/gfortran-install/gfortran_utils.sh | ||
source tools/gfortran_utils.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #216 I replaced this with an alias (and moved it inside the if $IS_OX
), since gfortran is installed in the macos image.
alias gfortran=gfortran-15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean we should remove gfortran-install directly? I believe maintainers should be able to commit directly to this PR. If my understanding is wrong, please feel free to commit.
Using cibuildwheel has the following advantages:
Here are the answers to other questions:
|
# Report OS as given by uname | ||
# Use any Python that comes to hand. | ||
$(any_python) -c 'import platform; print(platform.uname()[0])' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions don't seem used nor necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code can be deleted, but as I mentioned before, I'm trying to avoid modifying the original script as much as possible to reduce the difficulty of the PR. If you think this code should be deleted directly, you can commit it—that's no problem.
# -e MB_ML_VER=${manylinux} \ | ||
# -e MB_ML_LIBC=${libc} \ | ||
# -v $PWD:/io \ | ||
# $docker_image /io/tools/docker_build_wrap.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove all of the commented out code from this PR, then it will be much clearer that the move to cibuildwheel
is a significant reduction in complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but please wait a moment. I'm trying to fix the Travis issue because I broke tools/build_wheel.sh. If I directly restore tools/build_wheel.sh, it will cause code redundancy issues (because I've split the code here into other places).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now is fine. If you need to delete code, you can commit directly. Or if there's anything you need help with, feel free to point it out directly. I'm happy to assist you with the related content.
Co-authored by: [email protected];
Co-authored by: [email protected];
Co-authored by: [email protected];
Co-authored by: [email protected];
Co-authored by: [email protected];
Co-authored by: [email protected];
@mattip |
Cool. Let's see what CI here thinks of this. |
The CI results show that only Additionally, it's important to note here that in this PR, the cibuildwheel refactoring does not include s390x/ppc64le and the Windows series. The reason is that I'm trying to minimize the burden of PR review as much as possible. If this PR gets merged, we can consider adding support for them using cibuildwheel in the future, but for now, let's keep the status quo. |
see #211
below is Ci result:
posix
Windows-on-ARM
Win
All CI results show that everything went smoothly.
The detailed refactoring process can be viewed in the replies within the issue. To avoid making the PR too lengthy, the refactoring using cibuildwheel was only applied to Linux and macOS.
For Windows and Windows ARM, cibuildwheel has not been used yet. If you think it should be implemented using cibuildwheel in this PR as well, feel free to @ me directly, and I will submit a commit to implement it as soon as possible.
other info
Co-authored by: [email protected];
git describe --tags --abbrev=8
in OpenBLAS at theOPENBLAS_COMMIT
. If I did not updateOPENBLAS_COMMIT
, I incremented the wheel build number (i.e. 0.3.29.0.0 to 0.3.29.0.1)