[Notes] [Git][BuildStream/buildstream][Qinusty/634-workspace-failed-builds] 17 commits: _scheduler/scheduler.py: Ignore interrupt events while terminating.

[Tristan Van Berkom <gitlab mg gitlab com>]

Title: GitLab

Tristan Van Berkom pushed to branch Qinusty/634-workspace-failed-builds at BuildStream / buildstream

Commits:

• a0814399
  by Tristan Van Berkom at 2018-10-03T13:05:52Z

  _scheduler/scheduler.py: Ignore interrupt events while terminating.
  
  For some reason, we now receive a SIGINT from the main loop even
  when the SIGINT occurred with the handler disconnected in an interactive
  prompt. This patch simply ignores any received SIGINT events from the
  main loop in the case that we are already in the process of terminating.
  
  This fixes issue #693
  

• 0a1f8e3c
  by Tristan Van Berkom at 2018-10-03T13:42:20Z

  Merge branch 'tristan/fix-double-terminate-prompt' into 'master'
  
  _scheduler/scheduler.py: Ignore interrupt events while terminating.
  
  Closes #693
  
  See merge request BuildStream/buildstream!852

• 11161f99
  by Tristan Van Berkom at 2018-10-03T13:44:02Z

  utils.py: Give save_file_atomic() a tempdir argument
  
  Allow callers to decide where the temporary file will be created.
  

• 3e797bb9
  by Tristan Van Berkom at 2018-10-03T13:44:02Z

  _artifactcache/cascache.py: Don't create temporary files in the CAS storage
  
  Use the designated tempdir when creating refs, we expect that temporary
  files are not created in the storage directory ever, they should be
  only ever created in the designated temporary directory.
  
  This fixes race conditions where utils._get_dir_size() throws an
  unhandled exception when attempting to stat the file which inadvertantly
  disappears.
  

• d9020e43
  by Tristan Van Berkom at 2018-10-03T13:44:02Z

  utils.py: Document _get_dir_size() expectations.
  
  This function assumes that files do not disappear while
  walking the given directory.
  

• 3e5ff5a9
  by Tristan Van Berkom at 2018-10-03T14:09:51Z

  Merge branch 'tristan/fix-cache-size-race' into 'master'
  
  fix cache size race
  
  See merge request BuildStream/buildstream!854

• e8179c34
  by Jim MacArthur at 2018-10-03T16:06:59Z

  element.py: Put the temporary staging directory in artifactdir
  

• 3cf38c8e
  by Jim MacArthur at 2018-10-03T16:44:02Z

  Merge branch 'jmac/temporaries-inside-cachedir' into 'master'
  
  Move the temporary staging directory to artifactdir
  
  See merge request BuildStream/buildstream!856

• 59c92bda
  by Daniel Silverstone at 2018-10-04T08:12:20Z

  sandbox/_sandboxdummy.py: Take a reason for use
  
  Since the SandboxDummy sandbox is only used in very specific
  circumstances and with good reason, this adjusts the SandboxDummy
  class to take a reason and to return it if `.run()` has to
  raise an exception because this is a dummy sandbox.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• b9ddcd0e
  by Daniel Silverstone at 2018-10-04T08:12:20Z

  _platform/darwin.py: Give reason for use of dummy sandbox
  
  Since Darwin is limited to the dummy sandbox for now due to OSXFUSE
  being unsupported as yet, and there being no suitable sandboxing option
  for use on OSX, give this as the reason so that it can be reported if
  the user attempts a local build.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• df0d5a8b
  by Daniel Silverstone at 2018-10-04T09:04:21Z

  _platform/linux.py: Refactor checks for sandboxing
  
  To better report issues in the absence of a suitable bwrap, or the
  FUSE devices, this refactors the checks for sandboxing in the Linux
  platform to cover the various possibilities.  The reasons are then
  collated and passed to the dummy sandbox for later reporting to the
  user if a local build is attempted.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• b8421a9c
  by Daniel Silverstone at 2018-10-04T09:04:21Z

  _site.py: Protect against failure running `bwrap --version`
  
  On some potentially broken systems, running `bwrap --version` might
  fail with an error code.  This patch corrects the oversight ensuring
  that we cleanly return False for version checking in such cases.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• c74bfbe5
  by Daniel Silverstone at 2018-10-04T09:04:21Z

  _site.py: Reduce complexity of bwrap version comparison
  
  Instead of an if/else ladder which is quite complex, this patch
  uses the fact that Python supports by-component tuple comparison
  to simply compare two (major, minor, patch) tuples
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• c46a7e87
  by Daniel Silverstone at 2018-10-04T09:13:04Z

  setup.py: Change bwrap assertion to a warning
  
  Since there are use-cases where BuildStream could be installed
  onto systems which do not have BubbleWrap (e.g. for remote-build-only
  scenarios) it is not correct to assert a dependency on bwrap during
  installation.  This patch makes the assertion a warning, and also
  clarifies the message somewhat.  This should fix #644
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• fd6a9573
  by Jürg Billeter at 2018-10-04T09:45:58Z

  Merge branch 'danielsilverstone-ct/bwrap-check-runtime-only' into 'master'
  
  Make bwrap check runtime only
  
  Closes #644
  
  See merge request BuildStream/buildstream!847

• c5778941
  by Josh Smith at 2018-10-04T13:43:10Z

  element.py: Do not save workspace on failed build
  
  This fixes #634, BuildStream no longer saves workspace configuration on
  a failed build.
  

• 788cde6a
  by Josh Smith at 2018-10-04T13:43:10Z

  testing: Add regression test for workspace bug
  

10 changed files:

• buildstream/_artifactcache/cascache.py
• buildstream/_platform/darwin.py
• buildstream/_platform/linux.py
• buildstream/_scheduler/scheduler.py
• buildstream/_site.py
• buildstream/element.py
• buildstream/sandbox/_sandboxdummy.py
• buildstream/utils.py
• setup.py
• tests/frontend/workspace.py

Changes:

buildstream/_artifactcache/cascache.py

---

...	...	@@ -506,7 +506,7 @@ class CASCache(ArtifactCache):
506	506	def set_ref(self, ref, tree):
507	507	refpath = self._refpath(ref)
508	508	os.makedirs(os.path.dirname(refpath), exist_ok=True)
509		- with utils.save_file_atomic(refpath, 'wb') as f:
	509	+ with utils.save_file_atomic(refpath, 'wb', tempdir=self.tmpdir) as f:
510	510	f.write(tree.SerializeToString())
511	511
512	512	# resolve_ref():

buildstream/_platform/darwin.py

---

...	...	@@ -34,6 +34,9 @@ class Darwin(Platform):
34	34	super().__init__()
35	35
36	36	def create_sandbox(self, *args, **kwargs):
	37	+ kwargs['dummy_reason'] = \
	38	+ "OSXFUSE is not supported and there are no supported sandbox" + \
	39	+ "technologies for OSX at this time"
37	40	return SandboxDummy(*args, **kwargs)
38	41
39	42	def check_sandbox_config(self, config):

buildstream/_platform/linux.py

---

...	...	@@ -37,25 +37,27 @@ class Linux(Platform):
37	37	self._uid = os.geteuid()
38	38	self._gid = os.getegid()
39	39
	40	+ self._have_fuse = os.path.exists("/dev/fuse")
	41	+ self._bwrap_exists = _site.check_bwrap_version(0, 0, 0)
	42	+ self._have_good_bwrap = _site.check_bwrap_version(0, 1, 2)
	43	+
	44	+ self._local_sandbox_available = self._have_fuse and self._have_good_bwrap
	45	+
40	46	self._die_with_parent_available = _site.check_bwrap_version(0, 1, 8)
41	47
42		- if self._local_sandbox_available():
	48	+ if self._local_sandbox_available:
43	49	self._user_ns_available = self._check_user_ns_available()
44	50	else:
45	51	self._user_ns_available = False
46	52
47	53	def create_sandbox(self, *args, **kwargs):
48		- if not self._local_sandbox_available():
49		- return SandboxDummy(*args, **kwargs)
	54	+ if not self._local_sandbox_available:
	55	+ return self._create_dummy_sandbox(*args, **kwargs)
50	56	else:
51		- from ..sandbox._sandboxbwrap import SandboxBwrap
52		- # Inform the bubblewrap sandbox as to whether it can use user namespaces or not
53		- kwargs['user_ns_available'] = self._user_ns_available
54		- kwargs['die_with_parent_available'] = self._die_with_parent_available
55		- return SandboxBwrap(*args, **kwargs)
	57	+ return self._create_bwrap_sandbox(*args, **kwargs)
56	58
57	59	def check_sandbox_config(self, config):
58		- if not self._local_sandbox_available():
	60	+ if not self._local_sandbox_available:
59	61	# Accept all sandbox configs as it's irrelevant with the dummy sandbox (no Sandbox.run).
60	62	return True
61	63
...	...	@@ -70,11 +72,26 @@ class Linux(Platform):
70	72	################################################
71	73	# Private Methods #
72	74	################################################
73		- def _local_sandbox_available(self):
74		- try:
75		- return os.path.exists(utils.get_host_tool('bwrap')) and os.path.exists('/dev/fuse')
76		- except utils.ProgramNotFoundError:
77		- return False
	75	+
	76	+ def _create_dummy_sandbox(self, *args, **kwargs):
	77	+ reasons = []
	78	+ if not self._have_fuse:
	79	+ reasons.append("FUSE is unavailable")
	80	+ if not self._have_good_bwrap:
	81	+ if self._bwrap_exists:
	82	+ reasons.append("`bwrap` is too old (bst needs at least 0.1.2)")
	83	+ else:
	84	+ reasons.append("`bwrap` executable not found")
	85	+
	86	+ kwargs['dummy_reason'] = " and ".join(reasons)
	87	+ return SandboxDummy(*args, **kwargs)
	88	+
	89	+ def _create_bwrap_sandbox(self, *args, **kwargs):
	90	+ from ..sandbox._sandboxbwrap import SandboxBwrap
	91	+ # Inform the bubblewrap sandbox as to whether it can use user namespaces or not
	92	+ kwargs['user_ns_available'] = self._user_ns_available
	93	+ kwargs['die_with_parent_available'] = self._die_with_parent_available
	94	+ return SandboxBwrap(*args, **kwargs)
78	95
79	96	def _check_user_ns_available(self):
80	97	# Here, lets check if bwrap is able to create user namespaces,

buildstream/_scheduler/scheduler.py

---

...	...	@@ -387,6 +387,15 @@ class Scheduler():
387	387	# A loop registered event callback for keyboard interrupts
388	388	#
389	389	def _interrupt_event(self):
	390	+
	391	+ # FIXME: This should not be needed, but for some reason we receive an
	392	+ # additional SIGINT event when the user hits ^C a second time
	393	+ # to inform us that they really intend to terminate; even though
	394	+ # we have disconnected our handlers at this time.
	395	+ #
	396	+ if self.terminated:
	397	+ return
	398	+
390	399	# Leave this to the frontend to decide, if no
391	400	# interrrupt callback was specified, then just terminate.
392	401	if self._interrupt_callback:

buildstream/_site.py

---

...	...	@@ -78,18 +78,12 @@ def check_bwrap_version(major, minor, patch):
78	78	if not bwrap_path:
79	79	return False
80	80	cmd = [bwrap_path, "--version"]
81		- version = str(subprocess.check_output(cmd).split()[1], "utf-8")
	81	+ try:
	82	+ version = str(subprocess.check_output(cmd).split()[1], "utf-8")
	83	+ except subprocess.CalledProcessError:
	84	+ # Failure trying to run bubblewrap
	85	+ return False
82	86	_bwrap_major, _bwrap_minor, _bwrap_patch = map(int, version.split("."))
83	87
84	88	# Check whether the installed version meets the requirements
85		- if _bwrap_major > major:
86		- return True
87		- elif _bwrap_major < major:
88		- return False
89		- else:
90		- if _bwrap_minor > minor:
91		- return True
92		- elif _bwrap_minor < minor:
93		- return False
94		- else:
95		- return _bwrap_patch >= patch
	89	+ return (_bwrap_major, _bwrap_minor, _bwrap_patch) >= (major, minor, patch)

buildstream/element.py

---

...	...	@@ -212,7 +212,7 @@ class Element(Plugin):
212	212	self.__staged_sources_directory = None # Location where Element.stage_sources() was called
213	213	self.__tainted = None # Whether the artifact is tainted and should not be shared
214	214	self.__required = False # Whether the artifact is required in the current session
215		- self.__build_result = None # The result of assembling this Element
	215	+ self.__build_result = None # The result of assembling this Element (success, description, detail)
216	216	self._build_log_path = None # The path of the build log for this Element
217	217
218	218	# hash tables of loaded artifact metadata, hashed by key
...	...	@@ -1379,10 +1379,10 @@ class Element(Plugin):
1379	1379	if not vdirectory.is_empty():
1380	1380	raise ElementError("Staging directory '{}' is not empty".format(vdirectory))
1381	1381
1382		- # While mkdtemp is advertised as using the TMP environment variable, it
1383		- # doesn't, so this explicit extraction is necesasry.
1384		- tmp_prefix = os.environ.get("TMP", None)
1385		- temp_staging_directory = tempfile.mkdtemp(prefix=tmp_prefix)
	1382	+ # It's advantageous to have this temporary directory on
	1383	+ # the same filing system as the rest of our cache.
	1384	+ temp_staging_location = os.path.join(self._get_context().artifactdir, "staging_temp")
	1385	+ temp_staging_directory = tempfile.mkdtemp(prefix=temp_staging_location)
1386	1386
1387	1387	try:
1388	1388	workspace = self._get_workspace()
...	...	@@ -1479,11 +1479,13 @@ class Element(Plugin):
1479	1479
1480	1480	self._update_state()
1481	1481
1482		- if self._get_workspace() and self._cached():
	1482	+ if self._get_workspace() and self._cached_success():
	1483	+ assert utils._is_main_process(), \
	1484	+ "Attempted to save workspace configuration from child process"
1483	1485	#
1484	1486	# Note that this block can only happen in the
1485		- # main process, since `self._cached()` cannot
1486		- # be true when assembly is completed in the task.
	1487	+ # main process, since `self._cached_success()` cannot
	1488	+ # be true when assembly is successful in the task.
1487	1489	#
1488	1490	# For this reason, it is safe to update and
1489	1491	# save the workspaces configuration

buildstream/sandbox/_sandboxdummy.py

---

...	...	@@ -23,6 +23,7 @@ from . import Sandbox
23	23	class SandboxDummy(Sandbox):
24	24	def __init__(self, *args, **kwargs):
25	25	super().__init__(*args, **kwargs)
	26	+ self._reason = kwargs.get("dummy_reason", "no reason given")
26	27
27	28	def run(self, command, flags, *, cwd=None, env=None):
28	29
...	...	@@ -37,4 +38,4 @@ class SandboxDummy(Sandbox):
37	38	"'{}'".format(command[0]),
38	39	reason='missing-command')
39	40
40		- raise SandboxError("This platform does not support local builds")
	41	+ raise SandboxError("This platform does not support local builds: {}".format(self._reason))

buildstream/utils.py

---

...	...	@@ -502,7 +502,7 @@ def get_bst_version():
502	502
503	503	@contextmanager
504	504	def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
505		- errors=None, newline=None, closefd=True, opener=None):
	505	+ errors=None, newline=None, closefd=True, opener=None, tempdir=None):
506	506	"""Save a file with a temporary name and rename it into place when ready.
507	507
508	508	This is a context manager which is meant for saving data to files.
...	...	@@ -529,8 +529,9 @@ def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
529	529	# https://bugs.python.org/issue8604
530	530
531	531	assert os.path.isabs(filename), "The utils.save_file_atomic() parameter ``filename`` must be an absolute path"
532		- dirname = os.path.dirname(filename)
533		- fd, tempname = tempfile.mkstemp(dir=dirname)
	532	+ if tempdir is None:
	533	+ tempdir = os.path.dirname(filename)
	534	+ fd, tempname = tempfile.mkstemp(dir=tempdir)
534	535	os.close(fd)
535	536
536	537	f = open(tempname, mode=mode, buffering=buffering, encoding=encoding,
...	...	@@ -562,6 +563,9 @@ def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
562	563	#
563	564	# Get the disk usage of a given directory in bytes.
564	565	#
	566	+# This function assumes that files do not inadvertantly
	567	+# disappear while this function is running.
	568	+#
565	569	# Arguments:
566	570	# (str) The path whose size to check.
567	571	#

setup.py

---

...	...	@@ -54,12 +54,13 @@ REQUIRED_BWRAP_MINOR = 1
54	54	REQUIRED_BWRAP_PATCH = 2
55	55
56	56
57		-def exit_bwrap(reason):
	57	+def warn_bwrap(reason):
58	58	print(reason +
59		- "\nBuildStream requires Bubblewrap (bwrap) for"
60		- " sandboxing the build environment. Install it using your package manager"
61		- " (usually bwrap or bubblewrap)")
62		- sys.exit(1)
	59	+ "\nBuildStream requires Bubblewrap (bwrap {}.{}.{} or better),"
	60	+ " during local builds, for"
	61	+ " sandboxing the build environment.\nInstall it using your package manager"
	62	+ " (usually bwrap or bubblewrap) otherwise you will be limited to"
	63	+ " remote builds only.".format(REQUIRED_BWRAP_MAJOR, REQUIRED_BWRAP_MINOR, REQUIRED_BWRAP_PATCH))
63	64
64	65
65	66	def bwrap_too_old(major, minor, patch):
...	...	@@ -76,18 +77,19 @@ def bwrap_too_old(major, minor, patch):
76	77	return False
77	78
78	79
79		-def assert_bwrap():
	80	+def check_for_bwrap():
80	81	platform = os.environ.get('BST_FORCE_BACKEND', '') or sys.platform
81	82	if platform.startswith('linux'):
82	83	bwrap_path = shutil.which('bwrap')
83	84	if not bwrap_path:
84		- exit_bwrap("Bubblewrap not found")
	85	+ warn_bwrap("Bubblewrap not found")
	86	+ return
85	87
86	88	version_bytes = subprocess.check_output([bwrap_path, "--version"]).split()[1]
87	89	version_string = str(version_bytes, "utf-8")
88	90	major, minor, patch = map(int, version_string.split("."))
89	91	if bwrap_too_old(major, minor, patch):
90		- exit_bwrap("Bubblewrap too old")
	92	+ warn_bwrap("Bubblewrap too old")
91	93
92	94
93	95	###########################################
...	...	@@ -126,7 +128,7 @@ bst_install_entry_points = {
126	128	}
127	129
128	130	if not os.environ.get('BST_ARTIFACTS_ONLY', ''):
129		- assert_bwrap()
	131	+ check_for_bwrap()
130	132	bst_install_entry_points['console_scripts'] += [
131	133	'bst = buildstream._frontend:cli'
132	134	]

tests/frontend/workspace.py

---

...	...	@@ -43,7 +43,8 @@ DATA_DIR = os.path.join(
43	43	)
44	44
45	45
46		-def open_workspace(cli, tmpdir, datafiles, kind, track, suffix='', workspace_dir=None, project_path=None):
	46	+def open_workspace(cli, tmpdir, datafiles, kind, track, suffix='', workspace_dir=None,
	47	+ project_path=None, element_attrs=None):
47	48	if not workspace_dir:
48	49	workspace_dir = os.path.join(str(tmpdir), 'workspace{}'.format(suffix))
49	50	if not project_path:
...	...	@@ -69,6 +70,8 @@ def open_workspace(cli, tmpdir, datafiles, kind, track, suffix='', workspace_dir
69	70	repo.source_config(ref=ref)
70	71	]
71	72	}
	73	+ if element_attrs:
	74	+ element = {**element, **element_attrs}
72	75	_yaml.dump(element,
73	76	os.path.join(element_path,
74	77	element_name))
...	...	@@ -854,3 +857,22 @@ def test_cache_key_workspace_in_dependencies(cli, tmpdir, datafiles, strict):
854	857
855	858	# Check that the original /usr/bin/hello is not in the checkout
856	859	assert not os.path.exists(os.path.join(checkout, 'usr', 'bin', 'hello'))
	860	+
	861	+
	862	+@pytest.mark.datafiles(DATA_DIR)
	863	+def test_multiple_failed_builds(cli, tmpdir, datafiles):
	864	+ element_config = {
	865	+ "kind": "manual",
	866	+ "config": {
	867	+ "configure-commands": [
	868	+ "unknown_command_that_will_fail"
	869	+ ]
	870	+ }
	871	+ }
	872	+ element_name, project, _ = open_workspace(cli, tmpdir, datafiles,
	873	+ "git", False, element_attrs=element_config)
	874	+
	875	+ for _ in range(2):
	876	+ result = cli.run(project=project, args=["build", element_name])
	877	+ assert "BUG" not in result.stderr
	878	+ assert cli.get_element_state(project, element_name) != "cached"

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