From f4e9fd1f964b2375eb60a28f14be15fa5af186a7 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 13:37:44 -0700 Subject: [PATCH] fix: support both old and new versions of protobuf (#79) The version numbering scheme for the protobuf compiler was updated which causes a mismatch warning to appear for versions of cmake less than 3.27. Later versions of protobuf are bundled with their own copy of abseil. Since protobuf always required abseil anyway, this fix also updates the code to rely on that (when available) instead of our own submodule. This should lead to greater consistency (especially with new protobuf code leaning on an experimental logging feature that won't be available in older versions of abseil). If the newer CONFIG-based method of findpackage does not work, we now fall back to the older method of using findpackage. The version of cmake that we require is still 3.20 as 3.27 isn't available in enough locations to advance to that minimum. --- CMakeLists.txt | 20 +++++++++++++++---- src/substrait/common/CMakeLists.txt | 2 -- src/substrait/proto/CMakeLists.txt | 2 +- .../textplan/converter/LoadBinary.cpp | 5 +++-- .../parser/SubstraitPlanRelationVisitor.cpp | 3 +-- src/substrait/textplan/tests/CMakeLists.txt | 1 - .../textplan/tests/RoundtripTest.cpp | 8 ++++---- third_party/CMakeLists.txt | 6 ++++-- third_party/abseil-cpp | 2 +- 9 files changed, 30 insertions(+), 19 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c6df8ce1..bd7fe7e6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -16,13 +16,25 @@ option( "Enable substrait-cpp tests. This will enable all other build options automatically." ON) -find_package(Protobuf REQUIRED) -include_directories(${PROTOBUF_INCLUDE_DIRS}) - -add_subdirectory(third_party) +# Local files come first. include_directories(include) include_directories(src) +# TODO: Simplify once we can require cmake 3.27 (where CONFIG is default). + +# Due to packaging changes we use the combined protobuf/absl packaging if +# available otherwise we fallback to the older protobuf method. +find_package(Protobuf QUIET CONFIG) +if(${Protobuf_FOUND}) + set(ABSL_INCLUDED_WITH_PROTOBUF ON) +else() + find_package(Protobuf REQUIRED) + include_directories(${Protobuf_INCLUDE_DIRS}) + set(ABSL_INCLUDED_WITH_PROTOBUF OFF) +endif() + +add_subdirectory(third_party) + list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake_modules") include(BuildUtils) diff --git a/src/substrait/common/CMakeLists.txt b/src/substrait/common/CMakeLists.txt index dac9006c..846d077f 100644 --- a/src/substrait/common/CMakeLists.txt +++ b/src/substrait/common/CMakeLists.txt @@ -1,7 +1,5 @@ # SPDX-License-Identifier: Apache-2.0 -find_package(fmt) - add_library(substrait_common Exceptions.cpp) target_link_libraries(substrait_common fmt::fmt-header-only) diff --git a/src/substrait/proto/CMakeLists.txt b/src/substrait/proto/CMakeLists.txt index d8dc327c..0c5d5a62 100644 --- a/src/substrait/proto/CMakeLists.txt +++ b/src/substrait/proto/CMakeLists.txt @@ -82,7 +82,7 @@ add_library(substrait_proto ${PROTO_SRCS} ${PROTO_HDRS} ProtoUtils.cpp ProtoUtils.h) # Include the protobuf library as a dependency to use this class. -target_link_libraries(substrait_proto ${PROTOBUF_LIBRARIES}) +target_link_libraries(substrait_proto protobuf::libprotobuf) # Make sure we can see our own generated include files. target_include_directories(substrait_proto diff --git a/src/substrait/textplan/converter/LoadBinary.cpp b/src/substrait/textplan/converter/LoadBinary.cpp index 69d98007..787ab054 100644 --- a/src/substrait/textplan/converter/LoadBinary.cpp +++ b/src/substrait/textplan/converter/LoadBinary.cpp @@ -70,8 +70,9 @@ PlanOrErrors loadFromJson(std::string_view json) { auto status = google::protobuf::util::JsonStringToMessage( std::string{usableJson}, &plan); if (!status.ok()) { - return PlanOrErrors({fmt::format( - "Failed to parse Substrait JSON: {}", status.message().ToString())}); + std::string msg{status.message()}; + return PlanOrErrors( + {fmt::format("Failed to parse Substrait JSON: {}", msg)}); } return PlanOrErrors(plan); } diff --git a/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp index 9ed04f84..abdb98b2 100644 --- a/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp +++ b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp @@ -790,8 +790,7 @@ std::any SubstraitPlanRelationVisitor::visitExpressionFunctionUse( expr.mutable_scalar_function()->set_function_reference(funcReference); for (const auto& exp : ctx->expression()) { if (endsWith(exp->getText(), "_enum")) { - auto str = exp->getText(); - str = absl::StripSuffix(str, "_enum"); + std::string str{absl::StripSuffix(exp->getText(), "_enum")}; expr.mutable_scalar_function()->add_arguments()->set_enum_(str); continue; } diff --git a/src/substrait/textplan/tests/CMakeLists.txt b/src/substrait/textplan/tests/CMakeLists.txt index 16da68ba..24be6792 100644 --- a/src/substrait/textplan/tests/CMakeLists.txt +++ b/src/substrait/textplan/tests/CMakeLists.txt @@ -36,7 +36,6 @@ if(${SUBSTRAIT_CPP_ROUNDTRIP_TESTING}) parse_result_matchers protobuf-matchers fmt::fmt-header-only - absl::strings gmock gtest gtest_main) diff --git a/src/substrait/textplan/tests/RoundtripTest.cpp b/src/substrait/textplan/tests/RoundtripTest.cpp index 0d5949ca..4cf2e3ea 100644 --- a/src/substrait/textplan/tests/RoundtripTest.cpp +++ b/src/substrait/textplan/tests/RoundtripTest.cpp @@ -10,8 +10,6 @@ #include #include -#include "absl/strings/str_split.h" -#include "gmock/gmock.h" #include "substrait/textplan/SymbolTablePrinter.h" #include "substrait/textplan/converter/LoadBinary.h" #include "substrait/textplan/converter/ParseBinary.h" @@ -28,10 +26,12 @@ namespace io::substrait::textplan { namespace { std::string addLineNumbers(const std::string& text) { + std::stringstream input{text}; std::stringstream result; int lineNum = 0; - for (absl::string_view sp : absl::StrSplit(text, '\n')) { - result << std::setw(4) << ++lineNum << " " << sp << std::endl; + std::string line; + while (std::getline(input, line, '\n')) { + result << std::setw(4) << ++lineNum << " " << line << std::endl; } return result.str(); } diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index 6ace2da8..7b8d8e60 100644 --- a/third_party/CMakeLists.txt +++ b/third_party/CMakeLists.txt @@ -1,7 +1,9 @@ # SPDX-License-Identifier: Apache-2.0 -set(ABSL_PROPAGATE_CXX_STD ON) -add_subdirectory(abseil-cpp) +if(NOT ${ABSL_INCLUDED_WITH_PROTOBUF}) + set(ABSL_PROPAGATE_CXX_STD ON) + add_subdirectory(abseil-cpp) +endif() set(BUILD_TZ_LIB ON) add_subdirectory(datetime) diff --git a/third_party/abseil-cpp b/third_party/abseil-cpp index c8a2f925..c2435f83 160000 --- a/third_party/abseil-cpp +++ b/third_party/abseil-cpp @@ -1 +1 @@ -Subproject commit c8a2f92586fe9b4e1aff049108f5db8064924d8e +Subproject commit c2435f8342c2d0ed8101cb43adfd605fdc52dca2