Merge pull request mesonbuild#5436 from dcbaker/cmake-lexer-spaces-in-args

Cmake lexer spaces in args

Author: jpakkane

mesonbuild/dependencies/base.py

@@ -1546,13 +1546,27 @@ def _var_to_bool(self, var):
             return True
         return False
 
-    def _cmake_set(self, tline: CMakeTraceLine):
+    def _cmake_set(self, tline: CMakeTraceLine) -> None:
+        """Handler for the CMake set() function in all variaties.
+
+        comes in three flavors:
+        set(<var> <value> [PARENT_SCOPE])
+        set(<var> <value> CACHE <type> <docstring> [FORCE])
+        set(ENV{<var>} <value>)
+
+        We don't support the ENV variant, and any uses of it will be ignored
+        silently. the other two variates are supported, with some caveats:
+        - we don't properly handle scoping, so calls to set() inside a
+          function without PARENT_SCOPE set could incorrectly shadow the
+          outer scope.
+        - We don't honor the type of CACHE arguments
+        """
         # DOC: https://cmake.org/cmake/help/latest/command/set.html
 
         # 1st remove PARENT_SCOPE and CACHE from args
         args = []
         for i in tline.args:
-            if i == 'PARENT_SCOPE' or len(i) == 0:
+            if not i or i == 'PARENT_SCOPE':
                 continue
 
             # Discard everything after the CACHE keyword
@@ -1564,13 +1578,19 @@ def _cmake_set(self, tline: CMakeTraceLine):
         if len(args) < 1:
             raise self._gen_exception('CMake: set() requires at least one argument\n{}'.format(tline))
 
-        if len(args) == 1:
+        # Now that we've removed extra arguments all that should be left is the
+        # variable identifier and the value, join the value back together to
+        # ensure spaces in the value are correctly handled. This assumes that
+        # variable names don't have spaces. Please don't do that...
+        identifier = args.pop(0)
+        value = ' '.join(args)
+
+        if not value:
             # Same as unset
-            if args[0] in self.vars:
-                del self.vars[args[0]]
+            if identifier in self.vars:
+                del self.vars[identifier]
         else:
-            values = list(itertools.chain(*map(lambda x: x.split(';'), args[1:])))
-            self.vars[args[0]] = values
+            self.vars[identifier] = value.split(';')
 
     def _cmake_unset(self, tline: CMakeTraceLine):
         # DOC: https://cmake.org/cmake/help/latest/command/unset.html
@@ -1619,7 +1639,7 @@ def _cmake_add_custom_target(self, tline: CMakeTraceLine):
 
         self.targets[tline.args[0]] = CMakeTarget(tline.args[0], 'CUSTOM', {})
 
-    def _cmake_set_property(self, tline: CMakeTraceLine):
+    def _cmake_set_property(self, tline: CMakeTraceLine) -> None:
         # DOC: https://cmake.org/cmake/help/latest/command/set_property.html
         args = list(tline.args)
 
@@ -1629,8 +1649,10 @@ def _cmake_set_property(self, tline: CMakeTraceLine):
 
         append = False
         targets = []
-        while len(args) > 0:
+        while args:
             curr = args.pop(0)
+            # XXX: APPEND_STRING is specifically *not* supposed to create a
+            # list, is treating them as aliases really okay?
             if curr == 'APPEND' or curr == 'APPEND_STRING':
                 append = True
                 continue
@@ -1640,60 +1662,75 @@ def _cmake_set_property(self, tline: CMakeTraceLine):
 
             targets.append(curr)
 
+        if not args:
+            raise self._gen_exception('CMake: set_property() faild to parse argument list\n{}'.format(tline))
+
         if len(args) == 1:
             # Tries to set property to nothing so nothing has to be done
             return
 
-        if len(args) < 2:
-            raise self._gen_exception('CMake: set_property() faild to parse argument list\n{}'.format(tline))
-
-        propName = args[0]
-        propVal = list(itertools.chain(*map(lambda x: x.split(';'), args[1:])))
-        propVal = list(filter(lambda x: len(x) > 0, propVal))
-
-        if len(propVal) == 0:
+        identifier = args.pop(0)
+        value = ' '.join(args).split(';')
+        if not value:
             return
 
         for i in targets:
             if i not in self.targets:
                 raise self._gen_exception('CMake: set_property() TARGET {} not found\n{}'.format(i, tline))
 
-            if propName not in self.targets[i].properies:
-                self.targets[i].properies[propName] = []
+            if identifier not in self.targets[i].properies:
+                self.targets[i].properies[identifier] = []
 
             if append:
-                self.targets[i].properies[propName] += propVal
+                self.targets[i].properies[identifier] += value
             else:
-                self.targets[i].properies[propName] = propVal
+                self.targets[i].properies[identifier] = value
 
-    def _cmake_set_target_properties(self, tline: CMakeTraceLine):
+    def _cmake_set_target_properties(self, tline: CMakeTraceLine) -> None:
         # DOC: https://cmake.org/cmake/help/latest/command/set_target_properties.html
         args = list(tline.args)
 
         targets = []
-        while len(args) > 0:
+        while args:
             curr = args.pop(0)
             if curr == 'PROPERTIES':
                 break
 
             targets.append(curr)
 
-        if (len(args) % 2) != 0:
-            raise self._gen_exception('CMake: set_target_properties() uneven number of property arguments\n{}'.format(tline))
-
-        while len(args) > 0:
-            propName = args.pop(0)
-            propVal = args.pop(0).split(';')
-            propVal = list(filter(lambda x: len(x) > 0, propVal))
-
-            if len(propVal) == 0:
-                continue
+        # Now we need to try to reconsitute the original quoted format of the
+        # arguments, as a property value could have spaces in it. Unlike
+        # set_property() this is not context free. There are two approaches I
+        # can think of, both have drawbacks:
+        #
+        #   1. Assume that the property will be capitalized, this is convention
+        #      but cmake doesn't require it.
+        #   2. Maintain a copy of the list here: https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#target-properties
+        #
+        # Neither of these is awesome for obvious reasons. I'm going to try
+        # option 1 first and fall back to 2, as 1 requires less code and less
+        # synchroniztion for cmake changes.
+
+        arglist = []  # type: typing.List[typing.Tuple[str, typing.List[str]]]
+        name = args.pop(0)
+        values = []
+        for a in args:
+            if a.isupper():
+                if values:
+                    arglist.append((name, ' '.join(values).split(';')))
+                name = a
+                values = []
+            else:
+                values.append(a)
+        if values:
+            arglist.append((name, ' '.join(values).split(';')))
 
+        for name, value in arglist:
             for i in targets:
                 if i not in self.targets:
                     raise self._gen_exception('CMake: set_target_properties() TARGET {} not found\n{}'.format(i, tline))
 
-                self.targets[i].properies[propName] = propVal
+                self.targets[i].properies[name] = value
 
     def _lex_trace(self, trace):
         # The trace format is: '<file>(<line>):  <func>(<args -- can contain \n> )\n'

run_unittests.py

@@ -3681,6 +3681,11 @@ def test_cmake_prefix_path(self):
         testdir = os.path.join(self.unit_test_dir, '60 cmake_prefix_path')
         self.init(testdir, extra_args=['-Dcmake_prefix_path=' + os.path.join(testdir, 'prefix')])
 
+    @skip_if_no_cmake
+    def test_cmake_parser(self):
+        testdir = os.path.join(self.unit_test_dir, '61 cmake parser')
+        self.init(testdir, extra_args=['-Dcmake_prefix_path=' + os.path.join(testdir, 'prefix')])
+
 class FailureTests(BasePlatformTests):
     '''
     Tests that test failure conditions. Build files here should be dynamically

test cases/unit/61 cmake parser/meson.build

@@ -0,0 +1,19 @@
+project('61 cmake parser')
+
+dep = dependency('mesontest')
+
+# Test a bunch of variations of the set() command
+assert(dep.get_variable(cmake : 'VAR_WITHOUT_SPACES') == 'NoSpaces', 'set() without spaces incorrect')
+assert(dep.get_variable(cmake : 'VAR_WITH_SPACES') == 'With Spaces', 'set() with spaces incorrect')
+
+assert(dep.get_variable(cmake : 'VAR_WITHOUT_SPACES_PS') == 'NoSpaces', 'set(PARENT_SCOPE) without spaces incorrect')
+assert(dep.get_variable(cmake : 'VAR_WITH_SPACES_PS') == 'With Spaces', 'set(PARENT_SCOPE) with spaces incorrect')
+
+assert(dep.get_variable(cmake : 'VAR_THAT_IS_UNSET', default_value : 'sentinal') == 'sentinal', 'set() to unset is incorrect')
+assert(dep.get_variable(cmake : 'CACHED_STRING_NS') == 'foo', 'set(CACHED) without spaces is incorrect')
+assert(dep.get_variable(cmake : 'CACHED_STRING_WS') == 'foo bar', 'set(CACHED STRING) with spaces is incorrect')
+assert(dep.get_variable(cmake : 'CACHED_STRING_ARRAY_NS') == ['foo', 'bar'], 'set(CACHED STRING) without spaces is incorrect')
+assert(dep.get_variable(cmake : 'CACHED_STRING_ARRAY_WS') == ['foo', 'foo bar', 'bar'], 'set(CACHED STRING[]) with spaces is incorrect')
+
+# We don't suppor this, so it should be unset.
+assert(dep.get_variable(cmake : 'ENV{var}', default_value : 'sentinal') == 'sentinal', 'set(ENV) should be ignored')

test cases/unit/61 cmake parser/prefix/lib/cmake/mesontest/mesontest-config.cmake

@@ -0,0 +1,63 @@
+# This should be enough to satisfy the basic parser
+set(MESONTEST_VERSION "1.2.3")
+set(MESONTEST_LIBRARIES "foo.so")
+set(MESONTEST_INCLUDE_DIR "")
+set(MESONTEST_FOUND "TRUE")
+
+## Tests for set() in its various forms
+
+# Basic usage
+set(VAR_WITHOUT_SPACES "NoSpaces")
+set(VAR_WITH_SPACES "With Spaces")
+
+# test of PARENT_SCOPE, requires a function to have a parent scope obviously...
+function(foo)
+    set(VAR_WITHOUT_SPACES_PS "NoSpaces" PARENT_SCOPE)
+    set(VAR_WITH_SPACES_PS "With Spaces" PARENT_SCOPE)
+endfunction(foo)
+foo()
+
+# Using set() to unset values
+set(VAR_THAT_IS_UNSET "foo")
+set(VAR_THAT_IS_UNSET)
+
+# The more advanced form that uses CACHE
+# XXX: Why don't we read the type and use that instead of always treat things as strings?
+set(CACHED_STRING_NS "foo" CACHE STRING "docstring")
+set(CACHED_STRING_WS "foo bar" CACHE STRING "docstring")
+set(CACHED_STRING_ARRAY_NS "foo;bar" CACHE STRING "doc string")
+set(CACHED_STRING_ARRAY_WS "foo;foo bar;bar" CACHE STRING "stuff" FORCE)
+
+set(CACHED_BOOL ON CACHE BOOL "docstring")
+
+set(CACHED_PATH_NS "foo/bar" CACHE PATH "docstring")
+set(CACHED_PATH_WS "foo bar/fin" CACHE PATH "docstring")
+
+set(CACHED_FILEPATH_NS "foo/bar.txt" CACHE FILEPATH "docstring")
+set(CACHED_FILEPATH_WS "foo bar/fin.txt" CACHE FILEPATH "docstring")
+
+# Set ENV, we don't support this so it shouldn't be showing up
+set(ENV{var}, "foo")
+
+
+## Tests for set_properties()
+# We need something to attach properties too
+add_custom_target(MESONTEST_FOO ALL)
+
+set_property(TARGET MESONTEST_FOO PROPERTY FOLDER "value")
+set_property(TARGET MESONTEST_FOO APPEND PROPERTY FOLDER "name")
+set_property(TARGET MESONTEST_FOO PROPERTY FOLDER "value")
+set_property(TARGET MESONTEST_FOO APPEND_STRING PROPERTY FOLDER "name")
+
+set_property(TARGET MESONTEST_FOO PROPERTY FOLDER "value space")
+set_property(TARGET MESONTEST_FOO PROPERTY FOLDER "value space")
+set_property(TARGET MESONTEST_FOO APPEND PROPERTY FOLDER "name space")
+set_property(TARGET MESONTEST_FOO PROPERTY FOLDER "value space")
+set_property(TARGET MESONTEST_FOO APPEND_STRING PROPERTY FOLDER "name space")
+
+## Tests for set_target_properties()
+set_target_properties(MESONTEST_FOO PROPERTIES FOLDER "value")
+set_target_properties(MESONTEST_FOO PROPERTIES FOLDER "value space")
+set_target_properties(MESONTEST_FOO PROPERTIES FOLDER "value" OUTPUT_NAME "another value")
+set_target_properties(MESONTEST_FOO PROPERTIES FOLDER "value space" OUTPUT_NAME "another value")
+set_target_properties(MESONTEST_FOO PROPERTIES FOLDER "value space" OUTPUT_NAME "value")