You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by we...@apache.org on 2019/05/30 23:55:54 UTC
[arrow] branch master updated: ARROW-5403: [C++] Use GTest shared
libraries with BUNDLED build, always use BUNDLED with MSVC
This is an automated email from the ASF dual-hosted git repository.
wesm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new c39db99 ARROW-5403: [C++] Use GTest shared libraries with BUNDLED build, always use BUNDLED with MSVC
c39db99 is described below
commit c39db9977dea5dec60650ee71b2d07d464ded682
Author: Wes McKinney <we...@apache.org>
AuthorDate: Thu May 30 18:55:38 2019 -0500
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 <we...@apache.org>
Closes #4380 from wesm/gtest-windows-dll and squashes the following commits:
515f75b0 <Wes McKinney> Only set GTest_SOURCE to BUNDLED on MSVC if not passed explicitly
8678632d <Wes McKinney> Revert more unwanted cmake changes
c88d125d <Wes McKinney> Revert unwanted cmake-format changes
ac73ae37 <Wes McKinney> Disable static CRT build
c83c7d63 <Wes McKinney> 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 f4c5877..74c68df 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 0320db9..54f947a 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 74eac72..b3ac9bb 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 6fa7fb5..853ff7a 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}")
@@ -310,6 +315,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 97d6383..7c7e7b5 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 56e049c..cd9f61f 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 3a63ee2..f95981a 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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~