Switch over fully to modern CMake targets and updates for testing against Trilinos (#299) #479
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bartlettroscoe
force-pushed
the
299-switch-to-modern-cmake-targets
branch
2 times, most recently
from
d6d80f5 to
1b00d18
Compare
bartlettroscoe
changed the base branch from
master
to
63-299-flat-package-deps-list
bartlettroscoe
force-pushed
the
299-switch-to-modern-cmake-targets
branch
5 times, most recently
from
06fa906 to
025fe3a
Compare
bartlettroscoe
force-pushed
the
299-switch-to-modern-cmake-targets
branch
from
9a2adfd to
320ef01
Compare
2 tasks
bartlettroscoe
force-pushed
the
63-299-flat-package-deps-list
branch
from
ad464df to
e1b3085
Compare
bartlettroscoe
force-pushed
the
299-switch-to-modern-cmake-targets
branch
from
320ef01 to
29d7410
Compare
bartlettroscoe
changed the base branch from
63-299-flat-package-deps-list
to
master
|
Now that PR #480 has been merged, I could change the target branch to 'master' and now we get auto GitHub Action testing. Not much left to do before this gets merged! |
bartlettroscoe
changed the title
This was referenced May 20, 2022
Closed
Merged
bartlettroscoe
changed the title
|
@KyleFromKitware, can you please do a quick light review of this PR? Mainly just look over the changes/additions to CHANGLOG.md and TribitsGuidesBody.rst pointed to above in the "Review Checklist". To understand the difference this refactoring makes to downstream CMake projects, compare: |
bartlettroscoe
changed the title
|
FYI: In testing with Albany + Trilinos, there were several issues discovered that will need to be tweaked with TriBITS. I will push WIP commits that validate against Albany, and then I will fix up the TriBITS tests and documentation to match. |
When you have a lot of packages, you have to avoid including their <Package>Config.cmake file more than once during a find_packge() call. Without these include guards, it took like 24 minutes to configure Albany. When I put them in, the configure time for Albany went down to just 7 sec!
…all tree (#299) Turns out that Albany explicitly looks for files under ${Trilinos_INCLUDE_DIRS}. Also, Albany was looking for files like "${Trilinos_INCLUDE_DIRS}/Kokkos_config.h" so it basically requires Trilinos_INCLUDE_DIRS to be just a single directory! Therefore, to avoid having to change Albany, I have set <Project>_INCLUDE_DIRS in the installed <Project>Config.cmake file. This was easy to do for this case. But note that <Project>_INCLUDE_DIRS in <Project>Config.cmake in the build dir still will be set to empty because there is no easy way to set it in that case. This is a stop-gap measure to allow APPs like Albany to keep working during this initial transition to modern CMake. But we really should remove this var from the <Project>Config.cmake file entirely at some point. NOTE: I have not fixed all of the TriBITS tests for this change yet. That is why this is a 'WIP' commit. I will fix the tests in a later commit.
I was trying too reproduce failure I saw testing with Albany where it was trying to link against MPI::all_libs. But this did not do it. But it is nice to have a test case where the MPI TPL is enabled and propgated to a downstream APP.
This will help to expose a defect in TriBITS and provides better TriBITS testing realted to the TriBITS MPI TPL.
This produces the same error I saw when testing Albany against Trilinos and
updated TriBITS:
CMake Error at CMakeLists.txt:??? (add_executable):
Target "app" links to target "MPI::all_libs" but the target was not found.
Perhaps a find_package() call is missing for an IMPORTED target, or an
ALIAS target is missing?
To generate this error, I add to add linking against
${TribitsExProj_TPL_LIBRARIES} which is what Albany currently does as well.
I think this is a common usage of old-style TriBITS so it is important to test
this case.
This avoids needing to check `if (TARGET <Pkg>::all_libs)` all over the place. This makes the failing test: TriBITS_TribitsOldSimpleExampleApp_STATIC_MPI_USE_DEPRECATED_TARGETS from the previous commit pass. Next, we can take out all of these silly checks.
Now that the MPI TPL produces MPI::all_libs, there is no more need for these silly checks. This also makes the code have stronger checking in case of mistakes.
bartlettroscoe
force-pushed
the
299-switch-to-modern-cmake-targets
branch
from
90d14c9 to
700f916
Compare
| foreach(depPkg IN LISTS ${PACKAGE_NAME}_TEST_ENABLED_DEPENDENCIES) | ||
| if (TARGET ${depPkg}::all_libs) | ||
| target_link_libraries(${EXE_BINARY_NAME} PUBLIC ${depPkg}::all_libs) | ||
| endif() |
There was a problem hiding this comment.
I see a lot of instances where you have the exact same logic for two different lists. You can combine both lists in foreach(IN LISTS). For example:
foreach(depPkg IN LISTS ${PACKAGE_NAME}_LIB_ENABLED_DEPENDENCIES ${PACKAGE_NAME}_TEST_ENABLED_DEPENDENCIES)
if(TARGET ${depPkg}::all_libs)
target_link_libraries(${EXE_BINARY_NAME} PUBLIC ${depPkg}::all_libs)
endif()
endforeach()There was a problem hiding this comment.
That is a good point. Please flag other instances you see like this
bartlettroscoe
commented
May 31, 2022
…n-cmake-targets I resolved the trivial conflicts in the files: * test/core/CMakeLists.txt * test/core/TestingFunctionMacro_UnitTests.cmake by just running the updated test suite and copy and pasting the number. This is one of the bad aspects about asserting the total number of unit tests run. It does not merge well between branches.
This fixes the test suite when the TriBITS project is configured with TPL_ENABLE_MPI=OFF.
) It would be bad to link non-testonly libs to these libraries because that would violate the TriBITS dependency handling system. It would allow a package to depend on a package listed as a test dependency and not listed in the lib dependency. And various algorithms like enabling downstream package and tests would be incorrect. Note that I actually had a test that showed this problem. The executable c/c_util was showing linking against libmixedlang.a but none of the dependencies of that (sub)package or its upstream packages have a lib dependency on the MixedLang package. This library was coming in through the TEST dependency on MixedLang in the package WithsubpackagesB. I was sloppy in just accepting the link line in this case as I did not carefully manaully inspect the package lib dependencies.
Might as well take advantage of the foreach( ... IN LISTS ...) command to make the code more compact and reduce duplication. This was based on review feedback from @KyleFromKitware.
bartlettroscoe
changed the title
|
Okay, I think this is ready to merge, and then on to sync this version of TriBITS to Trilinos! NOTE: I may still find issues with SPARC testing but I am hopeful that I will not. |
rabartlett1972
approved these changes
Jun 8, 2022
There was a problem hiding this comment.
@KyleFromKitware reviewed the bulk of this and I addressed his comments and suggestions. All of the TriBITS tests pass and I tested this against Albany and Nalu (see internal tribitsrefactoring#4 and internal tribitsrefactoring#5). I will test against SPARC next and will create a new TriBITS PR if I find any other issues.
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contain the final changes to switch over the only modern CMake targets
<Package>::all_libsfor all internal packages and external packages/TPLs. This strips out all of the old TriBITS logic that manually handled the list of include dirs and libraries. Modern CMake targets are now used for handling off of that. And as you can see, more lines of code were deleted than added in this changeset.The updated logic in
tribits_add_library()andtribits_add_executable()is now pretty easy to follow. There is no more complex magic.This PR also contain any changes needed for building Trilinos 'develop' with this version of TriBITS. (I just copied over the FindTPLNetcdf.cmake file from Trilinos).
This PR, however, does contain more backward compatible breaking changes (see the updates to the
CHANGELOG.mdfile).Please see the individual commit log messages for more details.
Review Checklist
CHANGLOG.mdTribitsGuidesBody.rst