-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-47196: [CI][C++] Add Windows ARM64 build #47811
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
|
|
|
|
0522a37 to
9a8b0a5
Compare
|
|
1 similar comment
|
|
9a8b0a5 to
9ce6253
Compare
9ce6253 to
84f64cf
Compare
84f64cf to
d2a3efd
Compare
|
Can this be made part of the "C++ Extra" workflows? We probably don't want to support this officially for now, do we? |
kou
left a comment
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.
Yes. Could you use cpp_extra.yml?
BTW, @pitrou could you start a discussion (and a vote?) for #46002 on [email protected] to clarify our support policy?
| if(MSVC AND "${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "ARM64") | ||
| set(BOOST_CONTEXT_IMPLEMENTATION | ||
| winfib | ||
| CACHE STRING "" FORCE) |
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 we need to use CACHE variable here?
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.
Yes, otherwise it will default to fcontext and the build will fail. If cmake is run a second time after the initial configuration failed, somehow it then works. I thus decided to use CACHE FORCE to make it more reliable.
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.
Hmm. Could you try CMP0126 https://cmake.org/cmake/help/latest/policy/CMP0126.html and normal (not cache) variable?
diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index e805694f52..ca6b9e8746 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -71,6 +71,12 @@ cmake_policy(SET CMP0090 NEW)
# MSVC runtime library flags are selected by an abstraction.
cmake_policy(SET CMP0091 NEW)
+# https://cmake.org/cmake/help/latest/policy/CMP0126.html
+#
+# The set(CACHE) command does not remove any normal variable of the
+# same name from the current scope.
+cmake_policy(SET CMP0126 NEW)
+
# https://cmake.org/cmake/help/latest/policy/CMP0135.html
#
# CMP0135 is for solving re-building and re-downloading.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.
sadly it didn't work :/
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.
Oh...
d2a3efd to
e861fdc
Compare
| if(MSVC AND "${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "ARM64") | ||
| set(BOOST_CONTEXT_IMPLEMENTATION | ||
| winfib | ||
| CACHE STRING "" FORCE) |
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.
Hmm. Could you try CMP0126 https://cmake.org/cmake/help/latest/policy/CMP0126.html and normal (not cache) variable?
diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index e805694f52..ca6b9e8746 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -71,6 +71,12 @@ cmake_policy(SET CMP0090 NEW)
# MSVC runtime library flags are selected by an abstraction.
cmake_policy(SET CMP0091 NEW)
+# https://cmake.org/cmake/help/latest/policy/CMP0126.html
+#
+# The set(CACHE) command does not remove any normal variable of the
+# same name from the current scope.
+cmake_policy(SET CMP0126 NEW)
+
# https://cmake.org/cmake/help/latest/policy/CMP0135.html
#
# CMP0135 is for solving re-building and re-downloading.e861fdc to
4d7e0b3
Compare
|
@pitrou @AntoinePrv #47573 has broken the Windows ARM64 MSVC build. I have addressed it in 3fb046c, but happy to create a separate PR for it if you want. |
|
|
kou
left a comment
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
It may be better that we split the bpacking_simd changes to a separated PR. @WillAyd may have a comment for the Meson part.
| if(MSVC AND "${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "ARM64") | ||
| set(BOOST_CONTEXT_IMPLEMENTATION | ||
| winfib | ||
| CACHE STRING "" FORCE) |
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.
Oh...
+1 for splitting indeed |
|
@pitrou @WillAyd @AntoinePrv split into #47910 |
4d7e0b3 to
fc90543
Compare
### Rationale for this change #47573 broke the Windows ARM64 MSVC build by trying to compile xsimd even when `ARROW_SIMD_LEVEL` is set to `NONE`. ### What changes are included in this PR? Make sure that xsimd is only included when SIMD is requested. ### Are these changes tested? Yes, in the Windows ARM64 MSVC build pipeline that I was developing on #47811 when I noticed the change. ### Are there any user-facing changes? No. * GitHub Issue: #47909 Authored-by: Jonathan Giannuzzi <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
fc90543 to
8f8edd7
Compare
- Use GitHub hosted runner `windows-11-arm` to build with MSVC ARM64 - `simd-level` is set `NONE` for now as xsimd does not yet support MSVC ARM64 - `msys2` is installed and used for the timezone data, as it is not guaranteed to exist on windows-11-arm images - A recent version of `cmake` is installed as support for finding Windows OpenSSL ARM64 builds was only added in 4.1.0 (see Kitware/CMake@bf52219) - The Boost context implementation is set to `winfib` when building with MSVC ARM64 as it's the only supported implementation for that platform (see boostorg/context#296 (comment) and boostorg/context#315) - Updated ccache installation script and version to enable a Windows ARM64 native version
8f8edd7 to
e859997
Compare
kou
left a comment
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
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 8dd9eba. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 57 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change apache#47573 broke the Windows ARM64 MSVC build by trying to compile xsimd even when `ARROW_SIMD_LEVEL` is set to `NONE`. ### What changes are included in this PR? Make sure that xsimd is only included when SIMD is requested. ### Are these changes tested? Yes, in the Windows ARM64 MSVC build pipeline that I was developing on apache#47811 when I noticed the change. ### Are there any user-facing changes? No. * GitHub Issue: apache#47909 Authored-by: Jonathan Giannuzzi <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
### Rationale for this change This PR adds a CI job to test building the C++ library on Windows ARM64 with MSVC, which will help ensuring that downstream projects like [`vcpkg`](https://github.com/microsoft/vcpkg) can build Arrow C++ on that platform. ### What changes are included in this PR? - The `windows` job from the `cpp.yml` workflow has been moved into a reusable workflow `cpp_windows.yml` - Use GitHub hosted runner `windows-11-arm` to build with MSVC ARM64 in `cpp_extra.yml` - `simd-level` is set `NONE` for now as xsimd does not yet support MSVC ARM64 - `msys2` is installed and used for the timezone data, as it is not guaranteed to exist on `windows-11-arm` images - A recent version of `cmake` is installed as support for finding Windows OpenSSL ARM64 builds was only added in 4.1.0 (see Kitware/CMake@bf52219) - The Boost context implementation is set to `winfib` when building with MSVC ARM64 as it's the only supported implementation for that platform (see boostorg/context#296 (comment) and boostorg/context#315) ### Are these changes tested? Yes, in the CI itself ### Are there any user-facing changes? No * GitHub Issue: apache#47196 Authored-by: Jonathan Giannuzzi <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Rationale for this change
This PR adds a CI job to test building the C++ library on Windows ARM64 with MSVC, which will help ensuring that downstream projects like
vcpkgcan build Arrow C++ on that platform.What changes are included in this PR?
windowsjob from thecpp.ymlworkflow has been moved into a reusable workflowcpp_windows.ymlwindows-11-armto build with MSVC ARM64 incpp_extra.ymlsimd-levelis setNONEfor now as xsimd does not yet support MSVC ARM64msys2is installed and used for the timezone data, as it is not guaranteed to exist onwindows-11-armimagescmakeis installed as support for finding Windows OpenSSL ARM64 builds was only added in 4.1.0 (see Kitware/CMake@bf52219)winfibwhen building with MSVC ARM64 as it's the only supported implementation for that platform (see Windows ARM64 Support boostorg/context#296 (comment) and Add Windows ARM64 support boostorg/context#315)Are these changes tested?
Yes, in the CI itself
Are there any user-facing changes?
No