GH-48091: [C++] Use FetchContent for bundled c-ares#48092
GH-48091: [C++] Use FetchContent for bundled c-ares#48092raulcd merged 4 commits intoapache:mainfrom
Conversation
|
@github-actions crossbow submit bundled |
|
Revision: 7e7f084 Submitted crossbow builds: ursacomputing/crossbow @ actions-d7ad44f8f1
|
|
@github-actions crossbow submit bundled |
|
Revision: 91f6b54 Submitted crossbow builds: ursacomputing/crossbow @ actions-490508cbda
|
|
@github-actions crossbow submit -g r -g cpp |
|
Revision: 91f6b54 Submitted crossbow builds: ursacomputing/crossbow @ actions-d22c15fa3f |
|
Some of the failures on the R job failures above (test-r-macos-as-cran) seem related to the previous abseil change, I'll investigate: |
|
@github-actions crossbow submit r-binary-packages |
|
Revision: 0b0a3d1 Submitted crossbow builds: ursacomputing/crossbow @ actions-f99573dc0b
|
|
@github-actions crossbow submit -g r -g cpp |
|
Revision: 0b0a3d1 Submitted crossbow builds: ursacomputing/crossbow @ actions-c43d12b696 |
| # When ABSL_ENABLE_INSTALL is ON, the real target is "time" not "absl_time" | ||
| # Cannot use set_property on alias targets (absl::time is an alias) | ||
| set_property(TARGET time |
There was a problem hiding this comment.
This is not required for c-ares but a bug fix required for macOS abseil as seen on some R failures
|
I have a working PoC on this branch where I have migrated Abseil, c-ares, protobuf, re2 and gRPC and I can reduce back c-ares and re2 to something like: function(build_re2)
list(APPEND CMAKE_MESSAGE_INDENT "RE2: ")
message(STATUS "Building RE2 from source using FetchContent")
set(RE2_VENDORED
TRUE
PARENT_SCOPE)
fetchcontent_declare(re2
URL ${RE2_SOURCE_URL}
URL_HASH "SHA256=${ARROW_RE2_BUILD_SHA256_CHECKSUM}")
prepare_fetchcontent()
fetchcontent_makeavailable(re2)
set(ARROW_BUNDLED_STATIC_LIBS
${ARROW_BUNDLED_STATIC_LIBS} re2::re2
PARENT_SCOPE)
list(POP_BACK CMAKE_MESSAGE_INDENT)
endfunction()and function(build_cares)
list(APPEND CMAKE_MESSAGE_INDENT "c-ares: ")
message(STATUS "Building c-ares from source using FetchContent")
set(CARES_VENDORED
TRUE
PARENT_SCOPE)
fetchcontent_declare(cares
URL ${CARES_SOURCE_URL}
URL_HASH "SHA256=${ARROW_CARES_BUILD_SHA256_CHECKSUM}")
prepare_fetchcontent()
set(CARES_SHARED OFF)
set(CARES_STATIC ON)
fetchcontent_makeavailable(cares)
if(APPLE)
# libresolv must be linked from c-ares version 1.16.1
find_library(LIBRESOLV_LIBRARY NAMES resolv libresolv REQUIRED)
set_target_properties(c-ares::cares PROPERTIES INTERFACE_LINK_LIBRARIES
"${LIBRESOLV_LIBRARY}")
endif()
set(ARROW_BUNDLED_STATIC_LIBS
${ARROW_BUNDLED_STATIC_LIBS} c-ares::cares
PARENT_SCOPE)
list(POP_BACK CMAKE_MESSAGE_INDENT)
endfunction()For Abseil and Protobuf as those are dependencies for other libraries too, it will take longer but we should be able to reduce them once everything is migrated to |
kou
left a comment
There was a problem hiding this comment.
+1
Is this ready to merge? Do we need more changes in this PR?
raulcd
left a comment
There was a problem hiding this comment.
Yes, this is ready to be merged. I just wanted to point out that we will be able to simplify it once the rest of the work is done :)
I'll merge it.
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit bc51a76. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change As a follow up of requiring a minimum CMake version >= 3.25 we discussed moving our dependencies from ExternalProject to FetchContent. This can heavily simplify our third party dependency management. Moving c-ares is the next step before moving grpc. ### What changes are included in this PR? The general change is moving from `ExternalProject` to `FetchContent`. It also add some required integration due to other dependencies, like grpc, using `ExternalProject`. We not only have to build but also install in order for those other dependencies to find c-ares. This causes some timing issues between config, build, install that requires us to create a custom target to depend on so the other dependencies find abseil. ### Are these changes tested? Yes, the changes are tested locally and on CI. ### Are there any user-facing changes? No * GitHub Issue: apache#48091 Authored-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
Rationale for this change
As a follow up of requiring a minimum CMake version >= 3.25 we discussed moving our dependencies from ExternalProject to FetchContent. This can heavily simplify our third party dependency management. Moving c-ares is the next step before moving grpc.
What changes are included in this PR?
The general change is moving from
ExternalProjecttoFetchContent.It also add some required integration due to other dependencies, like grpc, using
ExternalProject. We not only have to build but also install in order for those other dependencies to find c-ares. This causes some timing issues between config, build, install that requires us to create a custom target to depend on so the other dependencies find abseil.Are these changes tested?
Yes, the changes are tested locally and on CI.
Are there any user-facing changes?
No