From 2a2c253c57b4299a98ee3c7ad84b1d9479e39b8b Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 10 Jul 2023 11:18:39 +0900 Subject: [PATCH] GH-36479: [C++][FlightRPC] Use gRPC version detected by find_package() (#36581) ### Rationale for this change We don't need to use `try_compile()` by using gRPC version detected by `find_package()`. ### What changes are included in this PR? Use gRPC version detected by `find_package()`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: #36479 Authored-by: Sutou Kouhei Signed-off-by: Sutou Kouhei --- ci/scripts/cpp_build.sh | 1 - cpp/cmake_modules/FindgRPCAlt.cmake | 5 +- cpp/cmake_modules/ThirdpartyToolchain.cmake | 9 +- cpp/src/arrow/flight/CMakeLists.txt | 83 +++---------------- .../flight/try_compile/check_tls_opts_127.cc | 36 -------- .../flight/try_compile/check_tls_opts_132.cc | 36 -------- .../flight/try_compile/check_tls_opts_134.cc | 44 ---------- .../flight/try_compile/check_tls_opts_136.cc | 38 --------- .../flight/try_compile/check_tls_opts_143.cc | 37 --------- 9 files changed, 22 insertions(+), 267 deletions(-) delete mode 100644 cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc delete mode 100644 cpp/src/arrow/flight/try_compile/check_tls_opts_132.cc delete mode 100644 cpp/src/arrow/flight/try_compile/check_tls_opts_134.cc delete mode 100644 cpp/src/arrow/flight/try_compile/check_tls_opts_136.cc delete mode 100644 cpp/src/arrow/flight/try_compile/check_tls_opts_143.cc diff --git a/ci/scripts/cpp_build.sh b/ci/scripts/cpp_build.sh index 9854c5ff162af..91a570be977a2 100755 --- a/ci/scripts/cpp_build.sh +++ b/ci/scripts/cpp_build.sh @@ -109,7 +109,6 @@ cmake \ -DARROW_EXTRA_ERROR_CONTEXT=${ARROW_EXTRA_ERROR_CONTEXT:-OFF} \ -DARROW_FILESYSTEM=${ARROW_FILESYSTEM:-ON} \ -DARROW_FLIGHT=${ARROW_FLIGHT:-OFF} \ - -DARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS=${ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS:-OFF} \ -DARROW_FLIGHT_SQL=${ARROW_FLIGHT_SQL:-OFF} \ -DARROW_FUZZING=${ARROW_FUZZING:-OFF} \ -DARROW_GANDIVA_PC_CXX_FLAGS=${ARROW_GANDIVA_PC_CXX_FLAGS:-} \ diff --git a/cpp/cmake_modules/FindgRPCAlt.cmake b/cpp/cmake_modules/FindgRPCAlt.cmake index 4e38605235b11..c5d8c8a8126cf 100644 --- a/cpp/cmake_modules/FindgRPCAlt.cmake +++ b/cpp/cmake_modules/FindgRPCAlt.cmake @@ -33,8 +33,10 @@ pkg_check_modules(GRPCPP_PC grpc++) if(GRPCPP_PC_FOUND) set(gRPCAlt_VERSION "${GRPCPP_PC_VERSION}") set(GRPCPP_INCLUDE_DIRECTORIES ${GRPCPP_PC_INCLUDEDIR}) + # gRPC's pkg-config file neglects to specify pthreads. + find_package(Threads REQUIRED) if(ARROW_GRPC_USE_SHARED) - set(GRPCPP_LINK_LIBRARIES ${GRPCPP_PC_LINK_LIBRARIES}) + set(GRPCPP_LINK_LIBRARIES ${GRPCPP_PC_LINK_LIBRARIES} Threads::Threads) set(GRPCPP_LINK_OPTIONS ${GRPCPP_PC_LDFLAGS_OTHER}) set(GRPCPP_COMPILE_OPTIONS ${GRPCPP_PC_CFLAGS_OTHER}) else() @@ -45,6 +47,7 @@ if(GRPCPP_PC_FOUND) HINTS ${GRPCPP_PC_STATIC_LIBRARY_DIRS}) list(APPEND GRPCPP_LINK_LIBRARIES "${GRPCPP_LIBRARY_${GRPCPP_LIBRARY_NAME}}") endforeach() + list(APPEND GRPCPP_LINK_LIBRARIES Threads::Threads) set(GRPCPP_LINK_OPTIONS ${GRPCPP_PC_STATIC_LDFLAGS_OTHER}) set(GRPCPP_COMPILE_OPTIONS ${GRPCPP_PC_STATIC_CFLAGS_OTHER}) endif() diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 4a19e226f7b56..c9ad4f665b1ba 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -3971,7 +3971,7 @@ macro(build_grpc) endmacro() if(ARROW_WITH_GRPC) - set(ARROW_GRPC_REQUIRED_VERSION "1.17.0") + set(ARROW_GRPC_REQUIRED_VERSION "1.30.0") if(NOT Protobuf_SOURCE STREQUAL gRPC_SOURCE) # ARROW-15495: Protobuf/gRPC must come from the same source message(STATUS "Forcing gRPC_SOURCE to Protobuf_SOURCE (${Protobuf_SOURCE})") @@ -3986,10 +3986,17 @@ if(ARROW_WITH_GRPC) grpc++) if(GRPC_VENDORED) + # Remove "v" from "vX.Y.Z" + string(SUBSTRING ${ARROW_GRPC_BUILD_VERSION} 1 -1 ARROW_GRPC_VERSION) set(GRPCPP_PP_INCLUDE TRUE) # Examples need to link to static Arrow if we're using static gRPC set(ARROW_GRPC_USE_SHARED OFF) else() + if(gRPCAlt_VERSION) + set(ARROW_GRPC_VERSION ${gRPCAlt_VERSION}) + else() + set(ARROW_GRPC_VERSION ${gRPC_VERSION}) + endif() # grpc++ headers may reside in ${GRPC_INCLUDE_DIR}/grpc++ or ${GRPC_INCLUDE_DIR}/grpcpp # depending on the gRPC version. get_target_property(GRPC_INCLUDE_DIR gRPC::grpc++ INTERFACE_INCLUDE_DIRECTORIES) diff --git a/cpp/src/arrow/flight/CMakeLists.txt b/cpp/src/arrow/flight/CMakeLists.txt index 917c0c33211e0..7383a7eec9045 100644 --- a/cpp/src/arrow/flight/CMakeLists.txt +++ b/cpp/src/arrow/flight/CMakeLists.txt @@ -102,84 +102,21 @@ set(CMAKE_CXX_FLAGS_BACKUP "${CMAKE_CXX_FLAGS}") string(REPLACE "/WX" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") string(REPLACE "-Werror " " " CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") -# Probe the version of gRPC being used to see if it supports disabling server -# verification when using TLS. -# gRPC's pkg-config file neglects to specify pthreads. -find_package(Threads REQUIRED) -function(test_grpc_version DST_VAR DETECT_VERSION TEST_FILE) - if(NOT DEFINED ${DST_VAR}) - message(STATUS "Checking support for TlsCredentialsOptions (gRPC >= ${DETECT_VERSION})..." - ) - get_property(CURRENT_INCLUDE_DIRECTORIES - DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} - PROPERTY INCLUDE_DIRECTORIES) - # ARROW-13881: when detecting support, avoid mismatch between - # debug flags of gRPC and our probe (which results in LNK2038) - set(CMAKE_TRY_COMPILE_CONFIGURATION ${CMAKE_BUILD_TYPE}) - 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++ Threads::Threads - OUTPUT_VARIABLE TLS_CREDENTIALS_OPTIONS_CHECK_OUTPUT CXX_STANDARD 11) - if(HAS_GRPC_VERSION) - set(${DST_VAR} - "${DETECT_VERSION}" - CACHE INTERNAL "The detected (approximate) gRPC version.") - else() - message(STATUS "TlsCredentialsOptions (for gRPC ${DETECT_VERSION}) not found in grpc::experimental." - ) - if(ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS) - message(WARNING "Build output:") - list(APPEND CMAKE_MESSAGE_INDENT "${TEST_FILE}: ") - message(WARNING ${TLS_CREDENTIALS_OPTIONS_CHECK_OUTPUT}) - list(REMOVE_AT CMAKE_MESSAGE_INDENT -1) - else() - message(DEBUG "Build output:") - list(APPEND CMAKE_MESSAGE_INDENT "${TEST_FILE}: ") - message(DEBUG ${TLS_CREDENTIALS_OPTIONS_CHECK_OUTPUT}) - list(REMOVE_AT CMAKE_MESSAGE_INDENT -1) - endif() - endif() - endif() -endfunction() - -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.43" "check_tls_opts_143.cc") - 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" OR GRPC_VERSION EQUAL "1.35") +if(ARROW_GRPC_VERSION VERSION_GREATER_EQUAL "1.43") add_definitions(-DGRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS - -DGRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS_ROOT_CERTS - -DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental) -elseif(GRPC_VERSION EQUAL "1.36") + -DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental + -DGRPC_USE_CERTIFICATE_VERIFIER) +elseif(ARROW_GRPC_VERSION VERSION_GREATER_EQUAL "1.36") add_definitions(-DGRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS -DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental) -elseif((GRPC_VERSION EQUAL "1.43") OR (GRPC_VERSION EQUAL "1.46")) - # 1.46 is the bundled version +elseif(ARROW_GRPC_VERSION VERSION_GREATER_EQUAL "1.34") add_definitions(-DGRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS - -DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental - -DGRPC_USE_CERTIFICATE_VERIFIER) + -DGRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS_ROOT_CERTS + -DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental) +elseif(ARROW_GRPC_VERSION VERSION_GREATER_EQUAL "1.32") + add_definitions(-DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental) else() - message(STATUS "A proper version of gRPC could not be found to support TlsCredentialsOptions in Arrow Flight." - ) - message(STATUS "You may need a newer version of gRPC (>= 1.27), or the gRPC API has changed and Flight must be updated to match." - ) - if(ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS) - message(FATAL_ERROR "Halting build since ARROW_FLIGHT_REQUIRE_TLSCREDENTIALSOPTIONS is set." - ) - endif() + add_definitions(-DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc_impl::experimental) endif() # Restore the CXXFLAGS that were modified above diff --git a/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc b/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc deleted file mode 100644 index 11de4989911d2..0000000000000 --- a/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc +++ /dev/null @@ -1,36 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -// Dummy file for checking if TlsCredentialsOptions exists in -// the grpc_impl::experimental namespace. gRPC versions 1.27-1.31 -// put it here. This is for supporting disabling server -// validation when using TLS. - -#include -#include -#include - -static grpc_tls_server_verification_option check( - const grpc_impl::experimental::TlsCredentialsOptions* options) { - grpc_tls_server_verification_option server_opt = options->server_verification_option(); - return server_opt; -} - -int main(int argc, const char** argv) { - [[maybe_unused]] grpc_tls_server_verification_option opt = check(nullptr); - return 0; -} diff --git a/cpp/src/arrow/flight/try_compile/check_tls_opts_132.cc b/cpp/src/arrow/flight/try_compile/check_tls_opts_132.cc deleted file mode 100644 index fa5ba0f43d925..0000000000000 --- a/cpp/src/arrow/flight/try_compile/check_tls_opts_132.cc +++ /dev/null @@ -1,36 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -// Dummy file for checking if TlsCredentialsOptions exists in -// the grpc::experimental namespace. gRPC versions 1.32 and higher -// put it here. This is for supporting disabling server -// validation when using TLS. - -#include -#include -#include - -static grpc_tls_server_verification_option check( - const grpc::experimental::TlsCredentialsOptions* options) { - grpc_tls_server_verification_option server_opt = options->server_verification_option(); - return server_opt; -} - -int main(int argc, const char** argv) { - [[maybe_unused]] grpc_tls_server_verification_option opt = check(nullptr); - return 0; -} diff --git a/cpp/src/arrow/flight/try_compile/check_tls_opts_134.cc b/cpp/src/arrow/flight/try_compile/check_tls_opts_134.cc deleted file mode 100644 index 4ee2122ef57e7..0000000000000 --- a/cpp/src/arrow/flight/try_compile/check_tls_opts_134.cc +++ /dev/null @@ -1,44 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -// Dummy file for checking if TlsCredentialsOptions exists in -// the grpc::experimental namespace. gRPC starting from 1.34 -// put it here. This is for supporting disabling server -// validation when using TLS. - -#include -#include -#include - -// Dummy file for checking if TlsCredentialsOptions exists in -// the grpc::experimental namespace. gRPC starting from 1.34 -// puts it here. This is for supporting disabling server -// validation when using TLS. - -static void check() { - // In 1.34, there's no parameterless constructor; in 1.36, there's - // only a parameterless constructor - auto options = - std::make_shared(nullptr); - options->set_server_verification_option( - grpc_tls_server_verification_option::GRPC_TLS_SERVER_VERIFICATION); -} - -int main(int argc, const char** argv) { - check(); - return 0; -} diff --git a/cpp/src/arrow/flight/try_compile/check_tls_opts_136.cc b/cpp/src/arrow/flight/try_compile/check_tls_opts_136.cc deleted file mode 100644 index 638eec67ba723..0000000000000 --- a/cpp/src/arrow/flight/try_compile/check_tls_opts_136.cc +++ /dev/null @@ -1,38 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -// Dummy file for checking if TlsCredentialsOptions exists in -// the grpc::experimental namespace. gRPC starting from 1.36 -// puts it here. This is for supporting disabling server -// validation when using TLS. - -#include -#include -#include - -static void check() { - // In 1.34, there's no parameterless constructor; in 1.36, there's - // only a parameterless constructor - auto options = std::make_shared(); - options->set_server_verification_option( - grpc_tls_server_verification_option::GRPC_TLS_SERVER_VERIFICATION); -} - -int main(int argc, const char** argv) { - check(); - return 0; -} diff --git a/cpp/src/arrow/flight/try_compile/check_tls_opts_143.cc b/cpp/src/arrow/flight/try_compile/check_tls_opts_143.cc deleted file mode 100644 index 2fdaac9d6ef7e..0000000000000 --- a/cpp/src/arrow/flight/try_compile/check_tls_opts_143.cc +++ /dev/null @@ -1,37 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -// Dummy file for checking if TlsCredentialsOptions supports -// set_verify_server_certs. gRPC starting from 1.43 added this boolean -// flag as opposed to prior versions which used an enum. This is for -// supporting disabling server validation when using TLS. - -#include -#include -#include - -static void check() { - // 1.36 uses an enum; 1.43 uses booleans - auto options = std::make_shared(); - options->set_check_call_host(false); - options->set_verify_server_certs(false); -} - -int main(int argc, const char** argv) { - check(); - return 0; -}