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

Move to modern CMake target-based propagation of build information #299

Closed
9 of 10 tasks
bartlettroscoe opened this issue Feb 5, 2020 · 60 comments
Closed
9 of 10 tasks

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Feb 5, 2020

Parent Issue:

Child Issues:

Blocked By: trilinos/Trilinos#8498

Description

One of the things holding back TriBITS and projects using TriBITS in accessing new features of CMake for which TriBITS still using variables to manage compiler options and include directories. This issue is to move to using:

  • target_compile_definitions()
  • target_compile_features()
  • target_compile_options()
  • target_include_directories()
  • target_link_directories()
  • target_link_libraries()
  • target_link_options()

and to move completely to targets instead of variables for each TriBITS (internal and external) package.

This needs to be done as part of a comprehensive refactoring as part of #63.

This refactoring should implement and respect this proposed standard for <Package>Config.cmake files.

Related to:

  • SEPW-214

Tasks

@bartlettroscoe
Copy link
Member Author

This refactoring will make it easier to integrate Kokkos and KokkosKernels into Trilinos as described in the below email from @jjwilke.


From: Wilke, Jeremiah J [email protected]
Sent: Tuesday, February 4, 2020 4:18 PM
To: Wolf, Michael M [email protected]; Rajamanickam, Sivasankaran [email protected]; Bartlett, Roscoe A [email protected]; Willenbring, James M [email protected]; Trott, Christian Robert [email protected]; Teranishi, Keita [email protected]
Subject: To-do list for Trilinos

Below would be my (very specific) action items to get where we need to be.

• Move to CMake 3.13 (allows link_options and better with flag deduplication)
• Change variables to lists instead of strings (where appropriate)
• Change include_directories/compile_options to target_include_directories(….)
• Remove all dependencies between packages that rely on variables. Only use target_link_libraries(…) to propagate flags

I would guess this could be done in a month? Once this is done, it should be straightforward to design a system where Kokkos could be a TPL.

-Jeremy

@bartlettroscoe
Copy link
Member Author

The Trilinos issue to allow the upgrade of the minimum version of CMake from 3.10 to 3.13 for TriBITS as well is trilinos/Trilinos#6752.

@bartlettroscoe
Copy link
Member Author

@jjwilke, @sethrj,

This brings up another issue. These refactorings of TriBITS to use target-centric features in CMake may make it very difficult to maintain backward comparability with-respect to the build system. For example, it may not be possible keep supporting the generation of Makefile.export.<Package> files for customers like INL MOOSE. Trilinos backward-comparability requirements might make that difficult to allow.

@jjwilke
Copy link

jjwilke commented Feb 5, 2020

I think we can probably make this work? Everything that needs to be in the Makefile will end up in INTERFACE_COMPILE_OPTIONS, INTERFACE_LINK_LIBRARIES, etc. We could probably make a function

FUNCTION(EXPORT_TARGET target)
...

That doesn't rely on variables like Kokkos_CXX_FLAGS and instead calls get_property to figure everything out.

@bradking
Copy link
Contributor

bradking commented Feb 5, 2020

This will also be relevant to #127 (comment).

@sethrj
Copy link
Contributor

sethrj commented Feb 5, 2020

FWIW, SCALE/Exnihilo is only using the exported tribits config.cmake files for the downstream enrico project, not any makefile.export.

I'm definitely OK with a transition plan for removing backward compatibility gradually 😬

@jjwilke
Copy link

jjwilke commented Feb 5, 2020

I will say (and maybe @bradking already knows how to do this) - the reason I stopped short of exporting a Makefile.kokkos was generator expressions in the property lists.

@bradking
Copy link
Contributor

bradking commented Feb 5, 2020

stopped short of exporting a Makefile.kokkos was generator expressions in the property lists.

Modern CMake target-based usage requirements mean that much of the logic to convert project CMake code into the final build system command-lines is handled by CMake's generators. The input to that process is not exportable to Makefile.*, and the output is only available in generated build systems inside CMake-generated build trees. Easy export to Makefile snippets was not a design goal.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Feb 5, 2020

Easy export to Makefile snippets was not a design goal.

It occurred to me that it still might be possible to produce Makefile.* files. What you do is you create a dummy CMake project that pulls in TrilinosConfig.cmake or <Package>Config.cmake and then you use your own compiler and linker wrappers to capture the compiler and linker arguments and use that to generate the Makefile.export.* files. That is almost exactly what the ASC Sierra project does with their "bake" tool to generate Makefiles and build.ninja files given output from a Bjam build system. Crude but that would work.

@bartlettroscoe
Copy link
Member Author

FYI: I just got feedback from lead INL MOOSE framework lead that they dropped testing against Trilinos (because CASL has ended) and don't plan on supporting Trilinos going forward (unless something changes). Therefore, because I think we could add support for Makefile.export.* files like a described above, and it is not needed now, I will remove testing and support for this in TriBITS when I do this refactoring. Yea!

@bartlettroscoe
Copy link
Member Author

FYI: It just occurred to me that the SNL Sierra project might be using Makefile.export.Trilinos for its custom bJam/bake build system that uses the native Trilinos TriBITS/CMake build system. I have reached out to them to see if that is the case.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Feb 11, 2020

FYI: Got back a response from the SNL Sierra DevOps lead that Sierra actually does not use the Makefile.export.Trilinos file. They actually manually maintain a set of BJam files for building Trilinos and they get the info they need from that (even when building Trilinos using the native TriBITS CMake build system).

Therefore, it looks like we don't need to keep generating Makefile.export.Trilinos files based on SNL Sierra needs. That is good news :-)

@bartlettroscoe
Copy link
Member Author

Part of this refactoring is in the PR #300 from @bradking. It removes explicit handing of -std compiler options. It sets the global var CMAKE_CXX_STANDARD and it sets:

TARGET_COMPILE_FEATURES(<libtarget> PUBLIC <cxx_std>)

to that matching standard.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Feb 20, 2020

Someone just showed me the option CMAKE_EXPORT_COMPILE_COMMANDS which writes a comple_commands.json file after the generate phase is complete. From that you can easily extract the information needed for Makefile.export.* files (but that would have to be created by a build target, after configure and generate were complete). Now to get other things out of the way so I can get into this.

@bartlettroscoe
Copy link
Member Author

FYI: I just realized that PETSc is still using the Makefile.export.Trilinos file to get the list and order of Trilinos libraries. See:

So I think unless PETSc switches over to CMake, we are going to need to re-implement support for at least the Makefile.export.<Project> file as part of this refactoring :-(

@sethrj
Copy link
Contributor

sethrj commented May 7, 2020

Or just require PETSc to use older trilinos/tribits version until they switch?

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented May 8, 2020

Or just require PETSc to use older trilinos/tribits version until they switch?

Not sure that is a viable option because of the ECP xSDK and E4S that includes PETSc and Trilinos. But to get by, someone can just manually generate a Makefile.export.Trilinos file in the Spack Trilinos package.py file and that would satisfy PETSc for now for Spack and ECP.

And I have no idea what the timetable would be for PETSc to switch to CMake (or even if that is a firm plan).

CC: @keitat

@bartlettroscoe
Copy link
Member Author

FYI: Looks like the refactored TriBITS may need to need to keep defining the vars TPL_<TPLName>_INCLUDE_DIRS due to code like:

I wonder how much code like this exists in Trilinos and other projects using Trilinos?

bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 2, 2022
When find_package(<externalPkg>) uses a find module Find<exteranlPkg>.cmake
instead of a package config file <externalPkg>Config.cmake, the variable.
<externalPkg>_DIR is not set.  Therefore, it is best to not set
<externalPkg>_DIR in the TriBITS-geneated <tplName>Config.cmake file since
that is just confusing.

NOTE: This is only a situation that comes up when a TriBITS
FindTPL<tplName>.cmake module calls find_package(<externalPkg>).
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2022
When find_package(<externalPkg>) uses a find module Find<exteranlPkg>.cmake
instead of a package config file <externalPkg>Config.cmake, the variable.
<externalPkg>_DIR is not set.  Therefore, it is best to not set
<externalPkg>_DIR in the TriBITS-geneated <tplName>Config.cmake file since
that is just confusing.

NOTE: The case where TriBITS a FindTPL<tplName>.cmake module calls
find_package(<externalPkg>) is the only a situation where this is the case.
When one TriBITS TPL is calling find_dependency() on an upstream TriBITS TPL,
the <upstreamDepTpl>_DIR variable is always set.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…BITSPub#299, TriBITSPub#63)

This commit gets rid of several list variables that are not needed with the
move to modern CMake and some fairly complex code that was needed to compute
these list variables:

* <Project>_REVERSE_DEFINED_INTERNAL_PACKAGES
* <Project>_REVERSE_DEFINED_TPLS
* <Project>_TPL_LIST
* <Package>_PACKAGE_LIST
* <Package>_TPL_LIBRARIES
* <Package>_TPL_LIST
* <Package>_LIBRARY_DIRS
* <Package>_INCLUDE_DIR
* <Package>_TPL_LIBRARIES
* <Package>_TPL_LIBRARY_DIRS
* <Package>_TPL_INCLUDE_DIR
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…#299, TriBITSPub#63)

Unfortunately, I could not remove the computation of the full set of direct
and indirect dependencies for each pacakge <Package>_FULL_ENABLED_DEP_PACKAGES
because the function tribits_find_most_recent_file_timestamp() still needs
this.  The only customer that still needs that function is CASL VERA (to
compute when it is needed to rebuild MOOSE).

Therefore, for now, I can't remove this code (which is more complex and can be
an expensive computation if there are a **lot** of packages).
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…BITSPub#299, TriBITSPub#63)

This commit gets rid of several list variables that are not needed with the
move to modern CMake and some fairly complex code that was needed to compute
these list variables:

* <Project>_REVERSE_DEFINED_INTERNAL_PACKAGES
* <Project>_REVERSE_DEFINED_TPLS
* <Project>_TPL_LIST
* <Package>_PACKAGE_LIST
* <Package>_TPL_LIST
* <Package>_INCLUDE_DIR
* <Package>_LIBRARY_DIRS
* <Package>_TPL_INCLUDE_DIRS
* <Package>_TPL_LIBRARIES
* <Package>_TPL_LIBRARY_DIRS
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…#299, TriBITSPub#63)

Unfortunately, I could not remove the computation of the full set of direct
and indirect dependencies for each pacakge <Package>_FULL_ENABLED_DEP_PACKAGES
because the function tribits_find_most_recent_file_timestamp() still needs
this.  The only customer that still needs that function is CASL VERA (to
compute when it is needed to rebuild MOOSE).

Therefore, for now, I can't remove this code (which is more complex and can be
an expensive computation if there are a **lot** of packages).
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…#299, TriBITSPub#63)

Unfortunately, I could not remove the computation of the full set of direct
and indirect dependencies for each pacakge <Package>_FULL_ENABLED_DEP_PACKAGES
because the function tribits_find_most_recent_file_timestamp() still needs
this.  The only customer that still needs that function is CASL VERA (to
compute when it is needed to rebuild MOOSE).

Therefore, for now, I can't remove this code (which is more complex and can be
an expensive computation if there are a **lot** of packages).
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…BITSPub#299, TriBITSPub#63)

This commit gets rid of several list variables that are not needed with the
move to modern CMake and some fairly complex code that was needed to compute
these list variables:

* <Project>_REVERSE_DEFINED_INTERNAL_PACKAGES
* <Project>_REVERSE_DEFINED_TPLS
* <Project>_TPL_LIST
* <Package>_PACKAGE_LIST
* <Package>_TPL_LIST
* <Package>_INCLUDE_DIR
* <Package>_LIBRARY_DIRS
* <Package>_TPL_INCLUDE_DIRS
* <Package>_TPL_LIBRARIES
* <Package>_TPL_LIBRARY_DIRS
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…#299, TriBITSPub#63)

Unfortunately, I could not remove the computation of the full set of direct
and indirect dependencies for each pacakge <Package>_FULL_ENABLED_DEP_PACKAGES
because the function tribits_find_most_recent_file_timestamp() still needs
this.  The only customer that still needs that function is CASL VERA (to
compute when it is needed to rebuild MOOSE).

Therefore, for now, I can't remove this code (which is more complex and can be
an expensive computation if there are a **lot** of packages).
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 17, 2022
…BITSPub#299, TriBITSPub#63)

This commit gets rid of several list variables that are not needed with the
move to modern CMake and some fairly complex code that was needed to compute
these list variables:

* <Project>_REVERSE_DEFINED_INTERNAL_PACKAGES
* <Project>_REVERSE_DEFINED_TPLS
* <Project>_TPL_LIST
* <Package>_PACKAGE_LIST
* <Package>_TPL_LIST
* <Package>_INCLUDE_DIR
* <Package>_LIBRARY_DIRS
* <Package>_TPL_INCLUDE_DIRS
* <Package>_TPL_LIBRARIES
* <Package>_TPL_LIBRARY_DIRS

Some of these changes impact only internal code but some impact even customers
of the installed <Project>Config.cmake and <Package>Config.cmake files (see
the new entries in tribits/CHANGELOG.md).

This required some minor updates to the example projects:

* tribits/examples/TribitsExampleApp/
* tribits/examples/TribitsOldSimpleExampleApp/
* tribits/examples/TribitsSimpleExampleApp/

for some logic that used <Project>_TPL_LIST.  (There is no need to use that
with updated TriBITS.  See the diffs in this commit for those projects to see
how they were adjusted.)
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Nov 4, 2022
…SPub#299)

I noticed some of these issue while reading the documentation online.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Nov 7, 2022
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Nov 7, 2022
…SPub#299)

I noticed some of these issue while reading the documentation online.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Nov 7, 2022
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Dec 12, 2022
…riBITSPub#299)

This is to avoid clashes such as reported in:

* TriBITSPub#548

like CUDA::cufft from FindCUDAToolkit.cmake and fmt::fmt from Findfmd.cmake.
bartlettroscoe added a commit that referenced this issue Dec 12, 2022
This is to avoid clashes such as reported in:

* #548

like CUDA::cufft from FindCUDAToolkit.cmake and fmt::fmt from Findfmd.cmake.
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Feb 19, 2023
…rilinos#11545)

With the TriBITS modernization refactoring (TriBITSPub/TriBITS#299) and the
generalizated handling of intenral and external packages
(TriBITSPub/TriBITS#63), we need packages like Kokkos to set critical compiler
options as target properties so that they will be exported in the generated
IMPORTED targets of the Kokkos<Subpkg>Targets.cmake file.

This is needed, for example, to pass some critical compiler flags from the
pre-installed Kokkos to downstream CMake configures of KokkosKernels and the
rest of Trilinos (see trilinos#11545).
jwillenbring pushed a commit to jwillenbring/Trilinos that referenced this issue Jun 12, 2023
…rilinos#11545)

With the TriBITS modernization refactoring (TriBITSPub/TriBITS#299) and the
generalizated handling of intenral and external packages
(TriBITSPub/TriBITS#63), we need packages like Kokkos to set critical compiler
options as target properties so that they will be exported in the generated
IMPORTED targets of the Kokkos<Subpkg>Targets.cmake file.

This is needed, for example, to pass some critical compiler flags from the
pre-installed Kokkos to downstream CMake configures of KokkosKernels and the
rest of Trilinos (see trilinos#11545).
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Jun 29, 2023
…BITSPub/TriBITS#299)

Support for Makefile.export.* files was removed from TriBITS and snaphsotted
into Trilinos a long time ago.  This support was removed in the Trilinos 14.0
release.  There is no reason to keep this cache variable anymore.  It just
clutters up the base CMakeLists.txt file.
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Jul 7, 2023
…BITSPub/TriBITS#299)

Support for Makefile.export.* files was removed from TriBITS and snaphsotted
into Trilinos a long time ago.  This support was removed in the Trilinos 14.0
release.  There is no reason to keep this cache variable anymore.  It just
clutters up the base CMakeLists.txt file.
JacobDomagala pushed a commit to NexGenAnalytics/Trilinos that referenced this issue Aug 4, 2023
…BITSPub/TriBITS#299)

Support for Makefile.export.* files was removed from TriBITS and snaphsotted
into Trilinos a long time ago.  This support was removed in the Trilinos 14.0
release.  There is no reason to keep this cache variable anymore.  It just
clutters up the base CMakeLists.txt file.
@bartlettroscoe bartlettroscoe moved this to Done in TriBITS Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: Done
Development

No branches or pull requests

9 participants