-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Break cyclic dependencies of CMake targets #99
Comments
OK, one cycle can be broken very easily: diff --git a/src/substrait/textplan/converter/CMakeLists.txt b/src/substrait/textplan/converter/CMakeLists.txt
index 5ee2753..f328f81 100644
--- a/src/substrait/textplan/converter/CMakeLists.txt
+++ b/src/substrait/textplan/converter/CMakeLists.txt
@@ -24,7 +24,6 @@ target_link_libraries(
substrait_textplan_converter
substrait_common
substrait_expression
- substrait_io
substrait_proto
symbol_table
error_listener |
The other cycle is due to the two functions I'm calling it a day now but I'll probably try that out and create a PR tomorrow if nobody beats me to it. |
This eliminates the cycles described in #99. PlanPrinterVisitor is now part of symbol_table. BasePlanProtoVisitor (which PlanPrinterVisitor depends on) is now its own library.
Resolved by #100. |
There seem to be several cycles in the dependency graph of CMake targets. This isn't a problem for compiling static libraries, but if I build with
cmake -DBUILD_SHARED_LIBS=ON ...
, I get the following error (uninteresting parts omitted):I understand that there are reasons not to build shared libraries, but I argue that there are situations where it is useful. (I am working on a project with many top-level binaries that share 99% of the code and compilation and disk usage is a lot better with shared libs, so I hugely prefer that for developing and CI.)
I am currently trying to consume
substrait-cpp
as a dependency usingadd_subdirectory
. Since I build my top-level project withBUILD_SHARED_LIBS
,substrait-cpp
is built with the same configuration. For any scenario where that flag is desired or required for the top-level project, support that flag would be great.Finally, I believe that cyclic dependencies are often an indication for sloppy design or code organization and therefor a good idea to clean up. However, I haven't looked into the cycles yet, so I don't know yet whether there are good reasons to have them or things have just grown this way...
The text was updated successfully, but these errors were encountered: