You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by no...@apache.org on 2020/12/29 12:47:54 UTC
[buildstream] 01/16: Do not check for all variables in junctions
This is an automated email from the ASF dual-hosted git repository.
not-in-ldap pushed a commit to branch tristan/variables-refactor
in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit 331ac50347551bc4211fea2d0a061b5f7f6dda33
Author: Valentin David <va...@codethink.co.uk>
AuthorDate: Tue Jun 2 13:13:28 2020 +0200
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(-)
diff --git a/src/buildstream/_variables.pyx b/src/buildstream/_variables.pyx
index e3fcfc3..bc4b92d 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 6a0fa5f..d152630 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 e77d64f..0eefec3 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 c5e8eeb..5fad104 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 0000000..10ce559
--- /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 0000000..91511bf
--- /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 0000000..3a91e13
--- /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 0000000..e8a8ed0
--- /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 0000000..8276763
--- /dev/null
+++ b/tests/format/variables/partial_context/test.bst
@@ -0,0 +1,3 @@
+kind: manual
+variables:
+ eltvar: "%{var}/baz"