kou commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438396958
##########
File path: cpp/cmake_modules/FindZSTD.cmake
##########
@@ -65,4 +66,5 @@ if(ZSTD_FOUND)
set_target_properties(ZSTD::zstd
PROPERTIES IMPORTED_LOCATION "${ZSTD_LIB}"
INTERFACE_INCLUDE_DIRECTORIES
"${ZSTD_INCLUDE_DIR}")
+ set(ZSTD_LIBRARIES ZSTD::zstd)
Review comment:
I think that @tobim wants to say:
```cmake
if(ARROW_ZSTD_USE_SHARED)
add_library(ZSTD::zstd_shared ...)
else()
add_library(ZSTD::zstd_static ...)
endif
```
If we don't have this abstraction layer, we need to check
`ARROW_ZSTD_USE_SHARED` where we want to use zstd. We want to avoid it. So we
need this abstraction layer.
But we want to add this abstraction layer in
`cpp/cmake_modules/ThirdpartyToolchain.cmake` instead of `Find*.cmake` like
`ARROW_PROTOBUF_LIBPROTOC`:
```diff
diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index be6aeb038..8c05d4b84 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -676,8 +676,8 @@ if(ARROW_WITH_ZLIB)
endif()
if(ARROW_WITH_ZSTD)
- list(APPEND ARROW_STATIC_LINK_LIBS ${ZSTD_LIBRARIES})
- list(APPEND ARROW_STATIC_INSTALL_INTERFACE_LIBS ${ZSTD_LIBRARIES})
+ list(APPEND ARROW_STATIC_LINK_LIBS ${ARROW_ZSTD_LIBZSTD})
+ list(APPEND ARROW_STATIC_INSTALL_INTERFACE_LIBS ${ARROW_ZSTD_LIBZSTD})
endif()
if(ARROW_ORC)
diff --git a/cpp/cmake_modules/FindZSTD.cmake
b/cpp/cmake_modules/FindZSTD.cmake
index 91d7dd37f..6c1acaf89 100644
--- a/cpp/cmake_modules/FindZSTD.cmake
+++ b/cpp/cmake_modules/FindZSTD.cmake
@@ -66,5 +66,4 @@ if(ZSTD_FOUND)
set_target_properties(ZSTD::zstd
PROPERTIES IMPORTED_LOCATION "${ZSTD_LIB}"
INTERFACE_INCLUDE_DIRECTORIES
"${ZSTD_INCLUDE_DIR}")
- set(ZSTD_LIBRARIES ZSTD::zstd)
endif()
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake
b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 10ba6bfae..852da506b 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -1880,26 +1880,25 @@ macro(build_zstd)
add_dependencies(toolchain zstd_ep)
add_dependencies(ZSTD::zstd zstd_ep)
- set(ZSTD_LIBRARIES ZSTD::zstd)
endmacro()
if(ARROW_WITH_ZSTD)
resolve_dependency(ZSTD)
- # "SYSTEM" source will prioritize cmake config, which exports
- # zstd::libzstd_{static,shared}
- if(ARROW_ZSTD_USE_SHARED)
- if(TARGET zstd::libzstd_shared)
- set(ZSTD_LIBRARIES zstd::libzstd_shared)
- endif()
+ if(TARGET ZSTD::zstd)
+ set(ARROW_ZSTD_LIBZSTD ZSTD::zstd)
else()
- if(TARGET zstd::libzstd_static)
- set(ZSTD_LIBRARIES zstd::libzstd_static)
+ # "SYSTEM" source will prioritize cmake config, which exports
+ # zstd::libzstd_{static,shared}
+ if(ARROW_ZSTD_USE_SHARED)
+ set(ARROW_ZSTD_LIBZSTD zstd::libzstd_shared)
+ else()
+ set(ARROW_ZSTD_LIBZSTD zstd::libzstd_static)
endif()
endif()
# TODO: Don't use global includes but rather target_include_directories
- get_target_property(ZSTD_INCLUDE_DIR ${ZSTD_LIBRARIES}
INTERFACE_INCLUDE_DIRECTORIES)
+ get_target_property(ZSTD_INCLUDE_DIR ${ARROW_ZSTD_LIBZSTD}
INTERFACE_INCLUDE_DIRECTORIES)
include_directories(SYSTEM ${ZSTD_INCLUDE_DIR})
endif()
```
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1879,13 +1879,26 @@ macro(build_zstd)
add_dependencies(toolchain zstd_ep)
add_dependencies(ZSTD::zstd zstd_ep)
+ set(ZSTD_LIBRARIES ZSTD::zstd)
endmacro()
if(ARROW_WITH_ZSTD)
resolve_dependency(ZSTD)
+ # "SYSTEM" source will prioritize cmake config, which exports
+ # zstd::libzstd_{static,shared}
+ if(ARROW_ZSTD_USE_SHARED)
+ if(TARGET zstd::libzstd_shared)
+ set(ZSTD_LIBRARIES zstd::libzstd_shared)
+ endif()
+ else()
+ if(TARGET zstd::libzstd_static)
+ set(ZSTD_LIBRARIES zstd::libzstd_static)
+ endif()
+ endif()
+
# TODO: Don't use global includes but rather target_include_directories
- get_target_property(ZSTD_INCLUDE_DIR ZSTD::zstd
INTERFACE_INCLUDE_DIRECTORIES)
+ get_target_property(ZSTD_INCLUDE_DIR ${ZSTD_LIBRARIES}
INTERFACE_INCLUDE_DIRECTORIES)
Review comment:
Ah, this block will be able to be removed because `ZSTD_INCLUDE_DIR` is
already associated to the `ZSTD::zstd` target.
Could you try removing this block?
##########
File path: cpp/cmake_modules/FindZSTD.cmake
##########
@@ -65,4 +66,5 @@ if(ZSTD_FOUND)
set_target_properties(ZSTD::zstd
PROPERTIES IMPORTED_LOCATION "${ZSTD_LIB}"
INTERFACE_INCLUDE_DIRECTORIES
"${ZSTD_INCLUDE_DIR}")
+ set(ZSTD_LIBRARIES ZSTD::zstd)
Review comment:
(It's better that we rename our internal `ZSTD::zstd` target name to
`zstd::libzstd` because the official `ZSTDConfig.cmake` uses `zstd::libzstd*`.)
----------------------------------------------------------------
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:
[email protected]
