[Notes] [Git][BuildStream/buildstream][Qinusty/600-recursive-variables] 7 commits: Disable round-tripping when element is not modified

[Qinusty <gitlab mg gitlab com>]

Title: GitLab

Qinusty pushed to branch Qinusty/600-recursive-variables at BuildStream / buildstream

Commits:

• 2a2a79de
  by Valentin David at 2018-08-29T14:11:48Z

  Disable round-tripping when element is not modified
  

• 2df7607c
  by Tristan Van Berkom at 2018-08-29T14:43:55Z

  Merge branch 'valentindavid/roundtripping_only_when_modified' into 'master'
  
  Disable round-tripping when element is not modified
  
  See merge request BuildStream/buildstream!748

• c146dde5
  by Benjamin Schubert at 2018-08-29T16:00:24Z

  Log not-found objects in the cache as SKIPPED
  

• 19e87afb
  by Benjamin Schubert at 2018-08-29T16:01:12Z

  Add tests that not-found objects in cache are shown as SKIPPED
  

• 9cc5817f
  by Qinusty at 2018-08-29T16:25:48Z

  Merge branch 'bschubert/log-missed-cache' into 'master'
  
  Log not-found objects in the cache as SKIPPED
  
  See merge request BuildStream/buildstream!729

• 3c8646e3
  by Josh Smith at 2018-08-29T16:34:22Z

  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.
  

• d41c42d6
  by Josh Smith at 2018-08-29T16:34:22Z

  Add tests for cyclic variables check
  
  Note: This modifies the docker containers used for testing to supply the
  pytest-timeout package.
  

11 changed files:

• .gitlab-ci.yml
• buildstream/_artifactcache/cascache.py
• buildstream/_exceptions.py
• buildstream/_variables.py
• buildstream/source.py
• dev-requirements.txt
• tests/format/variables.py
• + tests/format/variables/cyclic_variables/cyclic.bst
• + tests/format/variables/cyclic_variables/project.conf
• tests/frontend/pull.py
• tests/testutils/runcli.py

Changes:

.gitlab-ci.yml

---

...	...	@@ -84,25 +84,25 @@ source_dist:
84	84	- coverage-linux/
85	85
86	86	tests-debian-9:
87		- image: buildstream/testsuite-debian:9-master-114-4cab18e3
	87	+ image: buildstream/testsuite-debian:9-master-117-aa3a33b3
88	88	<<: *linux-tests
89	89
90	90	tests-fedora-27:
91		- image: buildstream/testsuite-fedora:27-master-114-4cab18e3
	91	+ image: buildstream/testsuite-fedora:27-master-117-aa3a33b3
92	92	<<: *linux-tests
93	93
94	94	tests-fedora-28:
95		- image: buildstream/testsuite-fedora:28-master-114-4cab18e3
	95	+ image: buildstream/testsuite-fedora:28-master-117-aa3a33b3
96	96	<<: *linux-tests
97	97
98	98	tests-ubuntu-18.04:
99		- image: buildstream/testsuite-ubuntu:18.04-master-114-4cab18e3
	99	+ image: buildstream/testsuite-ubuntu:18.04-master-117-aa3a33b3
100	100	<<: *linux-tests
101	101
102	102	tests-unix:
103	103	# Use fedora here, to a) run a test on fedora and b) ensure that we
104	104	# can get rid of ostree - this is not possible with debian-8
105		- image: buildstream/testsuite-fedora:27-master-114-4cab18e3
	105	+ image: buildstream/testsuite-fedora:27-master-117-aa3a33b3
106	106	stage: test
107	107	variables:
108	108	BST_FORCE_BACKEND: "unix"

buildstream/_artifactcache/cascache.py

---

...	...	@@ -249,6 +249,13 @@ class CASCache(ArtifactCache):
249	249	if e.code() != grpc.StatusCode.NOT_FOUND:
250	250	raise ArtifactError("Failed to pull artifact {}: {}".format(
251	251	element._get_brief_display_key(), e)) from e
	252	+ else:
	253	+ self.context.message(Message(
	254	+ None,
	255	+ MessageType.SKIPPED,
	256	+ "Remote ({}) does not have {} cached".format(
	257	+ remote.spec.url, element._get_brief_display_key())
	258	+ ))
252	259
253	260	return False
254	261

buildstream/_exceptions.py

---

...	...	@@ -217,6 +217,9 @@ class LoadErrorReason(Enum):
217	217	# A recursive include has been encountered.
218	218	RECURSIVE_INCLUDE = 21
219	219
	220	+ # A recursive variable has been encountered
	221	+ RECURSIVE_VARIABLE = 22
	222	+
220	223
221	224	# LoadError
222	225	#

buildstream/_variables.py

---

...	...	@@ -61,7 +61,7 @@ class Variables():
61	61	# LoadError, if the string contains unresolved variable references.
62	62	#
63	63	def subst(self, string):
64		- substitute, unmatched = self._subst(string, self.variables)
	64	+ substitute, unmatched, _ = self._subst(string, self.variables)
65	65	unmatched = list(set(unmatched))
66	66	if unmatched:
67	67	if len(unmatched) == 1:
...	...	@@ -82,6 +82,7 @@ class Variables():
82	82	def subst_callback(match):
83	83	nonlocal variables
84	84	nonlocal unmatched
	85	+ nonlocal matched
85	86
86	87	token = match.group(0)
87	88	varname = match.group(1)
...	...	@@ -91,6 +92,7 @@ class Variables():
91	92	# We have to check if the inner string has variables
92	93	# and return unmatches for those
93	94	unmatched += re.findall(_VARIABLE_MATCH, value)
	95	+ matched += [varname]
94	96	else:
95	97	# Return unmodified token
96	98	unmatched += [varname]
...	...	@@ -98,10 +100,11 @@ class Variables():
98	100
99	101	return value
100	102
	103	+ matched = []
101	104	unmatched = []
102	105	replacement = re.sub(_VARIABLE_MATCH, subst_callback, string)
103	106
104		- return (replacement, unmatched)
	107	+ return (replacement, unmatched, matched)
105	108
106	109	# Variable resolving code
107	110	#
...	...	@@ -131,7 +134,15 @@ class Variables():
131	134	# Ensure stringness of the value before substitution
132	135	value = _yaml.node_get(variables, str, key)
133	136
134		- resolved_var, item_unmatched = self._subst(value, variables)
	137	+ resolved_var, item_unmatched, matched = self._subst(value, variables)
	138	+
	139	+ if _wrap_variable(key) in resolved_var:
	140	+ referenced_through = find_recursive_variable(key, matched, variables)
	141	+ raise LoadError(LoadErrorReason.RECURSIVE_VARIABLE,
	142	+ "{}: ".format(_yaml.node_get_provenance(variables, key)) +
	143	+ ("Variable '{}' expands to contain a reference to itself. " +
	144	+ "Perhaps '{}' contains '{}").format(key, referenced_through, _wrap_variable(key)))
	145	+
135	146	resolved[key] = resolved_var
136	147	unmatched += item_unmatched
137	148
...	...	@@ -168,8 +179,21 @@ class Variables():
168	179	# Helper function to fetch information about the node referring to a variable
169	180	#
170	181	def _find_references(self, varname):
171		- fullname = '%{' + varname + '}'
	182	+ fullname = _wrap_variable(varname)
172	183	for key, value in _yaml.node_items(self.original):
173	184	if fullname in value:
174	185	provenance = _yaml.node_get_provenance(self.original, key)
175	186	yield (key, provenance)
	187	+
	188	+
	189	+def find_recursive_variable(variable, matched_variables, all_vars):
	190	+ matched_values = (_yaml.node_get(all_vars, str, key) for key in matched_variables)
	191	+ for key, value in zip(matched_variables, matched_values):
	192	+ if _wrap_variable(variable) in value:
	193	+ return key
	194	+ else:
	195	+ return None
	196	+
	197	+
	198	+def _wrap_variable(var):
	199	+ return "%{" + var + "}"

buildstream/source.py

---

...	...	@@ -769,7 +769,8 @@ class Source(Plugin):
769	769	#
770	770	# Step 2 - Set the ref in memory, and determine changed state
771	771	#
772		- changed = self._set_ref(new_ref, node)
	772	+ if not self._set_ref(new_ref, node):
	773	+ return False
773	774
774	775	def do_save_refs(refs):
775	776	try:
...	...	@@ -806,7 +807,7 @@ class Source(Plugin):
806	807	.format(provenance.filename.shortname),
807	808	reason="tracking-junction-fragment")
808	809
809		- return changed
	810	+ return True
810	811
811	812	# Wrapper for track()
812	813	#

dev-requirements.txt

---

...	...	@@ -8,3 +8,4 @@ pytest-env
8	8	pytest-pep8
9	9	pytest-pylint
10	10	pytest-xdist
	11	+pytest-timeout

tests/format/variables.py

---

1	1	import os
2	2	import pytest
	3	+import sys
3	4	from buildstream import _yaml
4	5	from buildstream._exceptions import ErrorDomain, LoadErrorReason
5	6	from tests.testutils.runcli import cli
...	...	@@ -72,3 +73,20 @@ def test_missing_variable(cli, datafiles, tmpdir):
72	73	])
73	74	result.assert_main_error(ErrorDomain.LOAD,
74	75	LoadErrorReason.UNRESOLVED_VARIABLE)
	76	+
	77	+
	78	+@pytest.mark.timeout(3, method="signal")
	79	+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'cyclic_variables'))
	80	+def test_cyclic_variables(cli, datafiles):
	81	+ print_warning("Performing cyclic test, if this test times out it will " +
	82	+ "exit the test sequence")
	83	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	84	+ result = cli.run(project=project, silent=True, args=[
	85	+ "build", "cyclic.bst"
	86	+ ])
	87	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_VARIABLE)
	88	+
	89	+
	90	+def print_warning(msg):
	91	+ RED, END = "\033[91m", "\033[0m"
	92	+ print(("\n{}{}{}").format(RED, msg, END), file=sys.stderr)

tests/format/variables/cyclic_variables/cyclic.bst

---

	1	+kind: manual
	2	+
	3	+variables:
	4	+ a: "%{prefix}/a"
	5	+ prefix: "%{a}/some_prefix/"
		\ No newline at end of file

tests/format/variables/cyclic_variables/project.conf

---

1

+name: test

tests/frontend/pull.py

---

...	...	@@ -338,3 +338,22 @@ def test_pull_missing_blob(cli, tmpdir, datafiles):
338	338
339	339	# Assert that no artifacts were pulled
340	340	assert len(result.get_pulled_elements()) == 0
	341	+
	342	+
	343	+@pytest.mark.datafiles(DATA_DIR)
	344	+def test_pull_missing_notifies_user(caplog, cli, tmpdir, datafiles):
	345	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	346	+ caplog.set_level(1)
	347	+
	348	+ with create_artifact_share(os.path.join(str(tmpdir), 'artifactshare')) as share:
	349	+
	350	+ cli.configure({
	351	+ 'artifacts': {'url': share.repo}
	352	+ })
	353	+ result = cli.run(project=project, args=['build', 'target.bst'])
	354	+
	355	+ result.assert_success()
	356	+ assert not result.get_pulled_elements(), \
	357	+ "No elements should have been pulled since the cache was empty"
	358	+
	359	+ assert "SKIPPED Remote ({}) does not have".format(share.repo) in result.stderr

tests/testutils/runcli.py

---

...	...	@@ -94,14 +94,28 @@ class Result():
94	94	# error_reason (any): The reason field of the error which occurred
95	95	# fail_message (str): An optional message to override the automatic
96	96	# assertion error messages
	97	+ # debug (bool): If true, prints information regarding the exit state of the result()
97	98	# Raises:
98	99	# (AssertionError): If any of the assertions fail
99	100	#
100	101	def assert_main_error(self,
101	102	error_domain,
102	103	error_reason,
103		- fail_message=''):
104		-
	104	+ fail_message='',
	105	+ *, debug=False):
	106	+ if debug:
	107	+ print(
	108	+ """
	109	+ Exit code: {}
	110	+ Exception: {}
	111	+ Domain: {}
	112	+ Reason: {}
	113	+ """.format(
	114	+ self.exit_code,
	115	+ self.exception,
	116	+ self.exception.domain,
	117	+ self.exception.reason
	118	+ ))
105	119	assert self.exit_code == -1, fail_message
106	120	assert self.exc is not None, fail_message
107	121	assert self.exception is not None, fail_message

—
View it on GitLab.
You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can adjust your notification settings.