From 2e2c3c968c02c3d8e87aa9e1547a842cd793f045 Mon Sep 17 00:00:00 2001 From: Dylan Baker Date: Tue, 28 May 2019 16:49:21 -0700 Subject: [PATCH 1/4] tests: Test the cmake parser more thuroughly It turns out there are bugs, in particular with spaces in variables... --- mesonbuild/dependencies/base.py | 2 +- run_unittests.py | 5 ++ test cases/unit/61 cmake parser/meson.build | 19 ++++++ .../cmake/mesontest/mesontest-config.cmake | 63 +++++++++++++++++++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 test cases/unit/61 cmake parser/meson.build create mode 100644 test cases/unit/61 cmake parser/prefix/lib/cmake/mesontest/mesontest-config.cmake diff --git a/mesonbuild/dependencies/base.py b/mesonbuild/dependencies/base.py index 1ccbf6f87..86cc3f8ee 100644 --- a/mesonbuild/dependencies/base.py +++ b/mesonbuild/dependencies/base.py @@ -1666,7 +1666,7 @@ class CMakeDependency(ExternalDependency): else: self.targets[i].properies[propName] = propVal - 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) diff --git a/run_unittests.py b/run_unittests.py index b477aa348..b8f0bf23f 100755 --- a/run_unittests.py +++ b/run_unittests.py @@ -3681,6 +3681,11 @@ recommended as it is not supported on some platforms''') 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 diff --git a/test cases/unit/61 cmake parser/meson.build b/test cases/unit/61 cmake parser/meson.build new file mode 100644 index 000000000..1ca83876f --- /dev/null +++ b/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') \ No newline at end of file diff --git a/test cases/unit/61 cmake parser/prefix/lib/cmake/mesontest/mesontest-config.cmake b/test cases/unit/61 cmake parser/prefix/lib/cmake/mesontest/mesontest-config.cmake new file mode 100644 index 000000000..7474eb78d --- /dev/null +++ b/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") \ No newline at end of file From 99b848f469671f43b3ec6b1de333d7154d0d59ea Mon Sep 17 00:00:00 2001 From: Dylan Baker Date: Wed, 29 May 2019 10:19:34 -0700 Subject: [PATCH 2/4] dependencies/cmake: correctly handle spaces in variable names --- mesonbuild/dependencies/base.py | 34 ++++++++++++++++----- test cases/unit/61 cmake parser/meson.build | 8 ++--- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/mesonbuild/dependencies/base.py b/mesonbuild/dependencies/base.py index 86cc3f8ee..218ff108e 100644 --- a/mesonbuild/dependencies/base.py +++ b/mesonbuild/dependencies/base.py @@ -1546,13 +1546,27 @@ class CMakeDependency(ExternalDependency): 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( [PARENT_SCOPE]) + set( CACHE [FORCE]) + set(ENV{} ) + + 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 @@ class CMakeDependency(ExternalDependency): 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 diff --git a/test cases/unit/61 cmake parser/meson.build b/test cases/unit/61 cmake parser/meson.build index 1ca83876f..62b9124af 100644 --- a/test cases/unit/61 cmake parser/meson.build +++ b/test cases/unit/61 cmake parser/meson.build @@ -4,16 +4,16 @@ 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_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_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_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') +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') \ No newline at end of file From eaa232987e3410198fa3a1eb422ca5b8a3baba8c Mon Sep 17 00:00:00 2001 From: Dylan Baker Date: Wed, 29 May 2019 10:56:38 -0700 Subject: [PATCH 3/4] dependencies/cmake: Handle spaces in set_property calls --- mesonbuild/dependencies/base.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/mesonbuild/dependencies/base.py b/mesonbuild/dependencies/base.py index 218ff108e..5fc5c02fb 100644 --- a/mesonbuild/dependencies/base.py +++ b/mesonbuild/dependencies/base.py @@ -1639,7 +1639,7 @@ class CMakeDependency(ExternalDependency): 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) @@ -1649,8 +1649,10 @@ class CMakeDependency(ExternalDependency): 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 @@ -1660,31 +1662,29 @@ class CMakeDependency(ExternalDependency): 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) -> None: # DOC: https://cmake.org/cmake/help/latest/command/set_target_properties.html From 31ab9627533e1ae0a024c36226f703ac9ceab0a2 Mon Sep 17 00:00:00 2001 From: Dylan Baker Date: Wed, 29 May 2019 11:24:05 -0700 Subject: [PATCH 4/4] dependencies/cmake: Handle spaces in set_target_properties this is better, but it's still not perfect. cmake doesn't return quotes to us in the trace output, and being 100% the same as cmake is pretty much impossible without that information. What I've done should be a "good enough" implementation without having to maintain a copy of every property allowed in cmake, as well as custom properties. --- mesonbuild/dependencies/base.py | 41 +++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/mesonbuild/dependencies/base.py b/mesonbuild/dependencies/base.py index 5fc5c02fb..21cd821a5 100644 --- a/mesonbuild/dependencies/base.py +++ b/mesonbuild/dependencies/base.py @@ -1691,29 +1691,46 @@ class CMakeDependency(ExternalDependency): 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: '(): ( )\n'