Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
ARROW-5403: [C++] Use GTest shared libraries with BUNDLED build, alwa…
…ys 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 #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
  • Loading branch information
wesm committed May 30, 2019
1 parent c327af0 commit c39db99
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 70 deletions.
5 changes: 3 additions & 2 deletions appveyor.yml
Expand Up @@ -66,8 +66,9 @@ environment:
GENERATOR: Visual Studio 14 2015 Win64
CONFIGURATION: "Release"
ARROW_BUILD_GANDIVA: "ON"
- JOB: "Static_Crt_Build"
GENERATOR: Ninja
# NOTE: Since ARROW-5403 we have disabled the static CRT build
# - JOB: "Static_Crt_Build"
# GENERATOR: Ninja
- JOB: "Build_Debug"
GENERATOR: Ninja
CONFIGURATION: "Debug"
Expand Down
4 changes: 4 additions & 0 deletions ci/appveyor-cpp-build.bat
Expand Up @@ -26,6 +26,10 @@ if "%JOB%" == "Static_Crt_Build" (
@rem the Arrow DLL and the tests end up using a different instance of
@rem the CRT, which wreaks havoc.

@rem ARROW-5403(wesm): Since changing to using gtest DLLs we can no
@rem longer run the unit tests because gtest.dll and the unit test
@rem executables have different static copies of the CRT

mkdir cpp\build-debug
pushd cpp\build-debug

Expand Down
4 changes: 1 addition & 3 deletions ci/cpp-msvc-build-main.bat
Expand Up @@ -22,12 +22,10 @@ set ARROW_HOME=%CONDA_PREFIX%\Library
set CMAKE_ARGS=-DARROW_VERBOSE_THIRDPARTY_BUILD=OFF

if "%JOB%" == "Toolchain" (
@rem Toolchain gtest does not currently work with Visual Studio 2015
set CMAKE_ARGS=^
%CMAKE_ARGS% ^
-DARROW_WITH_BZ2=ON ^
-DARROW_DEPENDENCY_SOURCE=CONDA ^
-DGTest_SOURCE=BUNDLED
-DARROW_DEPENDENCY_SOURCE=CONDA
) else (
@rem We're in a conda enviroment but don't want to use it for the dependencies
set CMAKE_ARGS=%CMAKE_ARGS% -DARROW_DEPENDENCY_SOURCE=AUTO
Expand Down
73 changes: 41 additions & 32 deletions cpp/CMakeLists.txt
Expand Up @@ -24,6 +24,11 @@ string(REGEX MATCH "^[0-9]+\\.[0-9]+\\.[0-9]+" ARROW_BASE_VERSION "${ARROW_VERSI

project(arrow VERSION "${ARROW_BASE_VERSION}")

# if no build build type is specified, default to release builds
if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE Release)
endif(NOT CMAKE_BUILD_TYPE)

set(ARROW_VERSION_MAJOR "${arrow_VERSION_MAJOR}")
set(ARROW_VERSION_MINOR "${arrow_VERSION_MINOR}")
set(ARROW_VERSION_PATCH "${arrow_VERSION_PATCH}")
Expand Down Expand Up @@ -309,6 +314,42 @@ endif()

include(SetupCxxFlags)

#
# Build output directory
#

# set compile output directory
string(TOLOWER ${CMAKE_BUILD_TYPE} BUILD_SUBDIR_NAME)

# If build in-source, create the latest symlink. If build out-of-source, which is
# preferred, simply output the binaries in the build folder
if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_BINARY_DIR})
set(BUILD_OUTPUT_ROOT_DIRECTORY
"${CMAKE_CURRENT_BINARY_DIR}/build/${BUILD_SUBDIR_NAME}/")
# Link build/latest to the current build directory, to avoid developers
# accidentally running the latest debug build when in fact they're building
# release builds.
file(MAKE_DIRECTORY ${BUILD_OUTPUT_ROOT_DIRECTORY})
if(NOT APPLE)
set(MORE_ARGS "-T")
endif()
execute_process(COMMAND ln ${MORE_ARGS} -sf ${BUILD_OUTPUT_ROOT_DIRECTORY}
${CMAKE_CURRENT_BINARY_DIR}/build/latest)
else()
set(BUILD_OUTPUT_ROOT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/${BUILD_SUBDIR_NAME}/")
endif()

# where to put generated archives (.a files)
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
set(ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")

# where to put generated libraries (.so files)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
set(LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")

# where to put generated binaries
set(EXECUTABLE_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}")

#
# Dependencies
#
Expand Down Expand Up @@ -348,38 +389,6 @@ endif()
message(STATUS "CMAKE_C_FLAGS: ${CMAKE_C_FLAGS}")
message(STATUS "CMAKE_CXX_FLAGS: ${CMAKE_CXX_FLAGS}")

# set compile output directory
string(TOLOWER ${CMAKE_BUILD_TYPE} BUILD_SUBDIR_NAME)

# If build in-source, create the latest symlink. If build out-of-source, which is
# preferred, simply output the binaries in the build folder
if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_BINARY_DIR})
set(BUILD_OUTPUT_ROOT_DIRECTORY
"${CMAKE_CURRENT_BINARY_DIR}/build/${BUILD_SUBDIR_NAME}/")
# Link build/latest to the current build directory, to avoid developers
# accidentally running the latest debug build when in fact they're building
# release builds.
file(MAKE_DIRECTORY ${BUILD_OUTPUT_ROOT_DIRECTORY})
if(NOT APPLE)
set(MORE_ARGS "-T")
endif()
execute_process(COMMAND ln ${MORE_ARGS} -sf ${BUILD_OUTPUT_ROOT_DIRECTORY}
${CMAKE_CURRENT_BINARY_DIR}/build/latest)
else()
set(BUILD_OUTPUT_ROOT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/${BUILD_SUBDIR_NAME}/")
endif()

# where to put generated archives (.a files)
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
set(ARCHIVE_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")

# where to put generated libraries (.so files)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")
set(LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}")

# where to put generated binaries
set(EXECUTABLE_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}")

include_directories(${CMAKE_CURRENT_BINARY_DIR}/src)
include_directories(src)

Expand Down
4 changes: 0 additions & 4 deletions cpp/cmake_modules/SetupCxxFlags.cmake
Expand Up @@ -38,10 +38,6 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
# shared libraries
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

# if no build build type is specified, default to debug builds
if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE Release)
endif(NOT CMAKE_BUILD_TYPE)
string(TOUPPER ${CMAKE_BUILD_TYPE} CMAKE_BUILD_TYPE)

# compiler flags that are common across debug/release builds
Expand Down
81 changes: 60 additions & 21 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
Expand Up @@ -59,6 +59,14 @@ set(ARROW_THIRDPARTY_DEPENDENCIES
ZLIB
ZSTD)

# TODO(wesm): External GTest shared libraries are not currently
# supported when building with MSVC because of the way that
# conda-forge packages have 4 variants of the libraries packaged
# together
if(MSVC AND "${GTest_SOURCE}" STREQUAL "")
set(GTest_SOURCE "BUNDLED")
endif()

message(STATUS "Using ${ARROW_DEPENDENCY_SOURCE} approach to find dependencies")

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

if(MSVC)
set(GTEST_CMAKE_CXX_FLAGS "${GTEST_CMAKE_CXX_FLAGS} -DGTEST_CREATE_SHARED_LIBRARY=1")
endif()

set(GTEST_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/googletest_ep-prefix/src/googletest_ep")
set(GTEST_INCLUDE_DIR "${GTEST_PREFIX}/include")

if(MSVC)
set(_GTEST_IMPORTED_TYPE IMPORTED_IMPLIB)
set(_GTEST_LIBRARY_SUFFIX
"${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_IMPORT_LIBRARY_SUFFIX}")
else()
set(_GTEST_IMPORTED_TYPE IMPORTED_LOCATION)
set(_GTEST_LIBRARY_SUFFIX
"${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_SHARED_LIBRARY_SUFFIX}")
endif()

set(GTEST_SHARED_LIB
"${GTEST_PREFIX}/lib/${CMAKE_SHARED_LIBRARY_PREFIX}gtest${_GTEST_LIBRARY_SUFFIX}")
set(GMOCK_SHARED_LIB
"${GTEST_PREFIX}/lib/${CMAKE_SHARED_LIBRARY_PREFIX}gmock${_GTEST_LIBRARY_SUFFIX}")
set(
GTEST_STATIC_LIB
"${GTEST_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}gtest${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_STATIC_LIBRARY_SUFFIX}"
)
set(
GTEST_MAIN_STATIC_LIB
"${GTEST_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}gtest_main${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_STATIC_LIBRARY_SUFFIX}"
GTEST_MAIN_SHARED_LIB

"${GTEST_PREFIX}/lib/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_LIBRARY_SUFFIX}"
)
set(GTEST_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} "-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}"
"-DCMAKE_INSTALL_LIBDIR=lib"
-DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS})
set(GTEST_CMAKE_ARGS
${EP_COMMON_TOOLCHAIN}
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
"-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}"
"-DCMAKE_INSTALL_LIBDIR=lib"
-DBUILD_SHARED_LIBS=ON
-DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS}
-DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${GTEST_CMAKE_CXX_FLAGS})
set(GMOCK_INCLUDE_DIR "${GTEST_PREFIX}/include")
set(
GMOCK_STATIC_LIB
"${GTEST_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}gmock${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_STATIC_LIBRARY_SUFFIX}"
)

if(MSVC)
if("${CMAKE_GENERATOR}" STREQUAL "Ninja")
set(_GTEST_LIBRARY_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY})
else()
set(_GTEST_LIBRARY_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY}/${CMAKE_BUILD_TYPE})
endif()

set(GTEST_CMAKE_ARGS
${GTEST_CMAKE_ARGS} "-DCMAKE_RUNTIME_OUTPUT_DIRECTORY=${_GTEST_LIBRARY_DIR}"
"-DCMAKE_RUNTIME_OUTPUT_DIRECTORY_${CMAKE_BUILD_TYPE}=${_GTEST_LIBRARY_DIR}")
endif()

add_definitions(-DGTEST_LINKED_AS_SHARED_LIBRARY=1)

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

externalproject_add(googletest_ep
URL ${GTEST_SOURCE_URL}
BUILD_BYPRODUCTS ${GTEST_STATIC_LIB} ${GTEST_MAIN_STATIC_LIB}
${GMOCK_STATIC_LIB}
BUILD_BYPRODUCTS ${GTEST_SHARED_LIB} ${GTEST_MAIN_SHARED_LIB}
${GMOCK_SHARED_LIB}
CMAKE_ARGS ${GTEST_CMAKE_ARGS} ${EP_LOG_OPTIONS})

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

add_library(GTest::GTest STATIC IMPORTED)
add_library(GTest::GTest SHARED IMPORTED)
set_target_properties(GTest::GTest
PROPERTIES IMPORTED_LOCATION "${GTEST_STATIC_LIB}"
PROPERTIES ${_GTEST_IMPORTED_TYPE} "${GTEST_SHARED_LIB}"
INTERFACE_INCLUDE_DIRECTORIES "${GTEST_INCLUDE_DIR}")

add_library(GTest::Main STATIC IMPORTED)
add_library(GTest::Main SHARED IMPORTED)
set_target_properties(GTest::Main
PROPERTIES IMPORTED_LOCATION "${GTEST_MAIN_STATIC_LIB}"
PROPERTIES ${_GTEST_IMPORTED_TYPE} "${GTEST_MAIN_SHARED_LIB}"
INTERFACE_INCLUDE_DIRECTORIES "${GTEST_INCLUDE_DIR}")

add_library(GTest::GMock STATIC IMPORTED)
add_library(GTest::GMock SHARED IMPORTED)
set_target_properties(GTest::GMock
PROPERTIES IMPORTED_LOCATION "${GMOCK_STATIC_LIB}"
PROPERTIES ${_GTEST_IMPORTED_TYPE} "${GMOCK_SHARED_LIB}"
INTERFACE_INCLUDE_DIRECTORIES "${GTEST_INCLUDE_DIR}")
add_dependencies(toolchain-tests googletest_ep)
add_dependencies(GTest::GTest googletest_ep)
Expand Down
9 changes: 1 addition & 8 deletions docs/source/developers/cpp.rst
Expand Up @@ -696,16 +696,9 @@ an out of source build by generating a MSVC solution:
mkdir build
cd build
cmake .. -G "Visual Studio 14 2015 Win64" ^
-DARROW_BUILD_TESTS=ON ^
-DGTest_SOURCE=BUNDLED
-DARROW_BUILD_TESTS=ON
cmake --build . --config Release
.. note::

Currently building the unit tests does not work properly with googletest
from conda-forge, so we must use the ``BUNDLED`` source for building that
dependency

Building with Ninja and clcache
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down

0 comments on commit c39db99

Please sign in to comment.