[Notes] [Git][BuildStream/buildstream][chandan/element-filename-requirements] 7 commits: tests/loader/junctions.py: Use Result checking APIs instead of manually

[Tristan Van Berkom <gitlab mg gitlab com>]

Title: GitLab

Tristan Van Berkom pushed to branch chandan/element-filename-requirements at BuildStream / buildstream

Commits:

• a080b6c7
  by Tristan Van Berkom at 2018-12-31T19:03:25Z

  tests/loader/junctions.py: Use Result checking APIs instead of manually
  
  Better to use the same API everywhere for thecking CLI results.
  

• d7c8b54d
  by Tristan Van Berkom at 2018-12-31T19:14:16Z

  tests/loader/junctions.py: Test the error we expect when a cross junction element is not found
  

• 560a6342
  by Tristan Van Berkom at 2018-12-31T19:49:24Z

  Merge branch 'tristan/junction-tests' into 'master'
  
  Junction test addition
  
  See merge request BuildStream/buildstream!1029

• 8a020a5a
  by Chandan Singh at 2018-12-31T19:54:25Z

  _loader/loader.py: Refactor warnings about element names in one method
  
  Currently we some duplication in the way we check for invalid filenames.
  To make it more robust and allow room for adding more warnings, refactor
  it into a separate method `_warn_invalid_elements()` that handles just
  this.
  

• 87568074
  by Chandan Singh at 2018-12-31T19:54:25Z

  _loader/loader.py: Add warnings about invalid characters in filename
  
  See https://mail.gnome.org/archives/buildstream-list/2018-December/msg00061.html
  for some related discussion.
  

• 4b2992c1
  by Chandan Singh at 2018-12-31T19:54:25Z

  tests/buildcheckout.py: Add tests for invalid chars in filename
  

• ce9e9c2d
  by Chandan Singh at 2018-12-31T19:54:25Z

  doc: Add note about valid element names
  

9 changed files:

• buildstream/_loader/loader.py
• buildstream/types.py
• doc/source/format_declaring.rst
• tests/frontend/buildcheckout.py
• + tests/frontend/project/elements/invalid-chars-in-dep.bst
• + tests/frontend/project/elements/invalid-chars|<>-in-name.bst
• tests/frontend/project/project.conf
• tests/loader/junctions.py
• + tests/loader/junctions/invalid/missing-element.bst

Changes:

buildstream/_loader/loader.py

---

...	...	@@ -99,7 +99,6 @@ class Loader():
99	99	# Returns: The toplevel LoadElement
100	100	def load(self, targets, rewritable=False, ticker=None, fetch_subprojects=False):
101	101
102		- invalid_elements = []
103	102	for filename in targets:
104	103	if os.path.isabs(filename):
105	104	# XXX Should this just be an assertion ?
...	...	@@ -109,14 +108,8 @@ class Loader():
109	108	"path to the base project directory: {}"
110	109	.format(filename, self._basedir))
111	110
112		- if not filename.endswith(".bst"):
113		- invalid_elements.append(filename)
	111	+ self._warn_invalid_elements(targets)
114	112
115		- if invalid_elements:
116		- self._warn("Target elements '{}' do not have expected file extension `.bst` "
117		- "Improperly named elements will not be discoverable by commands"
118		- .format(invalid_elements),
119		- warning_token=CoreWarnings.BAD_ELEMENT_SUFFIX)
120	113	# First pass, recursively load files and populate our table of LoadElements
121	114	#
122	115	deps = []
...	...	@@ -280,12 +273,7 @@ class Loader():
280	273	self._elements[filename] = element
281	274
282	275	# Load all dependency files for the new LoadElement
283		- invalid_elements = []
284	276	for dep in element.deps:
285		- if not dep.name.endswith(".bst"):
286		- invalid_elements.append(dep.name)
287		- continue
288		-
289	277	if dep.junction:
290	278	self._load_file(dep.junction, rewritable, ticker, fetch_subprojects, yaml_cache)
291	279	loader = self._get_loader(dep.junction, rewritable=rewritable, ticker=ticker,
...	...	@@ -300,11 +288,9 @@ class Loader():
300	288	"{}: Cannot depend on junction"
301	289	.format(dep.provenance))
302	290
303		- if invalid_elements:
304		- self._warn("The following dependencies do not have expected file extension `.bst`: {} "
305		- "Improperly named elements will not be discoverable by commands"
306		- .format(invalid_elements),
307		- warning_token=CoreWarnings.BAD_ELEMENT_SUFFIX)
	291	+ deps_names = [dep.name for dep in element.deps]
	292	+ self._warn_invalid_elements(deps_names)
	293	+
308	294	return element
309	295
310	296	# _check_circular_deps():
...	...	@@ -679,3 +665,69 @@ class Loader():
679	665
680	666	message = Message(None, MessageType.WARN, brief)
681	667	self._context.message(message)
	668	+
	669	+ # Print warning messages if any of the specified elements have invalid names.
	670	+ #
	671	+ # Valid filenames should end with ".bst" extension.
	672	+ #
	673	+ # Args:
	674	+ # elements (list): List of element names
	675	+ #
	676	+ # Raises:
	677	+ # (:class:`.LoadError`): When warning_token is considered fatal by the project configuration
	678	+ #
	679	+ def _warn_invalid_elements(self, elements):
	680	+
	681	+ # invalid_elements
	682	+ #
	683	+ # A dict that maps warning types to the matching elements.
	684	+ invalid_elements = {
	685	+ CoreWarnings.BAD_ELEMENT_SUFFIX: [],
	686	+ CoreWarnings.BAD_CHARACTERS_IN_NAME: [],
	687	+ }
	688	+
	689	+ for filename in elements:
	690	+ if not filename.endswith(".bst"):
	691	+ invalid_elements[CoreWarnings.BAD_ELEMENT_SUFFIX].append(filename)
	692	+ if not self._valid_chars_name(filename):
	693	+ invalid_elements[CoreWarnings.BAD_CHARACTERS_IN_NAME].append(filename)
	694	+
	695	+ if invalid_elements[CoreWarnings.BAD_ELEMENT_SUFFIX]:
	696	+ self._warn("Target elements '{}' do not have expected file extension `.bst` "
	697	+ "Improperly named elements will not be discoverable by commands"
	698	+ .format(invalid_elements[CoreWarnings.BAD_ELEMENT_SUFFIX]),
	699	+ warning_token=CoreWarnings.BAD_ELEMENT_SUFFIX)
	700	+ if invalid_elements[CoreWarnings.BAD_CHARACTERS_IN_NAME]:
	701	+ self._warn("Target elements '{}' have invalid characerts in their name."
	702	+ .format(invalid_elements[CoreWarnings.BAD_CHARACTERS_IN_NAME]),
	703	+ warning_token=CoreWarnings.BAD_CHARACTERS_IN_NAME)
	704	+
	705	+ # Check if given filename containers valid characters.
	706	+ #
	707	+ # Args:
	708	+ # name (str): Name of the file
	709	+ #
	710	+ # Returns:
	711	+ # (bool): True if all characters are valid, False otherwise.
	712	+ #
	713	+ def _valid_chars_name(self, name):
	714	+ for char in name:
	715	+ char_val = ord(char)
	716	+
	717	+ # 0-31 are control chars, 127 is DEL, and >127 means non-ASCII
	718	+ if char_val <= 31 or char_val >= 127:
	719	+ return False
	720	+
	721	+ # Disallow characters that are invalid on Windows. The list can be
	722	+ # found at https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file
	723	+ #
	724	+ # Note that although : (colon) is not allowed, we do not raise
	725	+ # warnings because of that, since we use it as a separator for
	726	+ # junctioned elements.
	727	+ #
	728	+ # We also do not raise warnings on slashes since they are used as
	729	+ # path separators.
	730	+ if char in r'<>"|?*':
	731	+ return False
	732	+
	733	+ return True

buildstream/types.py

---

...	...	@@ -105,6 +105,12 @@ class CoreWarnings():
105	105	is referenced either on the command line or by another element
106	106	"""
107	107
	108	+ BAD_CHARACTERS_IN_NAME = "bad-characters-in-name"
	109	+ """
	110	+ This warning will be produces when filename for a target contains invalid
	111	+ characters in its name.
	112	+ """
	113	+
108	114
109	115	# _KeyStrength():
110	116	#

doc/source/format_declaring.rst

---

...	...	@@ -526,3 +526,27 @@ read-only variables are also dynamically declared by BuildStream:
526	526	build, support for this is conditional on the element type
527	527	and the build system used (any element using 'make' can
528	528	implement this).
	529	+
	530	+
	531	+Naming elements
	532	+---------------
	533	+When naming the element files, use the following rules:
	534	+
	535	+* The name of the file must have ``.bst`` extension.
	536	+
	537	+* All characters in the name must be printable 7-bit ASCII characters.
	538	+
	539	+* Following characters are reserved and must not be part of the name:
	540	+
	541	+ - ``<`` (less than)
	542	+ - ``>`` (greater than)
	543	+ - ``:`` (colon)
	544	+ - ``"`` (double quote)
	545	+ - ``/`` (forward slash)
	546	+ - ``\`` (backslash)
	547	+ - ``|`` (vertical bar)
	548	+ - ``?`` (question mark)
	549	+ - ``*`` (asterisk)
	550	+
	551	+BuildStream will attempt to raise warnings when any of these rules are violated
	552	+but that may not always be possible.

tests/frontend/buildcheckout.py

---

...	...	@@ -85,6 +85,20 @@ def test_build_invalid_suffix_dep(datafiles, cli, strict, hardlinks):
85	85	result.assert_main_error(ErrorDomain.LOAD, "bad-element-suffix")
86	86
87	87
	88	+@pytest.mark.datafiles(DATA_DIR)
	89	+def test_build_invalid_filename_chars(datafiles, cli):
	90	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	91	+ result = cli.run(project=project, args=strict_args(['build', 'invalid-chars|<>-in-name.bst'], 'non-strict'))
	92	+ result.assert_main_error(ErrorDomain.LOAD, "bad-characters-in-name")
	93	+
	94	+
	95	+@pytest.mark.datafiles(DATA_DIR)
	96	+def test_build_invalid_filename_chars_dep(datafiles, cli):
	97	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	98	+ result = cli.run(project=project, args=strict_args(['build', 'invalid-chars-in-dep.bst'], 'non-strict'))
	99	+ result.assert_main_error(ErrorDomain.LOAD, "bad-characters-in-name")
	100	+
	101	+
88	102	@pytest.mark.datafiles(DATA_DIR)
89	103	@pytest.mark.parametrize("deps", [("run"), ("none"), ("build")])
90	104	def test_build_checkout_deps(datafiles, cli, deps):

tests/frontend/project/elements/invalid-chars-in-dep.bst

---

	1	+kind: stack
	2	+description: |
	3	+
	4	+ This element itself has a valid name, but depends on elements that have
	5	+ invalid names. This should also result in a warning.
	6	+
	7	+depends:
	8	+- invalid-chars|<>-in-name.bst

tests/frontend/project/elements/invalid-chars|<>-in-name.bst

---

	1	+kind: stack
	2	+description: |
	3	+ The name of this files contains characters that are not allowed by
	4	+ BuildStream, using it should raise a warning.

tests/frontend/project/project.conf

---

...	...	@@ -5,3 +5,4 @@ element-path: elements
5	5
6	6	fatal-warnings:
7	7	- bad-element-suffix
	8	+- bad-characters-in-name

tests/loader/junctions.py

---

...	...	@@ -3,7 +3,7 @@ import pytest
3	3	import shutil
4	4
5	5	from buildstream import _yaml, ElementError
6		-from buildstream._exceptions import LoadError, LoadErrorReason
	6	+from buildstream._exceptions import ErrorDomain, LoadErrorReason
7	7	from tests.testutils import cli, create_repo
8	8	from tests.testutils.site import HAVE_GIT
9	9
...	...	@@ -38,9 +38,9 @@ def test_simple_build(cli, tmpdir, datafiles):
38	38
39	39	# Build, checkout
40	40	result = cli.run(project=project, args=['build', 'target.bst'])
41		- assert result.exit_code == 0
	41	+ result.assert_success()
42	42	result = cli.run(project=project, args=['checkout', 'target.bst', checkoutdir])
43		- assert result.exit_code == 0
	43	+ result.assert_success()
44	44
45	45	# Check that the checkout contains the expected files from both projects
46	46	assert(os.path.exists(os.path.join(checkoutdir, 'base.txt')))
...	...	@@ -54,7 +54,7 @@ def test_build_of_same_junction_used_twice(cli, tmpdir, datafiles):
54	54	# Check we can build a project that contains the same junction
55	55	# that is used twice, but named differently
56	56	result = cli.run(project=project, args=['build', 'target.bst'])
57		- assert result.exit_code == 0
	57	+ result.assert_success()
58	58
59	59
60	60	@pytest.mark.datafiles(DATA_DIR)
...	...	@@ -69,9 +69,9 @@ def test_nested_simple(cli, tmpdir, datafiles):
69	69
70	70	# Build, checkout
71	71	result = cli.run(project=project, args=['build', 'target.bst'])
72		- assert result.exit_code == 0
	72	+ result.assert_success()
73	73	result = cli.run(project=project, args=['checkout', 'target.bst', checkoutdir])
74		- assert result.exit_code == 0
	74	+ result.assert_success()
75	75
76	76	# Check that the checkout contains the expected files from all subprojects
77	77	assert(os.path.exists(os.path.join(checkoutdir, 'base.txt')))
...	...	@@ -93,9 +93,9 @@ def test_nested_double(cli, tmpdir, datafiles):
93	93
94	94	# Build, checkout
95	95	result = cli.run(project=project, args=['build', 'target.bst'])
96		- assert result.exit_code == 0
	96	+ result.assert_success()
97	97	result = cli.run(project=project, args=['checkout', 'target.bst', checkoutdir])
98		- assert result.exit_code == 0
	98	+ result.assert_success()
99	99
100	100	# Check that the checkout contains the expected files from all subprojects
101	101	assert(os.path.exists(os.path.join(checkoutdir, 'base.txt')))
...	...	@@ -115,45 +115,46 @@ def test_nested_conflict(cli, datafiles):
115	115	copy_subprojects(project, datafiles, ['foo', 'bar'])
116	116
117	117	result = cli.run(project=project, args=['build', 'target.bst'])
118		- assert result.exit_code != 0
119		- assert result.exception
120		- assert isinstance(result.exception, LoadError)
121		- assert result.exception.reason == LoadErrorReason.CONFLICTING_JUNCTION
	118	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.CONFLICTING_JUNCTION)
122	119
123	120
	121	+# Test that we error correctly when the junction element itself is missing
124	122	@pytest.mark.datafiles(DATA_DIR)
125		-def test_invalid_missing(cli, datafiles):
	123	+def test_missing_junction(cli, datafiles):
126	124	project = os.path.join(str(datafiles), 'invalid')
127	125
128	126	result = cli.run(project=project, args=['build', 'missing.bst'])
129		- assert result.exit_code != 0
130		- assert result.exception
131		- assert isinstance(result.exception, LoadError)
132		- assert result.exception.reason == LoadErrorReason.MISSING_FILE
	127	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
133	128
134	129
	130	+# Test that we error correctly when an element is not found in the subproject
	131	+@pytest.mark.datafiles(DATA_DIR)
	132	+def test_missing_subproject_element(cli, datafiles):
	133	+ project = os.path.join(str(datafiles), 'invalid')
	134	+ copy_subprojects(project, datafiles, ['base'])
	135	+
	136	+ result = cli.run(project=project, args=['build', 'missing-element.bst'])
	137	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
	138	+
	139	+
	140	+# Test that we error correctly when a junction itself has dependencies
135	141	@pytest.mark.datafiles(DATA_DIR)
136	142	def test_invalid_with_deps(cli, datafiles):
137	143	project = os.path.join(str(datafiles), 'invalid')
138	144	copy_subprojects(project, datafiles, ['base'])
139	145
140	146	result = cli.run(project=project, args=['build', 'junction-with-deps.bst'])
141		- assert result.exit_code != 0
142		- assert result.exception
143		- assert isinstance(result.exception, ElementError)
144		- assert result.exception.reason == 'element-forbidden-depends'
	147	+ result.assert_main_error(ErrorDomain.ELEMENT, 'element-forbidden-depends')
145	148
146	149
	150	+# Test that we error correctly when a junction is directly depended on
147	151	@pytest.mark.datafiles(DATA_DIR)
148	152	def test_invalid_junction_dep(cli, datafiles):
149	153	project = os.path.join(str(datafiles), 'invalid')
150	154	copy_subprojects(project, datafiles, ['base'])
151	155
152	156	result = cli.run(project=project, args=['build', 'junction-dep.bst'])
153		- assert result.exit_code != 0
154		- assert result.exception
155		- assert isinstance(result.exception, LoadError)
156		- assert result.exception.reason == LoadErrorReason.INVALID_DATA
	157	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)
157	158
158	159
159	160	@pytest.mark.datafiles(DATA_DIR)
...	...	@@ -165,9 +166,9 @@ def test_options_default(cli, tmpdir, datafiles):
165	166
166	167	# Build, checkout
167	168	result = cli.run(project=project, args=['build', 'target.bst'])
168		- assert result.exit_code == 0
	169	+ result.assert_success()
169	170	result = cli.run(project=project, args=['checkout', 'target.bst', checkoutdir])
170		- assert result.exit_code == 0
	171	+ result.assert_success()
171	172
172	173	assert(os.path.exists(os.path.join(checkoutdir, 'pony.txt')))
173	174	assert(not os.path.exists(os.path.join(checkoutdir, 'horsy.txt')))
...	...	@@ -182,9 +183,9 @@ def test_options(cli, tmpdir, datafiles):
182	183
183	184	# Build, checkout
184	185	result = cli.run(project=project, args=['build', 'target.bst'])
185		- assert result.exit_code == 0
	186	+ result.assert_success()
186	187	result = cli.run(project=project, args=['checkout', 'target.bst', checkoutdir])
187		- assert result.exit_code == 0
	188	+ result.assert_success()
188	189
189	190	assert(not os.path.exists(os.path.join(checkoutdir, 'pony.txt')))
190	191	assert(os.path.exists(os.path.join(checkoutdir, 'horsy.txt')))
...	...	@@ -199,9 +200,9 @@ def test_options_inherit(cli, tmpdir, datafiles):
199	200
200	201	# Build, checkout
201	202	result = cli.run(project=project, args=['build', 'target.bst'])
202		- assert result.exit_code == 0
	203	+ result.assert_success()
203	204	result = cli.run(project=project, args=['checkout', 'target.bst', checkoutdir])
204		- assert result.exit_code == 0
	205	+ result.assert_success()
205	206
206	207	assert(not os.path.exists(os.path.join(checkoutdir, 'pony.txt')))
207	208	assert(os.path.exists(os.path.join(checkoutdir, 'horsy.txt')))
...	...	@@ -228,14 +229,11 @@ def test_git_show(cli, tmpdir, datafiles):
228	229
229	230	# Verify that bst show does not implicitly fetch subproject
230	231	result = cli.run(project=project, args=['show', 'target.bst'])
231		- assert result.exit_code != 0
232		- assert result.exception
233		- assert isinstance(result.exception, LoadError)
234		- assert result.exception.reason == LoadErrorReason.SUBPROJECT_FETCH_NEEDED
	232	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_FETCH_NEEDED)
235	233
236	234	# Explicitly fetch subproject
237	235	result = cli.run(project=project, args=['source', 'fetch', 'base.bst'])
238		- assert result.exit_code == 0
	236	+ result.assert_success()
239	237
240	238	# Check that bst show succeeds now and the pipeline includes the subproject element
241	239	element_list = cli.get_pipeline(project, ['target.bst'])
...	...	@@ -263,9 +261,9 @@ def test_git_build(cli, tmpdir, datafiles):
263	261
264	262	# Build (with implicit fetch of subproject), checkout
265	263	result = cli.run(project=project, args=['build', 'target.bst'])
266		- assert result.exit_code == 0
	264	+ result.assert_success()
267	265	result = cli.run(project=project, args=['checkout', 'target.bst', checkoutdir])
268		- assert result.exit_code == 0
	266	+ result.assert_success()
269	267
270	268	# Check that the checkout contains the expected files from both projects
271	269	assert(os.path.exists(os.path.join(checkoutdir, 'base.txt')))
...	...	@@ -304,9 +302,9 @@ def test_build_git_cross_junction_names(cli, tmpdir, datafiles):
304	302
305	303	# Build (with implicit fetch of subproject), checkout
306	304	result = cli.run(project=project, args=['build', 'base.bst:target.bst'])
307		- assert result.exit_code == 0
	305	+ result.assert_success()
308	306	result = cli.run(project=project, args=['checkout', 'base.bst:target.bst', checkoutdir])
309		- assert result.exit_code == 0
	307	+ result.assert_success()
310	308
311	309	# Check that the checkout contains the expected files from both projects
312	310	assert(os.path.exists(os.path.join(checkoutdir, 'base.txt')))

tests/loader/junctions/invalid/missing-element.bst

---

	1	+# This refers to the `foo.bst` element through
	2	+# the `base.bst` junction. The `base.bst` junction
	3	+# exists but the `foo.bst` element does not exist
	4	+# in the subproject.
	5	+#
	6	+kind: stack
	7	+depends:
	8	+- junction: base.bst
	9	+ filename: foo.bst

—
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.