From e8a4edb75503be31bb594773a48542b61d6e2cb2 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Thu, 6 Jul 2023 13:43:59 -0700 Subject: [PATCH] fix: support both old and new versions of protobuf 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 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. --- .gitmodules | 3 --- CMakeLists.txt | 17 +++++++++++++---- src/substrait/proto/CMakeLists.txt | 2 +- src/substrait/textplan/converter/LoadBinary.cpp | 2 +- src/substrait/textplan/tests/CMakeLists.txt | 1 - src/substrait/textplan/tests/RoundtripTest.cpp | 8 ++++---- third_party/CMakeLists.txt | 3 --- third_party/abseil-cpp | 1 - 8 files changed, 19 insertions(+), 18 deletions(-) delete mode 160000 third_party/abseil-cpp diff --git a/.gitmodules b/.gitmodules index 6da52c17..89f198ea 100644 --- a/.gitmodules +++ b/.gitmodules @@ -10,9 +10,6 @@ [submodule "third_party/fmt"] path = third_party/fmt url = https://github.com/fmtlib/fmt -[submodule "third_party/abseil-cpp"] - path = third_party/abseil-cpp - url = https://github.com/abseil/abseil-cpp.git [submodule "third_party/datetime"] path = third_party/datetime url = https://github.com/HowardHinnant/date.git diff --git a/CMakeLists.txt b/CMakeLists.txt index c6df8ce1..4d95b285 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -16,13 +16,22 @@ 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) +add_subdirectory(third_party) + +#find_package(absl REQUIRED) +#include_directories(${ABSL_INCLUDE_DIRS}) + +find_package(Protobuf REQUIRED CONFIG) +#include_directories(${PROTOBUF_INCLUDE_DIRS}) + +message(STATUS "include dirs are ${Protobuf_INCLUDE_DIRS}") +message(STATUS "include dirs are ${PROTOBUF_INCLUDE_DIRS}") + + list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake_modules") include(BuildUtils) 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..486e9a5e 100644 --- a/src/substrait/textplan/converter/LoadBinary.cpp +++ b/src/substrait/textplan/converter/LoadBinary.cpp @@ -71,7 +71,7 @@ PlanOrErrors loadFromJson(std::string_view json) { std::string{usableJson}, &plan); if (!status.ok()) { return PlanOrErrors({fmt::format( - "Failed to parse Substrait JSON: {}", status.message().ToString())}); + "Failed to parse Substrait JSON: {}", status.message())}); } return PlanOrErrors(plan); } 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..06fdee18 100644 --- a/third_party/CMakeLists.txt +++ b/third_party/CMakeLists.txt @@ -1,8 +1,5 @@ # SPDX-License-Identifier: Apache-2.0 -set(ABSL_PROPAGATE_CXX_STD ON) -add_subdirectory(abseil-cpp) - set(BUILD_TZ_LIB ON) add_subdirectory(datetime) diff --git a/third_party/abseil-cpp b/third_party/abseil-cpp deleted file mode 160000 index c8a2f925..00000000 --- a/third_party/abseil-cpp +++ /dev/null @@ -1 +0,0 @@ -Subproject commit c8a2f92586fe9b4e1aff049108f5db8064924d8e