Merge branch 'TRILFRAME-128' into 'master'

Trilframe 128: Fix PARENT_SCOPE + FORCE error plus FORCE implies CACHE

See merge request trilinos-devops-consolidation/code/SetProgramOptions!16

Author: William McLendon

CHANGELOG.md

@@ -20,6 +20,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [0.3.1] <!-- YYYY-MM-DD --> [Unreleased]
 #### Added
 #### Changed
+- Changes to `opt-set-cmake-var` behaviour:
+  - Trigger a WARNING if `PARENT_SCOPE` and a _TYPE_ is provided since CMake
+    requires TYPED vars to be CACHE vars and `PARENT_SCOPE` will force the
+    var to be considered a non CACHE var that would store the parameters
+    before `PARENT_SCOPE` in the call as _list_ entries.
+  - Trigger an exception if `PARENT_SCOPE` and `FORCE` are both provided since
+    `FORCE` is only valid for CACHE variables but `PARENT_SCOPE` is only valid
+    for non-CACHE vars (and as noted above will _force_ the `set()` operation
+    to be a non-CACHE var which then triggers a CMake error because `FORCE` is
+    invalid to that form of `set()`.
+  - **bash** generated options that have a TYPE and `PARENT_SCOPE` are processed
+    to generate the same _list_ string that would be generated by the matching
+    `set()` operation (with the warning).
 - Revert(ish) the modifications from MR12 which removed the error when generating
   BASH output if there is an unhandled CMake variable left hanging around.
   Rather than make this an unavoidable error though, ExpandVarsInText now inherits

doc/source/SetProgramOptionsCMake.rst

@@ -15,6 +15,37 @@ Supported .ini File Operations
     :header-rows: 1
     :widths: 20,30,80
 
+``opt-set-cmake-var``
++++++++++++++++++++++
+**Usage**: ``opt-set-cmake-var <varname> [TYPE] [PARENT_SCOPE] [FORCE] : <value>``
+
+A CMake variable set operation. This can be used to genrate bash command line options
+of the form ``-D<varname>[:<type>]=<value>`` or in a CMake fragment we may generate
+a ``set()`` command.
+
+For information related to CMake Fragment generation of ``set()`` commands, see
+the `CMake docs <https://cmake.org/cmake/help/latest/command/set.html#command:set>`_ .
+
+``PARENT_SCOPE`` and ``FORCE`` are mutually exclusive options and will result in an
+error being thrown.
+
+An additional thing to note is that ``PARENT_SCOPE`` should not be used with ``CACHE``
+variables (i.e., *typed* variables). CMake will happily process this but you will likely
+get a result that you do not want. In a CMake fragment file:
+
+.. code-block:: cmake
+  :linenos:
+
+  set(FOO FOO_VALUE CACHE STRING "my docstring" PARENT_SCOPE)
+
+Will result in the value of ``FOO`` being set to ``FOO_VALUE;CACHE;STRING;my docstring``
+which is unlikely to match expectations, but that is what CMake will do. In this case,
+``SetProgramOptionsCMake`` will raise a warning and will generate a string to match what
+CMake generates when sent to a bash options generator
+(i.e., ``-DFOO="FOO_VALUE;CACHE;STRING;my docstring"``).
+When sent through the *cmake fragment* generator the output will be the equivalent ``set()``
+call.
+
 
 Useful Links
 ------------

src/setprogramoptions/SetProgramOptionsCMake.py

@@ -202,7 +202,14 @@ def _program_option_handler_opt_set_cmake_var_bash(self, params, value) -> str:
         param_opts = self._helper_opt_set_cmake_var_parse_parameters(params)
 
         params = ["-D", varname]
-        if param_opts['TYPE'] is not None:
+
+        if param_opts['VARIANT'] == 1:
+            # if PARENT_SCOPE was given to something that is typed and forced us to
+            # be a type-1 variant, then we assign the list "<value>;CACHE;<type>;<docstring>"
+            if param_opts['TYPE'] != None:
+                value += f";CACHE;{param_opts['TYPE']};"
+
+        if param_opts['VARIANT'] == 2 and param_opts['TYPE'] is not None:
             params.append(":" + param_opts['TYPE'])
 
         # Cache 'known' CMake vars here.
@@ -314,7 +321,7 @@ def _helper_opt_set_cmake_var_parse_parameters(self, params: list):
         because the relevance of some options depends on the back-end
         generator. For example, ``PARENT_SCOPE`` is relevant to a ``cmake_fragment``
         generator but does not get used for a ``bash`` option that would be
-        provided to the ``camke`` application on the command line.
+        provided to the ``cmake`` application on the command line.
 
         Called By:
 
@@ -330,14 +337,62 @@ def _helper_opt_set_cmake_var_parse_parameters(self, params: list):
             omission of flags that are applicable to a CMake Cache
             variable operation.
         """
+        default_cache_var_type = "STRING"
+
         output = {'FORCE': False, 'PARENT_SCOPE': False, 'TYPE': None}
 
         for option in params[: 4]:
             if option == "FORCE":
                 output['FORCE'] = True
+                # If FORCE is found but we have no TYPE yet, set to the default.
+                if output['TYPE'] is None:
+                    output['TYPE'] = default_cache_var_type
             elif option == "PARENT_SCOPE":
                 output['PARENT_SCOPE'] = True
             elif option in ["BOOL", "FILEPATH", "PATH", "STRING", "INTERVAL"]:
                 output['TYPE'] = option
 
+        # Sanity Check(s)
+
+        # Case 1: PARENT_SCOPE and FORCE will cause a CMake Error
+        if output['FORCE'] and output['PARENT_SCOPE']:
+            msg = "ERROR: CMake does not allow `FORCE` and `PARENT_SCOPE` to both be used."
+            self.exception_control_event("CATASTROPHIC", ValueError, message=msg)
+        # Case 2: PARENT_SCOPE and CACHE will cause a CMake warning
+        #         and the value will include the cache entries as a list:
+        #             `set(FOO "VAL" CACHE STRING "docstring" PARENT_SCOPE)`
+        #         will result in `FOO` being set to "VAL;CACHE;STRING;docstring"
+        #         in the parent module's scope. This is probably not what someone
+        #         intended. Let this be a WARNING event though.
+        #         TBH: this should probably be a CATASTROPHIC but for now I'll
+        #              at least warn about it which is more than CMake does.
+        if output['PARENT_SCOPE'] and output["TYPE"] != None:
+            msg = "WARNING: Setting `PARENT_SCOPE` with `CACHE` parameters will result\n"
+            msg += "         in a non-CACHE variable being set containing a list of the\n"
+            msg += "         CACHE options. i.e., '<value>;CACHE;<type>;<docstring>'\n"
+            msg += "         which is probably not what is intended, but CMake will\n"
+            msg += "         not error or warn on this."
+            self.exception_control_event("WARNING", ValueError, message=msg)
+
+        # Determine the variant of the ``set`` operation.
+        # Type 1: ``set(<variable> <value>... [PARENT_SCOPE])``
+        # Type 2: ``set(<variable> <value>... CACHE <type> <docstring> [FORCE])``
+
+        # PARENT_SCOPE forces Type-1 (i.e., non-cache var)
+        # - This will override CACHE, at least as of CMake 3.21.x
+        if output['PARENT_SCOPE']:
+            output['VARIANT'] = 1
+
+        # FORCE implies CACHE. If type wasn't provided then it's a STRING
+        elif output['FORCE']:
+            output['VARIANT'] = 2
+
+        # If a TYPE is provided then it's a type-2 (CACHE) assignment.
+        elif output['TYPE'] is not None:
+            output['VARIANT'] = 2
+
+        # Otherwise, a simple set is a type-1
+        else:
+            output['VARIANT'] = 1
+
         return output

src/setprogramoptions/unittests/files_ini/config_test_setprogramoptions.ini

@@ -46,29 +46,26 @@ use CMAKE_SOURCE_DIR
 opt-remove Trilinos_ENABLE_MueLu
 opt-remove MueLu SUBSTR
 
+
+[TEST_CMAKE_FAIL_ON_PARENT_SCOPE_AND_FORCE]
+# PARENT_SCOPE makes the value a non-cache option
+# but FORCE requires a cache type
+# CMake will error if you give both so we should raise an exception here.
+opt-set-cmake-var CMAKE_VAR_A PARENT_SCOPE FORCE : CMAKE_VAR_A_VAL
+
+
 [TEST_CMAKE_CACHE_PARAM_ORDER]
 opt-set-cmake-var CMAKE_VAR_A FORCE        : ON
 opt-set-cmake-var CMAKE_VAR_B PARENT_SCOPE : ON
 opt-set-cmake-var CMAKE_VAR_C BOOL         : ON
-
-opt-set-cmake-var CMAKE_VAR_D FORCE PARENT_SCOPE : ON
-opt-set-cmake-var CMAKE_VAR_E PARENT_SCOPE FORCE : ON
-
-opt-set-cmake-var CMAKE_VAR_F BOOL FORCE : ON
-opt-set-cmake-var CMAKE_VAR_G FORCE BOOL : ON
-
+opt-set-cmake-var CMAKE_VAR_F BOOL FORCE   : ON
+opt-set-cmake-var CMAKE_VAR_G FORCE BOOL   : ON
 opt-set-cmake-var CMAKE_VAR_H BOOL PARENT_SCOPE : ON
 opt-set-cmake-var CMAKE_VAR_I PARENT_SCOPE BOOL : ON
 
-opt-set-cmake-var CMAKE_VAR_J BOOL FORCE PARENT_SCOPE : ON
-opt-set-cmake-var CMAKE_VAR_K BOOL PARENT_SCOPE FORCE : ON
-opt-set-cmake-var CMAKE_VAR_L FORCE PARENT_SCOPE BOOL : ON
-opt-set-cmake-var CMAKE_VAR_M FORCE BOOL PARENT_SCOPE : ON
-opt-set-cmake-var CMAKE_VAR_N PARENT_SCOPE BOOL FORCE : ON
-opt-set-cmake-var CMAKE_VAR_O PARENT_SCOPE FORCE BOOL : ON
-
 [TEST_CMAKE_CACHE_PARAM_TEST_02]
 # Validate what happens if there's a bad param.
+# Note: FORCE option will make this a CACHE var of type STRING
 opt-set-cmake-var CMAKE_VAR_A FORCE FOOBAR: ON
 
 

src/setprogramoptions/unittests/test_SetProgramOptionsCMake.py

@@ -367,42 +367,26 @@ def test_SetProgramOptionsCMake_param_order_01(self):
         self._execute_parser(parser, section)
 
         option_list_bash_expect = [
-            '-DCMAKE_VAR_A=ON',
+            '-DCMAKE_VAR_A:STRING="ON"',
             '-DCMAKE_VAR_B=ON',
             '-DCMAKE_VAR_C:BOOL=ON',
-            '-DCMAKE_VAR_D=ON',
-            '-DCMAKE_VAR_E=ON',
             '-DCMAKE_VAR_F:BOOL=ON',
             '-DCMAKE_VAR_G:BOOL=ON',
-            '-DCMAKE_VAR_H:BOOL=ON',
-            '-DCMAKE_VAR_I:BOOL=ON',
-            '-DCMAKE_VAR_J:BOOL=ON',
-            '-DCMAKE_VAR_K:BOOL=ON',
-            '-DCMAKE_VAR_L:BOOL=ON',
-            '-DCMAKE_VAR_M:BOOL=ON',
-            '-DCMAKE_VAR_N:BOOL=ON',
-            '-DCMAKE_VAR_O:BOOL=ON'
+            '-DCMAKE_VAR_H=ON;CACHE;BOOL;',
+            '-DCMAKE_VAR_I=ON;CACHE;BOOL;'
         ]
 
         option_list_bash_actual = parser.gen_option_list(section, generator="bash")
         self.assertListEqual(option_list_bash_expect, option_list_bash_actual)
 
         option_list_cmake_fragment_expect = [
-            'set(CMAKE_VAR_A ON FORCE)',
+            'set(CMAKE_VAR_A ON CACHE STRING "from .ini configuration" FORCE)',
             'set(CMAKE_VAR_B ON PARENT_SCOPE)',
             'set(CMAKE_VAR_C ON CACHE BOOL "from .ini configuration")',
-            'set(CMAKE_VAR_D ON PARENT_SCOPE FORCE)',
-            'set(CMAKE_VAR_E ON PARENT_SCOPE FORCE)',
             'set(CMAKE_VAR_F ON CACHE BOOL "from .ini configuration" FORCE)',
             'set(CMAKE_VAR_G ON CACHE BOOL "from .ini configuration" FORCE)',
             'set(CMAKE_VAR_H ON CACHE BOOL "from .ini configuration" PARENT_SCOPE)',
             'set(CMAKE_VAR_I ON CACHE BOOL "from .ini configuration" PARENT_SCOPE)',
-            'set(CMAKE_VAR_J ON CACHE BOOL "from .ini configuration" PARENT_SCOPE FORCE)',
-            'set(CMAKE_VAR_K ON CACHE BOOL "from .ini configuration" PARENT_SCOPE FORCE)',
-            'set(CMAKE_VAR_L ON CACHE BOOL "from .ini configuration" PARENT_SCOPE FORCE)',
-            'set(CMAKE_VAR_M ON CACHE BOOL "from .ini configuration" PARENT_SCOPE FORCE)',
-            'set(CMAKE_VAR_N ON CACHE BOOL "from .ini configuration" PARENT_SCOPE FORCE)',
-            'set(CMAKE_VAR_O ON CACHE BOOL "from .ini configuration" PARENT_SCOPE FORCE)'
         ]
 
         option_list_cmake_fragment_actual = parser.gen_option_list(section, generator="cmake_fragment")
@@ -414,6 +398,8 @@ def test_SetProgramOptionsCMake_param_order_01(self):
 
     def test_SetProgramOptionsCMake_param_order_02(self):
         """
+        Tests that we correctly generate output if extra flags
+        are provided such as something to uniqueify a .ini option entry.
         """
         parser = self._create_standard_parser()
 
@@ -424,7 +410,7 @@ def test_SetProgramOptionsCMake_param_order_02(self):
         # parse a section
         self._execute_parser(parser, section)
 
-        option_list_bash_expect = ['-DCMAKE_VAR_A=ON']
+        option_list_bash_expect = ['-DCMAKE_VAR_A:STRING="ON"']
 
         option_list_bash_actual = parser.gen_option_list(section, generator="bash")
         self.assertListEqual(option_list_bash_expect, option_list_bash_actual)
@@ -433,6 +419,34 @@ def test_SetProgramOptionsCMake_param_order_02(self):
         print("OK")
         return
 
+    def test_SetProgramOptionsCMake_fail_on_FORCE_and_PARENT_SCOPE(self):
+        """
+        Tests the case that both PARENT_SCOPE and FORCE are provided.
+        This will cause a CMake error beacuse the existence of PARENT_SCOPE
+        forces CMake to use a Type-1 set operation, i.e. a NON-CACHEd
+        variable. However ``FORCE`` is only valid for a CACHED variable (Type-2).
+        These two options are mutually exclusive and CMake will fail.
+
+        In this case SetProgramOptionsCMake should raise a CATASTROPHIC
+        error because the operation provided is invalid.
+        """
+        parser = self._create_standard_parser()
+
+        print("-----[ TEST BEGIN ]----------------------------------------")
+        section = "TEST_CMAKE_FAIL_ON_PARENT_SCOPE_AND_FORCE"
+        print("Section  : {}".format(section))
+
+        # parse a section
+        self._execute_parser(parser, section)
+
+        with self.assertRaises(ValueError):
+            parser.gen_option_list(section, generator="bash")
+
+        print("-----[ TEST END ]------------------------------------------")
+
+        print("OK")
+        return
+
     def test_SetProgramOptionsCMake_test_STRING_value_surrounded_by_double_quotes(self):
         """
         Test STRING values are surrounded by double quotes.
@@ -445,10 +459,7 @@ def test_SetProgramOptionsCMake_test_STRING_value_surrounded_by_double_quotes(se
         section = "TEST_STRING_DOUBLE_QUOTES"
         print("Section  : {}".format(section))
 
-        option_list_expect = [
-            '-DFOO:STRING="foo::bar::baz<Type>"',
-            '-DBAR:STRING="600"'
-        ]
+        option_list_expect = ['-DFOO:STRING="foo::bar::baz<Type>"', '-DBAR:STRING="600"']
         option_list_actual = parser.gen_option_list(section, generator="bash")
 
         print("-" * 40)
@@ -478,15 +489,15 @@ def test_SetProgramOptionsCMake_opt_remove(self):
         section = "TEST_CMAKE_VAR_REMOVE"
         print("Section  : {}".format(section))
         option_list_bash_actual = parser.gen_option_list('TEST_CMAKE_VAR_REMOVE', 'bash')
-        option_list_bash_expect = [ '-DBAR_TEST=BAR', '-DFOO=BAZ' ]
+        option_list_bash_expect = ['-DBAR_TEST=BAR', '-DFOO=BAZ']
         self.assertListEqual(option_list_bash_expect, option_list_bash_actual)
         print("-----[ TEST END ]------------------------------------------")
 
         print("-----[ TEST BEGIN ]----------------------------------------")
         section = "TEST_CMAKE_VAR_REMOVE"
         print("Section  : {}".format(section))
         option_list_cmake_fragment_actual = parser.gen_option_list('TEST_CMAKE_VAR_REMOVE', 'cmake_fragment')
-        option_list_cmake_fragment_expect = [ 'set(BAR_TEST BAR)', 'set(FOO BAZ)' ]
+        option_list_cmake_fragment_expect = ['set(BAR_TEST BAR)', 'set(FOO BAZ)']
         self.assertListEqual(option_list_cmake_fragment_expect, option_list_cmake_fragment_actual)
         print("-----[ TEST END ]------------------------------------------")