Skip to content
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

fix: correct cmake library interdependencies #100

Merged
merged 6 commits into from
Mar 15, 2024

Conversation

EpsilonPrime
Copy link
Member

@EpsilonPrime EpsilonPrime commented Mar 14, 2024

This eliminates the cycles described in #99.

PlanPrinterVisitor is now part of symbol_table.
BasePlanProtoVisitor (which PlanPrinterVisitor depends on) is now its own library.

@ingomueller-net
Copy link
Contributor

ingomueller-net commented Mar 14, 2024

Awesome -- thank you for the quick fix! Probably a much better fix than what I had in mind :)

For completeness, here are the dependency graphs before and after this PR. Before the change, we can see several inter-dependent targets; after the change, it's a nice DAG.

Before:

before

After:

after

I generated them running cmake --graphviz=foo.dot .. -DBUILD_SHARED_LIBS=OFF && dot -Tpng -o foo.png foo.dot in the build folder, where I put a file called CMakeGraphVizOptions.cmake with the following content (for removing the external dependencies from the graph):

set(GRAPHVIZ_IGNORE_TARGETS "absl_*;gmock*;gtest*;protobuf*;yaml-cpp*;fmt*;-lrt;Threads*;/usr/*;.*_test"

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve if CI passes. I won't pretend to have fully internalized these changes but I don't see anything odd or out of place.

@EpsilonPrime EpsilonPrime changed the title bug: fix cmake target dependencies fix: correct cmake library interdependencies Mar 15, 2024
@EpsilonPrime
Copy link
Member Author

Thanks for finding this issue as well as providing the visualization of the dependencies. I'll probably use this trick to prevent such issues in the future.

@EpsilonPrime EpsilonPrime merged commit fd555a6 into substrait-io:main Mar 15, 2024
3 checks passed
@EpsilonPrime EpsilonPrime deleted the fix_cmake_targets branch March 15, 2024 08:11
@ingomueller-net
Copy link
Contributor

Thanks for finding this issue as well as providing the visualization of the dependencies. I'll probably use this trick to prevent such issues in the future.

Sure thing! Another possibility is to (also) build with BUILD_SHARED_LIBS in CI...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants