ARROW-9084: [C++] CMake is unable to find zstd target when ZSTD_SOURCE=SYSTEM #7388
Conversation
There was a problem hiding this 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.
There was a problem hiding this comment.
Why do you choose
zstd_staticnotzstd_shared.
I just used the same setting as is used for the vendored library:
arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake
Lines 1840 to 1841 in b058cf0
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_sharedfor security reason.
If zstd has a security problem, we just need to update zstd withzstd_shared. But we need to update zstd and rebuild Apache Arrow withzstd_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.
There was a problem hiding this 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
|
@kou Thanks a lot for format fix, I was unable to address that with cmake-format 0.6.10 |
|
We need cmake-format 0.5.2: https://github.com/apache/arrow/blob/master/dev/archery/requirements-lint.txt#L3 |
cpp/cmake_modules/FindZSTD.cmake
Outdated
There was a problem hiding this 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.
There was a problem hiding this comment.
I did not find a way to rename, add_library(foo ALIAS bar) doesn't work in this case.
There was a problem hiding this comment.
I think that @tobim wants to say:
if(ARROW_ZSTD_USE_SHARED)
add_library(ZSTD::zstd_shared ...)
else()
add_library(ZSTD::zstd_static ...)
endifIf 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 --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()There was a problem hiding this comment.
(It's better that we rename our internal ZSTD::zstd target name to zstd::libzstd because the official ZSTDConfig.cmake uses zstd::libzstd*.)
There was a problem hiding this comment.
I was thinking of something like
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.
There was a problem hiding this 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.
kou
left a comment
There was a problem hiding this comment.
Thanks.
It almost looks good. I'll merge this once the current comments are resolved.
cpp/cmake_modules/FindZSTD.cmake
Outdated
There was a problem hiding this comment.
I think that @tobim wants to say:
if(ARROW_ZSTD_USE_SHARED)
add_library(ZSTD::zstd_shared ...)
else()
add_library(ZSTD::zstd_static ...)
endifIf 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 --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()
cpp/cmake_modules/FindZSTD.cmake
Outdated
There was a problem hiding this comment.
(It's better that we rename our internal ZSTD::zstd target name to zstd::libzstd because the official ZSTDConfig.cmake uses zstd::libzstd*.)
cpp/cmake_modules/FindZSTD.cmake
Outdated
There was a problem hiding this comment.
Conda on Windows failed: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/33446250/job/5pyasx9g6mscq7hm
-- 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 :
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
There was a problem hiding this comment.
CMAKE_FIND_DEBUG_MODE will help us:
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
There was a problem hiding this comment.
It's probably because I intentionally removed plain "zstd" from the library names.
ZSTD::zstd target is expected to exist, but zstd exports
ZSTD::zstd_{shared,static}.
|
Wow. Thanks for finishing this up, @kou. I don't think I would have figured the |
ZSTD::zstd target is expected to exist, but zstd exports
ZSTD::zstd_{shared,static}.