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

Resolve circular dependencies #2023

Open
rrsettgast opened this issue Aug 22, 2022 · 3 comments
Open

Resolve circular dependencies #2023

rrsettgast opened this issue Aug 22, 2022 · 3 comments
Assignees
Labels
type: cleanup / refactor Non-functional change (NFC)

Comments

@rrsettgast
Copy link
Member

Describe the issue
At some point we were having problems running nsightcompute when dynamically linking our libraries, so we switched to static linking. This is undesirable for two reasons:

  1. GEOSX is LGPL, so we should be dynamically linking, and
  2. the size of the statically linked unit tests are very large. Each executable contains all the static objects. Not good

Proposed cleanup
Switch back to dynamic linking as default. I think that this requires us to clean up our circular dependencies once and for all. We should maintain the option to statically link in case there are any problems later.

@bmhan12
Copy link
Contributor

bmhan12 commented Sep 6, 2022

On the note of circular dependencies, I ran the tools we added last time we looked at circular dependencies in #1352:

Graphviz

Running config-build.py with the --graphviz option (code link) produces the graph of the dependencies according to CMake.

Note: In order to get this graph, I built GEOSX with shared libraries on and object libraries turned off. For whatever reason, I couldn't get CMake to detect the object libraries registered through blt to show up.

dependency_graphviz

By CMake's view, we don't have any circular dependencies currently.

circular_dependencies.sh

This script produces two files:
circular_dependencies.txt and dependency_matrix.txt (Excel view).

circular_dependencies.txt shows what header includes exist between two components.

Currently, there are 9:

  1. common & codingUtilities
  2. common & mesh
  3. codingUtilities & mainInterface
  4. constitutive & mesh
  5. constitutive & fileIO
  6. constitutive & physicSolvers
  7. mesh & finiteElement
  8. finiteVolume & physicsSolvers
  9. physicsSolvers & mainInterface

dependency_matrix.txt shows if a component contains headers to another component (yes/no)

find_cycles.py

I did the following to run this script:

ml ninja (load the module)
 
./scripts/config-build.py -hc host-configs/LLNL/quartz-clang\@10.0.0.cmake -ecc -G Ninja
 
python3 ./scripts/find_cycles.py -c build-quartz-clang\@10.0.0-debug/compile_commands.json

Output on quartz: find_cycles_quartz_output.txt

The output shows 42 cycles, but most of them look to be outside GEOSX and in individual third-party libraries.
Notably, the first cycle is a header including itself.

@TotoGaz
Copy link
Contributor

TotoGaz commented Sep 6, 2022

This is a really fruitful analysis, thx @bmhan12

In a nutshell, is it fair to state that we cannot really find real C++ cycles, but that our C++ architecture does not respect our cmake architecture?

PS: There is an option in find_cycles .py to remove some directories.
PPS: find_cycles.py could be improved to add the cmake clustering into the game! It's missing this option to find this other kind of cycles.

@TotoGaz TotoGaz added the type: cleanup / refactor Non-functional change (NFC) label Jan 31, 2023
@TotoGaz TotoGaz removed the type: build system Build system issue label Feb 18, 2023
@TotoGaz TotoGaz removed their assignment Jul 3, 2024
@rrsettgast rrsettgast reopened this Jul 17, 2024
@rrsettgast rrsettgast changed the title modify build strategy to use dynamic linking again. Resolve circular dependencies Jul 17, 2024
@rrsettgast rrsettgast assigned MelReyCG and wrtobin and unassigned wrtobin Jul 17, 2024
@rrsettgast
Copy link
Member Author

The current picture from cmake not looking good:
dependency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Development

No branches or pull requests

6 participants