-
Notifications
You must be signed in to change notification settings - Fork 566
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
WILL NOT MERGE: Fix and reapply: TriBITS: Pull in partial refactoring to modern CMake targets (TriBITSPub/TriBITS#299) (#9894, #9972, #9973) #9978
Conversation
@bartlettroscoe prior to the changes of #9894 I've had to configure my Trilinos builds (gcc compilers) that use locally installed BLAS and LAPACK (not found on system PATH) by specifying the libraries, as well as the gfortran and math libs to prevent linker errors, using sample configure commands like this (OpenBlas my local install referenced here):
After the #9894 changes my builds would fail at configuration with messages like:
Once this PR moves out of WIP, should TPL configure commands like I posted resume working? If not, is a different delimiter from the semi-colon expected, or will the process change for specifying additional libraries that need to be explicitly linked with TPLs? |
@ndellingwood, I was not aware that you could just pass in a library name I will update the logic in the new TriBITS code to allow a raw string name Thanks for reporting this! |
FYI: Another problem with the updated TriBITS in PR #9894 is not allowing options like which are:
We see errors like here showing:
I think to address this, we need to update the TriBITS logic to allow the handling of compiler options like |
FYI: I found the problem with #9972. The issue is that Trilinos/TriBITS generated the
I tried to reproduce with a full SEMS configuration of Trilinos and I could not reproduce. But this should be easy to debug (and hopefully produce a TriBITS test to catch without a lot of work). |
@bartlettroscoe Let me know if that means that anything needs to be fixed or improved in stk's cmake stuff. Thanks! |
@alanw0, it is definingly not a STK CMake bug, it is a TriBITS bug related to work for TriBITSPub/TriBITS#299. I have reproduced the problem with a native TriBITS test and will fix this. (I was missing a test case.) |
…Trilinos/tribits-299-modern-cmake-targets-1"" This reverts commit fd27a20. This gets us back to the state of the 'develop' branch after the PR trilinos#9894 that merged the branch 'tribits-299-modern-cmake-targets-1' was merged (as well as other PRs in the days after that). Now I can try to reproduce the errors in issues trilinos#9972 and trilinos#9973.
Origin repo remote tracking branch: 'github/master' Origin repo remote repo URL: 'github = git@github.com:TriBITSPub/TriBITS.git' At commit: commit 35d82aaa31fe81ca30a619320bb71fe481e9d4c7 Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Tue Dec 14 15:48:02 2021 -0700 Summary: Interpret a raw identifer in TPL_<tplName>_LIBRARIES as a lib name (trilinos#299, trilinos#433)
…targets-1-again (TriBITSPub/TriBITS#433) Should address all of the issues with the merge of PR trilinos#9894 listed out in TriBITSPub/TriBITS#433 (which is part of TriBITSPub/TriBITS#299). This should resolve the failures reported in trilinos#9972 and trilinos#9973.
dd88fc9
to
963854c
Compare
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_pullrequest_cuda_weaver
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_weaver_uvm_off
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_cuda_weaver
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_weaver_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_cuda_weaver
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_weaver_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
We are still trying to understand this latest change and why we are seeing diffs when running Nalu against this PR. Below is some recent activity: Good: 12/16/2021 NaluRtest Bad: commit 11940e2700d4b94d46b75efe572f2579148c098f (HEAD -> master, origin/master, origin/HEAD) 1/75 Test #75: waleElemXflowMixFrac3.5m ...............***Failed 285.16 sec However, running the current version of all codes results in: Good: 100% tests passed, 0 tests failed out of 75 |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
4 similar comments
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
5 similar comments
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
It does not look like this will be merged soon and the PR testing passing result as gone stale (and we are not getting daily reminders of this fact). Therefore, I am putting the label Once we get the green-light to merge this (i.e. after the Trilinos 13.4 release branch is created), then I will remove |
FYI: I am trying this at: |
@keitat, if you could try this with Spack with the xSDK, that would be good. |
CC: @keitat @jwillenbring and @ccober6, Does the below announcement mean that I can merge this Trilinos PR and other backward incompatible TriBITS changes to Trilinos 'develop' starting Thursday, May 19? From: Willenbring, James M jmwille@sandia.gov All, The Trilinos 13.4 release branch was created last week. The Trilinos 13.4 release is anticipated to be finalized in the next couple of weeks. Beginning on May 19th, Trilinos developers will be removing deprecated code from the develop branch in preparation for the Trilinos 14 release, which we currently anticipate will be completed in about 3 months. If you have questions or concerns, please contact Curt Ober or Jim Willenbring. Thanks Jim |
FYI: I think it is pretty clear that given the PRs #10504 and #10533 and all of the changes to TriBITS 'master' that have been made since this PR was last changed or updated that I am going to need to create a new PR with an updated snapshot of TriBITS and cherry-picking the revert revert commit and the release notes changes from this PR. I will do some more Trilinos testing including against some application codes as mentioned above and then do this all over again. |
Okay, we have been given the okay to merge backwards incompatible changes. As I mentioned above, I need to construct a new PR that includes all of the changes in TriBITS (through TriBITSPub/TriBITS#479). I expect to be ready to create that PR, close this PR, and get that merged soon. From: Trilinos-developers <...> On Behalf Of Willenbring, James M All, At this time, you can remove code that you previously deprecated prior to the 13.4 release. While we have a couple small details to tie up with the 13.4 release, the develop branch is now beyond the point of branching for the 13.4 release. In approximately 3 months, we will cut a 14.0 release of Trilinos. At that time, the window for removing deprecated code will close again until after the final minor 14.x release. Jim |
Closing this PR for the new PR #10614. |
This PR reverts the revert commit from PR #9977 and therefore reapplies the merge of PR #9894.
This PR branch addresses the issues reported in #9972 and #9973 and listed in TriBITSPub/TriBITS#433 and offers the chance for developers to try out these changes in their own configurations so we can get this fixed.
In addition to reverting the revert commit from PR #9977, this also pulls in the additional TriBITS PRs (listed most recent to least recent):
This PR also addresses issues with several application codes.
NOTE: This PR breaks backward compatibility of the usage of
<Package>Config.cmake
files from Trilinos. (See release note for Trilinos 14.0 here)Tasks:
-I
to-isystem
for Trilinos includes.How was this tested?
Testing against ATDM Trilinos builds using -mkl
To verify the problem with the
-mkl
option is resolved with the ATDM Trilinos builds described below with this PR branch, I ran the ATDM Trilinos 'sems-rhel7' builds on the CEE machine 'cee-build015' and posted to CDash at:This showed no new failures. See details below.
Detailed ATDM Trilinos 'sems-rhel7' build and test results (click to expend)
.
Running ATDM Trilinos 'sems-rhel7' builds on 'cee-build015':
That is submitting to CDash (excluding the CUDA build since that machine does not have a GPU):
which gives:
(NOTE: The build errors for the build
Trilinos-atdm-sems-rhel7-clang-7.0.1-openmp-shared-release-debug
for the packageKrino
are also see in the reference Trilinos builds on 2021-12-12 and are therefore not caused by the TriBITS changes.)The configure output for the Intel 18 build showin here which uses
-mkl
shows:I also examined the generated generated files
BLASConfig.cmake
andLAPACKConfig.cmake
show which show:Trilinos-atdm-sems-rhel7-intel-18.0.5-openmp-shared-release-debug/SRC_AND_BUILD/BUILD/external_packages/BLAS/BLASConfig.cmake
Trilinos-atdm-sems-rhel7-intel-18.0.5-openmp-shared-release-debug/SRC_AND_BUILD/BUILD/external_packages/LAPACK/LAPACKConfig.cmake
Those look correct.
But note that the targets
BLAS::all_libs
andLAPACK:all_libs
are not used to actually link so this is not a full validation of this approach. However, I manually tested moving-mkl
option to the beginning of the link line and it still worked (but if you took,-mkl
off the link lines, the link failed).Therefore, I am pretty confident that this should work.
Other tests?*