Skip to content

ORC-2013: [C++] Better CMake integration with Apache Arrow #2437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −7
Open

ORC-2013: [C++] Better CMake integration with Apache Arrow #2437

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions cmake_modules/ThirdpartyToolchain.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Comment on lines +175 to +180
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using the standard target names instead of ARROW_PROTOBUF_* so that other downstream projects can use Apache ORC by FetchContent?

Suggested change
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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

elseif (NOT "${PROTOBUF_HOME}" STREQUAL "")
find_package (ProtobufAlt REQUIRED)

Expand All @@ -181,12 +187,6 @@ elseif (NOT "${PROTOBUF_HOME}" STREQUAL "")
orc_add_resolved_library (orc_protobuf ${PROTOBUF_LIBRARY} ${PROTOBUF_INCLUDE_DIR})
endif ()

if (ORC_PREFER_STATIC_PROTOBUF AND PROTOC_STATIC_LIB)
orc_add_resolved_library (orc_protoc ${PROTOC_STATIC_LIB} ${PROTOBUF_INCLUDE_DIR})
else ()
orc_add_resolved_library (orc_protoc ${PROTOC_LIBRARY} ${PROTOBUF_INCLUDE_DIR})
endif ()

list (APPEND ORC_SYSTEM_DEPENDENCIES ProtobufAlt)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:protobuf::libprotobuf>")
orc_provide_find_module (ProtobufAlt)
Expand Down Expand Up @@ -245,7 +245,10 @@ else ()

add_library(orc_protobuf INTERFACE IMPORTED)
target_link_libraries(orc_protobuf INTERFACE protobuf::libprotobuf)
set(PROTOBUF_EXECUTABLE protobuf::protoc)
# Sometimes downstream environments (e.g. conda) need to override PROTOBUF_EXECUTABLE
if(NOT PROTOBUF_EXECUTABLE)
set(PROTOBUF_EXECUTABLE protobuf::protoc)
endif()
endblock()
endif ()

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snappy::snappy is the standard target name provided by Snappy. So all downstream projects can use this by FetchContent:

Suggested change
# Used by Apache Arrow only, remove this once Arrow leverages FetchContent for Snappy
# Used for FetchContent by downstream projects

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)
Comment on lines +281 to +286
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ()

# Used by Apache Arrow only, remove this once Arrow leverages FetchContent for Snappy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Used by Apache Arrow only, remove this once Arrow leverages FetchContent for Snappy
# Used for FetchContent by downstream projects

add_library (orc_snappy INTERFACE IMPORTED)
target_link_libraries(orc_snappy INTERFACE Snappy::snappy-static)
message(STATUS "Using existing Snappy::snappy-static")
else ()
block(PROPAGATE ORC_SYSTEM_DEPENDENCIES ORC_INSTALL_INTERFACE_TARGETS)
prepare_fetchcontent()
Expand Down Expand Up @@ -357,6 +370,11 @@ elseif (NOT "${ZLIB_HOME}" STREQUAL "")
list (APPEND ORC_SYSTEM_DEPENDENCIES ZLIBAlt)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:ZLIB::ZLIB>")
orc_provide_find_module (ZLIBAlt)
elseif (TARGET ZLIB::ZLIB)
# Used by Apache Arrow only, remove this once Arrow leverages FetchContent for ZLIB
add_library (orc_zlib INTERFACE IMPORTED)
target_link_libraries(orc_zlib INTERFACE ZLIB::ZLIB)
message(STATUS "Using existing ZLIB::ZLIB")
else ()
block(PROPAGATE ORC_SYSTEM_DEPENDENCIES ORC_INSTALL_INTERFACE_TARGETS ZLIB_LIBRARIES ZLIB_INCLUDE_DIRS)
prepare_fetchcontent()
Expand Down Expand Up @@ -448,6 +466,16 @@ elseif (NOT "${ZSTD_HOME}" STREQUAL "")
endif ()
list (APPEND ORC_SYSTEM_DEPENDENCIES ZSTDAlt)
orc_provide_find_module (ZSTDAlt)
elseif (TARGET zstd::libzstd_shared)
# Used by Apache Arrow only, remove this once Arrow leverages FetchContent for zstd
add_library (orc_zstd INTERFACE IMPORTED)
target_link_libraries(orc_zstd INTERFACE zstd::libzstd_shared)
message(STATUS "Using existing zstd::libzstd_shared")
elseif (TARGET zstd::libzstd_static)
# Used by Apache Arrow only, remove this once Arrow leverages FetchContent for zstd
add_library (orc_zstd INTERFACE IMPORTED)
target_link_libraries(orc_zstd INTERFACE zstd::libzstd_static)
message(STATUS "Using existing zstd::libzstd_static")
else ()
block(PROPAGATE ORC_SYSTEM_DEPENDENCIES ORC_INSTALL_INTERFACE_TARGETS)
prepare_fetchcontent()
Expand Down
Loading