ORC-2013: [C++] Better CMake integration with Apache Arrow #2437
Conversation
|
@kou What do you think of this change? I've tested Apache Arrow in apache/arrow#47792 |
wgtmac
changed the title
| 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}") |
There was a problem hiding this comment.
How about using the standard target names instead of ARROW_PROTOBUF_* so that other downstream projects can use Apache ORC by FetchContent?
| 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}") | |
| 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).
There was a problem hiding this comment.
I was supposed to add this as a temporary solution for Apache Arrow. Apache Arrow has already migrated lz4 to use FetchContent so I don't need to do anything and FetchContent_Declare in Apache ORC can automatically find it. I assume Apache Arrow will eventually migrate all dependencies to use FetchContent, am I correct?
There was a problem hiding this comment.
Ah, I haven't used FetchContent_Declare(FIND_PACKAGE_ARGS) yet. Sorry.
If Apache Arrow migrates Protobuf, Snappy, zlib and Zstandard to FetchContent, we don't need this in Apache ORC, right? If so, let's improve Apache Arrow.
There was a problem hiding this comment.
As I have successfully migrated dependencies in the Apache ORC to use FetchContent with FIND_PACKAGE_ARGS, I think Snappy and Zstandard should be pretty easy to migrate. It is a little bit tricky for Protobuf to find protoc. ZLIB is more messy since it does not have a release with good CMake support so I am currently using an unreleased git tag. I can work on this in the Apache Arrow one by one but may need your help :)
There was a problem hiding this comment.
Sure. Let's work on this together. I'll also work on this one by one in a few weeks. (I'll fix 22.0.0 release blockers before this.)
| 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 |
There was a problem hiding this comment.
Snappy::snappy is the standard target name provided by Snappy. So all downstream projects can use this by FetchContent:
| # Used by Apache Arrow only, remove this once Arrow leverages FetchContent for Snappy | |
| # Used for FetchContent by downstream projects |
| 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 |
There was a problem hiding this comment.
| # Used by Apache Arrow only, remove this once Arrow leverages FetchContent for Snappy | |
| # Used for FetchContent by downstream projects |
| 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) |
There was a problem hiding this 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:
elseif (ORC_SNAPPY_USE_SHARED AND TARGET Snappy::snappy)
...
elseif (NOT ORC_SNAPPY_USE_SHARED AND TARGET Snappy::snappy-static)
...
else ()2 participants
What changes were proposed in this pull request?
To make it easier for Apache Arrow to integrate with Apache ORC.
Why are the changes needed?
Protobuf,ZLIB,Snappy, andzstd.lz4does not need this change because Apache Arrow has already used FetchContent for it.How was this patch tested?
Test it locally against Apache Arrow main branch.
Was this patch authored or co-authored using generative AI tooling?
No.