fix: correct cmake library interdependencies (#100)

This eliminates the cycles described in #99. PlanPrinterVisitor is now part of symbol_table. BasePlanProtoVisitor (which PlanPrinterVisitor depends on) is now its own library.
substrait-io · Mar 15, 2024 · fd555a6 · fd555a6
1 parent 3f84014
commit fd555a6
Show file tree

Hide file tree

Showing 9 changed files with 34 additions and 18 deletions.
diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh
@@ -18,8 +18,6 @@ sudo --preserve-env apt install -y \
  clang-tidy \
  git \
  wget \
- libprotobuf-dev \
- libprotobuf23 \
  protobuf-compiler \
  clang-format \
  uuid-dev \

diff --git a/src/substrait/proto/CMakeLists.txt b/src/substrait/proto/CMakeLists.txt
@@ -84,5 +84,7 @@ add_library(substrait_proto ${PROTO_SRCS} ${PROTO_HDRS} ProtoUtils.cpp
 target_link_libraries(substrait_proto protobuf::libprotobuf)
 
 # Make sure we can see our own generated include files.
-target_include_directories(substrait_proto
- PUBLIC "${PROTO_OUTPUT_TOPLEVEL_DIR}/src")
+target_include_directories(
+ substrait_proto
+ PUBLIC "${PROTO_OUTPUT_TOPLEVEL_DIR}/src"
+ PUBLIC "${protobuf_SOURCE_DIR}/src")
diff --git a/src/substrait/textplan/CMakeLists.txt b/src/substrait/textplan/CMakeLists.txt
@@ -7,6 +7,8 @@ add_library(
  symbol_table
  Location.cpp
  Location.h
+ PlanPrinterVisitor.cpp
+ PlanPrinterVisitor.h
  StringManipulation.cpp
  StringManipulation.h
  SymbolTable.cpp
@@ -23,8 +25,15 @@ add_library(parse_result ParseResult.cpp ParseResult.h)
 add_dependencies(symbol_table substrait_proto substrait_common absl::strings
  fmt::fmt-header-only)
 
-target_link_libraries(symbol_table fmt::fmt-header-only absl::strings
- substrait_textplan_converter)
+target_link_libraries(
+ symbol_table
+ substrait_base_proto_visitor
+ substrait_proto
+ substrait_common
+ substrait_expression
+ absl::strings
+ fmt::fmt-header-only
+ date::date)
 
 # Provide access to the generated protobuffer headers hierarchy.
 target_include_directories(symbol_table

diff --git a/...textplan/converter/PlanPrinterVisitor.cpp → ...substrait/textplan/PlanPrinterVisitor.cpp b/...textplan/converter/PlanPrinterVisitor.cpp → ...substrait/textplan/PlanPrinterVisitor.cpp
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: Apache-2.0 */
 
-#include "substrait/textplan/converter/PlanPrinterVisitor.h"
+#include "substrait/textplan/PlanPrinterVisitor.h"
 #include <unistd.h>
 
 #include <sstream>

diff --git a/...t/textplan/converter/PlanPrinterVisitor.h → src/substrait/textplan/PlanPrinterVisitor.h b/...t/textplan/converter/PlanPrinterVisitor.h → src/substrait/textplan/PlanPrinterVisitor.h
diff --git a/src/substrait/textplan/SymbolTablePrinter.cpp b/src/substrait/textplan/SymbolTablePrinter.cpp
@@ -8,9 +8,9 @@
 #include "substrait/common/Exceptions.h"
 #include "substrait/proto/algebra.pb.h"
 #include "substrait/textplan/Any.h"
+#include "substrait/textplan/PlanPrinterVisitor.h"
 #include "substrait/textplan/StructuredSymbolData.h"
 #include "substrait/textplan/SymbolTable.h"
-#include "substrait/textplan/converter/PlanPrinterVisitor.h"
 
 namespace io::substrait::textplan {
 

diff --git a/src/substrait/textplan/converter/CMakeLists.txt b/src/substrait/textplan/converter/CMakeLists.txt
@@ -5,30 +5,36 @@ include(../../../../third_party/datetime.cmake)
 set(TEXTPLAN_SRCS
  InitialPlanProtoVisitor.cpp
  InitialPlanProtoVisitor.h
- BasePlanProtoVisitor.cpp
- BasePlanProtoVisitor.h
  PipelineVisitor.cpp
  PipelineVisitor.h
- PlanPrinterVisitor.cpp
- PlanPrinterVisitor.h
  LoadBinary.cpp
  LoadBinary.h
  SaveBinary.cpp
  SaveBinary.h
  ParseBinary.cpp
  ParseBinary.h)
 
+add_library(substrait_base_proto_visitor BasePlanProtoVisitor.cpp
+ BasePlanProtoVisitor.h)
+
+target_link_libraries(
+ substrait_base_proto_visitor
+ substrait_common
+ substrait_proto
+ error_listener
+ fmt::fmt-header-only
+ absl::status
+ absl::statusor)
+
 add_library(substrait_textplan_converter ${TEXTPLAN_SRCS})
 
 target_link_libraries(
  substrait_textplan_converter
+ substrait_base_proto_visitor
  substrait_common
- substrait_expression
- substrait_io
  substrait_proto
  symbol_table
  error_listener
- date::date
  fmt::fmt-header-only
  absl::status
  absl::statusor)
@@ -39,7 +45,7 @@ endif()
 
 add_executable(planconverter Tool.cpp)
 
-target_link_libraries(planconverter substrait_textplan_converter)
+target_link_libraries(planconverter substrait_textplan_converter substrait_io)
 
 set(NORMALIZER_SRCS ReferenceNormalizer.cpp ReferenceNormalizer.h)
 

diff --git a/src/substrait/textplan/converter/ParseBinary.cpp b/src/substrait/textplan/converter/ParseBinary.cpp
@@ -3,9 +3,9 @@
 #include "substrait/textplan/converter/ParseBinary.h"
 
 #include "substrait/proto/plan.pb.h"
+#include "substrait/textplan/PlanPrinterVisitor.h"
 #include "substrait/textplan/converter/InitialPlanProtoVisitor.h"
 #include "substrait/textplan/converter/PipelineVisitor.h"
-#include "substrait/textplan/converter/PlanPrinterVisitor.h"
 
 namespace io::substrait::textplan {
 

diff --git a/src/substrait/textplan/tests/CMakeLists.txt b/src/substrait/textplan/tests/CMakeLists.txt
@@ -4,7 +4,8 @@ add_library(parse_result_matchers ParseResultMatchers.cpp ParseResultMatchers.h)
 
 add_dependencies(parse_result_matchers parse_result)
 
-target_link_libraries(parse_result_matchers parse_result substrait_proto gmock)
+target_link_libraries(parse_result_matchers parse_result symbol_table
+ substrait_proto gmock)
 
 add_test_case(
  symbol_table_test