-
Notifications
You must be signed in to change notification settings - Fork 104
Use cibuildwheel and update grpcio #57
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
Conversation
7f9ba27
to
ebe6762
Compare
e4c7602
to
0334677
Compare
# Rename the wheel file and confirm it is changed. We need to make it | ||
# py37 minimum interpreter and abi3 compat. | ||
wheel_file_pieces = existing_wheel_file.split("-") | ||
if len(wheel_file_pieces) < 4: |
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.
nit:
Can this be > 4?
If it can't you can destructure into part1, part2, _py, _abi
and replace the join with f"{part1}-{part2}-cp37-abi3"
(replace part1 and part2 with meaningful names)
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.
While I haven't seen more than 4 (file + the 3 platform compat pieces), I am intentionally not assuming the number of parts here, just that I need to replace the third and fourth.
# We separate out Linux aarch64 so we can choose not to run it during PR since | ||
# it is so slow in cibuildwheel (uses QEMU emulation). We can put this back in | ||
# the above matrix when Linux ARM runners are available. | ||
compile-binaries-linux-aarch64: |
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.
nit: you can merge this with the matrix above and add conditions to skip in PR CI
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.
The job if
can't include a matrix
variable in its expression. It is evaluated before the matrix is loaded. There is also no "exit early" step for a job. Technically I could add an if
to each step to skip linux aarch64 during PR, but that is ugly. There are also other ugly options: https://stackoverflow.com/questions/65384420/how-to-make-a-github-action-matrix-element-conditional.
This is the result of several attempts at several options (including workflow reuse which had little value and you can't use a strategy with a job use clause anyways).
(the original version did have these combined before it became clear Linux aarch64 was way too slow, but I still want to keep cibuildwheel's approach instead of custom cross-compile at least if/until we get Linux arm runners)
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.
Just a couple of nits, nothing blocking
What was changed
Use
cibuildwheel
in CI now and updategrpcio
package.Why?
cibuildwheel
at least for its Linux docker capabilitiesgrpcio
made a new release to fix its bad wheel naming for macOS arm buildsChecklist