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

Convert TriBITS to lower-case CMake command, macro, and function names (#274) #379

Merged
merged 22 commits into from
Jun 18, 2021

Conversation

bartlettroscoe
Copy link
Member

This PR adds a new tool tribits/python_utils/lower_case_cmake.py and driver tribits/refactoring/lower-case-cmake-tree.sh that I ran on all of TriBITS (which completes #274). I had to fix several things to get the TriBITS test suite to pass after this but this looks pretty good.

I tested this locally with the TriBITS test suite and I am also testing this locally against Trilinos using the ATDM Trilinos 'sems-rhel7' configuration with 'gnu-shared-dbg'. (There is some code that got broken that is not tested in TriBITS!)

This will be used to refactor all TriBITS code and documentation to lower case
command names.
See the updated documentation and unit tests for details.
Turns out it was required to have MACRO( and FUNCTION( in upper-case for any
macros and functions that were documented.  Now that is not the case.
)

This takes care of a couple of other bad cases I noticed.
Here I just ran the tool lower-case-cmake-tree.sh.  I will fix any issues in
future commits.

This version fails 97 tests locally with the way I have the tests configured.
…_DEPRECATED_MSG (TriBITSPub#73)

This is the problem with the lower_case_cmake.py tool.  If there is non-CMake
code in these files, then they will get treated like CMake code and the
pattern '<identifier>(' will get lower-cased.
…riBITSPub#274, TriBITSPub#200)

As part of this, I also fixed how the error messages were formatted.  Function
and macro names should have () after them and I fixed other formatting
problems.
We want this upper case for testing purposes.
Not really related to TriBITSPub#274 but I noticed these things when looking at the
document.
This is a case of C code being listed in a *.cmake file and getting made
lower-case by accident.

NOTE: There is not test in TriBITS for this code.  This code should not be in
TriBITS, it should be in Trilinos.  That needs to be a refactoring done in the
future to clear out TriBITS.
This is a case of C code being listed in a *.cmake file and getting made
lower-case by accident.

NOTE: There is not test in TriBITS for this code.  This code should not be in
TriBITS, it should be in Trilinos.  That needs to be a refactoring done in the
future to clear out TriBITS.
@bartlettroscoe bartlettroscoe merged commit 45de7f0 into TriBITSPub:master Jun 18, 2021
@bartlettroscoe
Copy link
Member Author

I tested this with Trilinos with an ATDM Trilinos 'sems-rhel7' configuration on my machine 'crf450' and it built and gave test results:

$ grep -A 100 "failed out of" ctest.out 
99% tests passed, 1 tests failed out of 1997

Subproject Time Summary:
Adelus                     =  12.87 sec*proc (4 tests)
Amesos2                    =   8.20 sec*proc (6 tests)
Belos                      = 390.89 sec*proc (139 tests)
Ifpack2                    = 146.33 sec*proc (52 tests)
Intrepid2                  = 1521.25 sec*proc (164 tests)
Kokkos                     = 420.55 sec*proc (32 tests)
KokkosKernels              = 374.35 sec*proc (13 tests)
MueLu                      = 1532.71 sec*proc (97 tests)
NOX                        = 466.54 sec*proc (109 tests)
Panzer                     = 4694.53 sec*proc (191 tests)
Phalanx                    =  22.85 sec*proc (33 tests)
Rythmos                    = 560.78 sec*proc (83 tests)
SEACAS                     =  73.64 sec*proc (27 tests)
STK                        =  10.38 sec*proc (2 tests)
Sacado                     = 140.23 sec*proc (299 tests)
Stratimikos                =  97.85 sec*proc (39 tests)
Teko                       = 759.44 sec*proc (18 tests)
Teuchos                    = 108.64 sec*proc (146 tests)
Thyra                      =  88.26 sec*proc (82 tests)
Tpetra                     = 832.87 sec*proc (262 tests)
TrilinosATDMConfigTests    =  72.73 sec*proc (12 tests)
TrilinosBuildStats         =   1.98 sec*proc (2 tests)
Xpetra                     =  24.36 sec*proc (18 tests)
Zoltan2                    = 683.75 sec*proc (167 tests)

Total Test time (real) = 1079.86 sec

The following tests FAILED:
        1996 - TrilinosBuildStats_Results (Failed)
Errors while running CTest

(NOTE: This is using the flaky gather method for the build-stats wrapper that will be fixed by Trilinos PR trilinos/Trilinos#8638. This failure is unrelated to this issue.)

TEST DETAILS: (click to expand)

.

$ ssh -X crf450

cd ~/Trilinos.base/BUILDS/ATDM/SEMS-RHEL7/CHECKIN/gnu-shared-dbg/

$ cat load-env.sh 
source /home/rabartl/Trilinos.base/Trilinos/cmake/std/atdm/load-env.sh gnu-shared-dbg

$ cat do-configure.base
cmake \
-GNinja \
-DTrilinos_ENABLE_BUILD_STATS=ON \
-DTrilinos_TRIBITS_DIR:STRING=TriBITS/tribits \
-DTrilinos_ENABLE_TESTS:BOOL=ON \
-DTrilinos_TEST_CATEGORIES:STRING=NIGHTLY \
-DTrilinos_ALLOW_NO_PACKAGES:BOOL=OFF \
-DDART_TESTING_TIMEOUT:STRING=600.0 \
-GNinja \
-DTrilinos_CONFIGURE_OPTIONS_FILE:STRING=cmake/std/atdm/ATDMDevEnv.cmake \
-DTrilinos_TRACE_ADD_TEST=ON \
"$@" \
/home/rabartl/Trilinos.base/Trilinos/cmake/std/atdm/../../..

$ cat do-configure
./do-configure.base \
-DTrilinos_ENABLE_ALL_OPTIONAL_PACKAGES:BOOL=ON \
-DTrilinos_ENABLE_ALL_PACKAGES:BOOL=ON \
-DTrilinos_ENABLE_ALL_FORWARD_DEP_PACKAGES:BOOL=OFF \
"$@"

$ rm -r CMake*

$  time ./do-configure &> configure.out 

real    0m55.063s
user    0m35.997s
sys     0m18.299s

$ time ninja -j16 &> make.out

real    27m44.107s
user    377m38.399s
sys     39m49.160s

$ time ctest -j16 &> ctest.out 

real    18m0.515s
user    192m51.135s
sys     10m54.974s

$ grep "failed out of" ctest.out 
99% tests passed, 1 tests failed out of 1997

bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this pull request Jun 23, 2021
…riBITSPub#382)

This is a follow-on from the lower-casing in TriBITSPub#274 with PR TriBITSPub#379.  It
lower-cased the function and macro names at the opening macro(<name> ...) and
function(<name> ...) but not the endmacro(<name> ...) and endfunction(<name>
...) calls.  (I did not realize the TriBITS had any of those still.)  Turns
out that when the text in the opening macro(<text>) and function(<text>) does
not exactly match the endmacro(<text>) and endfunction(<text>), you get a
nasty warning message:

  A logical block opening on the line ... closes on the line ... with
  mis-matching arguments.

(see TriBITSPub#382).

The TriBITS test suite does not actually run any of this code so this was
never seen prior to the creation of TriBITSPub#382.  Therefore, I don't know that this
solves the problem but I strongly suspect that it will.  (I will run a test
with an ATDM Trilinos build that should trigger this code.)
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this pull request Jun 23, 2021
…riBITSPub#382)

This is a follow-on from the lower-casing in TriBITSPub#274 with PR TriBITSPub#379.  It
lower-cased the function and macro names at the opening macro(<name> ...) and
function(<name> ...) but not the endmacro(<name> ...) and endfunction(<name>
...) calls.  (I did not realize the TriBITS had any of those still.)  Turns
out that when the text in the opening macro(<text>) and function(<text>) does
not exactly match the endmacro(<text>) and endfunction(<text>), you get a
nasty warning message:

  A logical block opening on the line ... closes on the line ... with
  mis-matching arguments.

(see TriBITSPub#382).

The TriBITS test suite does not actually run any of this code so this was
never seen prior to the creation of TriBITSPub#382.  Therefore, I don't know that this
solves the problem but I strongly suspect that it will.  (I will run a test
with an ATDM Trilinos build that should trigger this code.)
bartlettroscoe added a commit that referenced this pull request Aug 6, 2021
Revert some incorrect uppercase->lowercase changes

This was triggered by the changes in PR #379 (see #274)
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this pull request Sep 4, 2021
Brings in numerous refactorings to TriBITS over the last 3 months, but there
should be no breaks in backward compatibility.  Almost every file in TriBITS
is changed due to the lower-casing of command, macro and function names in PR
TriBITSPub/TriBITS#379.  But the main driver for this snapshot is to bring in
the change in PR TriBITSPub/TriBITS#413 that should make it so that Kokkos
INTERFACE_COMPILE_OPTIONS get propagated to downstream targets in TriBITS and
therefore to external customers through installed <Package>Config.cmake files
and IMPORTED targets.  I should have done several snapshots in the last few
months and not done a big snapshot like this (but I have been testing with
Trilinos locally along the way).

Overall, this merge brings in changes from a bunch of TriBITS PRs including
(from most recent):

* TriBITSPub/TriBITS#413: Change internal TriBITS target_link_libraries() to
  PUBLIC (TriBITSPub/TriBITS#299) component: core type: enhancement

* TriBITSPub/TriBITS#410: Upgrade from cmake 3.21.0 to 3.21.2
  (TriBITSPub/TriBITS#363, TriBITSPub/TriBITS#394)

* TriBITSPub/TriBITS#394: DO NOT MERGE: Show TriBITS test failures with CMake
  3.21.0 that don't occur with CMake 3.17.5 (TriBITSPub/TriBITS#363)

* TriBITSPub/TriBITS#409: Add getTestDictStatusField() to handle empty
  'status' field (SESW-383) component: ci_support type: enhancement

* TriBITSPub/TriBITS#408: Deal with spaces in CDash url parts (SESW-383)
  component: ci_support type: enhancement

* TriBITSPub/TriBITS#403: Spelling fixes

* TriBITSPub/TriBITS#407: Move tribits_get_build_url_and_write_to_file() to
  stand-alone module (TriBITSPub/TriBITS#154) component: ctest_driver type:
  enhancement

* TriBITSPub/TriBITS#388: Fixing typos (TriBITSPub/TriBITS#377)

* TriBITSPub/TriBITS#406: Fix tribits_ctest_driver() package-by-package mode
  for CMake 3.19+ (TriBITSPub/TriBITS#363, TriBITSPub/TriBITS#394) component:
  ctest_driver type: bug

* TriBITSPub/TriBITS#401: Improve GitHub Actions and CDash integration
  (TriBITSPub/TriBITS#154) type: enhancement

* TriBITSPub/TriBITS#366: CI: draft action yaml

* TriBITSPub/TriBITS#402: Revert some incorrect uppercase->lowercase changes

* TriBITSPub/TriBITS#387: Build and deploy TriBITS documentation with Sphinx
  (TriBITSPub/TriBITS#386) component: documentation type: enhancement

* TriBITSPub/TriBITS#398: Deal with pr null in not preprending build name
  (TriBITSPub/TriBITS#363) type: bug

* TriBITSPub/TriBITS#396: Send PR results to 'Pull Request' CDash group and
  add PR ID to build names (TriBITSPub/TriBITS#363) type: enhancement

* TriBITSPub/TriBITS#397: Print the ninja path and version
  (TriBITSPub/TriBITS#363)

* TriBITSPub/TriBITS#393: GitHub Actions based testing for TriBITS
  (TriBITSPub/TriBITS#363) type: enhancement

* TriBITSPub/TriBITS#389: TriBITS CI testing with GitHub Actions
  (TriBITSPub/TriBITS#363) type: enhancement

* TriBITSPub/TriBITS#392: Fix broken tests for non-Fortran and CMake 3.21
  builds (TriBITSPub/TriBITS#363) component: core

* TriBITSPub/TriBITS#391: Fix up build_docs.sh for Sphinx doc generation
  (TriBITSPub/TriBITS#386) component: documentation type: enhancement

* TriBITSPub/TriBITS#390: Add test for doc generation and fix usage of Python3
  component: documentation type: bug

* TriBITSPub/TriBITS#385: Replace last few references to
  TribitsDevelopersGuide.html (TriBITSPub/TriBITS#381) component:
  documentation type: enhancement

* TriBITSPub/TriBITS#384: Split TribitsDevelopersGuide.* into
  TribitsUsersGuide.* and TribitsMaintainersGuide.* (TriBITSPub/TriBITS#381)
  component: documentation type: enhancement

* TriBITSPub/TriBITS#383: Remove endfunction(<string>) and endmacro(<string>)
  (TriBITSPub/TriBITS#274, TriBITSPub/TriBITS#382) component: common_tpls
  type: bug

* TriBITSPub/TriBITS#380: More package-arch data-structure documentation
  updates (TriBITSPub/TriBITS#63) component: documentation type: enhancement

* TriBITSPub/TriBITS#379: Convert TriBITS to lower-case CMake command, macro,
  and function names (TriBITSPub/TriBITS#274)

The following files were conflicting where I went with what is on the Trilinos
'develop' branch:

* cmake/tribits/common_tpls/FindTPLBLAS.cmake
* cmake/tribits/common_tpls/FindTPLLAPACK.cmake
* cmake/tribits/common_tpls/FindTPLNetcdf.cmake

(It looks like the above changes never made it back into TriBITS proper.  The
conflicts were due to the case changes in cmake command calls in these files
due to TriBITSPub/TriBITS#379.)

There was also a conflict in the file:

* cmake/tribits/core/installation/TribitsProjectConfigTemplate.cmake.in

I looked at that one carefully and I think that may have been due to fixes on
both sides and then the case changes from TriBITSPub/TriBITS#379.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant