From c41743630c027b0cec9fb135f9cb4929c1f14af5 Mon Sep 17 00:00:00 2001 From: Valentin David Date: Tue, 2 Jun 2020 13:13:28 +0200 Subject: Do not check for all variables in junctions Junctions are loaded before all variables can loaded. So there should not be errors for unused variables that are defined using undefined variables. Also this checks for undefined variables in the same time as looking for cycles. This will allow to show more errors to the user when both type of errors happen in the same time. This also simplify error handling in `_variables.pyx`. --- src/buildstream/_variables.pyx | 77 +++++++++++++--------- src/buildstream/element.py | 2 + src/buildstream/exceptions.py | 7 +- tests/format/variables.py | 14 +++- tests/format/variables/partial_context/base.bst | 4 ++ .../variables/partial_context/base/project.conf | 3 + .../format/variables/partial_context/base/vars.yml | 2 + .../format/variables/partial_context/project.conf | 8 +++ tests/format/variables/partial_context/test.bst | 3 + 9 files changed, 82 insertions(+), 38 deletions(-) create mode 100644 tests/format/variables/partial_context/base.bst create mode 100644 tests/format/variables/partial_context/base/project.conf create mode 100644 tests/format/variables/partial_context/base/vars.yml create mode 100644 tests/format/variables/partial_context/project.conf create mode 100644 tests/format/variables/partial_context/test.bst 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" -- cgit v1.2.1