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

cibuildwheel: Add macOS arm64 support #7727

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

xavier2k6
Copy link
Contributor

@xavier2k6 xavier2k6 commented Aug 9, 2024

Closes #7308.
Closes #7473.
Closes #7536.
Closes #7650.
Closes #7683.
Closes #7729.

@xavier2k6
Copy link
Contributor Author

Note/Review: pypa/cibuildwheel#1926 (comment)

@xavier2k6 xavier2k6 force-pushed the wheel_macos_arm64 branch 3 times, most recently from b8a644f to 2519f14 Compare August 9, 2024 16:42
@xavier2k6
Copy link
Contributor Author

@qstokkink Can you give the created wheels a test?

@xavier2k6
Copy link
Contributor Author

@xavier2k6 xavier2k6 force-pushed the wheel_macos_arm64 branch 2 times, most recently from 6934d07 to bc511c5 Compare August 9, 2024 21:49
@qstokkink
Copy link

@qstokkink Can you give the created wheels a test?

Works on my machine. 👍

On behalf of the Tribler team, I would like to request you to keep {"os": "macos-12", "CIBW_BUILD": "cp39-*", "CIBW_ARCHS": "x86_64"} in the workflow dispatch matrix though.

@xavier2k6
Copy link
Contributor Author

@qstokkink

  1. I've added macOS-12/x86_x64 architecture with Python 3.9 to pull_request matrix for testing purposes (this can probably be removed again, once you've tested)
  2. Have included macOS-12/x86_x64 architecture with Python * to workflow dispatch unless of course You/Tribler team only want Python 3.9/x86_x64 on macOS-12??

@xavier2k6 xavier2k6 force-pushed the wheel_macos_arm64 branch 2 times, most recently from fc34c3a to c8add64 Compare August 10, 2024 18:16
@xavier2k6 xavier2k6 marked this pull request as ready for review August 11, 2024 19:56
@qstokkink
Copy link

qstokkink commented Aug 12, 2024

Thanks 👍 and, "correct" to 2: I copied the wrong string.

@xavier2k6
Copy link
Contributor Author

@qstokkink Just so the Tribler team are aware macOS-12 images are considered Deprecated & will be Removed by end of year!
See: actions/runner-images#9255 or screenshot below.


300882082-ac6ef1f1-8e18-4af1-a97f-d38cf82885df

@qstokkink
Copy link

🤔 that's sad. It would be nice to get a PyPI release before that gets killed though.

@qstokkink
Copy link

@xavier2k6 considering the other closed issues, it looks like you can add #7729 to your "Closes" list.

@xavier2k6
Copy link
Contributor Author

manylinux2014 image may be dropped in a upcoming PR too by "cibuildwheel" repo., so may need to change to manylinux_2_28 but this may also drop older python versions but that is already on the cards by "cibuildwheel" repo.

@xavier2k6
Copy link
Contributor Author

@arvidn Regarding to OpenSSL 1.1.1 End Of Life


  1. Should wheels for Python <3.10 be dropped?

  1. What minimum OpenSSL 3.x.x version should be declared for below?

export OPENSSL_ROOT=openssl-1.1.1l
export OPENSSL_HASH=0b7a3e5e59c34827fe0c3a74b7ec8baef302b98fa80088d7f9153aa16fa76bd1


  1. FORTIFY_SOURCE may be bumped to 3 if images are newer/all conditions/requirements are met.

MANYLINUX_CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2"

@xavier2k6 xavier2k6 force-pushed the wheel_macos_arm64 branch 10 times, most recently from 6069880 to 1c08548 Compare August 28, 2024 15:12
@qstokkink
Copy link

@xavier2k6 Do you have time to experiment a bit with the win32 build? If we're lucky, it could be as simple as inserting the following at line 106 of cibuildwheel.yml:

- name: Install OpenSSL
  if: ${{ endsWith(matrix.CIBW_BUILD, 'win32') }}
  run: choco install openssl

If all checks are green/passing, it would be easier to merge this PR, I guess.

@xavier2k6
Copy link
Contributor Author

xavier2k6 commented Oct 8, 2024

@qstokkink i'll be able to give that a try in about 3hrs (got a spare few mins now to do it), action needs to be updated too & python 3.13 is gone GA.

@xavier2k6 xavier2k6 force-pushed the wheel_macos_arm64 branch 4 times, most recently from 0413058 to 1687adb Compare October 8, 2024 18:48
@xavier2k6
Copy link
Contributor Author

vcpkg is on the windows runners, this could be used to install openssl perhaps.....

@xavier2k6 xavier2k6 force-pushed the wheel_macos_arm64 branch 2 times, most recently from ca610f2 to 1c150aa Compare October 8, 2024 19:27
@qstokkink
Copy link

@xavier2k6 Thanks for putting some time into this. Using vcpkg should also work, but the current setup was made for chocolatey:

libtorrent/Jamfile

Lines 442 to 446 in 790b662

# the de-facto windows installer is https://slproweb.com/products/Win32OpenSSL.html, which installs to c:\Program Files\OpenSSL-Win{32,64}.
# chocolatey appears to use this installer.
local address_model = [ feature.get-values <address-model> : $(properties) ] ;
OPENSSL_LIB += "C:/Program Files/OpenSSL-Win$(address_model)/lib" ;
OPENSSL_LIB += "C:/Program Files (x86)/OpenSSL-Win$(address_model)/lib" ;

libtorrent/Jamfile

Lines 473 to 474 in 790b662

OPENSSL_INCLUDE += "C:/Program Files/OpenSSL-Win$(address_model)/include" ;
OPENSSL_INCLUDE += "C:/Program Files (x86)/OpenSSL-Win$(address_model)/include" ;


Using your log output, switching the four paths to this should work:

if address_model = 32
{
    OPENSSL_LIB += "C:/vcpkg/packages/openssl_x86-windows-static-md/lib" ;
} else {
    OPENSSL_LIB += "C:/vcpkg/packages/openssl_x64-windows-static-md/lib" ;
}
if address_model = 32
{
    OPENSSL_INCLUDE += "C:/vcpkg/packages/openssl_x86-windows-static-md/include" ;
}  else {
    OPENSSL_INCLUDE += "C:/vcpkg/packages/openssl_x64-windows-static-md/include" ;
}

My Jam language is a bit rusty. So, this might need to be address_model = "32".

@xavier2k6
Copy link
Contributor Author

xavier2k6 commented Oct 9, 2024

@qstokkink Will make some changes later, will also need to make sure we use the right package as there's 3x release options I believe:
openssl:x86-windows-static
openssl:x86-windows-static-md
openssl:x86-windows

I'll have to look at vcpkg port/json files etc.

vcpkg gets updated each time there's a newer windows runner image released so that will bring newer versions of OpenSSL.

@qstokkink
Copy link

Just for motivation/verification: the macos-14 wheel (from this PR) is now part of the Tribler unit tests and everything is working correctly.

@xavier2k6
Copy link
Contributor Author

I'm not too sure if the Jamfile is the right approach as we only need vcpkg for the OpenSSL install in CI not as an overall change.

@xavier2k6
Copy link
Contributor Author

@arvidn Do you still plan/want to support 32bit?

@xavier2k6
Copy link
Contributor Author

@qstokkink @arvidn the 32bit OpenSSL issue is the only thing stopping this PR from being finished.....any help would be highly appreciated.

I could do a dry-run then (if really necessary) for all python versions/OS before marking for review.

@qstokkink
Copy link

@xavier2k6 I can help with that. Are you O.K. with me opening another PR that includes your commit?

@xavier2k6
Copy link
Contributor Author

@qstokkink Feel free! I would suggest however to remove all other OS's from Pull Request section & just have Windows x86 & Windows x64 for testing purposes of OpenSSL....this will reduce build time/waiting on runners etc...we know the other OS's work & they can be added in via a final push.

@@ -101,50 +114,50 @@ jobs:
id: cache-wheel
with:
path: wheelhouse
key: wheel-${{ matrix.CIBW_BUILD }}-${{ matrix.CIBW_ARCHS }}-${{ github.sha }}
key: wheel-${{ matrix.os }}-${{ matrix.CIBW_BUILD }}

- uses: docker/setup-qemu-action@v3
if: steps.cache-wheel.outputs.cache-hit != 'true' && runner.os == 'Linux'

Choose a reason for hiding this comment

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

Ok, maybe a separate PR is overkill. This is all you need:

Suggested change
- name: Install OpenSSL (win32)
if: ${{ endsWith(matrix.CIBW_BUILD, 'win32') }}
run: |
Remove-Item -Path "C:\Program Files\OpenSSL" -Force -Recurse
vcpkg install openssl:x86-windows
New-Item -Path "C:\Program Files\OpenSSL" -ItemType SymbolicLink -Value "C:\vcpkg\packages\openssl_x86-windows\"

There's a mini build here: https://github.com/qstokkink/libtorrent/actions/runs/11439039359

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qstokkink Thank you!, added your change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qstokkink Do you think we should use the same vcpkg method for x64? (consistency)

Choose a reason for hiding this comment

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

In my opinion: absolutely not. This is much slower (it adds 6~7 minutes to the job). That is way too much added time just for the sake of consistency.

@xavier2k6 xavier2k6 marked this pull request as ready for review October 21, 2024 13:03
@arvidn arvidn merged commit 569f738 into arvidn:RC_2_0 Oct 25, 2024
55 checks passed
@arvidn
Copy link
Owner

arvidn commented Oct 25, 2024

thanks everyone!

@zakkarry
Copy link

zakkarry commented Oct 26, 2024

@arvidn would it be possible before support for 1.x musl and any other support is dropped to get a pypi build for 3.12 of 1.2.19 published as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants