diff options
| -rw-r--r-- | src/buildstream/_variables.pyx | 77 | ||||
| -rw-r--r-- | src/buildstream/element.py | 2 | ||||
| -rw-r--r-- | src/buildstream/exceptions.py | 7 | ||||
| -rw-r--r-- | tests/format/variables.py | 14 | ||||
| -rw-r--r-- | tests/format/variables/partial_context/base.bst | 4 | ||||
| -rw-r--r-- | tests/format/variables/partial_context/base/project.conf | 3 | ||||
| -rw-r--r-- | tests/format/variables/partial_context/base/vars.yml | 2 | ||||
| -rw-r--r-- | tests/format/variables/partial_context/project.conf | 8 | ||||
| -rw-r--r-- | tests/format/variables/partial_context/test.bst | 3 |
9 files changed, 82 insertions, 38 deletions
diff --git a/src/buildstream/_variables.pyx b/src/buildstream/_variables.pyx index e3fcfc3a0..bc4b92d33 100644 --- a/src/buildstream/_variables.pyx +++ b/src/buildstream/_variables.pyx @@ -22,6 +22,7 @@ import re import sys +import itertools from ._exceptions import LoadError from .exceptions import LoadErrorReason @@ -72,11 +73,18 @@ cdef class Variables: def __init__(self, MappingNode node): self.original = node self._expstr_map = self._resolve(node) - self._check_for_missing() - self._check_for_cycles() + + def check(self): + self._check_variables() def __getitem__(self, str name): - return _expand_var(self._expstr_map, name) + if name not in self._expstr_map: + raise KeyError(name) + try: + return _expand_var(self._expstr_map, name) + except (KeyError, RecursionError): + self._check_variables(subset=[name]) + raise def __contains__(self, str name): return name in self._expstr_map @@ -105,7 +113,7 @@ cdef class Variables: cpdef str get(self, str name): if name not in self._expstr_map: return None - return _expand_var(self._expstr_map, name) + return self[name] # expand() # @@ -146,7 +154,9 @@ cdef class Variables: try: return _expand_expstr(self._expstr_map, expstr) - except KeyError: + except (KeyError, RecursionError): + # We also check for unmatch for recursion errors since _check_variables expects + # subset to be defined unmatched = [] # Look for any unmatched variable names in the expansion string @@ -161,6 +171,9 @@ cdef class Variables: ) raise LoadError(message, LoadErrorReason.UNRESOLVED_VARIABLE) + + # Otherwise the missing key is from a variable definition + self._check_variables(subset=expstr[1::2]) # Otherwise, re-raise the KeyError since it clearly came from some # other unknowable cause. raise @@ -186,39 +199,41 @@ cdef class Variables: ret[sys.intern(key)] = _parse_expstr(value) return ret - def _check_for_missing(self): - # First the check for anything unresolvable + + def _check_variables(self, *, subset=None): summary = [] - for key, expstr in self._expstr_map.items(): - for var in expstr[1::2]: - if var not in self._expstr_map: - line = " unresolved variable '{unmatched}' in declaration of '{variable}' at: {provenance}" - provenance = self.original.get_scalar(key).get_provenance() - summary.append(line.format(unmatched=var, variable=key, provenance=provenance)) - if summary: - raise LoadError("Failed to resolve one or more variable:\n{}\n".format("\n".join(summary)), - LoadErrorReason.UNRESOLVED_VARIABLE) - def _check_for_cycles(self): - # And now the cycle checks - def cycle_check(expstr, visited, cleared): + def rec_check(name, expstr, visited, cleared): for var in expstr[1::2]: if var in cleared: continue - if var in visited: - raise LoadError("{}: ".format(self.original.get_scalar(var).get_provenance()) + - ("Variable '{}' expands to contain a reference to itself. " + - "Perhaps '{}' contains '%{{{}}}").format(var, visited[-1], var), - LoadErrorReason.RECURSIVE_VARIABLE) - visited.append(var) - cycle_check(self._expstr_map[var], visited, cleared) - visited.pop() - cleared.add(var) + elif var in visited: + chain = list(itertools.takewhile(lambda x: x != var, reversed(visited))) + chain.append(var) + chain.reverse() + for i in range(len(chain)-1): + line = " Variable '{variable}' recusively uses '{rec}' at: {provenance}" + provenance = self.original.get_scalar(chain[i]).get_provenance() + summary.append(line.format(rec=chain[i+1], variable=chain[i], provenance=provenance)) + elif var not in self._expstr_map: + line = " unresolved variable '{unmatched}' in declaration of '{variable}' at: {provenance}" + provenance = self.original.get_scalar(name).get_provenance() + summary.append(line.format(unmatched=var, variable=name, provenance=provenance)) + else: + visited.append(var) + rec_check(var, self._expstr_map[var], visited, cleared) + visited.pop() + cleared.add(var) cleared = set() - for key, expstr in self._expstr_map.items(): - if key not in cleared: - cycle_check(expstr, [key], cleared) + for key in subset if subset is not None else self._expstr_map: + visited = [] + rec_check(key, self._expstr_map[key], visited, cleared) + cleared.add(key) + + if summary: + raise LoadError("Failed to resolve one or more variable:\n{}\n".format("\n".join(summary)), + LoadErrorReason.UNRESOLVED_VARIABLE) # Cache for the parsed expansion strings. While this is nominally # something which might "waste" memory, in reality each of these diff --git a/src/buildstream/element.py b/src/buildstream/element.py index 6a0fa5fab..d15263000 100644 --- a/src/buildstream/element.py +++ b/src/buildstream/element.py @@ -285,6 +285,8 @@ class Element(Plugin): variables = self.__extract_variables(project, meta) variables["element-name"] = self.name self.__variables = Variables(variables) + if not meta.is_junction: + self.__variables.check() # Collect the composited environment now that we have variables unexpanded_env = self.__extract_environment(project, meta) diff --git a/src/buildstream/exceptions.py b/src/buildstream/exceptions.py index 2c455be84..6e14d2fd5 100644 --- a/src/buildstream/exceptions.py +++ b/src/buildstream/exceptions.py @@ -131,13 +131,10 @@ class LoadErrorReason(Enum): RECURSIVE_INCLUDE = 21 """A recursive include has been encountered""" - RECURSIVE_VARIABLE = 22 - """A recursive variable has been encountered""" - - PROTECTED_VARIABLE_REDEFINED = 23 + PROTECTED_VARIABLE_REDEFINED = 22 """An attempt was made to set the value of a protected variable""" - DUPLICATE_DEPENDENCY = 24 + DUPLICATE_DEPENDENCY = 23 """A duplicate dependency was detected""" LINK_FORBIDDEN_DEPENDENCIES = 25 diff --git a/tests/format/variables.py b/tests/format/variables.py index c5e8eebad..5fad104d1 100644 --- a/tests/format/variables.py +++ b/tests/format/variables.py @@ -67,7 +67,7 @@ def test_simple_cyclic_variables(cli, datafiles): print_warning("Performing cyclic test, if this test times out it will " + "exit the test sequence") project = str(datafiles) result = cli.run(project=project, silent=True, args=["build", "simple-cyclic.bst"]) - result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_VARIABLE) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.UNRESOLVED_VARIABLE) @pytest.mark.timeout(15, method="signal") @@ -76,7 +76,7 @@ def test_cyclic_variables(cli, datafiles): print_warning("Performing cyclic test, if this test times out it will " + "exit the test sequence") project = str(datafiles) result = cli.run(project=project, silent=True, args=["build", "cyclic.bst"]) - result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_VARIABLE) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.UNRESOLVED_VARIABLE) @pytest.mark.parametrize("protected_var", PROTECTED_VARIABLES) @@ -168,3 +168,13 @@ def test_variables_resolving_errors_in_public_section(cli, datafiles): result = cli.run(project=project, args=["show", "--format", "%{public}", "public_unresolved.bst"]) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.UNRESOLVED_VARIABLE) + + +@pytest.mark.datafiles(os.path.join(DATA_DIR, "partial_context")) +def test_partial_context_junctions(cli, datafiles): + project = str(datafiles) + + result = cli.run(project=project, args=["show", "--format", "%{vars}", "test.bst"]) + result.assert_success() + result_vars = _yaml.load_data(result.output) + assert result_vars.get_str("eltvar") == "/bar/foo/baz" diff --git a/tests/format/variables/partial_context/base.bst b/tests/format/variables/partial_context/base.bst new file mode 100644 index 000000000..10ce559a9 --- /dev/null +++ b/tests/format/variables/partial_context/base.bst @@ -0,0 +1,4 @@ +kind: junction +sources: +- kind: local + path: base diff --git a/tests/format/variables/partial_context/base/project.conf b/tests/format/variables/partial_context/base/project.conf new file mode 100644 index 000000000..91511bfe9 --- /dev/null +++ b/tests/format/variables/partial_context/base/project.conf @@ -0,0 +1,3 @@ +name: base +min-version: 2.0 + diff --git a/tests/format/variables/partial_context/base/vars.yml b/tests/format/variables/partial_context/base/vars.yml new file mode 100644 index 000000000..3a91e1310 --- /dev/null +++ b/tests/format/variables/partial_context/base/vars.yml @@ -0,0 +1,2 @@ +variables: + subvar: "/bar" diff --git a/tests/format/variables/partial_context/project.conf b/tests/format/variables/partial_context/project.conf new file mode 100644 index 000000000..e8a8ed038 --- /dev/null +++ b/tests/format/variables/partial_context/project.conf @@ -0,0 +1,8 @@ +name: test +min-version: 2.0 + +(@): base.bst:vars.yml + +variables: + var: "%{subvar}/foo" + diff --git a/tests/format/variables/partial_context/test.bst b/tests/format/variables/partial_context/test.bst new file mode 100644 index 000000000..8276763c3 --- /dev/null +++ b/tests/format/variables/partial_context/test.bst @@ -0,0 +1,3 @@ +kind: manual +variables: + eltvar: "%{var}/baz" |