Re: [PR] GH-34163: [C++][CI] Ensure using the same Zstandard with bundled ORC [arrow]

["wgtmac (via GitHub)" <gi...@apache.org>]

wgtmac commented on code in PR #34164:
URL: https://github.com/apache/arrow/pull/34164#discussion_r1573190465


##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4446,7 +4446,9 @@ macro(build_orc)
       "-DPROTOBUF_LIBRARY=${ORC_PROTOBUF_LIBRARY}"
       "-DPROTOC_LIBRARY=${ORC_PROTOBUF_LIBRARY}"
       "-DLZ4_HOME=${ORC_LZ4_ROOT}"
-      "-DZSTD_HOME=${ORZ_ZSTD_ROOT}")
+      "-DZSTD_HOME=${ORC_ZSTD_ROOT}"
+      "-DZSTD_INCLUDE_DIR=$<TARGET_PROPERTY:${ARROW_ZSTD_LIBZSTD},INTERFACE_INCLUDE_DIRECTORIES>"

Review Comment:
   I don't quite understand why we have to define `ZSTD_INCLUDE_DIR` and `ZSTD_LIBRARY`? Isn't it enough to define `ZSTD_HOME` for building ORC as the FindZSTD.cmake provided by ORC will set `ZSTD_INCLUDE_DIR` and `ZSTD_LIBRARY` properly.
   
   I guess these definitions are not necessary to build Apache ORC. However, I have found `list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake_modules")` in the cpp/CMakeLists.txt of Apache Arrow. Does it mean that we have overridden the module path so Apache ORC no longer finds package using the default FindXXX.cmake and these flags are missing so we have to provide on our own?
   
   Sorry for the newbie question. I'm still not a CMake expert. @kou   



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org