kou commented on code in PR #2437:
URL: https://github.com/apache/orc/pull/2437#discussion_r2423902188
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -172,6 +172,12 @@ elseif (ORC_PACKAGE_KIND STREQUAL "vcpkg")
list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::libprotobuf>")
set (PROTOBUF_EXECUTABLE protobuf::protoc)
+elseif (TARGET ${ARROW_PROTOBUF_LIBPROTOBUF})
+ # Used by Apache Arrow only, remove this once Arrow leverages FetchContent
for Protobuf
+ add_library (orc_protobuf INTERFACE IMPORTED)
+ target_link_libraries(orc_protobuf INTERFACE ${ARROW_PROTOBUF_LIBPROTOBUF})
+ set (PROTOBUF_EXECUTABLE ${ARROW_PROTOBUF_PROTOC})
+ message(STATUS "Using existing ${ARROW_PROTOBUF_LIBPROTOBUF}")
Review Comment:
How about using the standard target names instead of `ARROW_PROTOBUF_*` so
that other downstream projects can use Apache ORC by `FetchContent`?
```suggestion
elseif (TARGET protobuf::libprotobuf AND TARGET protobuf::protoc)
# Used for FetchContent by downstream projects
add_library (orc_protobuf INTERFACE IMPORTED)
target_link_libraries(orc_protobuf INTERFACE protobuf::libprotobuf)
set (PROTOBUF_EXECUTABLE protobuf::protoc)
message(STATUS "Using existing protobuf::libprotobuf and protobuf::protoc")
```
Apache Arrow can provide `protobuf::libprotobuf`/`protobuf::protoc` targets
by `add_library(ALIAS)`.
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -275,6 +278,16 @@ elseif (NOT "${SNAPPY_HOME}" STREQUAL "")
list (APPEND ORC_SYSTEM_DEPENDENCIES SnappyAlt)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:Snappy::snappy>")
orc_provide_find_module (SnappyAlt)
+elseif (TARGET Snappy::snappy)
+ # Used by Apache Arrow only, remove this once Arrow leverages FetchContent
for Snappy
Review Comment:
`Snappy::snappy` is the standard target name provided by Snappy. So all
downstream projects can use this by `FetchContent`:
```suggestion
# Used for FetchContent by downstream projects
```
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -275,6 +278,16 @@ elseif (NOT "${SNAPPY_HOME}" STREQUAL "")
list (APPEND ORC_SYSTEM_DEPENDENCIES SnappyAlt)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:Snappy::snappy>")
orc_provide_find_module (SnappyAlt)
+elseif (TARGET Snappy::snappy)
+ # Used by Apache Arrow only, remove this once Arrow leverages FetchContent
for Snappy
+ add_library (orc_snappy INTERFACE IMPORTED)
+ target_link_libraries(orc_snappy INTERFACE Snappy::snappy)
+ message(STATUS "Using existing Snappy::snappy")
+elseif (TARGET Snappy::snappy-static)
Review Comment:
`find_package(Snappy)` may provide both of `Snappy::snappy` and
`Snappy::snappy-static`. Apache Arrow wants to control which is used. (Apache
Arrow uses ``ARROW_SNAPPY_USE_SHARED` for it.)
Can Apache ORC provide a CMake variable to control it? For example:
```cmake
elseif (ORC_SNAPPY_USE_SHARED AND TARGET Snappy::snappy)
...
elseif (NOT ORC_SNAPPY_USE_SHARED AND TARGET Snappy::snappy-static)
...
else ()
```
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -275,6 +278,16 @@ elseif (NOT "${SNAPPY_HOME}" STREQUAL "")
list (APPEND ORC_SYSTEM_DEPENDENCIES SnappyAlt)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:Snappy::snappy>")
orc_provide_find_module (SnappyAlt)
+elseif (TARGET Snappy::snappy)
+ # Used by Apache Arrow only, remove this once Arrow leverages FetchContent
for Snappy
+ add_library (orc_snappy INTERFACE IMPORTED)
+ target_link_libraries(orc_snappy INTERFACE Snappy::snappy)
+ message(STATUS "Using existing Snappy::snappy")
+elseif (TARGET Snappy::snappy-static)
+ # Used by Apache Arrow only, remove this once Arrow leverages FetchContent
for Snappy
Review Comment:
```suggestion
# Used for FetchContent by downstream projects
```
--
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: issues-unsubscr...@orc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
