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

cyclic dependencies #3

Closed
nschloe opened this issue Apr 10, 2015 · 16 comments
Closed

cyclic dependencies #3

nschloe opened this issue Apr 10, 2015 · 16 comments

Comments

@nschloe
Copy link

nschloe commented Apr 10, 2015

albanyLib uses symbols from albanySTK (e.g., Albany::STKDiscretization::getOwnedDOF) which in turn uses symbols from albanyLib (e.g., createEpetraCommFromTeuchosComm). There's probably more.
How to best get this straight?

@ambrad
Copy link
Contributor

ambrad commented Apr 10, 2015

Hi Nico,

That's a big can of worms. Albany has a lot of circular dependencies. We
put the lib listing on the link line three times (though two ought to do
it) to resolve these.

May I ask why you're looking into these issues?

Cheers,
Andrew

On Fri, Apr 10, 2015 at 3:35 PM, Nico Schlömer [email protected]
wrote:

albanyLib uses symbols from albanySTK (e.g.,
Albany::STKDiscretization::getOwnedDOF) which in turn uses symbols from
albanyLib (e.g., createEpetraCommFromTeuchosComm). There's probably more.
How to best get this straight?


Reply to this email directly or view it on GitHub
https://github.com/gahansen/Albany/issues/3.

@nschloe
Copy link
Author

nschloe commented Apr 11, 2015

@ambrad I submitted a PR for TriBits (TriBITSPub/TriBITS#60) that changes Trilinos' exported CMake configuration files. To assert that the change doesn't break anything, @bartlettroscoe asked me to verify this against Albany builds. To make sure that all symbols are still defined, I enabled builds with -Wl,--no-undefined which brought this up.

@ambrad
Copy link
Contributor

ambrad commented Apr 11, 2015

Hi Nico,

Would it be enough to build with a basic but full configuration of Albany?
Something like this I think would be comprehensive:

cmake -DALBANY_TRILINOS_DIR:PATH=/path/to/trilinos/install
-DENABLE_LCM:BOOL=ON -DENABLE_HYDRIDE:BOOL=ON -DENABLE_FELIX:BOOL=ON
-DENABLE_AERAS:BOOL=ON -DENABLE_QCAD:BOOL=ON -DENABLE_MOR:BOOL=ON
-DENABLE_ATO:BOOL=ON -DENABLE_SEE:BOOL=ON -DENABLE_AMP:BOOL=ON ..

Cheers,
Andrew

On Sat, Apr 11, 2015 at 1:45 PM, Nico Schlömer [email protected]
wrote:

@ambrad https://github.com/ambrad I submitted a PR for TriBits (
TriBITSPub/TriBITS#60 TriBITSPub/TriBITS#60)
that changes Trilinos' exported CMake configuration files. To assert that
the change doesn't break anything, @bartlettroscoe
https://github.com/bartlettroscoe asked me to verify this against
Albany builds. To make sure that all symbols are still defined, I enabled
builds with -Wl,--no-undefined which brought this up.


Reply to this email directly or view it on GitHub
https://github.com/gahansen/Albany/issues/3#issuecomment-91915121.

@nschloe
Copy link
Author

nschloe commented Apr 12, 2015

Would it be enough to build with a basic but full configuration of Albany?

A standard build results in missing symbols from STK, for example, e.g.,

[...]
[ 96%] Built target albanySTKRebalance
Linking CXX executable Albany
[...]
disc/stk/libalbanySTK.so: undefined reference to `Ioss::Region::get_element_blocks() const'

Indeed, libalbanySTK is underlinked since it requires symbols from Trilinos::STK, but doesn't link against them. To fix this, I had added -Wl,--no-undefined which then showed those other errors.

@nschloe
Copy link
Author

nschloe commented Apr 12, 2015

I managed to compile Albany now, but these dependency loops should still be fixed.

If libAlbany and libAlbanySTK mutually depend on each other such that you can't use one without the other, one may ask why they are separated into two different libraries in the first place. One relatively easy to implement fix for this is to just all all sources to libAlbany.

@nschloe nschloe mentioned this issue Apr 12, 2015
@gahansen
Copy link
Member

Hi Nico,

Is this a remaining issue, or can I close this one too?

Glen

@bartlettroscoe
Copy link
Contributor

Why does Albany ha e circular dependencies?

@bartlettroscoe
Copy link
Contributor

How do you do shared pin builds?

@agsalin
Copy link
Contributor

agsalin commented Jan 29, 2016

There was a point a few years ago where Glen discovered that dumping all Albany source into a single library caused an error that was fixed when braking it into multiple libraries. This occurred when he upgraded to a newer gcc (4.7 perhaps). We then broke albany into pieces based on directories (also projects), like libLCM and libQCAD, which led to circular dependencies. We fixed that by adding the libraries multiple times on the link line.

Since this has not caused problems, we haven't seen the need to fix it. Is there a reason to?

@bartlettroscoe
Copy link
Contributor

Why not just make Albany one big library since that is what it is anyway if you have circular dependencies? Then you can use shared libs.

@agsalin
Copy link
Contributor

agsalin commented Jan 29, 2016

Albany works fine with shared builds, as far as I know. I think the opposite behaviors is true: that shared libs allow you to get away with circular dependencies without repeating them on the link line.

I'm not sure if we would have the same problem that Glen saw before, like there was a limit of how many *.o's could be linked in a single library and we were exceeding it.

Anyway, I'm curious if there is any harm in what we are doing currently.

@gahansen
Copy link
Member

I don't recall the version of gcc - back at about 4.7 several users encountered a linking error with debug builds. I don't recall if it was a file size issue or a number of symbols issue, but we resolved it at the time by breaking the single library into multiple. The original reason for doing this probably no longer valid, but I don't know if anyone has looked at it again since then.

@bartlettroscoe
Copy link
Contributor

Can't do circular shared libs

https://cmake.org/cmake/help/v3.3/command/target_link_libraries.html

@bartlettroscoe
Copy link
Contributor

Nico,

I don't think we need to force Albany to clean this up (it is not really any of our business). We just need to add a configure-time option for the TriBITS/Trilinos build to disable -Wl,--no-undefined when Albany developers need to build/install Trilinos to use with Albany. It is just that the default development build of Trilinos will have this turned on by default.

@nschloe
Copy link
Author

nschloe commented Feb 2, 2016

It is just that the default development build of Trilinos will have this turned on by default.

If trilinos/Trilinos#2 gets in, yes.

We just need to add a configure-time option for the TriBITS/Trilinos build to disable -Wl,--no-undefined when Albany developers need to build/install Trilinos to use with Albany.

The Albany build is separate from the Trilinos build, right? First you build Trilinos (with whatever compiler flags, -Wl,--no-undefined & friends), install it, then build Albany with whatever other flags. Right?

ibaned added a commit that referenced this issue Jun 30, 2016
This fixes a link error when building Albany LCM
atop a Trilinos built with shared libraries.
When LCM_TEST_EXES and ALBANY_BGL are true, LCM
specifies its own target_link_libraries to its
test executables such as PartitionTest
(see src/LCM/CMakeLists.txt:426).
When these are shared libraries, CMake removes
duplicates and orders them according to the DAG
is knows about.
For the AlbanyT link line, albanySTKRebalance
comes after albanySTK.
However, before this commit their order would be
reversed when building the LCM test executables,
resulting in undefined references to rebalance
symbols in albanySTK.
After this commit the link succeeds.

This is somewhat related to #3
ibaned added a commit that referenced this issue Jul 19, 2016
this program triggers more of the circular
dependencies in Albany that are only currently
worked around by repeating static libraries.

this is related to issue #3.
@gahansen
Copy link
Member

I am going to close this issue as we are addressing Albany's circular dependency concerns in the new issue #579 .

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

No branches or pull requests

5 participants