-
Notifications
You must be signed in to change notification settings - Fork 499
ORC-2013: [C++] Bump to CMake 3.25+ to use FetchContent #2416
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.
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
base: main
Are you sure you want to change the base?
Conversation
92abe65
to
fd72130
Compare
fd72130
to
99fbd29
Compare
a211c31
to
f08a5b2
Compare
This PR is ready for review. The two failed CIs are due to outdated CMake versions from For now Can conda leverage improvement from this PR, or is there any additional change that is required to add? @h-vetinari @WillAyd |
list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:protobuf::libprotobuf>") | ||
orc_provide_find_module (ProtobufAlt) | ||
else () | ||
prepare_fetchcontent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we call this in top-level, changed variables in this context are visible after this clause.
How about using function()
or block()
?
function(build_protobuf)
prepare_fetchcontent()
...
endfunction()
build_protobuf()
https://cmake.org/cmake/help/latest/command/block.html
block()
prepare_fetchcontent()
...
endblock()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! I've switched to use blocks.
elseif (NOT "${SNAPPY_HOME}" STREQUAL "") | ||
find_package (Snappy REQUIRED) | ||
find_package (SnappyAlt REQUIRED) | ||
if (ORC_PREFER_STATIC_SNAPPY AND SNAPPY_STATIC_LIB) | ||
orc_add_resolved_library (orc_snappy ${SNAPPY_STATIC_LIB} ${SNAPPY_INCLUDE_DIR}) | ||
else () | ||
orc_add_resolved_library (orc_snappy ${SNAPPY_LIBRARY} ${SNAPPY_INCLUDE_DIR}) | ||
endif () | ||
list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy) | ||
list (APPEND ORC_SYSTEM_DEPENDENCIES SnappyAlt) | ||
list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:Snappy::snappy>") | ||
orc_provide_find_module (Snappy) | ||
orc_provide_find_module (SnappyAlt) | ||
else () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may be able to remove SnappyAlt
entirely by setting Snappy_ROOT
:
elseif (NOT "${SNAPPY_HOME}" STREQUAL "") | |
find_package (Snappy REQUIRED) | |
find_package (SnappyAlt REQUIRED) | |
if (ORC_PREFER_STATIC_SNAPPY AND SNAPPY_STATIC_LIB) | |
orc_add_resolved_library (orc_snappy ${SNAPPY_STATIC_LIB} ${SNAPPY_INCLUDE_DIR}) | |
else () | |
orc_add_resolved_library (orc_snappy ${SNAPPY_LIBRARY} ${SNAPPY_INCLUDE_DIR}) | |
endif () | |
list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy) | |
list (APPEND ORC_SYSTEM_DEPENDENCIES SnappyAlt) | |
list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:Snappy::snappy>") | |
orc_provide_find_module (Snappy) | |
orc_provide_find_module (SnappyAlt) | |
else () | |
else () | |
if (NOT "${SNAPPY_HOME}" STREQUAL "" AND "${Snappy_ROOT}" STREQUAL "") | |
set(Snappy_ROOT "${SNAPPY_HOME}") | |
endif() |
See also: https://cmake.org/cmake/help/latest/variable/PackageName_ROOT.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it break arrow-cpp (where SNAPPY_LIBRARY
and SNAPPY_INCLUDE_DIR
are explicitly provided)?
Update: I've tried to apply this change with or without CONFIG
in the FIND_PACKAGE_ARGS. In both cases, system snappy is used instead of arrow vendored one.
-- Apache ORC: Found the Protobuf headers: /Users/gangwu/Projects/arrow/cpp/orc_build/protobuf_ep-install/include
-- Apache ORC: Found the Protobuf library: arrow::protobuf::libprotobuf
-- Apache ORC: Found the Protoc library: arrow::protobuf::libprotoc
-- Apache ORC: Found the Protoc executable: arrow::protobuf::protoc
-- Apache ORC: Found the Protobuf static library: /Users/gangwu/Projects/arrow/cpp/orc_build/protobuf_ep-install/lib/libprotobuf.a
-- Apache ORC: Found the Protoc static library: /Users/gangwu/Projects/arrow/cpp/orc_build/protobuf_ep-install/lib/libprotoc.a
-- Apache ORC: Providing CMake module for FindProtobufAlt as part of CMake package
-- Apache ORC: Using system snappy <==================
-- Apache ORC: ZLIB_HOME: /Users/gangwu/Projects/arrow/cpp/orc_build/zlib_ep/src/zlib_ep-install
-- Apache ORC: Found the ZLIB header: /Users/gangwu/Projects/arrow/cpp/orc_build/zlib_ep/src/zlib_ep-install/include/zlib.h
-- Apache ORC: Found the ZLIB library: ZLIB::ZLIB
-- Apache ORC: Found the ZLIB static library: /Users/gangwu/Projects/arrow/cpp/orc_build/zlib_ep/src/zlib_ep-install/lib/libz.a
-- Apache ORC: Providing CMake module for FindZLIBAlt as part of CMake package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now using CONFIG
mode in the FIND_PACKAGE_ARGS
so it is not compatible if we remove XXXAlt
.
elseif (NOT "${ZLIB_HOME}" STREQUAL "") | ||
find_package (ZLIB REQUIRED) | ||
find_package (ZLIBAlt REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may be able to remove FindZLIBAlt.cmake
. CMake's FindZLIB
provides ZLIB_ROOT
hint: https://cmake.org/cmake/help/latest/module/FindZLIB.html#hints
install(FILES ${snappy_SOURCE_DIR}/snappy-c.h DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}") | ||
install(FILES ${snappy_SOURCE_DIR}/snappy-sinksource.h DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}") | ||
install(FILES ${snappy_SOURCE_DIR}/snappy.h DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}") | ||
install(FILES ${snappy_BINARY_DIR}/snappy-stubs-public.h DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not need to install header files.
ORC doesn't use Snappy in public headers, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to install it, otherwise downstream project may fail like below:
CMake Error at CMakeLists.txt:9 (target_link_libraries):
Cannot find source file:
/tmp/orc/include/snappy-c.h
The reason is that Snappy adds these 4 header files as install interface: https://github.com/google/snappy/blob/1.2.2/CMakeLists.txt#L266
f08a5b2
to
892a12d
Compare
What changes were proposed in this pull request?
XXX_HOME
or its friends are set. This includes:FetchContent
withFIND_PACKAGE_ARGS
. For 3rd party dependencies now, we try to find system installed one viaCONFIG
mode or fallback to vendor it. This includes:Why are the changes needed?
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No