[GitHub] [arrow] kou commented on a change in pull request #7696: ARROW-7605: [C++] Create and install "dependency bundle" static library including jemalloc, mimalloc, and any BUNDLED static library so that static linking to libarrow.a is possible

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

kou commented on a change in pull request #7696:
URL: https://github.com/apache/arrow/pull/7696#discussion_r452624987



##########
File path: cpp/examples/minimal_build/CMakeLists.txt
##########
@@ -19,10 +19,31 @@ cmake_minimum_required(VERSION 3.0)
 
 project(ArrowMinimalExample)
 
+option(ARROW_LINK_SHARED "Link to the Arrow shared library" ON)
+
 find_package(Arrow REQUIRED)
 
+set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_BUILD_TYPE Release)
+
 message(STATUS "Arrow version: ${ARROW_VERSION}")
 message(STATUS "Arrow SO version: ${ARROW_FULL_SO_VERSION}")
 
 add_executable(arrow_example example.cc)
-target_link_libraries(arrow_example PRIVATE arrow_shared)
+
+if (ARROW_LINK_SHARED)
+  target_link_libraries(arrow_example PRIVATE arrow_shared)
+else()
+  set(THREADS_PREFER_PTHREAD_FLAG ON)
+  find_package(Threads REQUIRED)
+
+  # We must also link to libarrow_bundled_dependencies.a
+  get_property(arrow_static_loc TARGET arrow_static PROPERTY LOCATION)
+  get_filename_component(arrow_lib_dir ${arrow_static_loc} DIRECTORY)
+
+  add_library(arrow_dependencies STATIC IMPORTED)
+  set_target_properties(arrow_dependencies
+    PROPERTIES IMPORTED_LOCATION
+    "${arrow_lib_dir}/${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}")

Review comment:
       Could you put this to `cpp/src/arrow/ArrowConfig.cmake.in`?
   Then users don't need to put this to their `CMakeLists.txt`.
   
   ```diff
   diff --git a/cpp/src/arrow/ArrowConfig.cmake.in b/cpp/src/arrow/ArrowConfig.cmake.in
   index 0e595066d..ccbc3d110 100644
   --- a/cpp/src/arrow/ArrowConfig.cmake.in
   +++ b/cpp/src/arrow/ArrowConfig.cmake.in
   @@ -40,4 +40,14 @@ include("${CMAKE_CURRENT_LIST_DIR}/ArrowOptions.cmake")
    # already existent target error.
    if(NOT (TARGET arrow_shared OR TARGET arrow_static))
      include("${CMAKE_CURRENT_LIST_DIR}/ArrowTargets.cmake")
   +
   +  if(TARGET arrow_static)
   +    get_property(arrow_static_loc TARGET arrow_static PROPERTY LOCATION)
   +    get_filename_component(arrow_lib_dir ${arrow_static_loc} DIRECTORY)
   +
   +    add_library(arrow_bundled_dependencies STATIC IMPORTED)
   +    set_target_properties(arrow_bundled_dependencies
   +      PROPERTIES IMPORTED_LOCATION
   +      "${arrow_lib_dir}/${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}")
   +  endif()
    endif()
   ```
   
   If we append `arrow_dependencies` (`arrow_bundled_dependencies` will be better) to `arrow_static`'s `INTERFACE_LINK_LIBRARIES`, users may not need to care about bundled dependencies. (I didn't test it. Sorry.):
   
   ```diff
   diff --git a/cpp/src/arrow/ArrowConfig.cmake.in b/cpp/src/arrow/ArrowConfig.cmake.in
   index 0e595066d..a2dac690f 100644
   --- a/cpp/src/arrow/ArrowConfig.cmake.in
   +++ b/cpp/src/arrow/ArrowConfig.cmake.in
   @@ -40,4 +40,18 @@ include("${CMAKE_CURRENT_LIST_DIR}/ArrowOptions.cmake")
    # already existent target error.
    if(NOT (TARGET arrow_shared OR TARGET arrow_static))
      include("${CMAKE_CURRENT_LIST_DIR}/ArrowTargets.cmake")
   +
   +  if(TARGET arrow_static)
   +    get_property(arrow_static_loc TARGET arrow_static PROPERTY LOCATION)
   +    get_filename_component(arrow_lib_dir ${arrow_static_loc} DIRECTORY)
   +
   +    add_library(arrow_bundled_dependencies STATIC IMPORTED)
   +    set_target_properties(arrow_bundled_dependencies
   +      PROPERTIES IMPORTED_LOCATION
   +      "${arrow_lib_dir}/${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}")
   +
   +    get_property(arrow_static_interface_link_libraries TARGET arrow_static PROPERTY INTERFACE_LINK_LIBRARIES)
   +    set_target_properties(arrow_static PROPERTEIS
   +      INTERFACE_ILINK_LIBRARIES "${arrow_static_interface_link_libraries};arrow_bundled_dependencies")
   +  endif()
    endif()
   ```

##########
File path: cpp/examples/minimal_build/Dockerfile
##########
@@ -22,5 +22,5 @@ ENV DEBIAN_FRONTEND=noninteractive
 RUN apt-get update -y -q && \
     apt-get install -y -q --no-install-recommends \
       build-essential \
-      cmake && \
+      cmake &&\

Review comment:
       Could you revert this?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org