ARROW-4352: [C++] Add support for system Google Test #3469
Conversation
Google Test is installed in /usr/lib/x86_64-linux-gnu on Debian. Google Test is installed as shared library on MSYS2. This change includes some fixes to build tests with MinGW. They are just for building some tests. They aren't completed changes yet. We need more works to run all tests with MinGW build.
| find_path( GTEST_INCLUDE_DIR NAMES gtest/gtest.h ) | ||
| find_library( GTEST_LIBRARIES NAMES gtest ) | ||
| endif () | ||
| if(_gtest_roots) |
There was a problem hiding this comment.
We should standardize on a CMake indentation rule, and ideally (but not sure it's possible) enforce it. Apparently there's cmake_format.
There was a problem hiding this comment.
@pitrou Can you make a JIRA for that? Then we can implement that.
There was a problem hiding this comment.
There was a problem hiding this comment.
We should standardize on a CMake indentation rule
I think so too.
| set(lib_dirs | ||
| "lib/${CMAKE_LIBRARY_ARCHITECTURE}" | ||
| "lib64" | ||
| "lib") |
There was a problem hiding this comment.
Is there an already-defined variable for this? It seems weird to have to hardcode such search paths for each system library.
There was a problem hiding this comment.
We have similar variable in FindProtobuf.cmake: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/FindProtobuf.cmake#L39
(I added it.)
We can remove it by removing NO_DEFAULT_PATH from our find_library() calls.
To be honest, I want to remove all our custom find codes. For example:
index c7496c6a..20cb55cf 100644
--- a/cpp/cmake_modules/FindGTest.cmake
+++ b/cpp/cmake_modules/FindGTest.cmake
@@ -33,13 +33,6 @@
# GTEST_SHARED_MAIN_LIB, path to libgtest_main's shared library
# GTEST_FOUND, whether gtest has been found
-if( NOT "${GTEST_HOME}" STREQUAL "")
- file( TO_CMAKE_PATH "${GTEST_HOME}" _native_path )
- list( APPEND _gtest_roots ${_native_path} )
-elseif ( GTest_HOME )
- list( APPEND _gtest_roots ${GTest_HOME} )
-endif()
-
set(GTEST_STATIC_LIB_NAME
${CMAKE_STATIC_LIBRARY_PREFIX}gtest${CMAKE_STATIC_LIBRARY_SUFFIX})
set(GTEST_MAIN_STATIC_LIB_NAME
@@ -49,35 +42,11 @@ set(GTEST_SHARED_LIB_NAME
set(GTEST_MAIN_SHARED_LIB_NAME
${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${CMAKE_SHARED_LIBRARY_SUFFIX})
-# Try the parameterized roots, if they exist
-if(_gtest_roots)
- find_path(GTEST_INCLUDE_DIR NAMES gtest/gtest.h
- PATHS ${_gtest_roots} NO_DEFAULT_PATH
- PATH_SUFFIXES "include")
- set(lib_dirs
- "lib/${CMAKE_LIBRARY_ARCHITECTURE}"
- "lib64"
- "lib")
- find_library(GTEST_STATIC_LIB NAMES ${GTEST_STATIC_LIB_NAME}
- PATHS ${_gtest_roots} NO_DEFAULT_PATH
- PATH_SUFFIXES ${lib_dirs})
- find_library(GTEST_MAIN_STATIC_LIB NAMES ${GTEST_MAIN_STATIC_LIB_NAME}
- PATHS ${_gtest_roots} NO_DEFAULT_PATH
- PATH_SUFFIXES ${lib_dirs})
- find_library(GTEST_SHARED_LIB NAMES ${GTEST_SHARED_LIB_NAME}
- PATHS ${_gtest_roots} NO_DEFAULT_PATH
- PATH_SUFFIXES ${lib_dirs})
- find_library(GTEST_MAIN_SHARED_LIB NAMES ${GTEST_MAIN_SHARED_LIB_NAME}
- PATHS ${_gtest_roots} NO_DEFAULT_PATH
- PATH_SUFFIXES ${lib_dirs})
-else()
- find_path(GTEST_INCLUDE_DIR NAMES gtest/gtest.h)
- find_library(GTEST_STATIC_LIB NAMES ${GTEST_STATIC_LIB_NAME})
- find_library(GTEST_MAIN_STATIC_LIB NAMES ${GTEST_MAIN_STATIC_LIB_NAME})
- find_library(GTEST_SHARED_LIB NAMES ${GTEST_SHARED_LIB_NAME})
- find_library(GTEST_MAIN_SHARED_LIB NAMES ${GTEST_MAIN_SHARED_LIB_NAME})
-endif()
-
+find_path(GTEST_INCLUDE_DIR NAMES gtest/gtest.h)
+find_library(GTEST_STATIC_LIB NAMES ${GTEST_STATIC_LIB_NAME})
+find_library(GTEST_MAIN_STATIC_LIB NAMES ${GTEST_MAIN_STATIC_LIB_NAME})
+find_library(GTEST_SHARED_LIB NAMES ${GTEST_SHARED_LIB_NAME})
+find_library(GTEST_MAIN_SHARED_LIB NAMES ${GTEST_MAIN_SHARED_LIB_NAME})
if(GTEST_INCLUDE_DIR AND
(GTEST_STATIC_LIB AND GTEST_MAIN_STATIC_LIB) OR
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 5a75a9b0..50fe37a1 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -29,35 +29,7 @@ set(ARROW_RE2_LINKAGE "static" CACHE STRING
set(THIRDPARTY_DIR "${arrow_SOURCE_DIR}/thirdparty")
if (NOT "$ENV{ARROW_BUILD_TOOLCHAIN}" STREQUAL "")
- set(BROTLI_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(BZ2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(CARES_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(DOUBLE_CONVERSION_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(FLATBUFFERS_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(GFLAGS_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(GLOG_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(GRPC_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- # Using gtest from the toolchain breaks AppVeyor builds
- if (NOT MSVC)
- set(GTEST_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- endif()
- set(JEMALLOC_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(LZ4_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- # orc disabled as it's not in conda-forge (but in Anaconda with an incompatible ABI)
- # set(ORC_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(PROTOBUF_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(RAPIDJSON_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(RE2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(SNAPPY_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(THRIFT_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(ZLIB_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
- set(ZSTD_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}")
-
- if (NOT DEFINED ENV{BOOST_ROOT})
- # Since we have to set this in the environment, we check whether
- # $BOOST_ROOT is defined inside here
- set(ENV{BOOST_ROOT} "$ENV{ARROW_BUILD_TOOLCHAIN}")
- endif()
+ list(INSERT CMAKE_PREFIX_PATH 0 "$ENV{ARROW_BUILD_TOOLCHAIN}")
endif()
# Home path for each third-party lib can be overriden with env vars
@@ -600,7 +572,10 @@ endif()
# Google gtest
if(ARROW_BUILD_TESTS OR ARROW_BUILD_BENCHMARKS)
- if("${GTEST_HOME}" STREQUAL "")
+ find_package(GTest)
+ if(GTest_FOUND)
+ set(GTEST_VENDORED 0)
+ else()
if(APPLE)
set(GTEST_CMAKE_CXX_FLAGS "-fPIC -DGTEST_USE_OWN_TR1_TUPLE=1 -Wno-unused-value -Wno-ignored-attributes")
elseif(NOT MSVC)
@@ -628,9 +603,6 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_BENCHMARKS)
BUILD_BYPRODUCTS ${GTEST_STATIC_LIB} ${GTEST_MAIN_STATIC_LIB}
CMAKE_ARGS ${GTEST_CMAKE_ARGS}
${EP_LOG_OPTIONS})
- else()
- find_package(GTest REQUIRED)
- set(GTEST_VENDORED 0)
endif()
message(STATUS "GTest include dir: ${GTEST_INCLUDE_DIR}")Users can change package search path by CMAKE_PREFIX_PATH. Users can also use <PackageName>_ROOT (GTest_Root for this case) CMake variable and environment variable for each package since CMake 3.12: https://cmake.org/cmake/help/v3.12/command/find_library.html
If called from within a find module loaded by
find_package(<PackageName>), search prefixes unique to the current package being found. Specifically look in the<PackageName>_ROOTCMake variable and the<PackageName>_ROOTenvironment variable. The package root variables are maintained as a stack so if called from nested find modules, root paths from the parent’s find module will be searched after paths from the current module, i.e.<CurrentPackage>_ROOT,ENV{<CurrentPackage>_ROOT},<ParentPackage>_ROOT,ENV{<ParentPackage>_ROOT}, etc.
Our custom find code uses <PackageName>_HOME for this propose.
There was a problem hiding this comment.
| STATIC_LIB ${GTEST_STATIC_LIB}) | ||
| ADD_THIRDPARTY_LIB(gtest_main | ||
| STATIC_LIB ${GTEST_MAIN_STATIC_LIB}) | ||
| if(GTEST_STATIC_LIB) |
There was a problem hiding this comment.
By the way, is there a reason to prefer the static lib over the shared lib? Is it for Windows?
There was a problem hiding this comment.
No, there isn't.
I just choose the static library because we used the static library before this change.
Google Test is installed in /usr/lib/x86_64-linux-gnu on Debian.
Google Test is installed as shared library on MSYS2.
This change includes some fixes to build tests with MinGW. They are
just for building some tests. They aren't completed changes yet. We
need more works to run all tests with MinGW build.