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 intoapache:masterfrom
Conversation
|
@github-actions crossbow submit verify-rc-source-windows |
|
Revision: b0051e9056becad23a3efed41967989ac578d3a6 Submitted crossbow builds: ursacomputing/crossbow @ actions-2040
|
|
@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
|
|
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 |
|
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 |
|
Thanks for the quick response, that worked locally! I'll check with some crossbow builds. |
|
@github-actions crossbow submit verify-rc-source-windows |
|
@github-actions crossbow submit test-build-vcpkg-win |
|
@github-actions crossbow submit conda-linux-gcc-py37-cpu-r40 |
|
@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
|
|
@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
|
jorisvandenbossche
changed the title
|
It seems AppVeyor is still broken with this PR? cc @kou |
|
In the Appveyor build cmake 3.17 is used, so |
|
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
force-pushed
the
ARROW-16507-fix-gtest2
branch
from
16b0abd to
627d44f
Compare
kou
force-pushed
the
ARROW-16507-fix-gtest2
branch
from
da9b18f to
7ec278b
Compare
|
@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.
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
|
Revision: 7ec278b Submitted crossbow builds: ursacomputing/crossbow @ actions-2094
|
|
Can you trigger an AppVeyor build on this PR? |
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. |
|
Perhaps it can simply be a helper class that doesn't inherit from |
|
Ah, that could work. We still need to link the testing library to gtest but at least we can avoid inheritance issues. |
| 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.
|
Oops, should have fixed the PR title before merging. Too bad. |
|
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. |
6 participants
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.