[GitHub] [arrow] kou commented on a diff in pull request #12861: ARROW-16168: [C++][CMake] Use target to add include paths

[GitBox <gi...@apache.org>]

kou commented on code in PR #12861:
URL: https://github.com/apache/arrow/pull/12861#discussion_r868974508


##########
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:
   Yes. Because we build `cpp/src/arrow/io_hdfs_internal.cc` unconditionally.
   
   Note that `arrow::hadoop` is a header only target that refers `cpp/thirdparty/hadoop/include/`.



##########
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:
   Yes.
   
   CMake 3.12 or later is required for object library and target: https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries
   
   > New in version 3.12: Object libraries can be linked to with [target_link_libraries()](https://cmake.org/cmake/help/latest/command/target_link_libraries.html#command:target_link_libraries).



##########
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:
   OK. I'll use `ARROW_BOOST_REQUIRED`.
   
   



##########
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:
   Yes. The `xsimd` target sets only include path. If we add the `xsimd` target to link libraries, we can add `-I...` to compiler flags.



##########
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:
   This is just copied from removed `FindBoostAlt.cmake`...
   To be honest, I don't know what should be done by this TODO. We may be able to just remove this TODO. 



-- 
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