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
ARROW-12326: [C++] Avoid needless c-ares detection #9977
Conversation
If we use system gRPC, we don't need to detect c-ares. This change also simplifies gRPC detection. System gRPC detection requires CMake config or pkg-config. System gRPC detection by gRPC_ROOT is removed because we can't maintain Abseil dependencies.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@github-actions crossbow submit -g nightly |
Revision: 056a87a Submitted crossbow builds: ursacomputing/crossbow @ actions-308 |
This works now. |
set(GRPCPP_COMPILE_OPTIONS ${GRPCPP_PC_CFLAGS_OTHER}) | ||
else() | ||
set(GRPCPP_LINK_LIBRARIES) | ||
foreach(GRPCPP_LIBRARY_NAME ${GRPCPP_PC_STATIC_LIBRARIES}) |
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.
Should there be an assertion here to check that the required libraries are in GRPCPP_PC_STATIC_LIBRARIES
before iterating over them?
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 don't think that we need it.
Because we already check GRPCC_PC_FOUND
. We can assume that pkg_check_modules()
sets GRPCPP_PC_*
and GRPCPP_PC_STATIC_*
variables.
(I don't think that it can be occurred but) If GRPCPP_PC_STATIC_LIBRARIES
is empty, the following list(GET GRPCPP_LINK_LIBRARIES 0 GRPCPP_IMPORTED_LOCATION)
is failed. So we can detect invalid value eventually.
"${GRPC_INCLUDE_DIR}") | ||
"${GRPCPP_INCLUDE_DIRECTORIES}" | ||
INTERFACE_LINK_LIBRARIES | ||
"${GRPCPP_LINK_LIBRARIES}" |
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 GRPCPP_LINK_LIBRARIES
contain extraneous libraries that might cause problems here?
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 grpc++.pc
is broken, it can be occurred. But it's a grpc++.pc
problem. We should fix grpc++.pc
instead of our .cmake
.
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.
LGTM
If we use system gRPC, we don't need to detect c-ares. This change also simplifies gRPC detection. System gRPC detection requires CMake config or pkg-config. System gRPC detection by gRPC_ROOT is removed because we can't maintain Abseil dependencies. Closes apache#9977 from kou/cpp-avoid-needless-c-ares-detection Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Krisztián Szűcs <szucs.krisztian@gmail.com>
If we use system gRPC, we don't need to detect c-ares. This change also simplifies gRPC detection. System gRPC detection requires CMake config or pkg-config. System gRPC detection by gRPC_ROOT is removed because we can't maintain Abseil dependencies. Closes apache#9977 from kou/cpp-avoid-needless-c-ares-detection Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Krisztián Szűcs <szucs.krisztian@gmail.com>
If we use system gRPC, we don't need to detect c-ares. This change also simplifies gRPC detection. System gRPC detection requires CMake config or pkg-config. System gRPC detection by gRPC_ROOT is removed because we can't maintain Abseil dependencies. Closes apache#9977 from kou/cpp-avoid-needless-c-ares-detection Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Krisztián Szűcs <szucs.krisztian@gmail.com>
If we use system gRPC, we don't need to detect c-ares. This change also simplifies gRPC detection. System gRPC detection requires CMake config or pkg-config. System gRPC detection by gRPC_ROOT is removed because we can't maintain Abseil dependencies. Closes apache#9977 from kou/cpp-avoid-needless-c-ares-detection Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Krisztián Szűcs <szucs.krisztian@gmail.com>
If we use system gRPC, we don't need to detect c-ares.
This change also simplifies gRPC detection. System gRPC detection
requires CMake config or pkg-config. System gRPC detection by
gRPC_ROOT is removed because we can't maintain Abseil dependencies.