From 3c8646e368ec50fcfc98bac8f99515249f7b6700 Mon Sep 17 00:00:00 2001 From: Josh Smith Date: Thu, 23 Aug 2018 12:14:32 +0100 Subject: Add cyclic check within variable resolution This aims to address #600, this will raise an exception when a resolved variable contains a reference to the variable. --- buildstream/_exceptions.py | 3 +++ buildstream/_variables.py | 32 ++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/buildstream/_exceptions.py b/buildstream/_exceptions.py index 3fb5e5775..55c5d6bbf 100644 --- a/buildstream/_exceptions.py +++ b/buildstream/_exceptions.py @@ -217,6 +217,9 @@ class LoadErrorReason(Enum): # A recursive include has been encountered. RECURSIVE_INCLUDE = 21 + # A recursive variable has been encountered + RECURSIVE_VARIABLE = 22 + # LoadError # diff --git a/buildstream/_variables.py b/buildstream/_variables.py index 8299f1c1e..1a52b5680 100644 --- a/buildstream/_variables.py +++ b/buildstream/_variables.py @@ -61,7 +61,7 @@ class Variables(): # LoadError, if the string contains unresolved variable references. # def subst(self, string): - substitute, unmatched = self._subst(string, self.variables) + substitute, unmatched, _ = self._subst(string, self.variables) unmatched = list(set(unmatched)) if unmatched: if len(unmatched) == 1: @@ -82,6 +82,7 @@ class Variables(): def subst_callback(match): nonlocal variables nonlocal unmatched + nonlocal matched token = match.group(0) varname = match.group(1) @@ -91,6 +92,7 @@ class Variables(): # We have to check if the inner string has variables # and return unmatches for those unmatched += re.findall(_VARIABLE_MATCH, value) + matched += [varname] else: # Return unmodified token unmatched += [varname] @@ -98,10 +100,11 @@ class Variables(): return value + matched = [] unmatched = [] replacement = re.sub(_VARIABLE_MATCH, subst_callback, string) - return (replacement, unmatched) + return (replacement, unmatched, matched) # Variable resolving code # @@ -131,7 +134,15 @@ class Variables(): # Ensure stringness of the value before substitution value = _yaml.node_get(variables, str, key) - resolved_var, item_unmatched = self._subst(value, variables) + resolved_var, item_unmatched, matched = self._subst(value, variables) + + if _wrap_variable(key) in resolved_var: + referenced_through = find_recursive_variable(key, matched, variables) + raise LoadError(LoadErrorReason.RECURSIVE_VARIABLE, + "{}: ".format(_yaml.node_get_provenance(variables, key)) + + ("Variable '{}' expands to contain a reference to itself. " + + "Perhaps '{}' contains '{}").format(key, referenced_through, _wrap_variable(key))) + resolved[key] = resolved_var unmatched += item_unmatched @@ -168,8 +179,21 @@ class Variables(): # Helper function to fetch information about the node referring to a variable # def _find_references(self, varname): - fullname = '%{' + varname + '}' + fullname = _wrap_variable(varname) for key, value in _yaml.node_items(self.original): if fullname in value: provenance = _yaml.node_get_provenance(self.original, key) yield (key, provenance) + + +def find_recursive_variable(variable, matched_variables, all_vars): + matched_values = (_yaml.node_get(all_vars, str, key) for key in matched_variables) + for key, value in zip(matched_variables, matched_values): + if _wrap_variable(variable) in value: + return key + else: + return None + + +def _wrap_variable(var): + return "%{" + var + "}" -- cgit v1.2.1 From d41c42d6c53884c63046a1b7df14f5ab87566551 Mon Sep 17 00:00:00 2001 From: Josh Smith Date: Wed, 29 Aug 2018 11:54:47 +0100 Subject: Add tests for cyclic variables check Note: This modifies the docker containers used for testing to supply the pytest-timeout package. --- .gitlab-ci.yml | 10 +++++----- dev-requirements.txt | 1 + tests/format/variables.py | 18 ++++++++++++++++++ tests/format/variables/cyclic_variables/cyclic.bst | 5 +++++ tests/format/variables/cyclic_variables/project.conf | 1 + tests/testutils/runcli.py | 18 ++++++++++++++++-- 6 files changed, 46 insertions(+), 7 deletions(-) create mode 100644 tests/format/variables/cyclic_variables/cyclic.bst create mode 100644 tests/format/variables/cyclic_variables/project.conf diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 16f7d04a9..d57d33f83 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -84,25 +84,25 @@ source_dist: - coverage-linux/ tests-debian-9: - image: buildstream/testsuite-debian:9-master-114-4cab18e3 + image: buildstream/testsuite-debian:9-master-117-aa3a33b3 <<: *linux-tests tests-fedora-27: - image: buildstream/testsuite-fedora:27-master-114-4cab18e3 + image: buildstream/testsuite-fedora:27-master-117-aa3a33b3 <<: *linux-tests tests-fedora-28: - image: buildstream/testsuite-fedora:28-master-114-4cab18e3 + image: buildstream/testsuite-fedora:28-master-117-aa3a33b3 <<: *linux-tests tests-ubuntu-18.04: - image: buildstream/testsuite-ubuntu:18.04-master-114-4cab18e3 + image: buildstream/testsuite-ubuntu:18.04-master-117-aa3a33b3 <<: *linux-tests tests-unix: # Use fedora here, to a) run a test on fedora and b) ensure that we # can get rid of ostree - this is not possible with debian-8 - image: buildstream/testsuite-fedora:27-master-114-4cab18e3 + image: buildstream/testsuite-fedora:27-master-117-aa3a33b3 stage: test variables: BST_FORCE_BACKEND: "unix" diff --git a/dev-requirements.txt b/dev-requirements.txt index ee2db0351..c88b4c723 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -8,3 +8,4 @@ pytest-env pytest-pep8 pytest-pylint pytest-xdist +pytest-timeout diff --git a/tests/format/variables.py b/tests/format/variables.py index d570bf01d..d01d87e5b 100644 --- a/tests/format/variables.py +++ b/tests/format/variables.py @@ -1,5 +1,6 @@ import os import pytest +import sys from buildstream import _yaml from buildstream._exceptions import ErrorDomain, LoadErrorReason from tests.testutils.runcli import cli @@ -72,3 +73,20 @@ def test_missing_variable(cli, datafiles, tmpdir): ]) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.UNRESOLVED_VARIABLE) + + +@pytest.mark.timeout(3, method="signal") +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'cyclic_variables')) +def test_cyclic_variables(cli, datafiles): + print_warning("Performing cyclic test, if this test times out it will " + + "exit the test sequence") + project = os.path.join(datafiles.dirname, datafiles.basename) + result = cli.run(project=project, silent=True, args=[ + "build", "cyclic.bst" + ]) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_VARIABLE) + + +def print_warning(msg): + RED, END = "\033[91m", "\033[0m" + print(("\n{}{}{}").format(RED, msg, END), file=sys.stderr) diff --git a/tests/format/variables/cyclic_variables/cyclic.bst b/tests/format/variables/cyclic_variables/cyclic.bst new file mode 100644 index 000000000..a05a40b27 --- /dev/null +++ b/tests/format/variables/cyclic_variables/cyclic.bst @@ -0,0 +1,5 @@ +kind: manual + +variables: + a: "%{prefix}/a" + prefix: "%{a}/some_prefix/" \ No newline at end of file diff --git a/tests/format/variables/cyclic_variables/project.conf b/tests/format/variables/cyclic_variables/project.conf new file mode 100644 index 000000000..b32753625 --- /dev/null +++ b/tests/format/variables/cyclic_variables/project.conf @@ -0,0 +1 @@ +name: test diff --git a/tests/testutils/runcli.py b/tests/testutils/runcli.py index ee4eb957e..8cd5bcb75 100644 --- a/tests/testutils/runcli.py +++ b/tests/testutils/runcli.py @@ -94,14 +94,28 @@ class Result(): # error_reason (any): The reason field of the error which occurred # fail_message (str): An optional message to override the automatic # assertion error messages + # debug (bool): If true, prints information regarding the exit state of the result() # Raises: # (AssertionError): If any of the assertions fail # def assert_main_error(self, error_domain, error_reason, - fail_message=''): - + fail_message='', + *, debug=False): + if debug: + print( + """ + Exit code: {} + Exception: {} + Domain: {} + Reason: {} + """.format( + self.exit_code, + self.exception, + self.exception.domain, + self.exception.reason + )) assert self.exit_code == -1, fail_message assert self.exc is not None, fail_message assert self.exception is not None, fail_message -- cgit v1.2.1