Allow 100% raw CMake to be used in a TriBITS-compliant package (#582) #591
Conversation
bartlettroscoe
force-pushed
the
582-raw-cmake-as-tribits-package
branch
4 times, most recently
from
3362b79
to
4232f87
Compare
bartlettroscoe
force-pushed
the
582-raw-cmake-as-tribits-package
branch
from
481aded
to
7930a20
Compare
bartlettroscoe
force-pushed
the
582-raw-cmake-as-tribits-package
branch
from
7930a20
to
62d632c
Compare
bartlettroscoe
force-pushed
the
582-raw-cmake-as-tribits-package
branch
from
1bd0d54
to
04b19ac
Compare
… changes (#582) This is really a very general function so I updated the documentation to be general. This will allow this function to be moved to tribits/core/utils/. I also renamed some local vars to be camelCase.
#582) This is a generic function (see comments and documentation from previous commit). NOTE: This move of the module was done in a separate commit from the last commit so as to not confuse Git in tracing filename changes.
…ject (#582) Now we can update TriBITS and the raw Package1 build system for it to work as a TriBITS package but without calling any TriBITS macros or functions.
This makes the documentation ad code a little more clear. As part of this, I renamed the function `tribits_write_package_client_export_files_install_targets()` to ` tribits_write_package_client_export_files_export_and_install_targets()`. That makes it a little more clear what these functions are doing.
Allow TriBITS package TribitsExampleProject/Package1 to use 100% raw CMake and
update TriBITS to allow that.
This raw CMake build mode for Package1 uses no TriBITS macros, functions, or
other functionality at all.
The only changes to TriBITS-proper needed to allow this were:
* Skip check for not calling tribits_package_postprocess() if the
`<Package>::all_libs` target is already defined.
* Build up the list of package libraries target for the `<Project>_libs`
target from `<Package>::all_libs` instead of `<Package>_libs` (since the
latter does not exist for a raw CMake package).
However, this also required some changes to the
TribitsExampleProject2/Package1 raw build system:
* Use `${CMAKE_PROJECT_NAME}_INSTALL_LIB_DIR}` as the library install location
instead of `${CMAKE_INSTALL_LIBDIR}` when building as a TriBITS package.
This seems to pass all of the TriBITS tests.
NOTE: This commit does **not** yet contain the changes to TriBITS to allow
calling project() in the package's CMakeLists.txt files and still use TriBITS
macros in the package's CMakeLists.txt files. Those changes will come later
as we add an example that calls some TriBITS functions.
This avoids the special logic in the file package1/CMakeLists.txt by having
TribitsExampleProject2/CMakeLists.txt set the var
${PROJECT_NAME}_USE_GNUINSTALLDIRS=ON. This resulted in everything being
installed under <prefix>/lib64/ on my current machine automatically.
However, to get this to work I had to set:
-DCMAKE_INSTALL_LIBDIR:STRING=lib
or the find_package(...) command with CMake 3.23.1 would no longer find
<Package>Config.cmake files under:
<prefix>/lib64/
This seems inconsistent with CMake documentation for find_package() as of
CMake 3.23.1 that suggests that it will look under the location:
<prefix>/(lib/<arch>|lib*|share)/cmake/<name>*/
Well that should pick up the directory:
<prefix>/lib*/cmake/<name>*/
But that was not the case :-(
Perhaps this is fixed in the latest CMake? In any case, we get around this by
by setting CMAKE_INSTALL_LIBDIR=lib.
Other changes made in this commit:
* Fixed the 'TribitsExampleApp2' tests to correctly switch between searching
for the top project-level package-config file or the individual
package-config files. (This required passing in
-DTribitsExApp2_FIND_INDIVIDUAL_PACKAGES=ON and updating the regex.)
…582) This avoids having to add: -DCMAKE_INSTALL_LIBDIR:STRING=lib for all of the TribitsExampleProject2_find_package tests. If it was not for the CMake defect with find_package() documented in the new updated comment, then we could set TribitsExProj2_USE_GNUINSTALLDIRS=ON and not have to set CMAKE_INSTALL_LIBDIR=lib.
…ge1 build under TriBITS project (#582) This test sets up to use the TriBITS test management functions from inside of a raw CMake package under a TriBITS project.
This will allow me to define and run the tests for Package1 using tribits_add_test().
This addeds the macros: tribits_advanced_set_cache_var_and_default() tribits_set_cache_var_and_default() This will make it easy to refactor code that sets a cache var and its default.
This makes it usable in a project that does not use all of TriBITS.
…e project build (#582)
This makes the failing test added in the last commit to pass.
This commit updates TriBITS to allow a raw CMake project to just include:
include("<tribitsDir>/core/package_arch/TribitsAddTest.cmake")
and then use tribits_add_test() without having to append CMAKE_MODULE_PATH.
To make this work, I had to change from:
include(<fileName>)
to use explicit file include with base dir:
include("<baseDir>/<fileName>.cmake")
This also makes the locations of these files more explicit, which I think is
good actually.
I updated TribitsExampleProject2/Package1 to use tribits_add_test() in a
stand-alone raw CMake build.
A few other things done:
* Removed the include of TribitsGeneralMacros.cmake from
TribitsAddTestHelpers.cmake since none of its macros/functions are used
any longer.
* Shortened 'PACKAGE1_USE_TRIBITS_TEST_FUCNTIONS' to
'PACKAGE1_USE_TRIBITS_TEST_FUNCS' to shorten the test name some.
This breaks some test support related modules for the function tribits_add_test() into their own subdirectory: tribits/core/test_support/ and include them with explicit relative and absolute includes and not relying on CMAKE_MODULE_PATH. (By using explicit includes we can better see and manage the dependencies between modules on these separate subdirs and better partition TriBITS.) To cleanly break out these test_support modules, I created a new subidr: tribits/core/common/ and put the modules TribitsCMakePolicies.cmake and TribitsConstants.cmake into them. And I also did not add this new subdir to CMAKE_MODULE_PATH for the same reason as above for test_support. This allows some non-test-related modules in tribits/core/package_arch/ to depend on tribits/core/common/ but not tribits/core/test_support/. Technically, this commit is the start of the componitization of TriBITS Core as per #368.
bartlettroscoe
force-pushed
the
582-raw-cmake-as-tribits-package
branch
from
04b19ac
to
c4b2745
Compare
This shortens the names of tests. I have used the acronym 'PBP' in ctest_driver tests already.
…#582) Changes to TriBITS: * Changed explicit includes from TribitsAddAdvancedTest.cmake (this module and its included modules will be moved to tribits/core/test_support/ in next commit). * Remove option to prefix test base name in tribits_add_advanced_test() by '${PROJECT_NAME}_' (since tribits_set_tribits_package_name() is now being called to set PACKAGE_NAME give PROJECT_NAME). * Changed tribits_add_advanced_test() to set explicit include of DriveAdvancedTest.cmake without setting CMAKE_MODULE_PATH. * Use more explicit includes in tribits/core/utils/*.cmake mdoules needed to get above to work. Changes to TribitsExampleProject2/Packages1: * Updated package1-prg to accept command-line arguments that are echoed to STDOUT. * Added new test Package1_Prg-advanced taking in command-line arguments using tribits_add_advanced_test() and in raw CMake build. Changes to tests: * Removed regex for CMAKE_MODULE_PATH from driver file for tribits_add_advanced_test() (which reduces the total number of checks by 1).
bartlettroscoe
changed the title
|
Hello @KyleFromKitware, if you have some time, can you please review this PR? It is likely better to review it commit-by-commit than all-at-once. |
| @@ -39,7 +39,7 @@ | |||
|
|
|||
|
|
|||
| # Standard TriBITS system includes | |||
| include(TribitsConstants) | |||
| include("${CMAKE_CURRENT_LIST_DIR}/../common/TribitsConstants.cmake") | |||
There was a problem hiding this comment.
Is there a rhyme or reason to which include()s get converted to .cmake paths and which remain as simple module names?
There was a problem hiding this comment.
Yes, it makes it so you can use modules out of TriBITS without having to append CMAKE_PREFIX_PATH. It will allow future refactorings without needing to mess with CMAKE_PREFIX_PATH. I know this is more verbose but I think it will make the dependencies more explicit and will make maintenance easier overall.
There was a problem hiding this comment.
Right, but why is it that only some of the include()s have been converted to ${CMAKE_CURRENT_LIST_DIR}? Why not all of them?
There was a problem hiding this comment.
Because, initially, I just did as many conversions as needed to support the creation of the tribits/core/test_support/ subdir and moving the test support modules there. A follow-on PR can convert the rest of the includes (and I would like to use a script for as much of refactoring as possible).
|
@KyleFromKitware, I think this is ready to merge as it has resolved all of your comments. Can you approve the PR if it looks good to you? |
This change was made several years ago but we forgot to update this documentation.
Several things were done here: * Added new section "How to implement a TriBITS-compliant internal package using raw CMake" * Added new section "How to implement a TriBITS-compliant external package using raw CMake" * Added subsection on example project TribitsExampleProject2 under the "Example TriBITS Projects" section. * Added generation of reduced versions of the package1/CMakeLists.raw.cmake file for different cases. (But will only trigger a re-make if the generated files change.) * Added make dependencies on generated *.cmake files * generate-guide.sh: Added time to 'make' command and discard STDOUT for 'cd -' command (makes output look better)
) Now just calls tribits_set_cache_var_and_default() then mark_as_advanced(). Suggested by @KyleFromKitware
bartlettroscoe
force-pushed
the
582-raw-cmake-as-tribits-package
branch
from
637fa77
to
c941426
Compare
There was a problem hiding this comment.
I am going to allow this to merge and set it up for a post-merge review so @KyleFromKitware can review the generated documentation in full.
|
Hello @KyleFromKitware, can you please do a post-merge review of the generated documentation at:
? This refers to the raw and CMake build system under: |
…bits This brings in the following TriBITS PRs: * TriBITSPub/TriBITS#591: Allow 100% raw CMake to be used in a TriBITS-compliant package (TriBITSPub/TriBITS#582) * TriBITSPub/TriBITS#588: gitdist: Pass in '-c color.status=always' when --dist-no-color is not added * TriBITSPub/TriBITS#590: Fix: gitdist: dist-repo-status: Display tag or SHA1 instead of 'HEAD' * TriBITSPub/TriBITS#587: gitdist: dist-repo-status: Display tag or SHA1 instead of 'HEAD' * TriBITSPub/TriBITS#586: More cleanup of packaging support (trilinos#11976) This goes back to TriBITS commits from June 2023.
| cmake_minimum_required(VERSION 3.23.0 FATAL_ERROR) | ||
|
|
||
| if (COMMAND tribits_package) | ||
| message("Configuring raw CMake package Package1") |
There was a problem hiding this comment.
Should mention TriBITS, maybe
| message("Configuring raw CMake package Package1") | |
| message("Configuring raw CMake package Package1 with TriBITS") |
There was a problem hiding this comment.
Good suggestion. I will create a follow-on PR for this.
Any more suggestions? Did the documentation at:
- https://tribitspub.github.io/TriBITS/users_guide/index.html#how-to-implement-a-tribits-compliant-internal-package-using-raw-cmake
- https://tribitspub.github.io/TriBITS/users_guide/index.html#how-to-implement-a-tribits-compliant-external-package-using-raw-cmake
make sense?
There was a problem hiding this comment.
Any more suggestions? Did the documentation at:
* [tribitspub.github.io/TriBITS/users_guide/index.html#how-to-implement-a-tribits-compliant-internal-package-using-raw-cmake](https://tribitspub.github.io/TriBITS/users_guide/index.html#how-to-implement-a-tribits-compliant-internal-package-using-raw-cmake) * [tribitspub.github.io/TriBITS/users_guide/index.html#how-to-implement-a-tribits-compliant-external-package-using-raw-cmake](https://tribitspub.github.io/TriBITS/users_guide/index.html#how-to-implement-a-tribits-compliant-external-package-using-raw-cmake)make sense?
Yes, I think it's pretty clear. I find
Provides CMake variables:
<Package>_CONFIG or <Package>_TRIBITS_COMPLIANT_PACKAGE_CONFIG_FILE: Points to the file <Package>Config.cmake (i.e. ${CMAKE_CURRENT_LIST_FILE})
a little surprising/unusual and
The test <fullTestName> must not be added if the cache variable <fullTestName>_DISABLE is set to TRUE or if the cache variable <fullTestName>_SET_DISABLED_AND_MSG is set to non-empty (and the message string should be printed to STDOUT).
also feels pretty intrusive (but a package could just choose not to allow running tests if they are a subproject).
The management of TPL's (e.g., Cuda) between external and internal raw CMake projects and TriBITS projects isn't discussed very much, though. Looking at https://github.com/TriBITSPub/TriBITS/tree/master/tribits/examples/TribitsExampleProject2/packages it seems that this should work as well, though. I guess we still need FindTPL* files for dependencies that are also used by the top-level TriBITS project, though?
There was a problem hiding this comment.
I find
Provides CMake variables:
<Package>_CONFIGor<Package>_TRIBITS_COMPLIANT_PACKAGE_CONFIG_FILE: Points to the file<Package>Config.cmake(i.e.${CMAKE_CURRENT_LIST_FILE})a little surprising/unusual
It is because some of those <Pakage>Config.cmake files are included instead of found and if you don't call find_package(<Package>) then the var <Package>_CONFIG does not get set. This is especially important for non-TriBITS-compliant external packages where TriBITS creates a <buildDir>/external_packages/<Package>/<Package>Config.cmake wrapper file that calls find_package(<Package>). This is mentioned in:
And we don't want to set <Package>_CONFIG or <Package>_DIR because you don't want to interfere with CMake code that may be calling find_package(<Package>) that is expecting to find the non-TriBITS-compliant <Package>Config.cmake file or the Find<Package>.cmake file. (This is an issue for HDF5, for example.) Some of the issues with non-TriBITS-compliant external packages are explained in the section:
but I need to mention the how the <Package>_CONFIG and <Package>_TRIBITS_COMPLIANT_PACKAGE_CONFIG_FILE vars work in this system to make this work correctly.
I likely need to add another section to the TriBITS Users Guide that explains this system in a more complete way, pulling everything together.
and
The test
<fullTestName>must not be added if the cache variable<fullTestName>_DISABLEis set toTRUEor if the cache variable<fullTestName>_SET_DISABLED_AND_MSGis set to non-empty (and the message string should be printed to STDOUT).also feels pretty intrusive (but a package could just choose not to allow running tests if they are a subproject).
That is correct. If a raw CMake package can't satisfy those requirements, they can just not turn off their tests. Users of a TriBITS project expect to be able to disable any test by setting either of those variables. Otherwise, it is very difficult to maintain the test suite across a bunch of build configurations and platforms. For example, look at the tests being disabled in the Trilinos PR builds listed here:
$ grep -n "_DISABLE " packages/framework/ini-files/config-specs.ini | wc -l
450
But the truth is that there is a more to having TriBITS-compliant tests that just disabling tests based on those vars. For example, to be submitted to CDash correctly, tests also need to set the label <Package> (I will add that to the requirements). Also, some individual Kokkos unit tests get disabled on some build configurations like you see in:
including:
$ grep -n "Kokkos_.*_EXTRA_ARGS" packages/framework/ini-files/config-specs.ini
352:opt-set-cmake-var Kokkos_AlgorithmsUnitTest_MPI_1_EXTRA_ARGS STRING : --gtest_filter=-*Random_XorShift64:-*Random_XorShift1024
355:opt-set-cmake-var Kokkos_CoreUnitTest_Serial1_MPI_1_EXTRA_ARGS STRING : --gtest_filter=-*join_backward_compatibility
356:opt-set-cmake-var Kokkos_CoreUnitTest_Serial1_EXTRA_ARGS STRING : --gtest_filter=-*join_backward_compatibility
358:opt-set-cmake-var Kokkos_CoreUnitTest_Default_MPI_1_EXTRA_ARGS STRING : --gtest_filter=-*defaultdevicetype.shared_space
359:opt-set-cmake-var Kokkos_CoreUnitTest_Default_EXTRA_ARGS STRING : --gtest_filter=-*defaultdevicetype.shared_space
1320:opt-set-cmake-var Kokkos_CoreUnitTest_Cuda_MPI_1_EXTRA_ARGS STRING : "--gtest_filter=-cuda.debug_pin_um_to_host:cuda.debug_serial_execution"
The framework team and other developers need to ability to surgically disable tests without disabling everything. (You don't want to be trusting a package on a given system if you can't run its test suite. It sucks to have to debug issues caused by an upstream package in a downstream test suite.)
The management of
TPL's (e.g., Cuda) between external and internal raw CMake projects and TriBITS projects isn't discussed very much, though.
In many ways, CUDA is no different than any other any other non-TriBITS-compliant external package/TPL. You need to provide a FindTPL<Package>.cmake file along the lines documented in:
In the case of CUDA, <tplName> is CUDA and <externalPkg> is CUDAToolkit. See:
TriBITS/tribits/core/std_tpls/FindTPLCUDA.cmake
Lines 56 to 60 in 685c8d5
Looking at https://github.com/TriBITSPub/TriBITS/tree/master/tribits/examples/TribitsExampleProject2/packages it seems that this should work as well, though. I guess we still need
FindTPL*files for dependencies that are also used by the top-levelTriBITSproject, though?
Yes, but it would be the responsibility of the top-level TriBITS project to provide those FindTPL*.cmake files, not the raw CMake packages that also use those external packages/TPLs. For example, this is why CUDA dependencies work with the raw CMake Kokkos build system when building under the TriBITS project Trilinos. Both FindTPLCUDA.cmake and the raw CMake Kokkos build system call find_package(CUDAToolkit) and we just need to make sure that both of those calls finds the same CUDA installation (which is not hard to ensure).
Thanks for looking all of this over!
I will be writing a SAND report that tries to explain the philosophy behind this system and why it offers something unique in productivity and build/test performance that you can't get with more course-grained packaging systems like Spack (but still providing the flexibility to build packages as individual CMake projects under Spack if you want).
Addresses:
Description
This PR provides the changes to TriBITS and the TribitsExampleProject2 project to allow 100% raw CMake (with no TriBITS commands or functionality at all) to be used to define a TriBITS-compliant package. Such a TriBITS package should a a valid [TriBITS-compliant internal package] (https://tribitspub.github.io/TriBITS/users_guide/index.html#tribits-compliant-internal-packages) and produce a TriBITS-compliant external package.
Tasks
tribits_add_test()in stand-alone raw CMake projecttribits/core/testing_support/and modules that can be directly included without settingCMAKE_MODULE_PATH?Update TriBITS to useNot needed for these use cases but will be needed if a TriBITS project is pulled into a larger CMake base project usingTRIBITS_PROJECT_NAME,${TRIBITS_PROJECT_NAME}_SOURCE_DIRand${TRIPTS_PROJECT_NAME}_BINARY_DIRinstead ofPROJECT_NAME,PROJECT_SOURCE_DIRandPROJECT_BINARY_DIR, respectively, and make all tests pass (to support use cases like the Sierra prototype CMake build that is including a TriBITS project under a non-TriBITS project usingadd_subdirectory()whereTRIBITS_PROJECT_NAME != CMAKE_PROJECT_NAMEorPROJECT_NAME)add_subdirectory().