-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-41696: [Python][Packaging] Bump MACOSX_DEPLOYMENT_TARGET to 12 instead of 11 #43137
Conversation
|
@github-actions crossbow submit wheel-macos-* |
@github-actions crossbow submit verify-rc-source--macos- |
Revision: e239385 Submitted crossbow builds: ursacomputing/crossbow @ actions-c9e99e37cf |
Revision: e239385 Submitted crossbow builds: ursacomputing/crossbow @ actions-56448c3d06 |
@github-actions crossbow submit wheel-macos-monterey-cp310-amd64 |
Revision: 93269ea Submitted crossbow builds: ursacomputing/crossbow @ actions-f0354ef8a9
|
I am not sure why the linking error is happening:
|
I have the same error when running vcpkg install qtbase:x64-osx-dynamic. Seems like Xcode toolchain issue. Edit: actually, qtbase:x64-osx-dynamic runs fine. Failure occurs when building qtbase:x64-osx with Qt customized to dynamic in triplet. Made an issue: microsoft/vcpkg#39805 |
@raulcd does it work if we leave the deployment target in the vcpkg files as is? (in the end for pypi what matters is the macos version that ends up in the wheels, so that older macos don't think they can install the wheel) |
@github-actions crossbow submit wheel-macos-monterey-cp310-amd64 |
Revision: aa286a4 Submitted crossbow builds: ursacomputing/crossbow @ actions-b3ee052429
|
This reverts commit 6585ff5.
Try reverting vpkg changes as suggested |
@github-actions crossbow submit wheel-macos-monterey-cp310-amd64 |
Revision: 6c31823 Submitted crossbow builds: ursacomputing/crossbow @ actions-3e4c817293
|
@github-actions crossbow submit wheel-macos-monterey-cp310-amd64 |
Revision: ae29215 Submitted crossbow builds: ursacomputing/crossbow @ actions-224dbbf786
|
@github-actions crossbow submit wheel-macos-* |
Revision: 823e0ef Submitted crossbow builds: ursacomputing/crossbow @ actions-e3d4d61b09 |
@github-actions crossbow submit wheel-macos-*-arm64 |
@kou the issue gets fixed if I explicitly link flight to OpenSSL, do you think there is a reason we were not explicitly linking? I am confused on why this wasn't the case |
Revision: 47df184 Submitted crossbow builds: ursacomputing/crossbow @ actions-833a228fad |
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.
+1
the issue gets fixed if I explicitly link flight to OpenSSL, do you think there is a reason we were not explicitly linking? I am confused on why this wasn't the case
Yes. In general, we don't need to link to OpenSSL because Flight doesn't use OpenSSL API directly (no #include <openssl/*.h>
) and gRPC also doesn't export OpenSSL API (grpcpp/*.h
doesn't have #include <openssl/*.h>
).
I'm not sure what has the cause of this (CoreFoundation? OpenSSL? linker?) but I think that this is the same workaround of #43137 (review) .
Could you add a comment that this is a workaround to the following part?
if(ARROW_USE_OPENSSL)
list(APPEND ARROW_FLIGHT_LINK_LIBS ${ARROW_OPENSSL_LIBS})
endif()
@github-actions crossbow submit wheel-macos-monterey-cp310-amd64 |
Revision: 2a2052e Submitted crossbow builds: ursacomputing/crossbow @ actions-d986864043
|
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.
Thanks for looking into this!
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6db12f2. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…12 instead of 11 (apache#43137) ### Rationale for this change As shown on the associated issue there seems to be a problem with `MACOSX_DEPLOYMENT_TARGET` 11 on the wheels. ### What changes are included in this PR? Update `MACOSX_DEPLOYMENT_TARGET` everywhere to the latest supported macOS version. ### Are these changes tested? Via CI, even though the issue was not reproducible on CI. ### Are there any user-facing changes? Yes, wheels won't be available for macOS 11 but those were crashing on the previous release. * GitHub Issue: apache#41696 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]>
…12 instead of 11 (apache#43137) ### Rationale for this change As shown on the associated issue there seems to be a problem with `MACOSX_DEPLOYMENT_TARGET` 11 on the wheels. ### What changes are included in this PR? Update `MACOSX_DEPLOYMENT_TARGET` everywhere to the latest supported macOS version. ### Are these changes tested? Via CI, even though the issue was not reproducible on CI. ### Are there any user-facing changes? Yes, wheels won't be available for macOS 11 but those were crashing on the previous release. * GitHub Issue: apache#41696 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]>
Rationale for this change
As shown on the associated issue there seems to be a problem with
MACOSX_DEPLOYMENT_TARGET
11 on the wheels.What changes are included in this PR?
Update
MACOSX_DEPLOYMENT_TARGET
everywhere to the latest supported macOS version.Are these changes tested?
Via CI, even though the issue was not reproducible on CI.
Are there any user-facing changes?
Yes, wheels won't be available for macOS 11 but those were crashing on the previous release.
import pyarrow
on MacOS 11.6 after pip update to pyarrow version 16.1.0 #41696