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

Switch to current correct TriBITS usage of libraries for TeuchosParameterList #778

Merged

Conversation

bartlettroscoe
Copy link
Contributor

This Albany CMakeLists.txt file was making assumptions about the implementation details of the TriBITS-generated <Package>Config.cmake files that it should not have been making. It was assuming that the raw TriBITS target
'teuchosparameterlist' existed which is a no-no. The correct old-school TriBITS usage is ${TeuchosParameterList_LIBRARIES} and the associated include directories.

This works with old TriBITS and refactored TriBITS as documented in detail with complete reproduction instructions in the internal issue comment:

With this, Albany builds against Trilinos built with the branch in trilinos/Trilinos#9978 for the repo versions:

*** Git Repo: Albany
a4ddf552088a876f908b0721010d38feca0d6a4a [Wed Jan 12 09:23:56 2022 -0700] <[email protected]>
Switch to current correct TriBITS usage of libraries for TeuchosParameterList
*** Git Repo: Trilinos
3ff4b9c0c7d0833547348ad3c5d607c0d0d65d58 [Tue Jan 11 20:05:37 2022 -0700] <[email protected]>
Merge commit '7b1fc5a1bf77449b6eeb53996cf30bcf15d0a7ee' into tribits-299-modern-cmake-targets-1-again-albany

and passes the Albany test suite run on

$ rm ctest.out ; time ctest -j15 &> ctest.out ; grep -B 1 -A 100 "failed out of" ctest.out

real    4m57.432s
user    77m24.118s
sys     9m17.570s

100% tests passed, 0 tests failed out of 137

Label Time Summary:
Adjoint        = 402.93 sec*proc (8 tests)
Analysis       = 158.33 sec*proc (4 tests)
Basic          = 1408.41 sec*proc (75 tests)
Demo           = 1391.30 sec*proc (46 tests)
Epetra         = 941.97 sec*proc (53 tests)
Forward        = 2238.45 sec*proc (109 tests)
ROL            = 107.63 sec*proc (3 tests)
RegressFail    = 155.54 sec*proc (3 tests)
Serial         = 654.27 sec*proc (19 tests)
Tempus         =  15.16 sec*proc (1 test)
Tpetra         = 1857.73 sec*proc (68 tests)

Total Test time (real) = 297.39 sec

This is related to trilinos/Trilinos#9972 and PR trilinos/Trilinos#9978

…eterList

This Albany CMakeLists.txt was making assumptions about the implentation
details of the TriBITS-generated <Package>Config.cmake files that it should
not have been making.  It was assuming that the raw TriBITS target
'teuchosparameterlist' existed which is a no-no.  The correct old-school
TriBITS usage is ${TeuchosParameterList_LIBRARIES} and the associated include
directories.

This works with old TriBITS and refactored TriBITS.

This is related to trilinos/Trilinos#9972 and PR trilinos/Trilinos#9978.
@bartlettroscoe
Copy link
Contributor Author

@ikalash, note, while this makes Albany work against Trilinos with the old TriBITGS and new TriBITS build systems, I am not sure we should merge this quite yet for Albany. The reason is that if other APPs are directly linking to raw IMPORTED targets instead of using the documented variables, then we may need to consider refactoring the updated TriBITS for now keep providing these old non-namespaced targets.

@bartgol
Copy link
Collaborator

bartgol commented Jan 12, 2022

@bartlettroscoe I'm not sure I understand why this PR should not be merged. Or did you mean that the trilinos PR should not be merged yet, and this PR won't work until that one goes in?

@bartlettroscoe
Copy link
Contributor Author

I'm not sure I understand why this PR should not be merged.

@bartgol, it is perfectly safe and proper to merge this PR to Albany 'master'. It is just that if we go with option-1 described in TriBITSPub/TriBITS#299 (comment) and bring back (temporarily) the non-namespaced IMPORTED targets like teuchosparameterlist then leaving Albany/src/CMakeLists.txt provides a test case for this. That is all. But I can test with Albany with this commit reverted as well so that is not a reason not to merge this PR.

So go ahead and merge the PR. That will be safe no matter how things are resolved.

@bartgol
Copy link
Collaborator

bartgol commented Jan 13, 2022

Ok, thanks. If that's the case, I'll go ahead and merge. I feel much better using a variable-based approach than a hard-coded name.

Thanks for the fix!

@bartgol bartgol self-requested a review January 13, 2022 00:04
@bartgol bartgol merged commit ae94a50 into sandialabs:master Jan 13, 2022
@mperego
Copy link
Collaborator

mperego commented Jan 13, 2022

Thanks for the fix @bartlettroscoe !

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