Merge branch 'Qinusty/600-recursive-variables' into 'master'

Add cyclic check within variable resolution

See merge request BuildStream/buildstream!712

8 files changed, 77 insertions, 11 deletions

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/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 + "}"
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