Skip to content

ARROW-4599: [C++] Add support for system GFlags #3675

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.

Sign up for GitHub

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

Closed
kou wants to merge 6 commits into from
Closed

ARROW-4599: [C++] Add support for system GFlags #3675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
34cb1a6
[C++] Add support for system GFlags
kou Feb 16, 2019
9516d2f
GFLAGS_STATIC isn't defined by CONFIG mode
kou Feb 18, 2019
ca0f67c
Remove build directory entirely before new configuration build
kou Feb 18, 2019
0f43fc9
Don't use vendored GFlags when GFLAGS_HOME is specified
kou Feb 18, 2019
2caf588
Define GFLAGS_IS_A_DLL by target
kou Feb 18, 2019
38f910f
Fix rebase artifact
wesm Feb 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/appveyor-cpp-build.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ if "%JOB%" == "Static_Crt_Build" (
cmake --build . --config Debug || exit /B
ctest --output-on-failure -j2 || exit /B
popd
rmdir /S /Q cpp\build-debug
Copy link
Member Author

@kou kou Feb 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because find_package(gflags CONFIG) can find GFlags in cpp\build-debug\gflags_ep-prefix\.


mkdir cpp\build-release
pushd cpp\build-release
Expand Down
3 changes: 2 additions & 1 deletion ci/travis_before_script_cpp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ CMAKE_COMMON_FLAGS="\
-DARROW_NO_DEPRECATED_API=ON \
-DARROW_EXTRA_ERROR_CONTEXT=ON"
CMAKE_LINUX_FLAGS=""
CMAKE_OSX_FLAGS=""
CMAKE_OSX_FLAGS="\
-DARROW_GFLAGS_USE_SHARED=OFF"
Copy link
Member Author

@kou kou Feb 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because @rpath can't be resolved with GFlags installed by conda-forge.

https://travis-ci.org/kou/arrow/jobs/494364820#L1993

 2/111 Test   #1: plasma-serialization_tests ...................***Failed    1.36 sec
Running plasma-serialization_tests, redirecting output into /Users/travis/build/kou/arrow/cpp-build/build/test-logs/plasma-serialization_tests.txt (attempt 1/1)
dyld: Library not loaded: @rpath/libgflags.2.2.dylib
 Referenced from: /Users/travis/build/kou/arrow/cpp-install/lib/libarrow.13.dylib
 Reason: image not found

We will be able to resolve it by running each test with DYLD_LIBRARY_PATH environment variable:

DYLD_LIBRARY_PATH=${MINICONDA}/lib ${ARROW_BUILD_TYPE}/plasma-serialization_tests

Because DYLD_LIBRARY_PATH can affect only the child process.

It's not useful. So I use static library than shared library.

Copy link
Member

@xhochy xhochy Feb 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working for all other libraries? Then there must be something wrong with libgflags.

Copy link
Member Author

@kou kou Feb 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other libraries are used as static library. So they work.

gflags-config.cmake provides @rpath/libgflags.2.2.dylib for IMPORTED_LOCATION. If gflags-config.cmake doesn't use @rpath, this will not be caused.

Generally, using @rpath isn't bad. GFlags installed from conda-forge can't assume where it's installed. So I think that using @rpath is reasonable for this case.

We will be able to resolve @rpath in our FindGFlags.cmake. This is another solution.

We can specify DYLD_LIBRARY_PATH to resolve @rpath in cpp/build-support/run-test.sh. This is also another solution.


if [ "$ARROW_TRAVIS_USE_TOOLCHAIN" == "1" ]; then
CMAKE_COMMON_FLAGS="${CMAKE_COMMON_FLAGS} -DARROW_JEMALLOC=ON"
Expand Down
2 changes: 2 additions & 0 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ Note that this requires linking Boost statically" OFF)
option(ARROW_PROTOBUF_USE_SHARED
"Rely on Protocol Buffers shared libraries where relevant" OFF)

option(ARROW_GFLAGS_USE_SHARED "Rely on GFlags shared libraries where relevant" ON)

option(ARROW_WITH_BACKTRACE "Build with backtrace support" ON)

option(ARROW_USE_GLOG "Build libraries with glog support for pluggable logging" ON)
Expand Down
18 changes: 17 additions & 1 deletion cpp/cmake_modules/BuildUtils.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
function(ADD_THIRDPARTY_LIB LIB_NAME)
set(options)
set(one_value_args SHARED_LIB STATIC_LIB)
set(multi_value_args DEPS)
set(multi_value_args DEPS INCLUDE_DIRECTORIES)
cmake_parse_arguments(ARG "${options}" "${one_value_args}" "${multi_value_args}" ${ARGN})
if(ARG_UNPARSED_ARGUMENTS)
message(SEND_ERROR "Error: unrecognized arguments: ${ARG_UNPARSED_ARGUMENTS}")
Expand All @@ -38,6 +38,10 @@ function(ADD_THIRDPARTY_LIB LIB_NAME)
PROPERTIES INTERFACE_LINK_LIBRARIES "${ARG_DEPS}")
endif()
message(STATUS "Added static library dependency ${AUG_LIB_NAME}: ${ARG_STATIC_LIB}")
if(ARG_INCLUDE_DIRECTORIES)
set_target_properties(${AUG_LIB_NAME}
PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${ARG_INCLUDE_DIRECTORIES}")
endif()

SET(AUG_LIB_NAME "${LIB_NAME}_shared")
add_library(${AUG_LIB_NAME} SHARED IMPORTED)
Expand All @@ -55,6 +59,10 @@ function(ADD_THIRDPARTY_LIB LIB_NAME)
PROPERTIES INTERFACE_LINK_LIBRARIES "${ARG_DEPS}")
endif()
message(STATUS "Added shared library dependency ${AUG_LIB_NAME}: ${ARG_SHARED_LIB}")
if(ARG_INCLUDE_DIRECTORIES)
set_target_properties(${AUG_LIB_NAME}
PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${ARG_INCLUDE_DIRECTORIES}")
endif()
elseif(ARG_STATIC_LIB)
SET(AUG_LIB_NAME "${LIB_NAME}_static")
add_library(${AUG_LIB_NAME} STATIC IMPORTED)
Expand All @@ -65,6 +73,10 @@ function(ADD_THIRDPARTY_LIB LIB_NAME)
PROPERTIES INTERFACE_LINK_LIBRARIES "${ARG_DEPS}")
endif()
message(STATUS "Added static library dependency ${AUG_LIB_NAME}: ${ARG_STATIC_LIB}")
if(ARG_INCLUDE_DIRECTORIES)
set_target_properties(${AUG_LIB_NAME}
PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${ARG_INCLUDE_DIRECTORIES}")
endif()
elseif(ARG_SHARED_LIB)
SET(AUG_LIB_NAME "${LIB_NAME}_shared")
add_library(${AUG_LIB_NAME} SHARED IMPORTED)
Expand All @@ -82,6 +94,10 @@ function(ADD_THIRDPARTY_LIB LIB_NAME)
set_target_properties(${AUG_LIB_NAME}
PROPERTIES INTERFACE_LINK_LIBRARIES "${ARG_DEPS}")
endif()
if(ARG_INCLUDE_DIRECTORIES)
set_target_properties(${AUG_LIB_NAME}
PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${ARG_INCLUDE_DIRECTORIES}")
endif()
else()
message(FATAL_ERROR "No static or shared library provided for ${LIB_NAME}")
endif()
Expand Down
147 changes: 105 additions & 42 deletions cpp/cmake_modules/FindGFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,58 +22,121 @@
# GFLAGS_STATIC_LIB, path to libgflags static library
# GFLAGS_FOUND, whether gflags has been found

if( NOT "${GFLAGS_HOME}" STREQUAL "")
file( TO_CMAKE_PATH "${GFLAGS_HOME}" _native_path )
list( APPEND _gflags_roots ${_native_path} )
elseif ( GFlags_HOME )
list( APPEND _gflags_roots ${GFlags_HOME} )
find_package(gflags CONFIG)
set(GFLAGS_FOUND ${gflags_FOUND})
if(GFLAGS_FOUND)
message(STATUS "GFlags include dir: ${GFLAGS_INCLUDE_DIR}")
string(TOUPPER "${CMAKE_BUILD_TYPE}" _CONFIG)
get_target_property(_GFLAGS_CONFIGURATIONS gflags IMPORTED_CONFIGURATIONS)
foreach(_GFLAGS_CONFIG IN LISTS _GFLAGS_CONFIGURATIONS)
if("${_GFLAGS_CONFIG}" STREQUAL "${_CONFIG}")
set(_GFLAGS_TARGET_CONFIG "${_GFLAGS_CONFIG}")
endif()
endforeach()
if(NOT _GFLAGS_TARGET_CONFIG)
list(GET _GFLAGS_CONFIGURATIONS 0 _GFLAGS_TARGET_CONFIG)
endif()
if(GFLAGS_SHARED)
get_target_property(GFLAGS_SHARED_LIB gflags_shared
IMPORTED_LOCATION_${_GFLAGS_TARGET_CONFIG})
message(STATUS "GFlags shared library: ${GFLAGS_SHARED_LIB}")
endif()
if(TARGET gflags_static)
set(GFLAGS_STATIC TRUE)
get_target_property(GFLAGS_STATIC_LIB gflags_static
IMPORTED_LOCATION_${_GFLAGS_TARGET_CONFIG})
message(STATUS "GFlags static library: ${GFLAGS_STATIC_LIB}")
endif()
unset(_CONFIG)
unset(_GFLAGS_CONFIGURATIONS)
unset(_GFLAGS_CONFIG)
unset(_GFLAGS_TARGET_CONFIG)
return()
endif()

if (MSVC AND NOT DEFINED GFLAGS_MSVC_STATIC_LIB_SUFFIX)
pkg_check_modules(GFLAGS gflags)
if(GFLAGS_FOUND)
set(GFLAGS_INCLUDE_DIR "${GFLAGS_INCLUDEDIR}")
message(STATUS "GFlags include dir: ${GFLAGS_INCLUDE_DIR}")
find_library(GFLAGS_SHARED_LIB
NAMES "${GFLAGS_LIBRARIES}"
PATHS "${GFLAGS_LIBDIR}"
NO_DEFAULT_PATH)
if(GFLAGS_SHARED_LIB)
message(STATUS "GFlags shared library: ${GFLAGS_SHARED_LIB}")
add_thirdparty_lib(gflags
SHARED_LIB
"${GFLAGS_SHARED_LIB}"
INCLUDE_DIRECTORIES
"${GFLAGS_INCLUDE_DIR}")
target_compile_definitions(gflags_shared INTERFACE "GFLAGS_IS_A_DLL=1")
set(GFLAGS_STATIC_LIB FLASE)
return()
else()
set(GFLAGS_FOUND FALSE)
endif()
endif()

if(GFLAGS_HOME)
set(GFlags_ROOT "${GFLAGS_HOME}")
endif()
if(CMAKE_VERSION VERSION_LESS 3.12)
list(INSERT CMAKE_PREFIX_PATH 0 "${GFlags_ROOT}")
endif()

if(MSVC AND NOT DEFINED GFLAGS_MSVC_STATIC_LIB_SUFFIX)
set(GFLAGS_MSVC_STATIC_LIB_SUFFIX "_static")
endif()

set(GFLAGS_STATIC_LIB_SUFFIX
"${GFLAGS_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}")
"${GFLAGS_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}")

set(GFLAGS_STATIC_LIB_NAME
${CMAKE_STATIC_LIBRARY_PREFIX}gflags${GFLAGS_STATIC_LIB_SUFFIX})
"${CMAKE_STATIC_LIBRARY_PREFIX}gflags${GFLAGS_STATIC_LIB_SUFFIX}")

message(STATUS "GFLAGS_HOME: ${GFLAGS_HOME}")
find_path(GFLAGS_INCLUDE_DIR NAMES gflags/gflags.h)
find_library(GFLAGS_SHARED_LIB NAMES gflags)
find_library(GFLAGS_STATIC_LIB NAMES ${GFLAGS_STATIC_LIB_NAME})

if ( _gflags_roots )
find_path(GFLAGS_INCLUDE_DIR NAMES gflags/gflags.h
PATHS ${_gflags_roots}
NO_DEFAULT_PATH
PATH_SUFFIXES "include" )
find_library(GFLAGS_STATIC_LIB NAMES ${GFLAGS_STATIC_LIB_NAME}
PATHS ${_gflags_roots}
NO_DEFAULT_PATH
PATH_SUFFIXES "lib" )
find_library(GFLAGS_SHARED_LIB NAMES gflags
PATHS ${_gflags_roots}
NO_DEFAULT_PATH
PATH_SUFFIXES "lib" )
if(GFLAGS_INCLUDE_DIR)
message(STATUS "GFlags include dir: ${GFLAGS_INCLUDE_DIR}")
if(GFLAGS_SHARED_LIB)
message(STATUS "GFlags shared library: ${GFLAGS_SHARED_LIB}")
set(GFLAGS_SHARED TRUE)
else()
set(GFLAGS_SHARED FALSE)
endif()
if(GFLAGS_STATIC_LIB)
message(STATUS "GFlags static library: ${GFLAGS_STATIC_LIB}")
set(GFLAGS_STATIC TRUE)
else()
set(GFLAGS_STATIC FALSE)
endif()
if(GFLAGS_SHARED OR GFLAGS_STATIC)
set(GFLAGS_FOUND TRUE)
if(MSVC)
set(GFLAGS_MSVC_DEPS "shlwapi.lib")
endif()
add_thirdparty_lib(gflags
SHARED_LIB
"${GFLAGS_SHARED_LIB}"
STATIC_LIB
"${GFLAGS_STATIC_LIB}"
INCLUDE_DIRECTORIES
"${GFLAGS_INCLUDE_DIR}"
DEPS
"${GFLAGS_MSVC_DEPS}")
if(GFLAGS_SHARED)
target_compile_definitions(gflags_shared INTERFACE "GFLAGS_IS_A_DLL=1")
endif()
if(GFLAGS_STATIC)
target_compile_definitions(gflags_static INTERFACE "GFLAGS_IS_A_DLL=0")
endif()
else()
set(GFLAGS_FOUND FALSE)
endif()
else()
find_path(GFLAGS_INCLUDE_DIR gflags/gflags.h
# make sure we don't accidentally pick up a different version
NO_CMAKE_SYSTEM_PATH
NO_SYSTEM_ENVIRONMENT_PATH)
find_library(GFLAGS_STATIC_LIB ${GFLAGS_STATIC_LIB_NAME}
NO_CMAKE_SYSTEM_PATH
NO_SYSTEM_ENVIRONMENT_PATH)
find_library(GFLAGS_SHARED_LIB gflags
NO_CMAKE_SYSTEM_PATH
NO_SYSTEM_ENVIRONMENT_PATH)
endif()

if (GFLAGS_INCLUDE_DIR AND GFLAGS_STATIC_LIB)
set(GFLAGS_FOUND TRUE)
else ()
set(GFLAGS_FOUND FALSE)
endif ()
endif()

mark_as_advanced(
GFLAGS_INCLUDE_DIR
GFLAGS_STATIC_LIB
)
mark_as_advanced(GFLAGS_INCLUDE_DIR GFLAGS_SHARED_LIB GFLAGS_STATIC_LIB)
46 changes: 27 additions & 19 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,19 @@ endif()

if(ARROW_NEED_GFLAGS)
# gflags (formerly Googleflags) command line parsing
if("${GFLAGS_HOME}" STREQUAL "")
find_package(GFlags)
if(GFLAGS_FOUND)
set(GFLAGS_VENDORED FALSE)
get_filename_component(GFLAGS_HOME "${GFLAGS_INCLUDE_DIR}" DIRECTORY)
if(ARROW_GFLAGS_USE_SHARED AND GFLAGS_SHARED)
set(GFLAGS_LIBRARY gflags_shared)
else()
set(GFLAGS_LIBRARY gflags_static)
endif()
elseif(GFLAGS_HOME)
message(FATAL_ERROR "No static or shared library provided for gflags: ${GFLAGS_HOME}")
else()
set(GFLAGS_VENDORED TRUE)
set(GFLAGS_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})

set(GFLAGS_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/gflags_ep-prefix/src/gflags_ep")
Expand Down Expand Up @@ -634,24 +646,20 @@ if(ARROW_NEED_GFLAGS)
CMAKE_ARGS ${GFLAGS_CMAKE_ARGS})

add_dependencies(toolchain gflags_ep)
else()
set(GFLAGS_VENDORED 0)
find_package(GFlags REQUIRED)
endif()

message(STATUS "GFlags include dir: ${GFLAGS_INCLUDE_DIR}")
message(STATUS "GFlags static library: ${GFLAGS_STATIC_LIB}")
include_directories(SYSTEM ${GFLAGS_INCLUDE_DIR})
ADD_THIRDPARTY_LIB(gflags
STATIC_LIB ${GFLAGS_STATIC_LIB})
if(MSVC)
set_target_properties(gflags_static
PROPERTIES
INTERFACE_LINK_LIBRARIES "shlwapi.lib")
endif()

if(GFLAGS_VENDORED)
add_dependencies(gflags_static gflags_ep)
message(STATUS "GFlags include dir: ${GFLAGS_INCLUDE_DIR}")
message(STATUS "GFlags static library: ${GFLAGS_STATIC_LIB}")
include_directories(SYSTEM ${GFLAGS_INCLUDE_DIR})
ADD_THIRDPARTY_LIB(gflags
STATIC_LIB ${GFLAGS_STATIC_LIB})
set(GFLAGS_LIBRARY gflags_static)
target_compile_definitions(${GFLAGS_LIBRARY} INTERFACE "GFLAGS_IS_A_DLL=0")
if(MSVC)
set_target_properties(${GFLAGS_LIBRARY}
PROPERTIES
INTERFACE_LINK_LIBRARIES "shlwapi.lib")
endif()
add_dependencies(${GFLAGS_LIBRARY} gflags_ep)
endif()
endif()

Expand Down Expand Up @@ -1757,6 +1765,6 @@ if (ARROW_USE_GLOG)
else()
ADD_THIRDPARTY_LIB(glog
STATIC_LIB ${GLOG_STATIC_LIB}
DEPS gflags_static)
DEPS ${GFLAGS_LIBRARY})
endif()
endif()
12 changes: 6 additions & 6 deletions cpp/src/arrow/flight/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,16 +112,16 @@ add_arrow_test(flight-test
# Build test server for unit tests or benchmarks
if(ARROW_BUILD_TESTS OR ARROW_BUILD_BENCHMARKS)
add_executable(flight-test-server test-server.cc)
target_link_libraries(flight-test-server ${ARROW_FLIGHT_TEST_LINK_LIBS} gflags_static
${GTEST_LIBRARY})
target_link_libraries(flight-test-server ${ARROW_FLIGHT_TEST_LINK_LIBS}
${GFLAGS_LIBRARY} ${GTEST_LIBRARY})

add_executable(flight-test-integration-server test-integration-server.cc)
target_link_libraries(flight-test-integration-server ${ARROW_FLIGHT_TEST_LINK_LIBS}
gflags_static gtest_static)
${GFLAGS_LIBRARY} ${GTEST_LIBRARY})

add_executable(flight-test-integration-client test-integration-client.cc)
target_link_libraries(flight-test-integration-client ${ARROW_FLIGHT_TEST_LINK_LIBS}
gflags_static gtest_static)
${GFLAGS_LIBRARY} ${GTEST_LIBRARY})

# This is needed for the unit tests
if(ARROW_BUILD_TESTS)
Expand All @@ -144,15 +144,15 @@ if(ARROW_BUILD_BENCHMARKS)
arrow_flight_shared
arrow_flight_testing_shared
${ARROW_FLIGHT_TEST_LINK_LIBS}
gflags_static
${GFLAGS_LIBRARY}
${GTEST_LIBRARY})

add_executable(flight-benchmark flight-benchmark.cc perf.pb.cc)
target_link_libraries(flight-benchmark
arrow_flight_static
arrow_testing_static
${ARROW_FLIGHT_TEST_LINK_LIBS}
gflags_static
${GFLAGS_LIBRARY}
${GTEST_LIBRARY})

add_dependencies(flight-benchmark flight-perf-server)
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/ipc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ add_arrow_test(json-simple-test PREFIX "arrow-ipc")
add_arrow_test(json-test PREFIX "arrow-ipc")

if(NOT ARROW_BOOST_HEADER_ONLY)
add_arrow_test(json-integration-test EXTRA_LINK_LIBS gflags_static)
add_arrow_test(json-integration-test EXTRA_LINK_LIBS ${GFLAGS_LIBRARY})

# Test is being built
if(TARGET arrow-json-integration-test)
Expand Down
5 changes: 0 additions & 5 deletions cpp/src/arrow/ipc/json-integration-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,6 @@
// specific language governing permissions and limitations
// under the License.

// ARROW-3837: Depending on how gflags was built, this might be set to
// 1 or 0 by default, making it so that we cannot statically link
// gflags on Windows if it is set to 1
#define GFLAGS_IS_A_DLL 0

#include <cstdint>
#include <cstdio>
#include <cstring>
Expand Down
1 change: 1 addition & 0 deletions run-cmake-format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

patterns = [
'cpp/CMakeLists.txt',
'cpp/cmake_modules/FindGFlags.cmake',
'cpp/src/**/CMakeLists.txt',
'cpp/tools/**/CMakeLists.txt',
'java/gandiva/CMakeLists.txt',
Expand Down