Skip to content

Commit c39db99

Browse files
wesmwesm
committed
ARROW-5403: [C++] Use GTest shared libraries with BUNDLED build, always use BUNDLED with MSVC
Without this change, failed assertions do not cause failed unit tests on Windows as Antoine had discovered today. The conda-forge test package seems unusable in shared library form so I'm forcing BUNDLED builds on MSVC (if not otherwise specified) and removing the hacks in the docs and Appveyor to pass `-DGTest_SOURCE=BUNDLED` After this change the static CRT Windows tests do not work anymore (because gtest.dll has its own copy of the static CRT, which leads to conflicts), so we will have to explore testing the static CRT build as follow up work. See ARROW-5426 Author: Wes McKinney <wesm+git@apache.org> Closes apache#4380 from wesm/gtest-windows-dll and squashes the following commits: 515f75b <Wes McKinney> Only set GTest_SOURCE to BUNDLED on MSVC if not passed explicitly 8678632 <Wes McKinney> Revert more unwanted cmake changes c88d125 <Wes McKinney> Revert unwanted cmake-format changes ac73ae3 <Wes McKinney> Disable static CRT build c83c7d6 <Wes McKinney> Use gtest shared lib
1 parent c327af0 commit c39db99

7 files changed

Lines changed: 110 additions & 70 deletions

File tree

‎appveyor.yml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,9 @@ environment:
6666
GENERATOR: Visual Studio 14 2015 Win64
6767
CONFIGURATION: "Release"
6868
ARROW_BUILD_GANDIVA: "ON"
69-
- JOB: "Static_Crt_Build"
70-
GENERATOR: Ninja
69+
# NOTE: Since ARROW-5403 we have disabled the static CRT build
70+
# - JOB: "Static_Crt_Build"
71+
# GENERATOR: Ninja
7172
- JOB: "Build_Debug"
7273
GENERATOR: Ninja
7374
CONFIGURATION: "Debug"

‎ci/appveyor-cpp-build.bat‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ if "%JOB%" == "Static_Crt_Build" (
2626
@rem the Arrow DLL and the tests end up using a different instance of
2727
@rem the CRT, which wreaks havoc.
2828

29+
@rem ARROW-5403(wesm): Since changing to using gtest DLLs we can no
30+
@rem longer run the unit tests because gtest.dll and the unit test
31+
@rem executables have different static copies of the CRT
32+
2933
mkdir cpp\build-debug
3034
pushd cpp\build-debug
3135

‎ci/cpp-msvc-build-main.bat‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,10 @@ set ARROW_HOME=%CONDA_PREFIX%\Library
2222
set CMAKE_ARGS=-DARROW_VERBOSE_THIRDPARTY_BUILD=OFF
2323

2424
if "%JOB%" == "Toolchain" (
25-
@rem Toolchain gtest does not currently work with Visual Studio 2015
2625
set CMAKE_ARGS=^
2726
%CMAKE_ARGS% ^
2827
-DARROW_WITH_BZ2=ON ^
29-
-DARROW_DEPENDENCY_SOURCE=CONDA ^
30-
-DGTest_SOURCE=BUNDLED
28+
-DARROW_DEPENDENCY_SOURCE=CONDA
3129
) else (
3230
@rem We're in a conda enviroment but don't want to use it for the dependencies
3331
set CMAKE_ARGS=%CMAKE_ARGS% -DARROW_DEPENDENCY_SOURCE=AUTO

‎cpp/CMakeLists.txt‎

Lines changed: 41 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ string(REGEX MATCH "^[0-9]+\\.[0-9]+\\.[0-9]+" ARROW_BASE_VERSION "${ARROW_VERSI
2424

2525
project(arrow VERSION "${ARROW_BASE_VERSION}")
2626

27+
# if no build build type is specified, default to release builds
28+
if(NOT CMAKE_BUILD_TYPE)
29+
set(CMAKE_BUILD_TYPE Release)
30+
endif(NOT CMAKE_BUILD_TYPE)
31+
2732
set(ARROW_VERSION_MAJOR "${arrow_VERSION_MAJOR}")
2833
set(ARROW_VERSION_MINOR "${arrow_VERSION_MINOR}")
2934
set(ARROW_VERSION_PATCH "${arrow_VERSION_PATCH}")
@@ -309,6 +314,42 @@ endif()
309314

310315
include(SetupCxxFlags)
311316

317+
#
318+
# Build output directory
319+
#
320+
321+
# set compile output directory
322+
string(TOLOWER ${CMAKE_BUILD_TYPE} BUILD_SUBDIR_NAME)
323+
324+
# If build in-source, create the latest symlink. If build out-of-source, which is
325+
# preferred, simply output the binaries in the build folder
326+
if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_BINARY_DIR})
327+
set(BUILD_OUTPUT_ROOT_DIRECTORY
328+
"${CMAKE_CURRENT_BINARY_DIR}/build/${BUILD_SUBDIR_NAME}/")
329+
# Link build/latest to the current build directory, to avoid developers
330+
# accidentally running the latest debug build when in fact they're building
331+
# release builds.
332+
file(MAKE_DIRECTORY ${BUILD_OUTPUT_ROOT_DIRECTORY})
333+
if(NOT APPLE)
334+
set(MORE_ARGS "-T")
335+
endif()
336+
execute_process(COMMAND ln ${MORE_ARGS} -sf ${BUILD_OUTPUT_ROOT_DIRECTORY}
337+
${CMAKE_CURRENT_BINARY_DIR}/build/latest)
338+
else()
339+
set(BUILD_OUTPUT_ROOT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/${BUILD_SUBDIR_NAME}/")
340+
endif()
341+
342+
# where to put generated archives (.a files)
343+
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
344+
set(ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
345+
346+
# where to put generated libraries (.so files)
347+
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
348+
set(LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
349+
350+
# where to put generated binaries
351+
set(EXECUTABLE_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}")
352+
312353
#
313354
# Dependencies
314355
#
@@ -348,38 +389,6 @@ endif()
348389
message(STATUS "CMAKE_C_FLAGS: ${CMAKE_C_FLAGS}")
349390
message(STATUS "CMAKE_CXX_FLAGS: ${CMAKE_CXX_FLAGS}")
350391

351-
# set compile output directory
352-
string(TOLOWER ${CMAKE_BUILD_TYPE} BUILD_SUBDIR_NAME)
353-
354-
# If build in-source, create the latest symlink. If build out-of-source, which is
355-
# preferred, simply output the binaries in the build folder
356-
if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_BINARY_DIR})
357-
set(BUILD_OUTPUT_ROOT_DIRECTORY
358-
"${CMAKE_CURRENT_BINARY_DIR}/build/${BUILD_SUBDIR_NAME}/")
359-
# Link build/latest to the current build directory, to avoid developers
360-
# accidentally running the latest debug build when in fact they're building
361-
# release builds.
362-
file(MAKE_DIRECTORY ${BUILD_OUTPUT_ROOT_DIRECTORY})
363-
if(NOT APPLE)
364-
set(MORE_ARGS "-T")
365-
endif()
366-
execute_process(COMMAND ln ${MORE_ARGS} -sf ${BUILD_OUTPUT_ROOT_DIRECTORY}
367-
${CMAKE_CURRENT_BINARY_DIR}/build/latest)
368-
else()
369-
set(BUILD_OUTPUT_ROOT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/${BUILD_SUBDIR_NAME}/")
370-
endif()
371-
372-
# where to put generated archives (.a files)
373-
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
374-
set(ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
375-
376-
# where to put generated libraries (.so files)
377-
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
378-
set(LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
379-
380-
# where to put generated binaries
381-
set(EXECUTABLE_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}")
382-
383392
include_directories(${CMAKE_CURRENT_BINARY_DIR}/src)
384393
include_directories(src)
385394

‎cpp/cmake_modules/SetupCxxFlags.cmake‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,6 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
3838
# shared libraries
3939
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
4040

41-
# if no build build type is specified, default to debug builds
42-
if(NOT CMAKE_BUILD_TYPE)
43-
set(CMAKE_BUILD_TYPE Release)
44-
endif(NOT CMAKE_BUILD_TYPE)
4541
string(TOUPPER ${CMAKE_BUILD_TYPE} CMAKE_BUILD_TYPE)
4642

4743
# compiler flags that are common across debug/release builds

‎cpp/cmake_modules/ThirdpartyToolchain.cmake‎

Lines changed: 60 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,14 @@ set(ARROW_THIRDPARTY_DEPENDENCIES
5959
ZLIB
6060
ZSTD)
6161

62+
# TODO(wesm): External GTest shared libraries are not currently
63+
# supported when building with MSVC because of the way that
64+
# conda-forge packages have 4 variants of the libraries packaged
65+
# together
66+
if(MSVC AND "${GTest_SOURCE}" STREQUAL "")
67+
set(GTest_SOURCE "BUNDLED")
68+
endif()
69+
6270
message(STATUS "Using ${ARROW_DEPENDENCY_SOURCE} approach to find dependencies")
6371

6472
# TODO: double-conversion check fails for conda, it should not
@@ -1183,51 +1191,82 @@ macro(build_gtest)
11831191
-Wno-unused-value -Wno-ignored-attributes)
11841192
endif()
11851193

1194+
if(MSVC)
1195+
set(GTEST_CMAKE_CXX_FLAGS "${GTEST_CMAKE_CXX_FLAGS} -DGTEST_CREATE_SHARED_LIBRARY=1")
1196+
endif()
1197+
11861198
set(GTEST_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/googletest_ep-prefix/src/googletest_ep")
11871199
set(GTEST_INCLUDE_DIR "${GTEST_PREFIX}/include")
1200+
1201+
if(MSVC)
1202+
set(_GTEST_IMPORTED_TYPE IMPORTED_IMPLIB)
1203+
set(_GTEST_LIBRARY_SUFFIX
1204+
"${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_IMPORT_LIBRARY_SUFFIX}")
1205+
else()
1206+
set(_GTEST_IMPORTED_TYPE IMPORTED_LOCATION)
1207+
set(_GTEST_LIBRARY_SUFFIX
1208+
"${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_SHARED_LIBRARY_SUFFIX}")
1209+
endif()
1210+
1211+
set(GTEST_SHARED_LIB
1212+
"${GTEST_PREFIX}/lib/${CMAKE_SHARED_LIBRARY_PREFIX}gtest${_GTEST_LIBRARY_SUFFIX}")
1213+
set(GMOCK_SHARED_LIB
1214+
"${GTEST_PREFIX}/lib/${CMAKE_SHARED_LIBRARY_PREFIX}gmock${_GTEST_LIBRARY_SUFFIX}")
11881215
set(
1189-
GTEST_STATIC_LIB
1190-
"${GTEST_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}gtest${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_STATIC_LIBRARY_SUFFIX}"
1191-
)
1192-
set(
1193-
GTEST_MAIN_STATIC_LIB
1194-
"${GTEST_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}gtest_main${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_STATIC_LIBRARY_SUFFIX}"
1216+
GTEST_MAIN_SHARED_LIB
1217+
1218+
"${GTEST_PREFIX}/lib/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_LIBRARY_SUFFIX}"
11951219
)
1196-
set(GTEST_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} "-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}"
1197-
"-DCMAKE_INSTALL_LIBDIR=lib"
1198-
-DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS})
1220+
set(GTEST_CMAKE_ARGS
1221+
${EP_COMMON_TOOLCHAIN}
1222+
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
1223+
"-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}"
1224+
"-DCMAKE_INSTALL_LIBDIR=lib"
1225+
-DBUILD_SHARED_LIBS=ON
1226+
-DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS}
1227+
-DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${GTEST_CMAKE_CXX_FLAGS})
11991228
set(GMOCK_INCLUDE_DIR "${GTEST_PREFIX}/include")
1200-
set(
1201-
GMOCK_STATIC_LIB
1202-
"${GTEST_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}gmock${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_STATIC_LIBRARY_SUFFIX}"
1203-
)
1229+
1230+
if(MSVC)
1231+
if("${CMAKE_GENERATOR}" STREQUAL "Ninja")
1232+
set(_GTEST_LIBRARY_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY})
1233+
else()
1234+
set(_GTEST_LIBRARY_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY}/${CMAKE_BUILD_TYPE})
1235+
endif()
1236+
1237+
set(GTEST_CMAKE_ARGS
1238+
${GTEST_CMAKE_ARGS} "-DCMAKE_RUNTIME_OUTPUT_DIRECTORY=${_GTEST_LIBRARY_DIR}"
1239+
"-DCMAKE_RUNTIME_OUTPUT_DIRECTORY_${CMAKE_BUILD_TYPE}=${_GTEST_LIBRARY_DIR}")
1240+
endif()
1241+
1242+
add_definitions(-DGTEST_LINKED_AS_SHARED_LIBRARY=1)
12041243

12051244
if(MSVC AND NOT ARROW_USE_STATIC_CRT)
12061245
set(GTEST_CMAKE_ARGS ${GTEST_CMAKE_ARGS} -Dgtest_force_shared_crt=ON)
12071246
endif()
12081247

12091248
externalproject_add(googletest_ep
12101249
URL ${GTEST_SOURCE_URL}
1211-
BUILD_BYPRODUCTS ${GTEST_STATIC_LIB} ${GTEST_MAIN_STATIC_LIB}
1212-
${GMOCK_STATIC_LIB}
1250+
BUILD_BYPRODUCTS ${GTEST_SHARED_LIB} ${GTEST_MAIN_SHARED_LIB}
1251+
${GMOCK_SHARED_LIB}
12131252
CMAKE_ARGS ${GTEST_CMAKE_ARGS} ${EP_LOG_OPTIONS})
12141253

12151254
# The include directory must exist before it is referenced by a target.
12161255
file(MAKE_DIRECTORY "${GTEST_INCLUDE_DIR}")
12171256

1218-
add_library(GTest::GTest STATIC IMPORTED)
1257+
add_library(GTest::GTest SHARED IMPORTED)
12191258
set_target_properties(GTest::GTest
1220-
PROPERTIES IMPORTED_LOCATION "${GTEST_STATIC_LIB}"
1259+
PROPERTIES ${_GTEST_IMPORTED_TYPE} "${GTEST_SHARED_LIB}"
12211260
INTERFACE_INCLUDE_DIRECTORIES "${GTEST_INCLUDE_DIR}")
12221261

1223-
add_library(GTest::Main STATIC IMPORTED)
1262+
add_library(GTest::Main SHARED IMPORTED)
12241263
set_target_properties(GTest::Main
1225-
PROPERTIES IMPORTED_LOCATION "${GTEST_MAIN_STATIC_LIB}"
1264+
PROPERTIES ${_GTEST_IMPORTED_TYPE} "${GTEST_MAIN_SHARED_LIB}"
12261265
INTERFACE_INCLUDE_DIRECTORIES "${GTEST_INCLUDE_DIR}")
12271266

1228-
add_library(GTest::GMock STATIC IMPORTED)
1267+
add_library(GTest::GMock SHARED IMPORTED)
12291268
set_target_properties(GTest::GMock
1230-
PROPERTIES IMPORTED_LOCATION "${GMOCK_STATIC_LIB}"
1269+
PROPERTIES ${_GTEST_IMPORTED_TYPE} "${GMOCK_SHARED_LIB}"
12311270
INTERFACE_INCLUDE_DIRECTORIES "${GTEST_INCLUDE_DIR}")
12321271
add_dependencies(toolchain-tests googletest_ep)
12331272
add_dependencies(GTest::GTest googletest_ep)

‎docs/source/developers/cpp.rst‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -696,16 +696,9 @@ an out of source build by generating a MSVC solution:
696696
mkdir build
697697
cd build
698698
cmake .. -G "Visual Studio 14 2015 Win64" ^
699-
-DARROW_BUILD_TESTS=ON ^
700-
-DGTest_SOURCE=BUNDLED
699+
-DARROW_BUILD_TESTS=ON
701700
cmake --build . --config Release
702701
703-
.. note::
704-
705-
Currently building the unit tests does not work properly with googletest
706-
from conda-forge, so we must use the ``BUNDLED`` source for building that
707-
dependency
708-
709702
Building with Ninja and clcache
710703
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
711704

0 commit comments

Comments
 (0)