Final fixes for TriBITS upgrade to modern CMake targets (#10614, #10774) #10930
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
I forgot to remove these vars in Trilinos PR trilinos#10813. They should not be printed anymore because they should not be used anymore.
…os#10774, TriBITSPub/TriBITS#516) A TriBITS update is exporting package cache vars to the <Package>Config.cmake file and you can't have a local var with the same name as a cache var with different values. In this case, it was just lucky that no downstream package was reading this var (through the cache var) because they would have gotten the wrong value. It seems that only code in CMakeLists.txt files under packages/pliris/ were reading this var.
…S#522) This is to simulate external projects that call find_package(Trilinos) that may require an older version of CMake than that is required by Trilinos/TriBITS. NOTE: This exposes the error: --------------------------------------------- CMake Warning (dev) at /ascldap/users/rabartl/Trilinos.base/BUILDS/PR/clang-10.0.0/install/lib/cmake/Kokkos/KokkosConfig.cmake:197 (IF): Policy CMP0057 is not set: Support new IN_LIST if() operator. Run "cmake --help-policy CMP0057" for policy details. Use the cmake_policy command to set the policy and suppress this warning. IN_LIST will be interpreted as an operator when the policy is set to NEW. Since the policy is not set the OLD behavior will be used. Call Stack (most recent call first): /ascldap/users/rabartl/Trilinos.base/BUILDS/PR/clang-10.0.0/install/lib/cmake/Trilinos/TrilinosConfig.cmake:114 (include) CMakeLists.txt:8 (find_package) This warning is for project developers. Use -Wno-dev to suppress it. CMake Error at /ascldap/users/rabartl/Trilinos.base/BUILDS/PR/clang-10.0.0/install/lib/cmake/Kokkos/KokkosConfig.cmake:197 (IF): if given arguments: "Kokkos_ENABLE_THREADS" "AND" "NOT" "DEPRECATED_CODE_3" "IN_LIST" "Kokkos_OPTIONS" Unknown arguments specified Call Stack (most recent call first): /ascldap/users/rabartl/Trilinos.base/BUILDS/PR/clang-10.0.0/install/lib/cmake/Trilinos/TrilinosConfig.cmake:114 (include) CMakeLists.txt:8 (find_package) -- Configuring incomplete, errors occurred! --------------------------------------------- reported in trilinos#10456 and TRILINOSHD-128 In a later commit, we will update the version of TriBITS that addresses this.
Origin repo remote tracking branch: 'github/master' Origin repo remote repo URL: 'github = git@github.com:TriBITSPub/TriBITS.git' Git describe: Vera4.0-RC1-start-1263-g8817a8f9 At commit: commit ab41942990e8d3655463551036a979c674b27fcc Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Wed Aug 24 16:50:38 2022 -0600 Summary: Add version check and calls to cmake_minimum_required() (trilinos#522)
14 tasks
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-10.1.243
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-10.1.243
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
|
rppawlo
previously approved these changes
Aug 25, 2022
| @@ -1,4 +1,7 @@ | |||
| cmake_minimum_required(VERSION 3.17) | |||
| cmake_minimum_required(VERSION 3.0) | |||
There was a problem hiding this comment.
I thought our required cmake version was going to be bumped from 3.17 to 3.23. Why set this to 3.0? Have seen this in a few other places as well.
There was a problem hiding this comment.
I thought our required cmake version was going to be bumped from 3.17 to 3.23. Why set this to 3.0? Have seen this in a few other places as well.
@rppawlo, yes, but we don't impose that on downstream CMake projects that call cmake_minimim_required(VERSION <cmake-version>) with <cmake-version> lower than even 3.3 (which is what happened with TRILINOSHD-128). These files are meant to act like external CMake projects that my use a lower version of CMake (even lower than 3.3) so that we can validate that we are doing our part to support such projects (because these projects actually exist).
The idea with CMake is that your project may require a higher version of CMake than the <Package>Config.cmake files produced by your project so you can support projects that require an older version of CMake. It seems that the goal is to write <Package>Config.cmake files in order to require the lowest version of CMake possible by downstream customers. In fact, the package-config targets files generated by CMake itself by the command export(EXPORT ... FILE <filename>) only requires CMake version 3.0.0 and uses polices from CMake 2.6.0! You can see this in the <Package>Targets.cmake files generated by CMake with Trilinos with the top showing:
# Generated by CMake
if("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" LESS 2.5)
message(FATAL_ERROR "CMake >= 2.6.0 required")
endif()
cmake_policy(PUSH)
cmake_policy(VERSION 2.6)
#----------------------------------------------------------------
# Generated CMake target import file.
#----------------------------------------------------------------
# Commands may need to know the format version.
set(CMAKE_IMPORT_FILE_VERSION 1)
if(CMAKE_VERSION VERSION_LESS 3.0.0)
message(FATAL_ERROR "This file relies on consumers using CMake 3.0.0 or greater.")
endif()
(Take a look at the <Package>Targets.cmake files generated under <buildDir>/cmake_packages/ to see for yourself.)
For more context, see TriBITSPub/TriBITS#522 and the commit log message for d03c584.
If you take a step back, you can see why it has to be this way. Suppose there is some old CMake project that ended active development 8 years ago that used Trilinos and then you want to build it was a newer version of Trilinos. That project may have cmake_minimum_required(VERSION 3.1) or something in its top-level CMakeLists.txt file. We would no longer be able to build that project against a modern install of Trilinos unless someone went in and manually modified that CMakeLists.txt file. But why? That project has not changed at all. Why should it have to bump up its minimum required CMake version? By taking care of features used in our <Package>Config.cmake files ourselves, we can spare old downstream projects from having to change anything.
If you have more questions about this, let's have a short conversation to clear this up.
This was referenced Aug 25, 2022
|
FYI: The Pliris test failures in the last PR testing iteration have nothing to do with this PR (see #10931). |
Because Seconddary Tested code is disable in ATS2 builds, we don't want a change to Pliris to trigger the enable since it is already broken on thse ATS2 CUDA builds. For all of the details, see trilinos#10931.
For some reason, CMake is not respecting the force disable of Trilinos_ENABLE_Pliris from the last commit. Rather than try to debug this, I am just disabling the tests which will allow PR trilinos#10930 to pass the PR builds and merge. There are other issues with the GenConfig files that will need to be addressed that I am seeing.
Origin repo remote tracking branch: 'github/master' Origin repo remote repo URL: 'github = git@github.com:TriBITSPub/TriBITS.git' Git describe: Vera4.0-RC1-start-1265-gaa9a4256 At commit: commit 142e536230040523bcf1f3de903df52f91c76d79 Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Thu Aug 25 08:43:09 2022 -0600 Summary: Change cmake_minimum_required() to upper bound TRIBITS_CMAKE_MINIMUM_REQUIRED (trilinos#522)
|
FYI: I tested the configure of Trilinos for the ATS2 build on 'vortex' to verify that the Pliris tests are being disabled (see details below). For some reason, my attempt to force disable Pliris in commithttps://github.com//pull/10930/commits/1f9274ca940907d0d9c808e694eb73f10821fe60 did not work and yielded Pliris getting enabled anyway even through the GenConfig fragment file contained: But with the commit to disable the Pliris tests 075198b, the configure output shows: Details on testing configure of ATS2 PR build on 'vortex': (click to expand)So the Pliris tests from #10931 are disabled so this PR should now pass. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-10.1.243
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
rppawlo
approved these changes
Aug 25, 2022
|
Well, I can't explain how, but Pliris is being correctly force disabled in the actual ATS2 CUDA PR build shown here shownig: The difference from what I ran above is that the disable is being done in the Jenkins PR build using the file |
|
So, after some experimentation I have determined that with CMake 3.18.0 used on 'vortex', that a cache force set() statement in a file processed with gives the desired behavior where the But if you set the cache var on the CMake command-line with: then the value
However, if you use the TriBITS option then the command I will document this behavior for TriBITS at: Experimentation with handling of *.cmake fragment files and cache vars showing undocumented CMake behavior for -C file.cmake entries: (click to expand)Okay, so I did an experiment and put the enable for Pliris in a and configured on 'vortex' with: and now we see: So that shows that for force disable statement in the indeed does override the cache var enable set() statement show above in the file But if you put we see in the configure output: and the cache var shows: That is very interesting. That shows that the: statements in the files However, if you include this file with the TriBITS option you see: and the cache file shows: So that shows behavior of CMake that I never knew existed and seems to be undocumented, even with the current version of CMake at:
I will ask Kitware to improve the documentation to clarify this behavior. |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-7.2.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-10.1.243
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ rppawlo ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
|
Merge on Pull Request# 10930: IS A SUCCESS - Pull Request successfully merged |
|
NOTE: As a suggestion in TriBITSPub/TriBITS#525 (comment), I moved the order of the and it showed: and: So the order of |
This was referenced Aug 26, 2022
Thanks for tracking this down and documenting it, @bartlettroscoe. |
cgcgcg
pushed a commit
to cgcgcg/Trilinos
that referenced
this pull request
Sep 12, 2022
For some reason, CMake is not respecting the force disable of Trilinos_ENABLE_Pliris from the last commit. Rather than try to debug this, I am just disabling the tests which will allow PR trilinos#10930 to pass the PR builds and merge. There are other issues with the GenConfig files that will need to be addressed that I am seeing.
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.
Description
This should resolve the remaining issues listed in #10774 from the merge of the last major TriBITS upgrade to modern CMake targets in PR #10614.
This brings in the TriBITS PRs:
How was this tested?
I tested against a Trilinos build as documented with the most recent TriBITS PR TriBITSPub/TriBITS#523 (comment).
Also, technically this version of TriBITS was also tested against Charon + Trilinos (see #10925).