You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/10 08:07:26 UTC
[GitHub] [arrow] pitrou commented on a diff in pull request #12861: ARROW-16168: [C++][CMake] Use target to add include paths
pitrou commented on code in PR #12861:
URL: https://github.com/apache/arrow/pull/12861#discussion_r868935053
##########
cpp/CMakeLists.txt:
##########
@@ -692,10 +692,15 @@ endif()
#
# Libraries to link statically with libarrow.so
-set(ARROW_LINK_LIBS)
-set(ARROW_STATIC_LINK_LIBS)
+set(ARROW_LINK_LIBS arrow::flatbuffers arrow::hadoop)
Review Comment:
Hmm, is `arrow::hadoop` enabled unconditionally??
##########
cpp/cmake_modules/BuildUtils.cmake:
##########
@@ -271,7 +202,9 @@ function(ADD_ARROW_LIB LIB_NAME)
set(OUTPUT_PATH ${BUILD_OUTPUT_ROOT_DIRECTORY})
endif()
- if(WIN32 OR (CMAKE_GENERATOR STREQUAL Xcode))
+ if(WIN32
+ OR (CMAKE_GENERATOR STREQUAL Xcode)
+ OR CMAKE_VERSION VERSION_LESS 3.12)
Review Comment:
Can you add a comment explaining the 3.12 version requirement here?
##########
cpp/CMakeLists.txt:
##########
@@ -808,6 +813,16 @@ if(ARROW_WITH_RE2)
endif()
endif()
+if(ARROW_WITH_RAPIDJSON)
+ list(APPEND ARROW_LINK_LIBS rapidjson::rapidjson)
+ list(APPEND ARROW_STATIC_LINK_LIBS rapidjson::rapidjson)
+endif()
+
+if(TARGET xsimd)
+ list(APPEND ARROW_LINK_LIBS xsimd)
+ list(APPEND ARROW_STATIC_LINK_LIBS xsimd)
Review Comment:
AFAIK, xsimd is header-only, is this necessary?
##########
cpp/cmake_modules/BuildUtils.cmake:
##########
@@ -59,78 +43,25 @@ function(ADD_THIRDPARTY_LIB LIB_NAME)
message(SEND_ERROR "Error: unrecognized arguments: ${ARG_UNPARSED_ARGUMENTS}")
endif()
- if(ARG_STATIC_LIB AND ARG_SHARED_LIB)
- set(AUG_LIB_NAME "${LIB_NAME}_static")
- add_library(${AUG_LIB_NAME} STATIC IMPORTED)
- set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_LOCATION
- "${ARG_STATIC_LIB}")
- if(ARG_DEPS)
- set_target_properties(${AUG_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)
-
- if(WIN32)
- # Mark the ".lib" location as part of a Windows DLL
- set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_IMPLIB
- "${ARG_SHARED_LIB}")
- else()
- set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_LOCATION
- "${ARG_SHARED_LIB}")
- endif()
- if(ARG_DEPS)
- set_target_properties(${AUG_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)
- set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_LOCATION
- "${ARG_STATIC_LIB}")
- if(ARG_DEPS)
- set_target_properties(${AUG_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)
-
Review Comment:
Nice simplification here!
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -966,31 +1002,72 @@ else()
endif()
if(ARROW_BOOST_REQUIRED)
+ if(ARROW_BOOST_USE_SHARED)
+ # Find shared Boost libraries.
+ set(Boost_USE_STATIC_LIBS OFF)
+ set(BUILD_SHARED_LIBS_KEEP ${BUILD_SHARED_LIBS})
+ set(BUILD_SHARED_LIBS ON)
+ else()
+ # Find static boost headers and libs
+ # TODO Differentiate here between release and debug builds
Review Comment:
Can you open a JIRA for this TODO?
##########
cpp/CMakeLists.txt:
##########
@@ -692,10 +692,15 @@ endif()
#
# Libraries to link statically with libarrow.so
-set(ARROW_LINK_LIBS)
-set(ARROW_STATIC_LINK_LIBS)
+set(ARROW_LINK_LIBS arrow::flatbuffers arrow::hadoop)
+set(ARROW_STATIC_LINK_LIBS arrow::flatbuffers arrow::hadoop)
set(ARROW_STATIC_INSTALL_INTERFACE_LIBS)
+if(TARGET Boost::headers)
Review Comment:
This looks a bit weird, shouldn't the condition be based on enabled Arrow components instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org