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

@@ -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)
 

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)

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

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);
 }

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;
  }

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)

src/substrait/textplan/tests/RoundtripTest.cpp

@@ -10,8 +10,6 @@
 #include <gtest/gtest.h>
 #include <protobuf-matchers/protocol-buffer-matchers.h>
 
-#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();
 }

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)