TRILFRAME-128: Fixes to opt-set-cmake-options

Author: William McLendon

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 ]------------------------------------------")