lidavidm commented on a change in pull request #9746: URL: https://github.com/apache/arrow/pull/9746#discussion_r598650430
########## File path: cpp/cmake_modules/ThirdpartyToolchain.cmake ########## @@ -2371,30 +2371,30 @@ macro(build_grpc) set(ABSL_LIBRARIES) # Abseil libraries gRPC depends on + # Follows grpc++ package config template for link order of libraries + # https://github.com/grpc/grpc/blob/v1.35.0/CMakeLists.txt#L16361 set(_ABSL_LIBS Review comment: This is specific to the version of Abseil, right? Maybe there's some way to extract this order if we have the CMake and/or PkgConfig files available to us? That said, it still builds in CI where I think a few versions are used so it seems at least that's ok. But I'm worried this part is a little brittle. ########## File path: cpp/src/arrow/flight/CMakeLists.txt ########## @@ -102,19 +102,24 @@ 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(GRPC_VENDORED) + # v1.35.0 -> 1.35 + string(REGEX MATCH "[0-9]+\\.[0-9]+" GRPC_VERSION "${ARROW_GRPC_BUILD_VERSION}") +else() + 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") add_definitions(-DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental) -elseif(GRPC_VERSION EQUAL "1.34") +elseif(GRPC_VERSION EQUAL "1.34" OR GRPC_VERSION EQUAL "1.35") Review comment: This might break if we update the bundled gRPC; unfortunately CI doesn't set the `ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS` flag yet. -- 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: us...@infra.apache.org
