-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-4352: [C++] Add support for system Google Test #3469
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,9 @@ | |
| # GTEST_INCLUDE_DIR, directory containing headers | ||
| # GTEST_LIBS, directory containing gtest libraries | ||
| # GTEST_STATIC_LIB, path to libgtest.a | ||
| # GTEST_STATIC_MAIN_LIB, path to libgtest_main.a | ||
| # GTEST_SHARED_LIB, path to libgtest's shared library | ||
| # GTEST_SHARED_MAIN_LIB, path to libgtest_main's shared library | ||
| # GTEST_FOUND, whether gtest has been found | ||
|
|
||
| if( NOT "${GTEST_HOME}" STREQUAL "") | ||
|
|
@@ -38,34 +40,60 @@ 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 | ||
| ${CMAKE_STATIC_LIBRARY_PREFIX}gtest_main${CMAKE_STATIC_LIBRARY_SUFFIX}) | ||
| set(GTEST_SHARED_LIB_NAME | ||
| ${CMAKE_SHARED_LIBRARY_PREFIX}gtest${CMAKE_SHARED_LIBRARY_SUFFIX}) | ||
| 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" ) | ||
| find_library( GTEST_LIBRARIES NAMES gtest gtest_main | ||
| PATHS ${_gtest_roots} NO_DEFAULT_PATH | ||
| PATH_SUFFIXES "lib" ) | ||
| else () | ||
| find_path( GTEST_INCLUDE_DIR NAMES gtest/gtest.h ) | ||
| find_library( GTEST_LIBRARIES NAMES gtest ) | ||
| endif () | ||
| 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") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an already-defined variable for this? It seems weird to have to hardcode such search paths for each system library.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have similar variable in We can remove it by removing 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
Our custom find code uses
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps open a JIRA for that?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| 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() | ||
|
|
||
|
|
||
| if (GTEST_INCLUDE_DIR AND GTEST_LIBRARIES) | ||
| if(GTEST_INCLUDE_DIR AND | ||
| (GTEST_STATIC_LIB AND GTEST_MAIN_STATIC_LIB) OR | ||
| (GTEST_SHARED_LIB AND GTEST_MAIN_SHARED_LIB)) | ||
| set(GTEST_FOUND TRUE) | ||
| get_filename_component( GTEST_LIBS ${GTEST_LIBRARIES} PATH ) | ||
| set(GTEST_LIB_NAME gtest) | ||
| set(GTEST_STATIC_LIB ${GTEST_LIBS}/${CMAKE_STATIC_LIBRARY_PREFIX}${GTEST_LIB_NAME}${CMAKE_STATIC_LIBRARY_SUFFIX}) | ||
| set(GTEST_MAIN_STATIC_LIB ${GTEST_LIBS}/${CMAKE_STATIC_LIBRARY_PREFIX}${GTEST_LIB_NAME}_main${CMAKE_STATIC_LIBRARY_SUFFIX}) | ||
| set(GTEST_SHARED_LIB ${GTEST_LIBS}/${CMAKE_SHARED_LIBRARY_PREFIX}${GTEST_LIB_NAME}${CMAKE_SHARED_LIBRARY_SUFFIX}) | ||
| else () | ||
| else() | ||
| set(GTEST_FOUND FALSE) | ||
| endif () | ||
| endif() | ||
|
|
||
| if (GTEST_FOUND) | ||
| if (NOT GTest_FIND_QUIETLY) | ||
| message(STATUS "Found the GTest library: ${GTEST_LIBRARIES}") | ||
| message(STATUS "Found the GTest library:") | ||
| message(STATUS "GTEST_STATIC_LIB: ${GTEST_STATIC_LIB}") | ||
| message(STATUS "GTEST_MAIN_STATIC_LIB: ${GTEST_MAIN_STATIC_LIB}") | ||
| message(STATUS "GTEST_SHARED_LIB: ${GTEST_SHARED_LIB}") | ||
| message(STATUS "GTEST_MAIN_SHARED_LIB: ${GTEST_MAIN_SHARED_LIB}") | ||
| endif () | ||
| else () | ||
| if (NOT GTest_FIND_QUIETLY) | ||
|
|
@@ -86,7 +114,8 @@ endif () | |
| mark_as_advanced( | ||
| GTEST_INCLUDE_DIR | ||
| GTEST_LIBS | ||
| GTEST_LIBRARIES | ||
| GTEST_STATIC_LIB | ||
| GTEST_MAIN_STATIC_LIB | ||
| GTEST_SHARED_LIB | ||
| GTEST_MAIN_SHARED_LIB | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -634,16 +634,28 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_BENCHMARKS) | |
| endif() | ||
|
|
||
| message(STATUS "GTest include dir: ${GTEST_INCLUDE_DIR}") | ||
| message(STATUS "GTest static library: ${GTEST_STATIC_LIB}") | ||
| include_directories(SYSTEM ${GTEST_INCLUDE_DIR}) | ||
| ADD_THIRDPARTY_LIB(gtest | ||
| STATIC_LIB ${GTEST_STATIC_LIB}) | ||
| ADD_THIRDPARTY_LIB(gtest_main | ||
| STATIC_LIB ${GTEST_MAIN_STATIC_LIB}) | ||
| if(GTEST_STATIC_LIB) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, is there a reason to prefer the static lib over the shared lib? Is it for Windows?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, there isn't. |
||
| message(STATUS "GTest static library: ${GTEST_STATIC_LIB}") | ||
| ADD_THIRDPARTY_LIB(gtest | ||
| STATIC_LIB ${GTEST_STATIC_LIB}) | ||
| ADD_THIRDPARTY_LIB(gtest_main | ||
| STATIC_LIB ${GTEST_MAIN_STATIC_LIB}) | ||
| set(GTEST_LIBRARY gtest_static) | ||
| set(GTEST_MAIN_LIBRARY gtest_main_static) | ||
| else() | ||
| message(STATUS "GTest shared library: ${GTEST_SHARED_LIB}") | ||
| ADD_THIRDPARTY_LIB(gtest | ||
| SHARED_LIB ${GTEST_SHARED_LIB}) | ||
| ADD_THIRDPARTY_LIB(gtest_main | ||
| SHARED_LIB ${GTEST_MAIN_SHARED_LIB}) | ||
| set(GTEST_LIBRARY gtest_shared) | ||
| set(GTEST_MAIN_LIBRARY gtest_main_shared) | ||
| endif() | ||
|
|
||
| if(GTEST_VENDORED) | ||
| add_dependencies(gtest_static googletest_ep) | ||
| add_dependencies(gtest_main_static googletest_ep) | ||
| add_dependencies(${GTEST_LIBRARY} googletest_ep) | ||
| add_dependencies(${GTEST_MAIN_LIBRARY} googletest_ep) | ||
| endif() | ||
| endif() | ||
|
|
||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pitrou Can you make a JIRA for that? Then we can implement that.
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.
See https://issues.apache.org/jira/browse/ARROW-4363
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.
I think so too.