-
Notifications
You must be signed in to change notification settings - Fork 45
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
Convert TriBITS over to use self-contained modern CMake targets for internal packages (#299) #424
Merged
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
This hyperlink spelling did not git fixed when the anchor got fixed in commit c716a74. This link was broken in the RST documentation.
I had forgotten to remove these tests when I copied them over to the file TribitsExampleMetaProject_Tests.cmake and then include that file.
bartlettroscoe
force-pushed
the
299-modern-cmake-targets-1
branch
from
October 18, 2021 20:03
654dd43
to
c520cb7
Compare
I messed up again in my earlier refactoring when I split off the tests into different files.
Noticed this while looking over code as part of working on #299.
* Switch to include_guard() * Remove hack for CMAKE_CURRENT_LIST_DIR needed for CMake versions < 2.8.3 * Quote some file paths * Reformat to be less wide
This also provides the INTERFACE_INCLUDE_DIRECTORIES property of the libraries in the installed <Package>Config.cmake file!
Now that the imported targets have the INTERFACE_INCLUDE_DIRS property set, there is no need to explicitly set the include dirs. CMake does it automatically!
The TribitsExampleApp tests using TribitExProj <Package>Config.cmake files from the build dir were broken so this commit fixes that and more. Had to do several things that get the tests passing again: * Moved all of the <Package>Config.cmake files generated in the build tree to be under <buildDir>/cmake_packages/<packageName>/ to make them easy to find wtih find_package(). (See entry to release notes.) This allowed striping out code from TribitsExampleApp that had to do a complex find(GLOB_RECURSE ...) to find *Config.cmake files and then get the full set of directories and prepend that to CMAKE_PREFIX_PATH. Now it just prepends <buildDir>/cmake_packages. * Changed from generating a single export targets file <buildDir>/<Project>Targets.cmake included by all of the <Package>Config.cmake files, to generating a different <Package>Targets.cmake file for each package separately. I am not sure this is correct for packages and subpackages but the existing tests seem to pass okay so let's go with it for now. * Had to change the arguments to tribits_write_flexible_package_client_export_files() to allow calling correctly during unit testing. I think the arguments are more clear now anyway. * Changed name from tribits_write_project_client_export_files_install_targets() to tribits_write_package_client_export_files_install_targets() and added argument packageConfigForInstallBaseDir to eliminate duplication. I also added Maintainers Guide documentation for this entry. * I updated the unit tests for tribits_write_flexible_package_client_export_files() to check the generated <Package>Config.cmake files before libs get added to the package. But I don't think it necessarily makes sense to write a <Package>Config.cmake file for even the build tree before the libraries get added for a package. However, there may be a use case for this in the future for inserted packages that use the CMake build system and need to get upstream dependencies. But I think they would just call find_package() which would find <Package>Config.cmake files in the build tree for upstream packages and TPLs. But I will leave these tests for now. * NOTE: The export(EXPORT ...) command does not generate a <Package>ConfigTargets.cmake file that also sets the INTERFACE_INCLUDE_DIRECTORIES target property. Therefore, the customer has to manually deal with the include directories so I had to conditionally add back manually setting the include directories in the app when using <Package>Config.cmake files from the upstream build tree. This is very bad (and I will report this to Kitware). There was a very simple copy-and-paste defect (that will be obvious when you look at the diff). With this, TribitsExampleApp can build against all of the TribitsExProj packages from the build tree, including WithSubpackages. The generated <Package>Config.cmake files like correct for these cases given the current TriBITS implementation. As of this point, the only problem with the current implementation that tries to use target_include_directories() is that export(EXPORT ...) does not set the INTERFACE_INCLUDE_DIRECTORIES in the generated <Package>Targets.cmake file. I will file a bug to Kitware about that one.
This is not being used anywhere but I put in a temp print statement to show that it is being created correctly. NOTE: This will be used in the future to allow downstream packages to use raw cmake to creating targets.
I have not actually used this target but it seems to do the right thing, mostly. It will need to be fixed up but it is close. I also added a function tribits_get_all_build_targets_including_in_subdirs() that may be handy in the future. This can replace a function I wrote for build-stats in Trilinos.
…make for build tree (#299) Now external clients get the include directories correctly from <Package>Config.cmake files from the build tree or the install tree.
This changes to use <Package>:: namespaced targets for libraries created in TriBITS packages. Since this also supports installation testing that reads in <Package>Config.cmake files, this has to also change over the ${PACKAGE_NAME}_LIBRARIES variable to store a list of namespaced library targets. This maintains perfect backward compatiblity as long as clients are they are just linking against ${${PACKAGE_NAME}_LIBRARIES} as now the imported targets are namespaced as well. Very nice.
This gets some code inside an if statement.
…#299) * Updated TribitsExampleApp to use these targets and stop using any XXX_LIBRARIES or XXX_INCLUDE_DIRS variables at all.
bartlettroscoe
force-pushed
the
299-modern-cmake-targets-1
branch
from
October 18, 2021 21:08
c520cb7
to
5b59795
Compare
* Removed the option TribitsExApp_FIND_UNDER_BUILD_DIR and the function getTribitsExProjStuffByPackageUnderBuildDir() as you can now just set CMAKE_PREFIX_DIR=<buildDir>/cmake_packages and it will find all of these packages. * Remove unneeded temp var APP_DEPS_PACKAGE_LIB_TARGETS. * Add function setCompilersForAppFromConfigFileCompilers(prefix) and call for both cases where TribitsExProjConfig.cmake and individual <Package>Config.cmake files are read in. * Change name of getTribitsExProjStuff() to getTribitsExProjStuffForApp() and append 'ForApp' to macros it calls as well. * Factor out function getExpectedAppDepsStr() from CMakeLists.txt to abstract this operation some and make CMakeLists.txt fit in one screen. * Change name of appendDepsStr() to appendExpectedAppDepsStr(). * Rearrange lines in CMakeLists.txt some. * Removed some commented-out code to run test with MPI. * Added some more comments.
This is just a simpler version of TribitsExampleApp that does not have any conditional behavior and therefore is easier to follow.
bartlettroscoe
force-pushed
the
299-modern-cmake-targets-1
branch
from
October 19, 2021 14:05
5b59795
to
c111d68
Compare
bartlettroscoe
changed the title
WIP: Convert TriBITS over to use self-contained modern CMake targets - 1 (#299)
Convert TriBITS over to use self-contained modern CMake targets for internal packages (#299)
Oct 19, 2021
10 tasks
I am going to merge this and keep working on #299. A post-merge review can occur. |
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.
Addressing #299
This PR converts internal TriBITS packages to use modern CMake targets according to the specification given in #342 and in this Google Doc. This includes:
target_include_directories()
to attach the include directories to all library targets created for internal packagesALIAS
namespaced targets<Package>::<libname>
for each library created in a package<Package>::all_libs
targets for each package<Package>Config.cmake
with<Pacakge>::
<Project>::all_libs
and<Project>::all_selected_libs
in installed<Project>Config.cmake
fileTribitsExampleApp
to only use<Project>::all_selected_libs
,<Project>::all_ibs
or individual<Package>::all_libs
targets and remove all CMake code that deals with include directories<Package>Config.cmake
files generated in the build tree under the<buildDir>/cmake_package/<Package>/
directory to make easy to find byfind_package()
and get<Package>Config.cmake
working for all packages (even those with subpackages).TribitsSimpleExampleApp
that makes very obvious how using the updated*Config.cmake
files and IMPORTED targets really simplifies things.See the individual commit messages, code comments, and diff for more details.
NOTE: Things that have not been done in this PR but will be addressed in future PRs to fully address #299 include:
<tplName>Config.cmake
files and IMPORTED targets).What to review
This is an intermediate refactoring of some pretty complex code so I don't expect a detailed line-by-line review. But I would like a detailed review of the examples and the documentation including:
Reviews of these files can be done here.
How this PR was this tested: