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.

Sign up for GitHub

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

Jump to bottom

Convert TriBITS over to use self-contained modern CMake targets for internal packages (#299) #424

Merged
merged 17 commits into from
Oct 19, 2021
Merged

Convert TriBITS over to use self-contained modern CMake targets for internal packages (#299) #424

merged 17 commits into from
Oct 19, 2021

Conversation

bartlettroscoe
Copy link
Member

@bartlettroscoe bartlettroscoe commented Oct 12, 2021 •
edited
Loading

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:

  • Using target_include_directories() to attach the include directories to all library targets created for internal packages
  • Creating ALIAS namespaced targets <Package>::<libname> for each library created in a package
  • Add <Package>::all_libs targets for each package
  • Namespace all library targets in the generated <Package>Config.cmake with <Pacakge>::
  • Add targets <Project>::all_libs and <Project>::all_selected_libs in installed <Project>Config.cmake file
  • Refactor TribitsExampleApp 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
  • Move <Package>Config.cmake files generated in the build tree under the <buildDir>/cmake_package/<Package>/ directory to make easy to find by find_package() and get <Package>Config.cmake working for all packages (even those with subpackages).
  • Added a new example 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:

  • Covert the handling of external packages/TPL to use modern CMake targets (and create <tplName>Config.cmake files and IMPORTED targets).
  • Strip out all of the old, complex TriBITS code that manually handles lists of include directories, libraries, and header files.

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:

  • Run native TriBITS test suite
  • Run Trilinos build and tests (but not do a Trilinos install yet)
  • ???

@bartlettroscoe
Fix spelling of RST hyperlink to match previous change in PR #403
4d88007 
This hyperlink spelling did not git fixed when the anchor got fixed in commit
c716a74.  This link was broken in the RST
documentation.
@bartlettroscoe bartlettroscoe self-assigned this Oct 12, 2021
@bartlettroscoe bartlettroscoe added this to In Progress in TriBITS Refactor via automation Oct 12, 2021
@bartlettroscoe
Finish breaking off TribitsExampleMetaProject tests (#299)
f5f518f 
I had forgotten to remove these tests when I copied them over to the file
TribitsExampleMetaProject_Tests.cmake and then include that file.
@bartlettroscoe
Finish breaking off TribitsExampleProjectAddons tests (#299)
dc42431 
I messed up again in my earlier refactoring when I split off the tests into
different files.
@bartlettroscoe
Standarize comments some (#299)
79671e1 
Noticed this while looking over code as part of working on #299.
@bartlettroscoe
Some cleanup of *Config.cmake files (#299)
d482db1 
* 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
@bartlettroscoe
WIP: Switch to target_include_directories() for interal libs (#299)
09aee5a 
This also provides the INTERFACE_INCLUDE_DIRECTORIES property of the libraries
in the installed <Package>Config.cmake file!
@bartlettroscoe
WIP: TribitsExampleApp: Remove explictly setting include dirs (#299)
0eff5c1 
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!
@bartlettroscoe
WIP: Fix usage of <Package>Config.cmake files in build dir (#299)
2bd6b5a 
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.
@bartlettroscoe
WIP: Add ALIAS <Package>::<libname> target libraries (#299)
1a781da 
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.
@bartlettroscoe
WIP: Add <Package>::all_libs target (#299)
4981987 
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.
@bartlettroscoe
WIP: Fix setting INTERFACE_INCLUDE_DIRECTORIES and <Package>Targets.c…
cb53eb7 
…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.
@bartlettroscoe
WIP: Use NAMESPACE:: for targets in <Package>Config.cmake files (#299)
47d4d2b 
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.
@bartlettroscoe
WIP: Move some code inside if statement (#299)
153e944 
This gets some code inside an if statement.
@bartlettroscoe
WIP: Add <Project>::all_libs and <Project>::all_selected_libs targets (…
063747d 
…#299)

* Updated TribitsExampleApp to use these targets and stop using any
  XXX_LIBRARIES or XXX_INCLUDE_DIRS variables at all.
@bartlettroscoe
WIP: Refactor TribitsExampleApp to clean up (#299)
6d53b63 
* 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.
@bartlettroscoe
WIP: Add TribitsSimpleExampleApp (#299)
c7648fe 
This is just a simpler version of TribitsExampleApp that does not have any
conditional behavior and therefore is easier to follow.
@bartlettroscoe
WIP: Add documentation for TribitsExampleApp and all_libs targets (#299)
c111d68
@bartlettroscoe 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
@bartlettroscoe bartlettroscoe mentioned this pull request Oct 19, 2021
Closed
10 tasks
@bartlettroscoe
Copy link
Member Author

I am going to merge this and keep working on #299. A post-merge review can occur.

@bartlettroscoe bartlettroscoe merged commit bc14759 into master Oct 19, 2021
TriBITS Refactor automation moved this from In Progress to Done Oct 19, 2021
@bartlettroscoe bartlettroscoe mentioned this pull request Nov 1, 2021
Merged
@bartlettroscoe bartlettroscoe mentioned this pull request Nov 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bartlettroscoe bartlettroscoe

Labels
component: core type: enhancement
Projects
TriBITS Refactor
Status: Done
TriBITS Refactor
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@bartlettroscoe