From c39db9977dea5dec60650ee71b2d07d464ded682 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Thu, 30 May 2019 18:55:38 -0500 Subject: [PATCH] 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 Closes #4380 from wesm/gtest-windows-dll and squashes the following commits: 515f75b0 Only set GTest_SOURCE to BUNDLED on MSVC if not passed explicitly 8678632d Revert more unwanted cmake changes c88d125d Revert unwanted cmake-format changes ac73ae37 Disable static CRT build c83c7d63 Use gtest shared lib --- appveyor.yml | 5 +- ci/appveyor-cpp-build.bat | 4 + ci/cpp-msvc-build-main.bat | 4 +- cpp/CMakeLists.txt | 73 +++++++++++-------- cpp/cmake_modules/SetupCxxFlags.cmake | 4 - cpp/cmake_modules/ThirdpartyToolchain.cmake | 81 +++++++++++++++------ docs/source/developers/cpp.rst | 9 +-- 7 files changed, 110 insertions(+), 70 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index f4c587733df7f..74c68df9c19eb 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -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" diff --git a/ci/appveyor-cpp-build.bat b/ci/appveyor-cpp-build.bat index 0320db9c4d7bd..54f947ae1de72 100644 --- a/ci/appveyor-cpp-build.bat +++ b/ci/appveyor-cpp-build.bat @@ -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 diff --git a/ci/cpp-msvc-build-main.bat b/ci/cpp-msvc-build-main.bat index 74eac7266deab..b3ac9bbf7ea0f 100644 --- a/ci/cpp-msvc-build-main.bat +++ b/ci/cpp-msvc-build-main.bat @@ -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 diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index 6fa7fb5005f73..853ff7a1d42d3 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -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}") @@ -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 # @@ -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) diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake index 97d6383b89541..7c7e7b59dfe1d 100644 --- a/cpp/cmake_modules/SetupCxxFlags.cmake +++ b/cpp/cmake_modules/SetupCxxFlags.cmake @@ -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 diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 56e049ced3414..cd9f61f56297b 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -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 @@ -1183,24 +1191,55 @@ 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) @@ -1208,26 +1247,26 @@ macro(build_gtest) 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) diff --git a/docs/source/developers/cpp.rst b/docs/source/developers/cpp.rst index 3a63ee264ec0c..f95981a1712c6 100644 --- a/docs/source/developers/cpp.rst +++ b/docs/source/developers/cpp.rst @@ -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 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~