[GitHub] [arrow] kou commented on a change in pull request #7842: ARROW-9412: [C++] Add non-bundled dependencies to INTERFACE_LINK_LIBRARIES of static libarrow

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

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



##########
File path: cpp/src/arrow/ArrowConfig.cmake.in
##########
@@ -38,45 +38,42 @@ include("${CMAKE_CURRENT_LIST_DIR}/ArrowOptions.cmake")
 
 include(CMakeFindDependencyMacro)
 
-if(ARROW_USE_STATIC)
-  set(LIB_PATH_SUFFIXES
-      "${CMAKE_LIBRARY_ARCHITECTURE}"
-      "lib/${CMAKE_LIBRARY_ARCHITECTURE}"
-      "lib64"
-      "lib32"
-      "lib"
-      "bin"
-      "Library"
-      "Library/lib"
-      "Library/bin")
-  set(INCLUDE_PATH_SUFFIXES "include" "Library" "Library/include")
-
-  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
-  set(THREADS_PREFER_PTHREAD_FLAG TRUE)
-  find_dependency(Threads)
-
-  if(DEFINED CMAKE_MODULE_PATH)
-    set(_CMAKE_MODULE_PATH_OLD ${CMAKE_MODULE_PATH})
-  endif()
-  set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}")
-
-@ARROW_FIND_DEPENDENCY_LIST@
-
-  if(DEFINED _CMAKE_MODULE_PATH_OLD)
-    set(CMAKE_MODULE_PATH ${_CMAKE_MODULE_PATH_OLD})
-    unset(_CMAKE_MODULE_PATH_OLD)
-  else()
-    unset(CMAKE_MODULE_PATH)
-  endif()
-endif()
-
-
 # Load targets only once. If we load targets multiple times, CMake reports
 # already existent target error.
 if(NOT (TARGET arrow_shared OR TARGET arrow_static))
   include("${CMAKE_CURRENT_LIST_DIR}/ArrowTargets.cmake")
 
   if(TARGET arrow_static)
+    set(LIB_PATH_SUFFIXES

Review comment:
       Yes. Is it strange?
   Do you know a CMake convention when a CMake package provides both shard and static libraries?
   Do users need to specify "we want to use static library" explicitly?
   
   Should we use the `COMPONENTS` feature in `find_package`?

##########
File path: cpp/cmake_modules/FindLz4.cmake
##########
@@ -24,13 +24,14 @@ set(LZ4_STATIC_LIB_SUFFIX "${LZ4_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_S
 set(LZ4_LIB_NAMES lz4 liblz4)
 
 if(NOT ARROW_LZ4_USE_SHARED)
+  set(static_names_)

Review comment:
       If someone defines `static_name_` before calling `FindLz4.cmake`, the `static_name_` value is used.
   Is it wrong?

##########
File path: cpp/cmake_modules/Findzstd.cmake
##########
@@ -54,10 +54,11 @@ else()
   if(ZSTD_PC_FOUND)
     set(ZSTD_INCLUDE_DIR "${ZSTD_PC_INCLUDEDIR}")
 
-    if(NOT ARROW_ZSTD_USE_SHARED)
+    if(ARROW_ZSTD_USE_SHARED)
+      list(APPEND ZSTD_PC_LIBRARY_DIRS "${ZSTD_PC_LIBDIR}")
+    else()

Review comment:
       Oh...
   Could you show one of the libzstd.pc versions environment? I want to try it.




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