-
Notifications
You must be signed in to change notification settings - Fork 46
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
Change from upper-case SOME_FUNCTION_NAME() to lower-cases some_function_name() #274
Comments
This was mentioned again in #337 (comment). I really should do this as part of the epic #367. I think I am going to write a tool to do this right after I merge the branch '63-merge-tpls-packages-1' for #63. That will make further refactoring nicer to do. |
Do you know of a robust took that can convert CMake code to lower-case function, macro, and command names? That it, that can take CMake code like:
and likewise and only change the case of command, macro and functions names to all lower case like:
That is, I want to tool to change every name of a macro, function, and built-in CMake command to lower-case and don't get messed up by any crazy usage of horizontal or vertical whitespace or change whitespace in any way. If such a robust tool does not exist, does someone at Kitware want to write one? A nice little Python script to do this would be great. (We can support this under the existing contract.) |
I found this https://github.com/cheshirekow/cmake_format... I have also reached out to our EU office to see if they have something and they will get back to me. |
Thanks @billhoffman. Sadly, I don't seem to be able to use it. It requires pip to install it and I can't seem get pip installed on my RHEL7 machine (not sure if I can say more about that here). I really hate these Python packages that require special install processes. |
I was able to run pip on my windows box and run it. It did not handle all of the cases. So, this might require some coding. But, I think it is something we can certainly look into implementing at Kitware if we don't find something that is already done. |
@billhoffman, it occurred to me that some specialized regexes with replacements in python might do the trick (a general command-call case, a macro definition case, and a function definition case). I may time-box a little effort to see what I can do with regex and replacements with Python. |
This will be used to refactor all TriBITS code and documentation to lower case command names.
This will be used to refactor all TriBITS code and documentation to lower case command names.
Okay, so I have a tool
because it matches the command-call pattern This is a huge problem as there is a lot of prose in CMake comments and I already had to put in special logic to avoid lower-casing logical operator names like in code:
that otherwise becomes:
which is invalid CMake code. Note that I want to uniformly lower-case the command names in CMake code, CMake comments, and Option-1: Create a tool that generates a huge list of possible command names that fit the pattern Option-2: Update Option-3: Another option is to keep do what I am doing but to manually modify the places that need to be fixed that have lower-cased too much. But that is also a lot of manual work. It seems like 'option-2' would be pretty robust and would involve the least manual work on modified files. If it missed some lower-casing on some rare cases, then who cares. You could manually fix those later as you see them. Given the need to lower-case any code on branches, it seems like this would be the winning option. I will look at the matches in the various files some more and think about this some before deciding on my next step. |
See the updated documentation and unit tests for details.
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 brings in a huge number of changes to TriBITS that will impact nearly every yet-to-be merged branch. Build/Test Cases Summary Enabled Packages: Enabled all Packages 0) MPI_DEBUG => Test case MPI_DEBUG was not run! => Does not affect push readiness! (-1.00 min) 1) SERIAL_RELEASE => Test case SERIAL_RELEASE was not run! => Does not affect push readiness! (-1.00 min) 2) MPI_DEBUG_CMake-3.17.0 => passed: passed=374,notpassed=0 (1.88 min) 3) SERIAL_RELEASE_CMake-3.17.0 => passed: passed=374,notpassed=0 (1.96 min) 4) MPI_DEBUG_CMake-3.17.0_Python-3.5.2 => passed: passed=374,notpassed=0 (1.64 min) Other local commits for this build/test group: df8e98c, f0ca933, 3d9ec0b, 743df53, ae845b3, 35a5879, 4b1d0c8, 16d267a, 7de9fb0, 715e4b7, 192edf3, a211fac, 26758d5, 7f936e9, d1e4913, f61da27, 7f1dbbd, cacd2b6, c71fd5d, ee0c578, 595683f, 277c40b
I created PR #379 (which is now merged) that adds the tool and the updated documentation at: |
…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.)
…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.)
Build/Test Cases Summary Enabled Packages: Enabled all Packages 0) MPI_DEBUG => Test case MPI_DEBUG was not run! => Does not affect push readiness! (-1.00 min) 1) SERIAL_RELEASE => Test case SERIAL_RELEASE was not run! => Does not affect push readiness! (-1.00 min) 2) MPI_DEBUG_CMake-3.17.0 => passed: passed=374,notpassed=0 (1.68 min) 3) SERIAL_RELEASE_CMake-3.17.0 => passed: passed=374,notpassed=0 (1.61 min) 4) MPI_DEBUG_CMake-3.17.0_Python-3.5.2 => passed: passed=374,notpassed=0 (1.76 min) Other local commits for this build/test group: e9ea581
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.
The prior patch pulled over from Trilinos PR trilinos/Trilinos#9641 did not use lower-case for set().
This should have been changed about a year ago when this change was made in TriBITS.
This should have been changed about a year ago when this change was made in TriBITS.
I don't know why I did not close this. It should have been closed a long time ago. |
TriBITSPub#299) This was snapshotted from the Trilinos commit: commit 8c1028df724eb0096cd688b381285112bc6f914a Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Wed May 18 15:08:17 2022 -0600 FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub#274) soon to be merged to Trilinos 'develop' from Trilinos PR #10533. However, this is not an snapshot as I made the following changing before creating this commit: * Kept the spelling fix for 'separated' made in TriBITS 'master' branch from commit c716a74; Author: Greg Sjaardema <gsjaardema@gmail.com>; Date: Mon Aug 2 16:02:20 2021 -0600; "Spelling fixes" * I removed a few trailing spaces at the end of some lines Why? There really is no value in using a different version of FindTPLNetcdf.cmake in TriBITS that what is used in Trilinos. When testing Trilinos against updated versions of TriBITS, you really want the logic to match. In the case in testing with Trilinos (as part of TriBITSPub#299 and testing with trilinos/Trilinos#10533), the old version of FindTPLNetcdf.cmake before this change was not properly setting TPL_Netcdf_PARALLEL which was not allowing the enable of the test SEACASIoss_exodus_fpp_serialize. With this sync, we get the exact same tests.
TriBITSPub#299) This was snapshotted from the Trilinos commit: commit 8c1028df724eb0096cd688b381285112bc6f914a Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Wed May 18 15:08:17 2022 -0600 FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub#274) soon to be merged to Trilinos 'develop' from Trilinos PR #10533. However, this is not an snapshot as I made the following changing before creating this commit: * Kept the spelling fix for 'separated' made in TriBITS 'master' branch from commit c716a74; Author: Greg Sjaardema <gsjaardema@gmail.com>; Date: Mon Aug 2 16:02:20 2021 -0600; "Spelling fixes" * I removed a few trailing spaces at the end of some lines Why? There really is no value in using a different version of FindTPLNetcdf.cmake in TriBITS that what is used in Trilinos. When testing Trilinos against updated versions of TriBITS, you really want the logic to match. In the case in testing with Trilinos (as part of TriBITSPub#299 and testing with trilinos/Trilinos#10533), the old version of FindTPLNetcdf.cmake before this change was not properly setting TPL_Netcdf_PARALLEL which was not allowing the enable of the test SEACASIoss_exodus_fpp_serialize. With this sync, we get the exact same tests.
…s:develop' (f7c3706). * trilinos-develop: FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274) Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534) Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299) ATDM: Strip leading and trailing whitespace from flags for newer CMakes
…s:develop' (f7c3706). * trilinos-develop: FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274) Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534) Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299) ATDM: Strip leading and trailing whitespace from flags for newer CMakes
TriBITSPub#299) This was snapshotted from the Trilinos commit: commit 8c1028df724eb0096cd688b381285112bc6f914a Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Wed May 18 15:08:17 2022 -0600 FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub#274) soon to be merged to Trilinos 'develop' from Trilinos PR #10533. However, this is not an snapshot as I made the following changing before creating this commit: * Kept the spelling fix for 'separated' made in TriBITS 'master' branch from commit c716a74; Author: Greg Sjaardema <gsjaardema@gmail.com>; Date: Mon Aug 2 16:02:20 2021 -0600; "Spelling fixes" * I removed a few trailing spaces at the end of some lines Why? There really is no value in using a different version of FindTPLNetcdf.cmake in TriBITS that what is used in Trilinos. When testing Trilinos against updated versions of TriBITS, you really want the logic to match. In the case in testing with Trilinos (as part of TriBITSPub#299 and testing with trilinos/Trilinos#10533), the old version of FindTPLNetcdf.cmake before this change was not properly setting TPL_Netcdf_PARALLEL which was not allowing the enable of the test SEACASIoss_exodus_fpp_serialize. With this sync, we get the exact same tests.
…Trilinos:develop' (7d907aa). * trilinos/develop: (31 commits) MiniEM: Fix lambda capture issues in evaluators Phalanx: fix for non-cuda spaces Cmake: Tpetra deprecation removal nightlies Phalanx: cleanup Phalanx: add new ctor to v_of_v Phalanx: another view-of-view utility FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274) Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534) Panzer MiniEM: Disable tests for 2nd order elements MueLu Maxwell1: Small fix MiniEM: Add "p coarsen schedule" Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299) Phalanx: Array of Views acceptance test. ATDM: Strip leading and trailing whitespace from flags for newer CMakes Ifpack2 Hiptmair: Add parameter "hiptmair: implicit transpose" Ifpack2 Hiptmair: timers ...
It seems the current CMake convention is to use lower case for CMake functions and local variables and upper case for global variables, built-in CMake variables, etc. Therefore, TriBITS should change this in all TriBITS code, examples, and documentation.
To make this change in all of TriBITS (including documentation and examples) the one could make make a script that looked for
SOME_FUNC_NAME(
and replaced it withsome_func_name(
. So we would provide a script called something liketribits_upper_to_lower_case_func_names.sh <file-name>
and a driver scripttribits_upper_to_lower_case_func_names_recurse.sh
that would apply to filesCMakeLists.txt
,*.cmake
to be safe. (Then you could apply manually to*.rst
and other files if you like.) This would likely not be all that hard to do. But would would need to very carefully look at the changes to make sure that no changes that were not expected were made.To not completely break branches of TriBITS, we would need to provide scripts to run on a given branch to update all of the names to match and then this should avoid conflicts.
Also, other TriBITS projects could use these scripts to update their CMake code and documentation.
The text was updated successfully, but these errors were encountered: