[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

[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