Uh oh!
There was an error while loading. Please reload this page.
ARROW-16507: [CI][C++] Use system gtest with mamba/conda - #13101
ARROW-16507: [CI][C++] Use system gtest with mamba/conda #13101assignUser wants to merge 27 commits into
Conversation
assignUser
commented
May 9, 2022
@github-actions crossbow submit verify-rc-source-windows |
Revision: b0051e9056becad23a3efed41967989ac578d3a6 Submitted crossbow builds: ursacomputing/crossbow @ actions-2040
|
assignUser
commented
May 9, 2022
@github-actions crossbow submit verify-rc-source-windows |
@kou cmake is not detecting the shared libraries from conda, I took a look at the cmake logic but was unable to fix it "properly" I have implemented your suggestion from here #13063 (comment) |
Revision: 80d8b8de2bb66a94a9189372221ac30f033569a3 Submitted crossbow builds: ursacomputing/crossbow @ actions-2043
|
kou
commented
May 9, 2022
It seems that the gtest package for Windows doesn't include CMake package files:
It seems that
How about trying it? diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 865406e705..7dfe388bb7 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1969,11 +1969,16 @@ macro(build_gtest)
endmacro()
if(ARROW_TESTING)
+ if(CMAKE_VERSION VERSION_LESS 3.23)+ set(GTEST_USE_CONFIG FALSE)+ else()+ set(GTEST_USE_CONFIG TRUE)+ endif()
resolve_dependency(GTest
REQUIRED_VERSION
1.10.0
USE_CONFIG
- TRUE)+ ${GTEST_USE_CONFIG})
if(NOT GTEST_VENDORED)
# TODO(wesm): This logic does not work correctly with the MSVC static libraries |
@kou Thank you for the suggestion! With cmake 3.23.1 this does not work on my machine :(, setting |
kou
commented
May 9, 2022
Oh, sorry. --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1969,11 +1969,16 @@ macro(build_gtest)
endmacro()
if(ARROW_TESTING)
+ if(CMAKE_VERSION VERSION_LESS 3.23)+ set(GTEST_USE_CONFIG TRUE)+ else()+ set(GTEST_USE_CONFIG FALSE)+ endif()
resolve_dependency(GTest
REQUIRED_VERSION
1.10.0
USE_CONFIG
- TRUE)+ ${GTEST_USE_CONFIG})
if(NOT GTEST_VENDORED)
# TODO(wesm): This logic does not work correctly with the MSVC static libraries |
assignUser
commented
May 9, 2022
Thanks for the quick response, that worked locally! I'll check with some crossbow builds. |
assignUser
commented
May 9, 2022
@github-actions crossbow submit verify-rc-source-windows |
assignUser
commented
May 9, 2022
@github-actions crossbow submit test-build-vcpkg-win |
assignUser
commented
May 9, 2022
@github-actions crossbow submit conda-linux-gcc-py37-cpu-r40 |
assignUser
commented
May 9, 2022
@github-actions crossbow submit conda-osx-clang-py37-r40 |
Revision: f5e83753d21fc6d5370c407b7a0bcb8a4a528940 Submitted crossbow builds: ursacomputing/crossbow @ actions-2046
|
|
Revision: f5e83753d21fc6d5370c407b7a0bcb8a4a528940 Submitted crossbow builds: ursacomputing/crossbow @ actions-2047
|
assignUser
commented
May 9, 2022
@github-actions crossbow submit test-build-vcpkg-win |
Revision: f5e83753d21fc6d5370c407b7a0bcb8a4a528940 Submitted crossbow builds: ursacomputing/crossbow @ actions-2048
|
Revision: f5e83753d21fc6d5370c407b7a0bcb8a4a528940 Submitted crossbow builds: ursacomputing/crossbow @ actions-2049
|
pitrou
commented
May 10, 2022
It seems AppVeyor is still broken with this PR? cc @kou |
In the Appveyor build cmake 3.17 is used, so |
kou
commented
May 10, 2022
How about just stopping pinning CMake? diff --git a/ci/appveyor-cpp-setup.bat b/ci/appveyor-cpp-setup.bat
index 936306296f..bbe2c38d1e 100644
--- a/ci/appveyor-cpp-setup.bat+++ b/ci/appveyor-cpp-setup.bat@@ -71,7 +71,6 @@ if "%JOB%" NEQ "Build_Debug" (
mamba create -n arrow -q -y -c conda-forge ^
--file=ci\conda_env_python.txt ^
%CONDA_PACKAGES% ^
- "cmake=3.17" ^
"ninja" ^
"nomkl" ^
"pandas" ^This was introduced by #7865 / ARROW-9599 as a workaround for CMake 3.18. The problem may be resolved with the latest CMake. |
@kou the issue from ARROW-9599 seems to still be there with cmake 3.23 https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/43526687/job/i2gse6ev1ghe0alu#L529 |
kou
commented
May 16, 2022
@github-actions crossbow submit test-debian--cpp- test-fedora-35-cpp test-ubuntu-20.04-cpp-bundled test-ubuntu-22.04-cpp verify-rc-source-cpp-linux-almalinux-8-amd64 verify-rc-source-cpp-linux-ubuntu-* verify-rc-source-integration-linux-ubuntu-* verify-rc-source-integration-macos-conda-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
kou
commented
May 16, 2022
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
Revision: 7ec278b Submitted crossbow builds: ursacomputing/crossbow @ actions-2094
|
pitrou
commented
May 16, 2022
Can you trigger an AppVeyor build on this PR? |
lidavidm
commented
May 16, 2022
Hmm, part of the motivation was to try to save some compile time instead of having to include the full definition of all tests. But if it's causing problems I can move it into a header only library instead. |
pitrou
commented
May 16, 2022
Perhaps it can simply be a helper class that doesn't inherit from |
lidavidm
commented
May 16, 2022
Ah, that could work. We still need to link the testing library to gtest but at least we can avoid inheritance issues. |
kou
commented
May 16, 2022
This is ready to merge. AppVeyor build: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/43563294
Note that the nightly job at 2022-05-09 was canceled by GitHub Actions' default 6 hours timeout because we ran tests without ctest's |
| set(ARROW_FLIGHT_TEST_LINKAGE "static") | ||
| endif() | ||
| if(ARROW_TESTING) | ||
| if(WIN32) |
There was a problem hiding this comment.
Hopefully this can be removed next in @lidavidm 's PR.
pitrou
commented
May 17, 2022
Oops, should have fixed the PR title before merging. Too bad. |
ursabot
commented
May 17, 2022
Benchmark runs are scheduled for baseline = f17b09b and contender = acbfd60. acbfd60 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
The gtest package for Windows provided by conda-forge doesn't provide
GTestConfig.cmake.See also: https://github.com/conda-forge/gtest-feedstock/blob/main/recipe/bld.bat
And
FindGTest.cmakeprovided by CMake can't findgtest_dll.dllthat is a sharedlibrary version of GoogleTest.
See also: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindGTest.cmake
It means that we can find only static version of GoogleTest on Windows with Conda
without a custom
FindGTestAlt.cmake.Shared library version
arrow_flight_testingrequires shared library version GoogleTest onWindows because it defines
arrow::flight::FlightTestthat inheritstesting::Test.See also: https://github.com/apache/arrow/blob/master/cpp/src/arrow/flight/test_definitions.h
We must use the same library type for them on Windows.
To support
ARROW_FLIGHT=ON/ARROW_BUILD_SHARED=ON/ARROW_BUILD_STATIC=OFF/ARROW_BUILD_TESTS=ONwith static version of GoogleTest, we need to build a static library notshared library for
arrow_flight_testing.