[Notes] [Git][BuildStream/buildstream][tlater/message-lines] 9 commits: .gitlab-ci.yml: Add overnight tests logs to artifacts.

[Tristan Maat <gitlab mg gitlab com>]

Title: GitLab

Tristan Maat pushed to branch tlater/message-lines at BuildStream / buildstream

Commits:

• 56ec33cc
  by Valentin David at 2019-01-18T11:14:41Z

  .gitlab-ci.yml: Add overnight tests logs to artifacts.
  

• ad2df651
  by Javier Jardón at 2019-01-18T12:01:56Z

  Merge branch 'valentindavid/overnight-tests-logs-artifacts' into 'master'
  
  .gitlab-ci.yml: Add overnight tests logs to artifacts.
  
  See merge request BuildStream/buildstream!1084

• f874295f
  by Tristan Van Berkom at 2019-01-18T15:59:28Z

  bzr source plugin: Use lock files to avoid corrupting the source cache
  
  This patch by itself fixes #868
  

• 42a2fe3c
  by Tristan Van Berkom at 2019-01-18T15:59:28Z

  bzr source plugins: Remove all of the atomic backup directory business logic
  
  Follow up of last commit which uses exclusive locking to
  protect bzr operations instead.
  

• a8713ed2
  by Tristan Van Berkom at 2019-01-18T15:59:28Z

  testutils/runcli.py: Added cli.get_element_states()
  
  With get_element_state(), you need to invoke BuildStream once
  for every element state you want to observe in a pipeline.
  
  The new get_element_states() reports a dictionary with
  the element state hashed by element name and is better to use
  if you have more than one element to observe the state of.
  

• a895cb2a
  by Tristan Van Berkom at 2019-01-18T15:59:28Z

  tests/frontend/track.py: Use test_track_recurse() to stress test Sources
  
  This causes multiple source instances to interact with the same
  backing data store at the same time, increasing the likelyhood
  of triggering issues around concurrent access.
  
  This more reliably triggers issue #868
  

• 4236bcc7
  by Tristan Van Berkom at 2019-01-18T16:35:23Z

  Merge branch 'tristan/fix-bzr-race' into 'master'
  
  Fix bzr race conditions
  
  Closes #868
  
  See merge request BuildStream/buildstream!1083

• 08ca5667
  by Tristan Maat at 2019-01-18T17:08:00Z

  testutils/runcli.py: Allow removing artifacts from arbitrary dirs
  
  `remove_artifact_from_cache` used a hard-coded path to remove
  artifacts, which wasn't sufficient for integration tests.
  

• 49f9eb3c
  by Tristan Maat at 2019-01-18T17:08:00Z

  widget.py: Avoid "showing 0 lines" messages when there are no lines
  
  This happened when bst is invoked with --message-lines 0 or
  --error-lines 0, and was arguably a little too verbose (the user
  explicitly asked us not to show them any lines, after all).
  
  Fixes #779
  

6 changed files:

• .gitlab-ci.yml
• buildstream/_frontend/widget.py
• buildstream/plugins/sources/bzr.py
• tests/frontend/track.py
• + tests/integration/messages.py
• tests/testutils/runcli.py

Changes:

.gitlab-ci.yml

---

...	...	@@ -185,6 +185,9 @@ docs:
185	185	- pip3 install --user -e ${BST_EXT_URL}@${BST_EXT_REF}#egg=bst_ext
186	186	- git clone https://gitlab.com/freedesktop-sdk/freedesktop-sdk.git
187	187	- git -C freedesktop-sdk checkout ${FD_SDK_REF}
	188	+ artifacts:
	189	+ paths:
	190	+ - "${HOME}/.cache/buildstream/logs"
188	191	only:
189	192	- schedules
190	193

buildstream/_frontend/widget.py

---

...	...	@@ -647,8 +647,9 @@ class LogLine(Widget):
647	647	abbrev = False
648	648	if message.message_type not in ERROR_MESSAGES \
649	649	and not frontend_message and n_lines > self._message_lines:
650		- abbrev = True
651	650	lines = lines[0:self._message_lines]
	651	+ if self._message_lines > 0:
	652	+ abbrev = True
652	653	else:
653	654	lines[n_lines - 1] = lines[n_lines - 1].rstrip('\n')
654	655
...	...	@@ -674,7 +675,7 @@ class LogLine(Widget):
674	675	if self.context is not None and not self.context.log_verbose:
675	676	text += self._indent + self._err_profile.fmt("Log file: ")
676	677	text += self._indent + self._logfile_widget.render(message) + '\n'
677		- else:
	678	+ elif self._log_lines > 0:
678	679	text += self._indent + self._err_profile.fmt("Printing the last {} lines from log file:"
679	680	.format(self._log_lines)) + '\n'
680	681	text += self._indent + self._logfile_widget.render(message, abbrev=False) + '\n'

buildstream/plugins/sources/bzr.py

---

...	...	@@ -56,6 +56,7 @@ details on common configuration options for sources.
56	56
57	57	import os
58	58	import shutil
	59	+import fcntl
59	60	from contextlib import contextmanager
60	61
61	62	from buildstream import Source, SourceError, Consistency
...	...	@@ -84,10 +85,12 @@ class BzrSource(Source):
84	85	if self.ref is None or self.tracking is None:
85	86	return Consistency.INCONSISTENT
86	87
87		- if self._check_ref():
88		- return Consistency.CACHED
89		- else:
90		- return Consistency.RESOLVED
	88	+ # Lock for the _check_ref()
	89	+ with self._locked():
	90	+ if self._check_ref():
	91	+ return Consistency.CACHED
	92	+ else:
	93	+ return Consistency.RESOLVED
91	94
92	95	def load_ref(self, node):
93	96	self.ref = self.node_get_member(node, str, 'ref', None)
...	...	@@ -100,7 +103,7 @@ class BzrSource(Source):
100	103
101	104	def track(self):
102	105	with self.timed_activity("Tracking {}".format(self.url),
103		- silent_nested=True):
	106	+ silent_nested=True), self._locked():
104	107	self._ensure_mirror(skip_ref_check=True)
105	108	ret, out = self.check_output([self.host_bzr, "version-info",
106	109	"--custom", "--template={revno}",
...	...	@@ -114,7 +117,7 @@ class BzrSource(Source):
114	117
115	118	def fetch(self):
116	119	with self.timed_activity("Fetching {}".format(self.url),
117		- silent_nested=True):
	120	+ silent_nested=True), self._locked():
118	121	self._ensure_mirror()
119	122
120	123	def stage(self, directory):
...	...	@@ -141,6 +144,26 @@ class BzrSource(Source):
141	144	"--directory={}".format(directory), url],
142	145	fail="Failed to switch workspace's parent branch to {}".format(url))
143	146
	147	+ # _locked()
	148	+ #
	149	+ # This context manager ensures exclusive access to the
	150	+ # bzr repository.
	151	+ #
	152	+ @contextmanager
	153	+ def _locked(self):
	154	+ lockdir = os.path.join(self.get_mirror_directory(), 'locks')
	155	+ lockfile = os.path.join(
	156	+ lockdir,
	157	+ utils.url_directory_name(self.original_url) + '.lock'
	158	+ )
	159	+ os.makedirs(lockdir, exist_ok=True)
	160	+ with open(lockfile, 'w') as lock:
	161	+ fcntl.flock(lock, fcntl.LOCK_EX)
	162	+ try:
	163	+ yield
	164	+ finally:
	165	+ fcntl.flock(lock, fcntl.LOCK_UN)
	166	+
144	167	def _check_ref(self):
145	168	# If the mirror doesnt exist yet, then we dont have the ref
146	169	if not os.path.exists(self._get_branch_dir()):
...	...	@@ -157,83 +180,27 @@ class BzrSource(Source):
157	180	return os.path.join(self.get_mirror_directory(),
158	181	utils.url_directory_name(self.original_url))
159	182
160		- def _atomic_replace_mirrordir(self, srcdir):
161		- """Helper function to safely replace the mirror dir"""
	183	+ def _ensure_mirror(self, skip_ref_check=False):
	184	+ mirror_dir = self._get_mirror_dir()
	185	+ bzr_metadata_dir = os.path.join(mirror_dir, ".bzr")
	186	+ if not os.path.exists(bzr_metadata_dir):
	187	+ self.call([self.host_bzr, "init-repo", "--no-trees", mirror_dir],
	188	+ fail="Failed to initialize bzr repository")
	189	+
	190	+ branch_dir = os.path.join(mirror_dir, self.tracking)
	191	+ branch_url = self.url + "/" + self.tracking
	192	+ if not os.path.exists(branch_dir):
	193	+ # `bzr branch` the branch if it doesn't exist
	194	+ # to get the upstream code
	195	+ self.call([self.host_bzr, "branch", branch_url, branch_dir],
	196	+ fail="Failed to branch from {} to {}".format(branch_url, branch_dir))
162	197
163		- if not os.path.exists(self._get_mirror_dir()):
164		- # Just move the srcdir to the mirror dir
165		- try:
166		- os.rename(srcdir, self._get_mirror_dir())
167		- except OSError as e:
168		- raise SourceError("{}: Failed to move srcdir '{}' to mirror dir '{}'"
169		- .format(str(self), srcdir, self._get_mirror_dir())) from e
170	198	else:
171		- # Atomically swap the backup dir.
172		- backupdir = self._get_mirror_dir() + ".bak"
173		- try:
174		- os.rename(self._get_mirror_dir(), backupdir)
175		- except OSError as e:
176		- raise SourceError("{}: Failed to move mirrordir '{}' to backup dir '{}'"
177		- .format(str(self), self._get_mirror_dir(), backupdir)) from e
	199	+ # `bzr pull` the branch if it does exist
	200	+ # to get any changes to the upstream code
	201	+ self.call([self.host_bzr, "pull", "--directory={}".format(branch_dir), branch_url],
	202	+ fail="Failed to pull new changes for {}".format(branch_dir))
178	203
179		- try:
180		- os.rename(srcdir, self._get_mirror_dir())
181		- except OSError as e:
182		- # Attempt to put the backup back!
183		- os.rename(backupdir, self._get_mirror_dir())
184		- raise SourceError("{}: Failed to replace bzr repo '{}' with '{}"
185		- .format(str(self), srcdir, self._get_mirror_dir())) from e
186		- finally:
187		- if os.path.exists(backupdir):
188		- shutil.rmtree(backupdir)
189		-
190		- @contextmanager
191		- def _atomic_repodir(self):
192		- """Context manager for working in a copy of the bzr repository
193		-
194		- Yields:
195		- (str): A path to the copy of the bzr repo
196		-
197		- This should be used because bzr does not give any guarantees of
198		- atomicity, and aborting an operation at the wrong time (or
199		- accidentally running multiple concurrent operations) can leave the
200		- repo in an inconsistent state.
201		- """
202		- with self.tempdir() as repodir:
203		- mirror_dir = self._get_mirror_dir()
204		- if os.path.exists(mirror_dir):
205		- try:
206		- # shutil.copytree doesn't like it if destination exists
207		- shutil.rmtree(repodir)
208		- shutil.copytree(mirror_dir, repodir)
209		- except (shutil.Error, OSError) as e:
210		- raise SourceError("{}: Failed to copy bzr repo from '{}' to '{}'"
211		- .format(str(self), mirror_dir, repodir)) from e
212		-
213		- yield repodir
214		- self._atomic_replace_mirrordir(repodir)
215		-
216		- def _ensure_mirror(self, skip_ref_check=False):
217		- with self._atomic_repodir() as repodir:
218		- # Initialize repo if no metadata
219		- bzr_metadata_dir = os.path.join(repodir, ".bzr")
220		- if not os.path.exists(bzr_metadata_dir):
221		- self.call([self.host_bzr, "init-repo", "--no-trees", repodir],
222		- fail="Failed to initialize bzr repository")
223		-
224		- branch_dir = os.path.join(repodir, self.tracking)
225		- branch_url = self.url + "/" + self.tracking
226		- if not os.path.exists(branch_dir):
227		- # `bzr branch` the branch if it doesn't exist
228		- # to get the upstream code
229		- self.call([self.host_bzr, "branch", branch_url, branch_dir],
230		- fail="Failed to branch from {} to {}".format(branch_url, branch_dir))
231		-
232		- else:
233		- # `bzr pull` the branch if it does exist
234		- # to get any changes to the upstream code
235		- self.call([self.host_bzr, "pull", "--directory={}".format(branch_dir), branch_url],
236		- fail="Failed to pull new changes for {}".format(branch_dir))
237	204	if not skip_ref_check and not self._check_ref():
238	205	raise SourceError("Failed to ensure ref '{}' was mirrored".format(self.ref),
239	206	reason="ref-not-mirrored")

tests/frontend/track.py

---

...	...	@@ -73,14 +73,36 @@ def test_track(cli, tmpdir, datafiles, ref_storage, kind):
73	73	assert not os.path.exists(os.path.join(project, 'project.refs'))
74	74
75	75
	76	+# NOTE:
	77	+#
	78	+# This test checks that recursive tracking works by observing
	79	+# element states after running a recursive tracking operation.
	80	+#
	81	+# However, this test is ALSO valuable as it stresses the source
	82	+# plugins in a situation where many source plugins are operating
	83	+# at once on the same backing repository.
	84	+#
	85	+# Do not change this test to use a separate 'Repo' per element
	86	+# as that would defeat the purpose of the stress test, otherwise
	87	+# please refactor that aspect into another test.
	88	+#
76	89	@pytest.mark.datafiles(DATA_DIR)
	90	+@pytest.mark.parametrize("amount", [(1), (10)])
77	91	@pytest.mark.parametrize("kind", [(kind) for kind in ALL_REPO_KINDS])
78		-def test_track_recurse(cli, tmpdir, datafiles, kind):
	92	+def test_track_recurse(cli, tmpdir, datafiles, kind, amount):
79	93	project = os.path.join(datafiles.dirname, datafiles.basename)
80	94	dev_files_path = os.path.join(project, 'files', 'dev-files')
81	95	element_path = os.path.join(project, 'elements')
82		- element_dep_name = 'track-test-dep-{}.bst'.format(kind)
83		- element_target_name = 'track-test-target-{}.bst'.format(kind)
	96	+
	97	+ # Try to actually launch as many fetch jobs as possible at the same time
	98	+ #
	99	+ # This stresses the Source plugins and helps to ensure that
	100	+ # they handle concurrent access to the store correctly.
	101	+ cli.configure({
	102	+ 'scheduler': {
	103	+ 'fetchers': amount,
	104	+ }
	105	+ })
84	106
85	107	# Create our repo object of the given source type with
86	108	# the dev files, and then collect the initial ref.
...	...	@@ -89,18 +111,26 @@ def test_track_recurse(cli, tmpdir, datafiles, kind):
89	111	ref = repo.create(dev_files_path)
90	112
91	113	# Write out our test targets
92		- generate_element(repo, os.path.join(element_path, element_dep_name))
93		- generate_element(repo, os.path.join(element_path, element_target_name),
94		- dep_name=element_dep_name)
	114	+ element_names = []
	115	+ last_element_name = None
	116	+ for i in range(amount + 1):
	117	+ element_name = 'track-test-{}-{}.bst'.format(kind, i + 1)
	118	+ filename = os.path.join(element_path, element_name)
	119	+
	120	+ element_names.append(element_name)
	121	+
	122	+ generate_element(repo, filename, dep_name=last_element_name)
	123	+ last_element_name = element_name
95	124
96	125	# Assert that a fetch is needed
97		- assert cli.get_element_state(project, element_dep_name) == 'no reference'
98		- assert cli.get_element_state(project, element_target_name) == 'no reference'
	126	+ states = cli.get_element_states(project, last_element_name)
	127	+ for element_name in element_names:
	128	+ assert states[element_name] == 'no reference'
99	129
100	130	# Now first try to track it
101	131	result = cli.run(project=project, args=[
102	132	'source', 'track', '--deps', 'all',
103		- element_target_name])
	133	+ last_element_name])
104	134	result.assert_success()
105	135
106	136	# And now fetch it: The Source has probably already cached the
...	...	@@ -109,12 +139,16 @@ def test_track_recurse(cli, tmpdir, datafiles, kind):
109	139	# is the job of fetch.
110	140	result = cli.run(project=project, args=[
111	141	'source', 'fetch', '--deps', 'all',
112		- element_target_name])
	142	+ last_element_name])
113	143	result.assert_success()
114	144
115		- # Assert that the dependency is buildable and the target is waiting
116		- assert cli.get_element_state(project, element_dep_name) == 'buildable'
117		- assert cli.get_element_state(project, element_target_name) == 'waiting'
	145	+ # Assert that the base is buildable and the rest are waiting
	146	+ states = cli.get_element_states(project, last_element_name)
	147	+ for element_name in element_names:
	148	+ if element_name == element_names[0]:
	149	+ assert states[element_name] == 'buildable'
	150	+ else:
	151	+ assert states[element_name] == 'waiting'
118	152
119	153
120	154	@pytest.mark.datafiles(DATA_DIR)

tests/integration/messages.py

---

	1	+#
	2	+# Copyright (C) 2018 Codethink Limited
	3	+#
	4	+# This program is free software; you can redistribute it and/or
	5	+# modify it under the terms of the GNU Lesser General Public
	6	+# License as published by the Free Software Foundation; either
	7	+# version 2 of the License, or (at your option) any later version.
	8	+#
	9	+# This library is distributed in the hope that it will be useful,
	10	+# but WITHOUT ANY WARRANTY; without even the implied warranty of
	11	+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
	12	+# Lesser General Public License for more details.
	13	+#
	14	+# You should have received a copy of the GNU Lesser General Public
	15	+# License along with this library. If not, see <http://www.gnu.org/licenses/>.
	16	+#
	17	+# Authors: Tristan Maat <tristan maat codethink co uk>
	18	+#
	19	+
	20	+import os
	21	+import pytest
	22	+
	23	+from buildstream import _yaml
	24	+from buildstream._exceptions import ErrorDomain
	25	+
	26	+from tests.testutils import cli_integration as cli
	27	+
	28	+
	29	+pytestmark = pytest.mark.integration
	30	+
	31	+
	32	+# Project directory
	33	+DATA_DIR = os.path.join(
	34	+ os.path.dirname(os.path.realpath(__file__)),
	35	+ "project",
	36	+)
	37	+
	38	+
	39	+@pytest.mark.integration
	40	+@pytest.mark.datafiles(DATA_DIR)
	41	+def test_disable_message_lines(cli, tmpdir, datafiles):
	42	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	43	+ element_path = os.path.join(project, 'elements')
	44	+ element_name = 'message.bst'
	45	+
	46	+ element = {
	47	+ 'kind': 'manual',
	48	+ 'depends': [{
	49	+ 'filename': 'base.bst'
	50	+ }],
	51	+ 'config': {
	52	+ 'build-commands':
	53	+ ['echo "Silly message"'],
	54	+ 'strip-commands': []
	55	+ }
	56	+ }
	57	+
	58	+ os.makedirs(os.path.dirname(os.path.join(element_path, element_name)), exist_ok=True)
	59	+ _yaml.dump(element, os.path.join(element_path, element_name))
	60	+
	61	+ # First we check that we get the "Silly message"
	62	+ result = cli.run(project=project, args=["build", element_name])
	63	+ result.assert_success()
	64	+ assert 'echo "Silly message"' in result.stderr
	65	+
	66	+ # Let's now build it again, but with --message-lines 0
	67	+ cli.remove_artifact_from_cache(project, element_name)
	68	+ result = cli.run(project=project, args=["--message-lines", "0",
	69	+ "build", element_name])
	70	+ result.assert_success()
	71	+ assert not "Message contains " in result.stderr
	72	+
	73	+@pytest.mark.integration
	74	+@pytest.mark.datafiles(DATA_DIR)
	75	+def test_disable_error_lines(cli, tmpdir, datafiles):
	76	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	77	+ element_path = os.path.join(project, 'elements')
	78	+ element_name = 'message.bst'
	79	+
	80	+ element = {
	81	+ 'kind': 'manual',
	82	+ 'depends': [{
	83	+ 'filename': 'base.bst'
	84	+ }],
	85	+ 'config': {
	86	+ 'build-commands':
	87	+ ['This is a syntax error > >'],
	88	+ 'strip-commands': []
	89	+ }
	90	+ }
	91	+
	92	+ os.makedirs(os.path.dirname(os.path.join(element_path, element_name)), exist_ok=True)
	93	+ _yaml.dump(element, os.path.join(element_path, element_name))
	94	+
	95	+ # First we check that we get the syntax error
	96	+ result = cli.run(project=project, args=["--error-lines", "0",
	97	+ "build", element_name])
	98	+ result.assert_main_error(ErrorDomain.STREAM, None)
	99	+ assert "This is a syntax error" in result.stderr
	100	+
	101	+ # Let's now build it again, but with --error-lines 0
	102	+ cli.remove_artifact_from_cache(project, element_name)
	103	+ result = cli.run(project=project, args=["--error-lines", "0",
	104	+ "build", element_name])
	105	+ result.assert_main_error(ErrorDomain.STREAM, None)
	106	+ assert not "Printing the last" in result.stderr

tests/testutils/runcli.py

---

...	...	@@ -245,8 +245,12 @@ class Cli():
245	245
246	246	def remove_artifact_from_cache(self, project, element_name,
247	247	*, cache_dir=None):
	248	+ # Read configuration to figure out where artifacts are stored
248	249	if not cache_dir:
249		- cache_dir = os.path.join(project, 'cache', 'artifacts')
	250	+ cache_dir = self.config.get(
	251	+ 'artifactdir',
	252	+ os.path.join(project, 'cache', 'artifacts')
	253	+ )
250	254
251	255	cache_dir = os.path.join(cache_dir, 'cas', 'refs', 'heads')
252	256
...	...	@@ -375,6 +379,9 @@ class Cli():
375	379	# Fetch an element state by name by
376	380	# invoking bst show on the project with the CLI
377	381	#
	382	+ # If you need to get the states of multiple elements,
	383	+ # then use get_element_states(s) instead.
	384	+ #
378	385	def get_element_state(self, project, element_name):
379	386	result = self.run(project=project, silent=True, args=[
380	387	'show',
...	...	@@ -385,6 +392,25 @@ class Cli():
385	392	result.assert_success()
386	393	return result.output.strip()
387	394
	395	+ # Fetch the states of elements for a given target / deps
	396	+ #
	397	+ # Returns a dictionary with the element names as keys
	398	+ #
	399	+ def get_element_states(self, project, target, deps='all'):
	400	+ result = self.run(project=project, silent=True, args=[
	401	+ 'show',
	402	+ '--deps', deps,
	403	+ '--format', '%{name}||%{state}',
	404	+ target
	405	+ ])
	406	+ result.assert_success()
	407	+ lines = result.output.splitlines()
	408	+ states = {}
	409	+ for line in lines:
	410	+ split = line.split(sep='||')
	411	+ states[split[0]] = split[1]
	412	+ return states
	413	+
388	414	# Fetch an element's cache key by invoking bst show
389	415	# on the project with the CLI
390	416	#

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