-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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-38653: [Packaging][Java][Python][Ruby] Raise the minimum macOS version to 10.15 catalina to allow using new APIs in C++17 #38677
Conversation
|
I am not very familiar with CI scripts in the project, and I only search for "10.14" and replace the possible usages from "10.14" to "10.15", and if there is some usage missing in the PR, please let me know. Thanks. |
We build arrow C++ on 10.13 for the R package, because CRAN still builds on that (hopefully only until next april...). We do this succesfully by disabeling availability checks by adding In that case we have to support the massivly eol macOS version due to CRAN but in general I would be ok with raising the deployment target, that can probably also happen independently for each implementation depending on the eco system standard (e.g. for python iirc 10.14 or even 11 is normal?) |
@github-actions crossbow submit java-jars -g wheel |
Revision: a46e6eb Submitted crossbow builds: ursacomputing/crossbow @ actions-b9ab40b2df |
It seems that we need the following: diff --git a/dev/tasks/java-jars/github.yml b/dev/tasks/java-jars/github.yml
index 7dc53a35e..fbce12ee4 100644
--- a/dev/tasks/java-jars/github.yml
+++ b/dev/tasks/java-jars/github.yml
@@ -81,7 +81,7 @@ jobs:
- { runs_on: ["macos-latest"], arch: "x86_64"}
- { runs_on: ["self-hosted", "macOS", "arm64", "devops-managed"], arch: "aarch_64" }
env:
- MACOSX_DEPLOYMENT_TARGET: "10.13"
+ MACOSX_DEPLOYMENT_TARGET: "10.15"
steps:
{{ macros.github_checkout_arrow()|indent }}
- name: Set up Python |
Or how about using https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/io_util.h instead of |
… newer API in C++17 on macOS.
a46e6eb
to
c131dbd
Compare
Updated that file |
@github-actions crossbow submit java-jars |
Revision: c131dbd Submitted crossbow builds: ursacomputing/crossbow @ actions-8301fee69d
|
I am okay with that, and I can submit another PR for this change. Currently it is just an internal test file so I think it is easy to change. But we may have to document such requirement, otherwise, people submitting new PR may trigger similar issue accidentally later (I assume there are other classes in C++ 17 that could cause this issue, actually since arrow switched to C++17 for a while, I am a bit surprised that I am the first one having PR triggering such an issue). Please let me know if this is needed and I am glad to submit a PR to revise the usage of |
@raulcd I'm not sure that it's related but can we remove https://github.com/apache/arrow/releases/tag/apache-arrow-14.0.1.dev ? The java-jars job failed by 14.0.1-SNAPSHOT related files:
|
@niyue Could you try We may want to set diff --git a/.github/workflows/cpp.yml b/.github/workflows/cpp.yml
index e6ae6c60b..daf77935b 100644
--- a/.github/workflows/cpp.yml
+++ b/.github/workflows/cpp.yml
@@ -181,6 +181,7 @@ jobs:
ARROW_WITH_ZLIB: ON
ARROW_WITH_ZSTD: ON
GTest_SOURCE: BUNDLED
+ MACOSX_DEPLOYMENT_TARGET: "10.13"
steps:
- name: CPU Info
run: | |
I've deleted it, I still have to bump-versions after 14.0.1 but I don't think this tag should be necessary. |
@github-actions crossbow submit java-jars |
Revision: c131dbd Submitted crossbow builds: ursacomputing/crossbow @ actions-cd86a2e13a
|
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
@niyue Hmm. It seems that https://lists.apache.org/thread/rbfqs9fzy66rv509wjx5j10z5ygqvvnx
Could you open a new issue for this and use |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 9e5c5b5. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
…OS version to 10.15 catalina to allow using new APIs in C++17 (apache#38677) ### Rationale for this change For macOS, some new APIs in C++17, e.g. `std::filesystem::path` is only visible when the compiling target is >= macOS 10.15 (Catalina). But currently the minimum macOS target is set to 10.14 (Mojave). * macOS 10.14 Mojave was released on 2018-09-24 [1] * macOS 10.15 Catalina was released on 2019-10-07 [2] * Both macOS 10.14 and 10.15 are end of life [3] There is a [similar issue](ClickHouse/ClickHouse#8541) [4] and [its fix](ClickHouse/ClickHouse#8600) [5] for ClickHouse on year 2020, which raised minimum macOS version from 10.14 to 10.15. ### What changes are included in this PR? Raise the minimum macOS version from 10.14 to 10.15 ### Are these changes tested? This should be tested and verified on CI. ### Are there any user-facing changes? * Users using macOS <= 10.14 may not able to run these CI jobs locally. * The new version of python wheels may not be able to be deployed to environments with macOS <= 10.14 (not completely sure how this affects the deployment) * Closes: apache#38653 ### References * [1] https://en.wikipedia.org/wiki/MacOS_Mojave * [2] https://en.wikipedia.org/wiki/MacOS_Catalina * [3] https://endoflife.date/macos * [4] ClickHouse/ClickHouse#8541 * [5] ClickHouse/ClickHouse#8600 Authored-by: Yue Ni <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Rationale for this change
For macOS, some new APIs in C++17, e.g.
std::filesystem::path
is only visible when the compiling target is >= macOS 10.15 (Catalina). But currently the minimum macOS target is set to 10.14 (Mojave).There is a similar issue [4] and its fix [5] for ClickHouse on year 2020, which raised minimum macOS version from 10.14 to 10.15.
What changes are included in this PR?
Raise the minimum macOS version from 10.14 to 10.15
Are these changes tested?
This should be tested and verified on CI.
Are there any user-facing changes?
References