-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-48248: [C++] Use FetchContent for bundled gRPC #48250
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
|
@github-actions crossbow submit -g cpp |
|
Revision: 9386a28 Submitted crossbow builds: ursacomputing/crossbow @ actions-bbf70f60fe |
|
@github-actions crossbow submit -g r |
|
Revision: 9386a28 Submitted crossbow builds: ursacomputing/crossbow @ actions-b3bd179582 |
|
@github-actions crossbow submit -g cpp |
|
Revision: 1144f25 Submitted crossbow builds: ursacomputing/crossbow @ actions-c9de41cc83 |
|
@github-actions crossbow submit test-ubuntu-22.04-cpp-bundled |
|
Revision: 9bddf67 Submitted crossbow builds: ursacomputing/crossbow @ actions-de489c8583
|
|
@github-actions crossbow submit -g r |
|
Revision: 9bddf67 Submitted crossbow builds: ursacomputing/crossbow @ actions-3f45b50a5c |
|
@github-actions crossbow submit -g cpp |
|
Revision: 9bddf67 Submitted crossbow builds: ursacomputing/crossbow @ actions-0e07cbb9ef |
…t and make config conditional to vendored third parties
… be a submodule of grpc
|
@github-actions crossbow submit -g cpp |
|
Revision: a219eff Submitted crossbow builds: ursacomputing/crossbow @ actions-b3962c3cec |
|
@github-actions crossbow submit -g r |
|
Revision: a219eff Submitted crossbow builds: ursacomputing/crossbow @ actions-ada8c5b28b |
| URL ${RE2_SOURCE_URL} | ||
| URL_HASH "SHA256=${ARROW_RE2_BUILD_SHA256_CHECKSUM}") | ||
| URL_HASH "SHA256=${ARROW_RE2_BUILD_SHA256_CHECKSUM}" | ||
| EXCLUDE_FROM_ALL) |
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.
Could you use ${FC_DECLARE_COMMON_OPTIONS} instead like
arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake
Lines 1770 to 1774 in 5aa7dd1
| fetchcontent_declare(thrift | |
| ${FC_DECLARE_COMMON_OPTIONS} | |
| PATCH_COMMAND ${THRIFT_PATCH_COMMAND} | |
| URL ${THRIFT_SOURCE_URL} | |
| URL_HASH "SHA256=${ARROW_THRIFT_BUILD_SHA256_CHECKSUM}") |
| # Disable install rules for RE2 so it is not installed on our Linux-packages. | ||
| set(CMAKE_SKIP_INSTALL_RULES ON) |
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.
Can we use
arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake
Lines 1818 to 1820 in 5aa7dd1
| if(CMAKE_VERSION VERSION_LESS 3.28) | |
| set_property(DIRECTORY ${thrift_SOURCE_DIR} PROPERTY EXCLUDE_FROM_ALL TRUE) | |
| endif() |
| if(PROTOBUF_VENDORED) | ||
| set(_gRPC_PROTOBUF_LIBRARIES | ||
| "protobuf::libprotobuf" | ||
| CACHE STRING "" FORCE) |
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.
Can we use normal variable not cache variable for all gRPC related variables?
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 simplify our third party dependency management.
What changes are included in this PR?
The general change is moving gRPC from
ExternalProjecttoFetchContent.We can clean up previously required installation on RE2 and C-Ares. We can't do it for Abseil or protobuf as those are also used on other dependencies. We will be able to clean those later.
Are these changes tested?
Yes, the changes are tested locally and on CI.
Are there any user-facing changes?
No