From e8a4edb75503be31bb594773a48542b61d6e2cb2 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Thu, 6 Jul 2023 13:43:59 -0700 Subject: [PATCH 01/18] 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 From 1d7cf0b6bc55401c625bf17357add18009496a92 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 20:12:27 -0700 Subject: [PATCH 02/18] Ran clang format. --- CMakeLists.txt | 6 ++---- src/substrait/textplan/converter/LoadBinary.cpp | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4d95b285..e5696c47 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,16 +22,14 @@ include_directories(src) add_subdirectory(third_party) -#find_package(absl REQUIRED) -#include_directories(${ABSL_INCLUDE_DIRS}) +# find_package(absl REQUIRED) include_directories(${ABSL_INCLUDE_DIRS}) find_package(Protobuf REQUIRED CONFIG) -#include_directories(${PROTOBUF_INCLUDE_DIRS}) +# 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/textplan/converter/LoadBinary.cpp b/src/substrait/textplan/converter/LoadBinary.cpp index 486e9a5e..6d259791 100644 --- a/src/substrait/textplan/converter/LoadBinary.cpp +++ b/src/substrait/textplan/converter/LoadBinary.cpp @@ -70,8 +70,8 @@ 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())}); + return PlanOrErrors( + {fmt::format("Failed to parse Substrait JSON: {}", status.message())}); } return PlanOrErrors(plan); } From f2a6d4e78124df0621d7c9c73bfcf4d6002e4059 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 20:19:10 -0700 Subject: [PATCH 03/18] Now with the intended code. --- CMakeLists.txt | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e5696c47..6575a2ce 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,13 +22,14 @@ include_directories(src) add_subdirectory(third_party) -# find_package(absl REQUIRED) include_directories(${ABSL_INCLUDE_DIRS}) +find_package(Protobuf QUIET CONFIG) +if(NOT ${Protobuf_FOUND}) + 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}") + find_package(Protobuf REQUIRED) + include_directories(${Protobuf_INCLUDE_DIRS}) +endif() list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake_modules") include(BuildUtils) From 339237eb53ede5371706a2a5953bd044cdfaeaad Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 20:31:49 -0700 Subject: [PATCH 04/18] Add explicit absl package dependency. --- scripts/setup-ubuntu.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh index 32ff4925..332f7c02 100755 --- a/scripts/setup-ubuntu.sh +++ b/scripts/setup-ubuntu.sh @@ -21,6 +21,7 @@ sudo --preserve-env apt install -y \ libprotobuf-dev \ libprotobuf23 \ protobuf-compiler \ + libabsl-dev \ clang-format \ uuid-dev \ default-jre \ From e750098806a20d698bf5844554c6ade8460b0bf5 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 20:50:44 -0700 Subject: [PATCH 05/18] Updated syntax to avoid later change in signature. --- src/substrait/textplan/converter/LoadBinary.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/substrait/textplan/converter/LoadBinary.cpp b/src/substrait/textplan/converter/LoadBinary.cpp index 6d259791..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()) { + std::string msg{status.message()}; return PlanOrErrors( - {fmt::format("Failed to parse Substrait JSON: {}", status.message())}); + {fmt::format("Failed to parse Substrait JSON: {}", msg)}); } return PlanOrErrors(plan); } From 5d86e64706e6dbe8047ca8fc129645ec8a9364a5 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 21:01:46 -0700 Subject: [PATCH 06/18] Fix an abseil-related signature change. --- src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp index 9ed04f84..46fc0e08 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"); + auto str = absl::StripSuffix(exp->getText(), "_enum"); expr.mutable_scalar_function()->add_arguments()->set_enum_(str); continue; } From 6d19bcc207df913832e4a8f55c00d44dc5d39410 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 21:49:34 -0700 Subject: [PATCH 07/18] More fixes to make the code compatible with ancient absl. --- src/substrait/common/CMakeLists.txt | 2 -- src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) 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/textplan/parser/SubstraitPlanRelationVisitor.cpp b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp index 46fc0e08..e31c2289 100644 --- a/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp +++ b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp @@ -790,7 +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 = absl::StripSuffix(exp->getText(), "_enum"); + std::string str = absl::StripSuffix(exp->getText(), "_enum"); expr.mutable_scalar_function()->add_arguments()->set_enum_(str); continue; } From 8c1a99a43bf00f8775eb14333cecda74a0de475c Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 22:19:46 -0700 Subject: [PATCH 08/18] Rebundling abseil as the old version on Ubuntu does not have SimpleHexAtoi. --- .gitmodules | 3 +++ src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp | 2 +- third_party/abseil-cpp | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) create mode 160000 third_party/abseil-cpp diff --git a/.gitmodules b/.gitmodules index 89f198ea..6da52c17 100644 --- a/.gitmodules +++ b/.gitmodules @@ -10,6 +10,9 @@ [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/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp index e31c2289..abdb98b2 100644 --- a/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp +++ b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp @@ -790,7 +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")) { - std::string str = absl::StripSuffix(exp->getText(), "_enum"); + std::string str{absl::StripSuffix(exp->getText(), "_enum")}; expr.mutable_scalar_function()->add_arguments()->set_enum_(str); continue; } diff --git a/third_party/abseil-cpp b/third_party/abseil-cpp new file mode 160000 index 00000000..c8a2f925 --- /dev/null +++ b/third_party/abseil-cpp @@ -0,0 +1 @@ +Subproject commit c8a2f92586fe9b4e1aff049108f5db8064924d8e From 1a62692d107380cc023a048f90259e1294eecbeb Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 22:26:50 -0700 Subject: [PATCH 09/18] Readded absl submodule. --- CMakeLists.txt | 3 --- third_party/CMakeLists.txt | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6575a2ce..3ffa6f08 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -24,9 +24,6 @@ add_subdirectory(third_party) find_package(Protobuf QUIET CONFIG) if(NOT ${Protobuf_FOUND}) - find_package(absl REQUIRED) - include_directories(${absl_INCLUDE_DIRS}) - find_package(Protobuf REQUIRED) include_directories(${Protobuf_INCLUDE_DIRS}) endif() diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index 06fdee18..6ace2da8 100644 --- a/third_party/CMakeLists.txt +++ b/third_party/CMakeLists.txt @@ -1,5 +1,8 @@ # SPDX-License-Identifier: Apache-2.0 +set(ABSL_PROPAGATE_CXX_STD ON) +add_subdirectory(abseil-cpp) + set(BUILD_TZ_LIB ON) add_subdirectory(datetime) From f5813a1d7d2cd8eb8cf4a9a643544e3c720edc23 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 7 Jul 2023 22:28:46 -0700 Subject: [PATCH 10/18] Locked absl to latest release (20230125.3). --- third_party/abseil-cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From d96422843d7bb18e661674dcec7274925e626f61 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 10:24:17 -0700 Subject: [PATCH 11/18] Handle review notes. --- CMakeLists.txt | 3 +++ scripts/setup-ubuntu.sh | 1 - third_party/abseil-cpp | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3ffa6f08..f07d574b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,6 +22,9 @@ include_directories(src) add_subdirectory(third_party) +# Due to packaging changes we use the combined protobuf/absl packaging if +# available otherwise we fallback to the older protobuf method. +# TODO: Simplify once we can require cmake 3.27 (where CONFIG is default). find_package(Protobuf QUIET CONFIG) if(NOT ${Protobuf_FOUND}) find_package(Protobuf REQUIRED) diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh index 332f7c02..32ff4925 100755 --- a/scripts/setup-ubuntu.sh +++ b/scripts/setup-ubuntu.sh @@ -21,7 +21,6 @@ sudo --preserve-env apt install -y \ libprotobuf-dev \ libprotobuf23 \ protobuf-compiler \ - libabsl-dev \ clang-format \ uuid-dev \ default-jre \ diff --git a/third_party/abseil-cpp b/third_party/abseil-cpp index c2435f83..c8a2f925 160000 --- a/third_party/abseil-cpp +++ b/third_party/abseil-cpp @@ -1 +1 @@ -Subproject commit c2435f8342c2d0ed8101cb43adfd605fdc52dca2 +Subproject commit c8a2f92586fe9b4e1aff049108f5db8064924d8e From 4df4e734efa5bae8a21169bb74d02884b4ac5fa1 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 10:37:17 -0700 Subject: [PATCH 12/18] Update to 20230125.3 (latest release) of abseil-cpp. --- third_party/abseil-cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From f3c6d2440280303f847fe336d1cd3ac990080964 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 10:57:20 -0700 Subject: [PATCH 13/18] Only load absl from third party if we do not use the bundled version from protobuf. --- CMakeLists.txt | 8 +++++--- third_party/CMakeLists.txt | 6 ++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f07d574b..a3085102 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -23,10 +23,12 @@ include_directories(src) add_subdirectory(third_party) # Due to packaging changes we use the combined protobuf/absl packaging if -# available otherwise we fallback to the older protobuf method. -# TODO: Simplify once we can require cmake 3.27 (where CONFIG is default). +# available otherwise we fallback to the older protobuf method. TODO: Simplify +# once we can require cmake 3.27 (where CONFIG is default). find_package(Protobuf QUIET CONFIG) -if(NOT ${Protobuf_FOUND}) +if(${Protobuf_FOUND}) + set(PROTOBUF_LOADED_AS_CONFIG) +else() find_package(Protobuf REQUIRED) include_directories(${Protobuf_INCLUDE_DIRS}) endif() diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index 6ace2da8..f1b01cc0 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 ${PROTOBUF_LOADED_AS_CONFIG}) + set(ABSL_PROPAGATE_CXX_STD ON) + add_subdirectory(abseil-cpp) +endif() set(BUILD_TZ_LIB ON) add_subdirectory(datetime) From 90be7d97a9118b46f5235694bf967c3233c84296 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 10:57:52 -0700 Subject: [PATCH 14/18] Better syntax --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a3085102..edd524c4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -27,7 +27,7 @@ add_subdirectory(third_party) # once we can require cmake 3.27 (where CONFIG is default). find_package(Protobuf QUIET CONFIG) if(${Protobuf_FOUND}) - set(PROTOBUF_LOADED_AS_CONFIG) + set(PROTOBUF_LOADED_AS_CONFIG ON) else() find_package(Protobuf REQUIRED) include_directories(${Protobuf_INCLUDE_DIRS}) From a18a18bc86d3ee18a8440ea9241bae71f1e26bf6 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 11:09:47 -0700 Subject: [PATCH 15/18] Set the flag both ways. --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index edd524c4..047b0ec3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -31,6 +31,7 @@ if(${Protobuf_FOUND}) else() find_package(Protobuf REQUIRED) include_directories(${Protobuf_INCLUDE_DIRS}) + set(PROTOBUF_LOADED_AS_CONFIG OFF) endif() list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake_modules") From bbdbe710c5ad26ada6ae2034c8b00b0e3e0475c9 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 11:23:23 -0700 Subject: [PATCH 16/18] Improved variables names to make it more obvious what's going on. --- CMakeLists.txt | 7 +++---- third_party/CMakeLists.txt | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 047b0ec3..9127cd0d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -26,12 +26,11 @@ add_subdirectory(third_party) # available otherwise we fallback to the older protobuf method. TODO: Simplify # once we can require cmake 3.27 (where CONFIG is default). find_package(Protobuf QUIET CONFIG) -if(${Protobuf_FOUND}) - set(PROTOBUF_LOADED_AS_CONFIG ON) -else() +set(ABSL_INCLUDED_WITH_PROTOBUF ON) +if(NOT ${Protobuf_FOUND}) find_package(Protobuf REQUIRED) include_directories(${Protobuf_INCLUDE_DIRS}) - set(PROTOBUF_LOADED_AS_CONFIG OFF) + set(ABSL_INCLUDED_WITH_PROTOBUF OFF) endif() list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake_modules") diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index f1b01cc0..7b8d8e60 100644 --- a/third_party/CMakeLists.txt +++ b/third_party/CMakeLists.txt @@ -1,6 +1,6 @@ # SPDX-License-Identifier: Apache-2.0 -if(NOT ${PROTOBUF_LOADED_AS_CONFIG}) +if(NOT ${ABSL_INCLUDED_WITH_PROTOBUF}) set(ABSL_PROPAGATE_CXX_STD ON) add_subdirectory(abseil-cpp) endif() From 96ca9fd8eeb9ec8b9dfd34ad29288ed943a69651 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 12:00:35 -0700 Subject: [PATCH 17/18] Updated. --- CMakeLists.txt | 10 ++++++---- third_party/CMakeLists.txt | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9127cd0d..42ad3892 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,12 +22,14 @@ include_directories(src) add_subdirectory(third_party) +# 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. TODO: Simplify -# once we can require cmake 3.27 (where CONFIG is default). +# available otherwise we fallback to the older protobuf method. find_package(Protobuf QUIET CONFIG) -set(ABSL_INCLUDED_WITH_PROTOBUF ON) -if(NOT ${Protobuf_FOUND}) +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) diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index 7b8d8e60..dfb1bc02 100644 --- a/third_party/CMakeLists.txt +++ b/third_party/CMakeLists.txt @@ -1,6 +1,8 @@ # SPDX-License-Identifier: Apache-2.0 -if(NOT ${ABSL_INCLUDED_WITH_PROTOBUF}) +if(${ABSL_INCLUDED_WITH_PROTOBUF}) + # Remove abseil-cpp from the repo when we can require cmake 3.27. +else() set(ABSL_PROPAGATE_CXX_STD ON) add_subdirectory(abseil-cpp) endif() From 73b48c97d77842de343bfec1a0f3acf61c3ba912 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 10 Jul 2023 12:40:10 -0700 Subject: [PATCH 18/18] Now calls third-party after checking protobuf dependencies to properly determine if abseil is needed. --- CMakeLists.txt | 4 ++-- third_party/CMakeLists.txt | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 42ad3892..bd7fe7e6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -20,8 +20,6 @@ option( include_directories(include) include_directories(src) -add_subdirectory(third_party) - # 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 @@ -35,6 +33,8 @@ else() 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/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index dfb1bc02..7b8d8e60 100644 --- a/third_party/CMakeLists.txt +++ b/third_party/CMakeLists.txt @@ -1,8 +1,6 @@ # SPDX-License-Identifier: Apache-2.0 -if(${ABSL_INCLUDED_WITH_PROTOBUF}) - # Remove abseil-cpp from the repo when we can require cmake 3.27. -else() +if(NOT ${ABSL_INCLUDED_WITH_PROTOBUF}) set(ABSL_PROPAGATE_CXX_STD ON) add_subdirectory(abseil-cpp) endif()