Replace the PARSE_ARGUMENTS() macro with the new CMake built-in command CMAKE_PARSE_ARGUMENTS()

[bartlettroscoe]

Since CMake 3.5, CMake now has a native argument parser function:

cmake_parse_arguments(<prefix> <options> <one_value_keywords>
  <multi_value_keywords> args...)

cmake_parse_arguments(PARSE_ARGV N <prefix> <options> <one_value_keywords>
  <multi_value_keywords>)

• https://cmake.org/cmake/help/v3.8/command/cmake_parse_arguments.html#command:cmake_parse_arguments

To support older versions of CMake, we would just include the module CMakeParseArgument.cmake:

• https://cmake.org/cmake/help/v2.8.11/cmake.html#module:CMakeParseArguments

To take full advantage of the new CMAKE_PARSE_ARGUMENTS() function (i.e. single-value arguments), we would likely need to replace the usage of PARSE_ARGUMENTS() with CMAKE_PARSE_ARGUMENTS() one call at a time. But that should not be too hard.

This will also allow us to shrink the size of TriBITS Core.

[bartlettroscoe]

I looked at the size of the function that implements PARSE_ARGUMENTS() and it is pretty small. But it will kill a bunch of documentation, which will be good. This should also be much faster with CMake 3.5+ due to the native CMake implementation (i.e. in C++ instead of CMake).

[bartlettroscoe]

@fryeguy52 and I pair programmed the start of this refactoring. Turns out that cmake_parse_arguments() does not assert one-value keyword args, it just ignores them! Therefore, we will not be using that feature :-(

Therefore, we will always pass in "" for <one_value_keywords> :-(

So we will need create a function TRIBITS_ASSERT_NUM_ARGS(<prefix> <arg_name> <num_args>) which is used like:

   TRIBITS_ASSERT_NUM_ARGS(PREFIX NAME 1)

[bartlettroscoe]

We fixed the last issue with the major part of the refactoring in the WIP commit 4a64534. Now, to finish this up we should:

1. Search all of TriBITS with find . -type f -exec grep -nHi parse_arguments {} \; | grep -v "\.html" | grep -v "ParseVariableArguments\.cmake" | grep -vi cmake_parse_arguments Update documentation that might still mention it. Remove PARSE_ARGUMENTS() from TribitDevelopersGuide (i.e. /developers_guide/UtilsMacroFunctionDocTemplate.rst).

2. Replace INCLUDE(ParseVariableArugments) with INCLUDE(CMakeParseArugments). (That ensures that none of TriBITS is including that file anymore.)

3. Add in a MESSAGE(WARNING ...) command at the top of tribits/core/utils/ParseVariableArguments.cmake to warn users of TriBITS that that file and PARSE_ARGUMENTS() is deprecated and should not be used (instead use CMAKE_PARSE_ARGUMENTS())'.

4. Put in a note in TriBITS/tribits/ReleaseNotes.txt about this change.

[bartlettroscoe]

Timing configure of TriBITS project itself for this branch vs. master ...

For the do-configure script:

#!/bin/bash

../../../Trilinos/TriBITS/dev_testing/generic/do-configure-mpi-debug \
-DCTEST_PARALLEL_LEVEL=16 \
-DTriBITS_ENABLE_CONFIGURE_TIMING=ON \
"$@"

On branch cmake-parse-arguments-181 at commit 3122db8 WIP: Remove commented-out code (#181):

$ time ./do-configure

real    0m4.901s
user    0m3.013s
sys     0m1.095s

On master branch at commit 3fe4ff1 Fix defect in setting <Package>_ENABLE_<TPL>=ON (TriBITSPub/TriBITS#188):

$ time ./do-configure


real    0m5.499s
user    0m3.550s
sys     0m1.192s

That is a 10% reduction for the full configure time. For a project like CASL VERA that uses long arguments lists for TRIBITS_ADD_ADVANCED_TEST() and lots of those. Therefore, this is a worthwhile change from just a performance perspective.

[bartlettroscoe]

I looked at finer-grained configure timing and this change reduces the time for configuring the TriBITS package and tests itself from 1.581s to just 0.894s. This is a 43% reduction in the configure time. Therefore, we should see a very sizable reduction in configure times for projects with many TriBITS packages like Trilinos and especially CASL VERA.

@fryeguy52, from looking at the commits you pushed yesterday, it looks like you are just about through the tasks listed out above. If that is the case, I think it makes sense to meet early next week and pair program the clean up this branch and then the final testing so we can merge and push this. After this, I am excited to try this out with Trilinos and especially CASL VERA to see the reduction in configure times that we might see.

---

DETAILED NOTES:

I realized that most of the time configuring the TriBITS CMake project is just finding compilers. Therefore, I reran the configures with internal timing turned on to show the impact this has on just configuring the TriBITS tests. The results are more striking.

For the do-configure script:

#!/bin/bash

../../../Trilinos/TriBITS/dev_testing/generic/do-configure-mpi-debug \
-DCTEST_PARALLEL_LEVEL=16 \
-DTriBITS_ENABLE_CONFIGURE_TIMING=ON \
"$@"

A) Timing on master:

For the following version on master:

1957aa4 Add documentation for <TRIBITS_PACKAGE>_SOURCE_DIR_OVERRIDE

the configure timing on my machine crf450 was:

$ rm -r CMake*

$ time ./do-configure &> configure.out ; grep "Total time" configure.out 

real    0m5.468s
user    0m3.555s
sys     0m1.134s

Total time to read in and process all package dependencies: 0m0.004s
Total time to adjust package and TPL enables: 0m0.005s
Total time to probe and setup the environment: 0m3.745s
Total time to configure enabled TPLs: 0m0.001s
Total time to configure enabled packages: 0m1.581s
Total time to configure TriBITS: 0m5.360s

B) Timing on cmake-parse-arguments-181:

For the following version on cmake-parse-arguments-181:

0891fb5 added release note about changinging to CMAKE_PARSE_ARGUMNTS() (#181)

timing configure timing on my machine crf450 was:

$ rm -r CMake*

$  time ./do-configure &> configure.out ; grep "Total time" configure.out 

real    0m4.785s
user    0m2.887s
sys     0m1.108s

Total time to read in and process all package dependencies: 0m0.004s
Total time to adjust package and TPL enables: 0m0.004s
Total time to probe and setup the environment: 0m3.708s
Total time to configure enabled TPLs: 0m0.002s
Total time to configure enabled packages: 0m0.894s
Total time to configure TriBITS: 0m4.635s

C) Analysis of timings:

From looking at the above more detailed timings, we can see that almost all of the total reduction in time is for actually configuring the enabled TriBITS package and its tests. That time dropped from 1.581s to just 0.894s. This is a 43% reduction in the configure time. That is pretty big and that shows just how expensive the parsing that goes on with the old PARSE_ARGUMENTS() macro really is. For larger TriBITS CMake projects like Trilinos and CASL VERA with lots of packages and tests, we should see a very sizable overall reduction in the configure time since the probe and setup of the env is a much smaller fraction of the overall time.

[bartlettroscoe]

@fryeguy52 and I spent some time yesterday reviewing the branch 'cmake-parse-arguments-181'. We made two final commits on the branch:

8c5207a "WIP: Add deprecated warning to file include (#181)"
Author: Roscoe A. Bartlett <rabartl@sandia.gov>
Date:   Wed May 31 14:38:17 2017 -0600 (16 hours ago)

M       tribits/core/utils/ParseVariableArguments.cmake

5818af9 "WIP: Remove links to cmake_parse_arguments() (#181)"
Author: Roscoe A. Bartlett <rabartl@sandia.gov>
Date:   Wed May 31 14:21:48 2017 -0600 (16 hours ago)

M       tribits/core/package_arch/TribitsAddAdvancedTest.cmake

The state of that branch was pushed to:

• https://github.com/bartlettroscoe/TriBITS/tree/cmake-parse-arguments-181-old

to archive it.

We then tried to rebase that branch on top of 'github/master'. That resulted in several merge conflicts on rebase due to the conflicting changes from #193 that I pushed recently. We made some mistakes in the merge and lost some work that we had to carefully bring back after careful review. The final squashed commit for the change, ready for final review and testing is 2f4c1ab.

To complete this story, I will now do the following:

• Finish reviewing changes …
• Test configure time for all Trilinos packages …
• Push to TriBITS GitHub repo …
• Snapshot to Trilinos repo …
• Test TriBITS Dashboard Driver for change (this is not under automated testing) …
• Push TriBITS snapshot to Trilinos …

[bartlettroscoe]

Final review of the squashed commit 2f4c1ab ...

Looking for remaining references to the old module:

[rabartl@crf450 TriBITS (cmake-parse-arguments-181)]$ find . -type f -exec grep -nHi parse_arguments {} \; | grep -v "\.html" | grep -v "\.git/" | grep -v "ParseVariableArguments\.cmake" | grep -vi cmake_parse_arguments
./tribits/ReleaseNotes.txt:6:(*) MINOR: PARSE_ARGUMENTS() has been depricated and replaced by
./tribits/ReleaseNotes.txt:8:    PARSE_ARGUMENTS() will warn users and tell them to use

[rabartl@crf450 TriBITS (cmake-parse-arguments-181)]$ find . -type f -exec grep -nHi "ParseVariableArguments\.cmake" {} \; | grep -v "\.git/"
[empty]

That is as it should be. The module is not included anywhere and PARSE_ARGUMENTS() is only referenced in the release notes file.

We also manually tested the include of ParseVariableArguments.cmake and calling PARSE_ARGUMENTS() and the cmake configure still worked, but all of the warnings were emitted.

I ran the full TriBITS test suite on crf450 and it passed:

100% tests passed, 0 tests failed out of 250

Label Time Summary:
TriBITS    = 520.11 sec (250 tests)

This included submitting to CDash:

• http://testing.sandia.gov/cdash/index.php?project=TribitsExProj&date=2017-06-01

for the new tests added in #193.

Now I will do configure performance timing with Trilinos ...

[bartlettroscoe]

I had to fix an issue in Trilinos for the udpated TriBITS, but once I did that, I ran configure times comparing the old and new TriBITS implementations (see details below). Bottom line, the time needed to configure all of the Trilinos packages in the standard CI went build down from 1m3.009s to 0m29.979s! That is a 54% reduction in the configure time for the Trilinos packages and tests. This resulted in big reduction in the total configure (but not generation) time from 1m12.353s to just 0m39.735s. Unfortunately, the generation cost for Trilinos is quite high (almost 2 minutes) so the total configure+generation time only comes down from 2m49.308s to 2m23.799s, or a 15% reduction. But cutting 30s off the configure time for all of Trilinos will be welcome. I would expect to see similar reductions for CASL VERA as well.

Anyway, this is positive but not as positive as I would have hopped (I forgot how expensive the generation time is for Trilinos).

---

DETAILED NOTES:

A) Introduction:

Running configure times on my machine crf450 with the Trilinos SEMS CI env of Trilinos for version:

ce82bb8 "Use CMAKE_PARSE_ARGUMENTS() instead of PARSE_ARGUMENTS() (#1378)"
Author: Roscoe A. Bartlett <rabartl@sandia.gov>
Date:   Thu Jun 1 09:02:19 2017 -0600 (2 minutes ago)

M       cmake/TrilinosCreateClientTemplateHeaders.cmake

(This is a patch, see #1378.)

Using CMake version:

$ which cmake
/projects/sems/install/rhel6-x86_64/sems/utility/cmake/3.5.2/bin/cmake

Using the do-configure script:

#!/bin/bash

cmake \
-DTPL_ENABLE_MPI=ON \
-DCMAKE_BUILD_TYPE=RELEASE \
-DTrilinos_ENABLE_DEBUG=ON \
-DBUILD_SHARED_LIBS=ON \
-DTrilinos_ENABLE_SECONDARY_TESTED_CODE=OFF \
-DTrilinos_CONFIGURE_OPTIONS_FILE:STRING=cmake/std/MpiReleaseDebugSharedPtSettings.cmake,cmake/std/BasicCiTestingSettings.cmake \
-DTrilinos_ENABLE_DEBUG_SYMBOLS=ON \
-DTrilinos_ENABLE_EXPLICIT_INSTANTIATION=ON \
-DTrilinos_ENABLE_CONFIGURE_TIMING=ON \
-DTrilinos_ENABLE_TESTS=ON \
-DTrilinos_TRACE_ADD_TEST=ON \
-DCTEST_BUILD_FLAGS=-j16 \
-DCTEST_PARALLEL_LEVEL=16 \
"$@" \
../../../Trilinos

B) Configure time for Trilinos with old version of TriBITS using PARSE_ARGUMENTS():

First I test with the current TriBITS implementation using PARSE_ARGUMENTS() already in Trilinos:

$ cd ~/Trilinos.base/BUILDS/GCC-4.9.3/MPI_RELEASE_DEBUG_SHARED_PT
$ . ~/Trilinos.base/Trilinos/cmake/load_ci_sems_dev_env.sh
$ rm -r CMake*
$ time ./do-configure -DTrilinos_ENABLE_ALL_PACKAGES=ON &> configure.out

real    2m49.308s
user    2m17.523s
sys     0m29.212s

$ grep "Total time" configure.out

Total time to read in and process all package dependencies: 0m0.421s
Total time to adjust package and TPL enables: 0m0.723s
Total time to probe and setup the environment: 0m5.547s
Total time to configure enabled TPLs: 0m0.271s
Total time to configure enabled packages: 1m3.009s
Total time to set up for CPack packaging: 0m0.214s
Total time to configure Trilinos: 1m12.353s

C) Configure time for Trilinos with new version of TriBITS using CMAKE_PARSE_ARGUMENTS():

Now to try the configure with the TriBITS version:

6201f8f "Remove duplicate 'NAME' arg in CMAKE_PARSE_ARGUMENTS() (#181)"
Author: Roscoe A. Bartlett <rabartl@sandia.gov>
Date:   Thu Jun 1 08:51:51 2017 -0600 (14 seconds ago)

Here is the configure timing of Trilinos now using that updated TriBITS:

$ time ./do-configure -DTrilinos_TRIBITS_DIR:STRING=TriBITS/tribits \
  -DTrilinos_ENABLE_ALL_PACKAGES=ON &> configure.out
                                   
real    2m23.799s
user    1m52.367s
sys     0m29.538s

$ grep "Total time" configure.out

Total time to read in and process all package dependencies: 0m0.381s
Total time to adjust package and TPL enables: 0m0.727s
Total time to probe and setup the environment: 0m5.503s
Total time to configure enabled TPLs: 0m0.471s
Total time to configure enabled packages: 0m29.979s
Total time to set up for CPack packaging: 0m0.293s
Total time to configure Trilinos: 0m39.735s

D) Analysis:

I ran these configure times several times for each case back and forth and got similar timings (within 5% or so). The reduction in the configure time for the packages was large. It went down from 1m3.009s to 0m29.979s. That is a 54% reduction in the configure time for the Trilinos packages and tests. That is huge. That huge reduction reduced the total configure (but not generation) time to come down from 1m12.353s to just 0m39.735s.

Unfortunately, the generation cost for Trilinos is high so the total configure+generation time only comes down from 2m49.308s to 2m23.799s, or a 15% reduction. But cutting 30s off the configure time for all of Trilinos will be welcome. I would expect to see similar reductions for CASL VERA as well.

[bartlettroscoe]

I snapshotted this into Trilinos and pushed (see trilinos/Trilinos#1378). I did not manually test the TriBITS Dashboard Driver, but actually a lot of Trilinos builds don't even use that anymore. Also, the changes were pretty minimal so I am hopeful that it will not be broken.

I am now putting this in review and we can watch and see what happens for a few days.

[bartlettroscoe]

It has been a long time and I have not heard of any more issues related to these changes. Therefore, I am closing.