kou commented on a change in pull request #9746:
URL: https://github.com/apache/arrow/pull/9746#discussion_r597329023
##########
File path: cpp/src/arrow/flight/CMakeLists.txt
##########
@@ -83,7 +83,7 @@ function(test_grpc_version DST_VAR DETECT_VERSION TEST_FILE)
try_compile(HAS_GRPC_VERSION ${CMAKE_CURRENT_BINARY_DIR}/try_compile
SOURCES
"${CMAKE_CURRENT_SOURCE_DIR}/try_compile/${TEST_FILE}"
CMAKE_FLAGS
"-DINCLUDE_DIRECTORIES=${CURRENT_INCLUDE_DIRECTORIES}"
- LINK_LIBRARIES gRPC::grpc gRPC::grpc++
+ LINK_LIBRARIES ${GRPC_STATIC_LIBRARY_GRPCPP} gRPC::grpc
gRPC::gpr gRPC::upb gRPC::address_sorting ${ABSL_LIBRARIES} ${ABSL_LIBRARIES}
-pthread
Review comment:
I think there are two problems here:
1. We should not run this check with bundled gRPC. We should always enable
this feature when we build bundled gRPC. (We should build bundled gRPC with
suitable configuration.)
```diff
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake
b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 029a89095..c52830ed2 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2566,6 +2566,7 @@ macro(build_grpc)
add_dependencies(gRPC::grpc++ grpc_ep)
add_dependencies(gRPC::grpc_cpp_plugin grpc_ep)
set(GRPC_VENDORED TRUE)
+ set(GRPC_VERSION ${ARROW_GRPC_BUILD_VERSION})
# ar -M rejects with the "libgrpc++.a" filename because "+" is a line
# continuation character in these scripts, so we have to create a copy of
the
diff --git a/cpp/src/arrow/flight/CMakeLists.txt
b/cpp/src/arrow/flight/CMakeLists.txt
index 5c53160a5..65ae0a2a6 100644
--- a/cpp/src/arrow/flight/CMakeLists.txt
+++ b/cpp/src/arrow/flight/CMakeLists.txt
@@ -102,14 +102,16 @@ function(test_grpc_version DST_VAR DETECT_VERSION
TEST_FILE)
endif()
endfunction()
-test_grpc_version(GRPC_VERSION "1.36" "check_tls_opts_136.cc")
-test_grpc_version(GRPC_VERSION "1.34" "check_tls_opts_134.cc")
-test_grpc_version(GRPC_VERSION "1.32" "check_tls_opts_132.cc")
-test_grpc_version(GRPC_VERSION "1.27" "check_tls_opts_127.cc")
-message(
- STATUS
- "Found approximate gRPC version: ${GRPC_VERSION}
(ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS=${ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS})"
- )
+if(NOT GRPC_VENDORED)
+ test_grpc_version(GRPC_VERSION "1.36" "check_tls_opts_136.cc")
+ test_grpc_version(GRPC_VERSION "1.34" "check_tls_opts_134.cc")
+ test_grpc_version(GRPC_VERSION "1.32" "check_tls_opts_132.cc")
+ test_grpc_version(GRPC_VERSION "1.27" "check_tls_opts_127.cc")
+ message(
+ STATUS
+ "Found approximate gRPC version: ${GRPC_VERSION}
(ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS=${ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS})"
+ )
+endif()
if(GRPC_VERSION EQUAL "1.27")
add_definitions(-DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc_impl::experimental)
elseif(GRPC_VERSION EQUAL "1.32")
```
2. We should set suitable `INTERFACE_LINK_LIBRARIES` property to
`gRPC::grpc++` instead of listing more libraries here.
For example:
```diff
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake
b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 029a89095..cf6dc769c 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2368,7 +2368,6 @@ macro(build_grpc)
base
cord
graphcycles_internal
- int128
malloc_internal
raw_logging_internal
spinlock_wait
@@ -2376,6 +2375,8 @@ macro(build_grpc)
status
statusor
str_format_internal
+ # str_format_internal depends on int128
+ int128
strings
strings_internal
symbolize
@@ -2553,7 +2554,7 @@ macro(build_grpc)
PROPERTIES IMPORTED_LOCATION
"${GRPC_STATIC_LIBRARY_GRPCPP}"
INTERFACE_LINK_LIBRARIES
-
"gRPC::grpc;gRPC::gpr;gRPC::upb;gRPC::address_sorting;${ABSL_LIBRARIES}"
+
"gRPC::grpc;gRPC::gpr;gRPC::upb;gRPC::address_sorting;${ABSL_LIBRARIES};Threads::Threads"
INTERFACE_INCLUDE_DIRECTORIES
"${GRPC_INCLUDE_DIR}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
