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 2020/06/09 23:11:00 UTC
[GitHub] [arrow] veprbl opened a new pull request #7388: ARROW-9084: cmake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
veprbl opened a new pull request #7388:
URL: https://github.com/apache/arrow/pull/7388
ZSTD::zstd target is expected to exist, but zstd exports
ZSTD::zstd_{shared,static}.
----------------------------------------------------------------
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
[GitHub] [arrow] veprbl commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
veprbl commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r437787687
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1879,13 +1879,19 @@ 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(TARGET zstd::libzstd_static)
+ set(ZSTD_LIBRARIES zstd::libzstd_static)
Review comment:
> Why do you choose `zstd_static` not `zstd_shared`.
I just used the same setting as is used for the vendored library:
https://github.com/apache/arrow/blob/b058cf0d1c26ad7984c104bb84322cc7dcc66f00/cpp/cmake_modules/ThirdpartyToolchain.cmake#L1840-L1841
I was not sure what implications shared linking would have when arrow-cpp is itself built as a static library, so I sticked to doing what is already being done.
> I think that we should `zstd_shared` for security reason.
> If zstd has a security problem, we just need to update zstd with `zstd_shared`. But we need to update zstd and rebuild Apache Arrow with `zstd_static`.
My use case is packaging for downstream, where the reverse dependencies (arrow-cpp) would be also rebuilt. Following your argument, one could argue that unbreaking build against non-vendored zstd is in itself a security improvement.
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#issuecomment-641650133
We need cmake-format 0.5.2: https://github.com/apache/arrow/blob/master/dev/archery/requirements-lint.txt#L3
----------------------------------------------------------------
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
[GitHub] [arrow] tobim commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
tobim commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438399869
##########
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 was thinking of something like
```cmake
if(ZSTD_FOUND)
if(ARROW_ZSTD_USE_SHARED)
set(ZSTD_TARGET zstd::libzstd_shared)
else()
set(ZSTD_TARGET zstd::libzstd_static)
endif()
add_library(${ZSTD_TARGET} UNKNOWN_IMPORTED)
set_target_properties(${ZSTD_TARGET}
PROPERTIES IMPORTED_LOCATION "${ZSTD_LIB}"
INTERFACE_INCLUDE_DIRECTORIES "${ZSTD_INCLUDE_DIR}")
endif()
```
The intention is to align the interface of `FindZSTD` with the exported targets file from upstream.
----------------------------------------------------------------
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
[GitHub] [arrow] tobim commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
tobim commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438407248
##########
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:
@kou, yes that is what I meant, it would make your suggested abstraction for `ARROW_ZSTD_LIBZSTD` a little simpler because you won't have to do an extra check for `ZSTD::zstd`.
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r439701060
##########
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:
This block is needed.
We need to set zstd dependencies explicitly.
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r437795928
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1879,13 +1879,19 @@ 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(TARGET zstd::libzstd_static)
+ set(ZSTD_LIBRARIES zstd::libzstd_static)
Review comment:
OK.
We use static library for vendored libraries and shared library for external libraries. If we use static library for external libraries, we provide an option to use static library and the option is off by default. e.g. `ARROW_BOOST_USE_SHARED` is `ON` by default: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/DefineOptions.cmake#L278
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [arrow] veprbl commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
veprbl commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438472409
##########
File path: cpp/cmake_modules/FindZSTD.cmake
##########
@@ -21,13 +21,17 @@ endif()
set(ZSTD_STATIC_LIB_SUFFIX "${ZSTD_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}")
set(ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${ZSTD_STATIC_LIB_SUFFIX})
+if(ARROW_ZSTD_USE_SHARED)
+ set(ZSTD_LIB_NAMES "${CMAKE_SHARED_LIBRARY_PREFIX}zstd${CMAKE_SHARED_LIBRARY_SUFFIX}")
+else()
+ set(ZSTD_LIB_NAMES "${ZSTD_STATIC_LIB_NAME}" "lib${ZSTD_STATIC_LIB_NAME}")
+endif()
# First, find via if specified ZTD_ROOT
if(ZSTD_ROOT)
message(STATUS "Using ZSTD_ROOT: ${ZSTD_ROOT}")
find_library(ZSTD_LIB
- NAMES zstd "${ZSTD_STATIC_LIB_NAME}" "lib${ZSTD_STATIC_LIB_NAME}"
- "${CMAKE_SHARED_LIBRARY_PREFIX}zstd${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ NAMES ${ZSTD_LIB_NAMES}
Review comment:
It's probably because I intentionally removed plain "zstd" from the library names.
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438470424
##########
File path: cpp/cmake_modules/FindZSTD.cmake
##########
@@ -21,13 +21,17 @@ endif()
set(ZSTD_STATIC_LIB_SUFFIX "${ZSTD_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}")
set(ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${ZSTD_STATIC_LIB_SUFFIX})
+if(ARROW_ZSTD_USE_SHARED)
+ set(ZSTD_LIB_NAMES "${CMAKE_SHARED_LIBRARY_PREFIX}zstd${CMAKE_SHARED_LIBRARY_SUFFIX}")
+else()
+ set(ZSTD_LIB_NAMES "${ZSTD_STATIC_LIB_NAME}" "lib${ZSTD_STATIC_LIB_NAME}")
+endif()
# First, find via if specified ZTD_ROOT
if(ZSTD_ROOT)
message(STATUS "Using ZSTD_ROOT: ${ZSTD_ROOT}")
find_library(ZSTD_LIB
- NAMES zstd "${ZSTD_STATIC_LIB_NAME}" "lib${ZSTD_STATIC_LIB_NAME}"
- "${CMAKE_SHARED_LIBRARY_PREFIX}zstd${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ NAMES ${ZSTD_LIB_NAMES}
Review comment:
`CMAKE_FIND_DEBUG_MODE` will help us:
```diff
diff --git a/ci/appveyor-cpp-build.bat b/ci/appveyor-cpp-build.bat
index a2ce07656..246029928 100644
--- a/ci/appveyor-cpp-build.bat
+++ b/ci/appveyor-cpp-build.bat
@@ -45,6 +45,7 @@ if "%JOB%" == "Build_Debug" (
-DARROW_USE_PRECOMPILED_HEADERS=OFF ^
-DARROW_VERBOSE_THIRDPARTY_BUILD=OFF ^
-DCMAKE_BUILD_TYPE="Debug" ^
+ -DCMAKE_FIND_DEBUG_MODE=ON ^
-DCMAKE_UNITY_BUILD=ON ^
.. || exit /B
```
----------------------------------------------------------------
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
[GitHub] [arrow] tobim commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
tobim commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438140636
##########
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:
Why do you introduce this extra indirection? The target could just as well be renamed to `zstd::libzstd_{shared,static}` depending on `ARROW_ZSTD_USE_SHARED`.
----------------------------------------------------------------
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
[GitHub] [arrow] veprbl commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
veprbl commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438157379
##########
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:
These are not specific to zstd, so I didn't look into 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
[GitHub] [arrow] tobim commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
tobim commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438407635
##########
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:
Yes, the comment above is also quite misleading actually.
----------------------------------------------------------------
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
[GitHub] [arrow] tobim commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
tobim commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438141186
##########
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:
It would be better to remove those 2 lines, but maybe not for this PR?
----------------------------------------------------------------
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
[GitHub] [arrow] veprbl commented on pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
veprbl commented on pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#issuecomment-641642689
@kou Thanks a lot for format fix, I was unable to address that with cmake-format 0.6.10
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438438723
##########
File path: cpp/cmake_modules/FindZSTD.cmake
##########
@@ -21,13 +21,17 @@ endif()
set(ZSTD_STATIC_LIB_SUFFIX "${ZSTD_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}")
set(ZSTD_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}zstd${ZSTD_STATIC_LIB_SUFFIX})
+if(ARROW_ZSTD_USE_SHARED)
+ set(ZSTD_LIB_NAMES "${CMAKE_SHARED_LIBRARY_PREFIX}zstd${CMAKE_SHARED_LIBRARY_SUFFIX}")
+else()
+ set(ZSTD_LIB_NAMES "${ZSTD_STATIC_LIB_NAME}" "lib${ZSTD_STATIC_LIB_NAME}")
+endif()
# First, find via if specified ZTD_ROOT
if(ZSTD_ROOT)
message(STATUS "Using ZSTD_ROOT: ${ZSTD_ROOT}")
find_library(ZSTD_LIB
- NAMES zstd "${ZSTD_STATIC_LIB_NAME}" "lib${ZSTD_STATIC_LIB_NAME}"
- "${CMAKE_SHARED_LIBRARY_PREFIX}zstd${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ NAMES ${ZSTD_LIB_NAMES}
Review comment:
Conda on Windows failed: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/33446250/job/5pyasx9g6mscq7hm
```text
-- Using ZSTD_ROOT: C:\Miniconda37-x64\envs\arrow/Library
CMake Error at C:/Miniconda37-x64/envs/arrow/Library/share/cmake-3.17/Modules/FindPackageHandleStandardArgs.cmake:164 (message):
Could NOT find ZSTD (missing: ZSTD_LIB)
Call Stack (most recent call first):
C:/Miniconda37-x64/envs/arrow/Library/share/cmake-3.17/Modules/FindPackageHandleStandardArgs.cmake:445 (_FPHSA_FAILURE_MESSAGE)
cmake_modules/FindZSTD.cmake:62 (find_package_handle_standard_args)
cmake_modules/ThirdpartyToolchain.cmake:156 (find_package)
cmake_modules/ThirdpartyToolchain.cmake:1887 (resolve_dependency)
CMakeLists.txt:463 (include)
```
Files in win-64/zstd-1.4.4-h9f78265_3.tar.bz2 at https://anaconda.org/conda-forge/zstd/files :
```text
zstd-1.4.4-h9f78265_3
|-- Library
| |-- bin
| | |-- libzstd.dll
| | |-- zstd.dll
| | `-- zstd.exe
| |-- include
| | |-- cover.h
| | |-- zbuff.h
| | |-- zdict.h
| | |-- zstd.h
| | `-- zstd_errors.h
| `-- lib
| |-- libzstd.lib
| |-- libzstd_static.lib
| |-- zstd.lib
| `-- zstd_static.lib
`-- info
|-- about.json
|-- files
|-- git
|-- hash_input.json
|-- index.json
|-- licenses
| `-- LICENSE
|-- paths.json
|-- recipe
| |-- bld.bat
| |-- build.sh
| |-- conda_build_config.yaml
| |-- meta.yaml
| `-- meta.yaml.template
|-- run_exports.json
`-- test
`-- run_test.bat
```
----------------------------------------------------------------
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
[GitHub] [arrow] veprbl commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
veprbl commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r438155204
##########
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 did not find a way to rename, `add_library(foo ALIAS bar)` doesn't work in this case.
----------------------------------------------------------------
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
[GitHub] [arrow] kou closed pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
kou closed pull request #7388:
URL: https://github.com/apache/arrow/pull/7388
----------------------------------------------------------------
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
[GitHub] [arrow] kou commented on a change in pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#discussion_r437780818
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1879,13 +1879,19 @@ 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(TARGET zstd::libzstd_static)
+ set(ZSTD_LIBRARIES zstd::libzstd_static)
Review comment:
Why do you choose `zstd_static` not `zstd_shared`.
I think that we should `zstd_shared` for security reason.
If zstd has a security problem, we just need to update zstd with `zstd_shared`. But we need to update zstd and rebuild Apache Arrow with `zstd_static`.
----------------------------------------------------------------
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
[GitHub] [arrow] veprbl commented on pull request #7388: ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
veprbl commented on pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#issuecomment-643558973
Wow. Thanks for finishing this up, @kou. I don't think I would have figured the `CMAKE_IMPORT_LIBRARY_SUFFIX` bit myself.
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #7388: ARROW-9084: cmake is unable to find zstd target when ZSTD_SOURCE=SYSTEM
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7388:
URL: https://github.com/apache/arrow/pull/7388#issuecomment-641629188
https://issues.apache.org/jira/browse/ARROW-9084
----------------------------------------------------------------
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