[Notes] [Git][BuildStream/buildstream][adamjones/labels] 10 commits: element.py: Always clean up the rootdir

[Agustin Benito Bethencourt <gitlab mg gitlab com>]

Title: GitLab

Agustin Benito Bethencourt pushed to branch adamjones/labels at BuildStream / buildstream

Commits:

• 09ef8b25
  by Jonathan Maw at 2018-10-30T15:42:05Z

  element.py: Always clean up the rootdir
  
  We shouldn't need it to persist now that we cache failed build dirs.
  
  This change breaks the test
  `tests/integration/shell.py::test_sysroot_workspace_visible`.
  I can no longer see a use-case for this test.
  AIUI, it tested that the failed build sysroot stored in the builddir has
  the workspace's files in, despite the workspace being unmounted.
  I believe this behaviour is made redundant by cached buildtrees.
  
  This fixes part of #539
  

• 37afde0a
  by Jonathan Maw at 2018-10-30T15:42:05Z

  Element: Use cached buildtree in build shells and failure shells
  
  This includes changes in app.py:
  * Interactive failure shell no longer uses the failed build sysroot,
    defaulting to the cached build tree.
  
  Changes in element.py are:
  * Errors caused by building don't store the failed build sysroot,
    instead storing that a sandbox can be created to debug the error.
  * When staging sources, will stage the element's cached build tree if it
    exists.
  
  Changes in _exceptions.py:
  * BstError.sandbox is now a flag of whether a sandbox can be opened up
    to debug the error.
  
  Changes in widget.py:
  * Don't try to print any information about the sandbox.
  
  Changes in _message.py:
  * Fix documentation so Message.sandbox is not a directory any more.
  
  This is part of #539
  

• 4bc71773
  by Jonathan Maw at 2018-10-30T15:42:05Z

  NEWS: Add item for cached buildtree behaviour
  

• 9b89564f
  by Jonathan Maw at 2018-10-30T15:42:05Z

  tests: Add test that cached build trees are staged in build shells
  
  This is related to #539
  

• ec76cbe1
  by Jonathan Maw at 2018-10-30T15:42:05Z

  sandbox.py: Remove redundant Sandbox.__directory
  

• 825963b5
  by Jonathan Maw at 2018-10-30T15:42:05Z

  element: Make "--sysroot" take a bare directory
  
  i.e. instead of taking a directory that must contain "root" and
  "scratch", and treating "root" as the root, use the directory directly.
  
  In element.py:
  * __sandbox takes the `bare_sandbox` arg, to pass into the sandbox's constructor
  
  In sandbox.py:
  * If bare_sandbox, `_root` is the passed-in directory, and `__scratch`
    is None.
  * Trying to use `__scratch` when bare_sandbox is True is a bug.
  
  In _mount.py:
  * Don't get the value of `__scratch` if it's not needed.
  
  This is part of #539
  

• cf721838
  by Jonathan Maw at 2018-10-30T15:42:05Z

  NEWS: Add item for bst shell --sysroot changes
  

• 79d1bb7f
  by Jonathan Maw at 2018-10-30T15:42:05Z

  tests: Add tests for 'bst shell --sysroot'
  
  This is related to #539
  

• c7ac7e7d
  by Jonathan Maw at 2018-10-30T16:19:17Z

  Merge branch 'jonathan/debug-remote-failed-builds' into 'master'
  
  Jonathan/debug remote failed builds
  
  See merge request BuildStream/buildstream!869

• 30ce5e97
  by Adam Jones at 2018-10-30T17:24:56Z

  Add a link to the label descriptions in the issue templates
  

14 changed files:

• .gitlab/issue_templates/bst_bug.md
• .gitlab/issue_templates/bst_task.md
• NEWS
• buildstream/_exceptions.py
• buildstream/_frontend/app.py
• buildstream/_frontend/widget.py
• buildstream/_message.py
• buildstream/element.py
• buildstream/sandbox/_mount.py
• buildstream/sandbox/sandbox.py
• + tests/integration/build-tree.py
• + tests/integration/project/elements/build-shell/buildtree-fail.bst
• + tests/integration/project/elements/build-shell/buildtree.bst
• tests/integration/shell.py

Changes:

.gitlab/issue_templates/bst_bug.md

---

...	...	@@ -33,4 +33,6 @@
33	33	* BuildStream version affected: /milestone %BuildStream_v1.x
34	34
35	35	----
	36	+[//]: # (To review information about possible relevant labels for this issue please view the list of labels: https://gitlab.com/BuildStream/buildstream/labels)
	37	+
36	38	/label ~bug

.gitlab/issue_templates/bst_task.md

---

...	...	@@ -15,3 +15,5 @@
15	15	[//]: # (Acceptance criteria should follow the S.M.A.R.T. principle https://en.wikipedia.org/wiki/SMART_criteria )
16	16
17	17	----
	18	+[//]: # (To review information about possible relevant labels for this issue please view the list of labels: https://gitlab.com/BuildStream/buildstream/labels)
	19	+

NEWS

---

...	...	@@ -31,6 +31,15 @@ buildstream 1.3.1
31	31	new the `conf-root` variable to make the process easier. And there has been
32	32	a bug fix to workspaces so they can be build in workspaces too.
33	33
	34	+ o Creating a build shell through the interactive mode or `bst shell --build`
	35	+ will now use the cached build tree. It is now easier to debug local build
	36	+ failures.
	37	+
	38	+ o `bst shell --sysroot` now takes any directory that contains a sysroot,
	39	+ instead of just a specially-formatted build-root with a `root` and `scratch`
	40	+ subdirectory.
	41	+
	42	+
34	43	=================
35	44	buildstream 1.1.5
36	45	=================

buildstream/_exceptions.py

---

...	...	@@ -111,10 +111,8 @@ class BstError(Exception):
111	111	#
112	112	self.detail = detail
113	113
114		- # The build sandbox in which the error occurred, if the
115		- # error occurred at element assembly time.
116		- #
117		- self.sandbox = None
	114	+ # A sandbox can be created to debug this error
	115	+ self.sandbox = False
118	116
119	117	# When this exception occurred during the handling of a job, indicate
120	118	# whether or not there is any point retrying the job.

buildstream/_frontend/app.py

---

...	...	@@ -597,7 +597,7 @@ class App():
597	597	click.echo("\nDropping into an interactive shell in the failed build sandbox\n", err=True)
598	598	try:
599	599	prompt = self.shell_prompt(element)
600		- self.stream.shell(element, Scope.BUILD, prompt, directory=failure.sandbox, isolate=True)
	600	+ self.stream.shell(element, Scope.BUILD, prompt, isolate=True)
601	601	except BstError as e:
602	602	click.echo("Error while attempting to create interactive shell: {}".format(e), err=True)
603	603	elif choice == 'log':

buildstream/_frontend/widget.py

---

...	...	@@ -668,17 +668,6 @@ class LogLine(Widget):
668	668
669	669	extra_nl = True
670	670
671		- if message.sandbox is not None:
672		- sandbox = self._indent + 'Sandbox directory: ' + message.sandbox
673		-
674		- text += '\n'
675		- if message.message_type == MessageType.FAIL:
676		- text += self._err_profile.fmt(sandbox, bold=True)
677		- else:
678		- text += self._detail_profile.fmt(sandbox)
679		- text += '\n'
680		- extra_nl = True
681		-
682	671	if message.scheduler and message.message_type == MessageType.FAIL:
683	672	text += '\n'
684	673

buildstream/_message.py

---

...	...	@@ -70,7 +70,7 @@ class Message():
70	70	self.elapsed = elapsed # The elapsed time, in timed messages
71	71	self.depth = depth # The depth of a timed message
72	72	self.logfile = logfile # The log file path where commands took place
73		- self.sandbox = sandbox # The sandbox directory where an error occurred (if any)
	73	+ self.sandbox = sandbox # The error that caused this message used a sandbox
74	74	self.pid = os.getpid() # The process pid
75	75	self.unique_id = unique_id # The plugin object ID issueing the message
76	76	self.task_id = task_id # The plugin object ID of the task

buildstream/element.py

---

...	...	@@ -1318,7 +1318,9 @@ class Element(Plugin):
1318	1318	@contextmanager
1319	1319	def _prepare_sandbox(self, scope, directory, deps='run', integrate=True):
1320	1320	# bst shell and bst checkout require a local sandbox.
1321		- with self.__sandbox(directory, config=self.__sandbox_config, allow_remote=False) as sandbox:
	1321	+ bare_directory = True if directory else False
	1322	+ with self.__sandbox(directory, config=self.__sandbox_config, allow_remote=False,
	1323	+ bare_directory=bare_directory) as sandbox:
1322	1324
1323	1325	# Configure always comes first, and we need it.
1324	1326	self.configure_sandbox(sandbox)
...	...	@@ -1385,6 +1387,7 @@ class Element(Plugin):
1385	1387	# the same filing system as the rest of our cache.
1386	1388	temp_staging_location = os.path.join(self._get_context().artifactdir, "staging_temp")
1387	1389	temp_staging_directory = tempfile.mkdtemp(prefix=temp_staging_location)
	1390	+ import_dir = temp_staging_directory
1388	1391
1389	1392	try:
1390	1393	workspace = self._get_workspace()
...	...	@@ -1395,12 +1398,16 @@ class Element(Plugin):
1395	1398	with self.timed_activity("Staging local files at {}"
1396	1399	.format(workspace.get_absolute_path())):
1397	1400	workspace.stage(temp_staging_directory)
	1401	+ elif self._cached():
	1402	+ # We have a cached buildtree to use, instead
	1403	+ artifact_base, _ = self.__extract()
	1404	+ import_dir = os.path.join(artifact_base, 'buildtree')
1398	1405	else:
1399	1406	# No workspace, stage directly
1400	1407	for source in self.sources():
1401	1408	source._stage(temp_staging_directory)
1402	1409
1403		- vdirectory.import_files(temp_staging_directory)
	1410	+ vdirectory.import_files(import_dir)
1404	1411
1405	1412	finally:
1406	1413	# Staging may produce directories with less than 'rwx' permissions
...	...	@@ -1566,9 +1573,8 @@ class Element(Plugin):
1566	1573	collect = self.assemble(sandbox) # pylint: disable=assignment-from-no-return
1567	1574	self.__set_build_result(success=True, description="succeeded")
1568	1575	except BstError as e:
1569		- # If an error occurred assembling an element in a sandbox,
1570		- # then tack on the sandbox directory to the error
1571		- e.sandbox = rootdir
	1576	+ # Shelling into a sandbox is useful to debug this error
	1577	+ e.sandbox = True
1572	1578
1573	1579	# If there is a workspace open on this element, it will have
1574	1580	# been mounted for sandbox invocations instead of being staged.
...	...	@@ -1683,8 +1689,8 @@ class Element(Plugin):
1683	1689	"unable to collect artifact contents"
1684	1690	.format(collect))
1685	1691
1686		- # Finally cleanup the build dir
1687		- cleanup_rootdir()
	1692	+ # Finally cleanup the build dir
	1693	+ cleanup_rootdir()
1688	1694
1689	1695	return artifact_size
1690	1696
...	...	@@ -2152,12 +2158,14 @@ class Element(Plugin):
2152	2158	# stderr (fileobject): The stream for stderr for the sandbox
2153	2159	# config (SandboxConfig): The SandboxConfig object
2154	2160	# allow_remote (bool): Whether the sandbox is allowed to be remote
	2161	+ # bare_directory (bool): Whether the directory is bare i.e. doesn't have
	2162	+ # a separate 'root' subdir
2155	2163	#
2156	2164	# Yields:
2157	2165	# (Sandbox): A usable sandbox
2158	2166	#
2159	2167	@contextmanager
2160		- def __sandbox(self, directory, stdout=None, stderr=None, config=None, allow_remote=True):
	2168	+ def __sandbox(self, directory, stdout=None, stderr=None, config=None, allow_remote=True, bare_directory=False):
2161	2169	context = self._get_context()
2162	2170	project = self._get_project()
2163	2171	platform = Platform.get_platform()
...	...	@@ -2188,6 +2196,7 @@ class Element(Plugin):
2188	2196	stdout=stdout,
2189	2197	stderr=stderr,
2190	2198	config=config,
	2199	+ bare_directory=bare_directory,
2191	2200	allow_real_directory=not self.BST_VIRTUAL_DIRECTORY)
2192	2201	yield sandbox
2193	2202
...	...	@@ -2197,7 +2206,7 @@ class Element(Plugin):
2197	2206
2198	2207	# Recursive contextmanager...
2199	2208	with self.__sandbox(rootdir, stdout=stdout, stderr=stderr, config=config,
2200		- allow_remote=allow_remote) as sandbox:
	2209	+ allow_remote=allow_remote, bare_directory=False) as sandbox:
2201	2210	yield sandbox
2202	2211
2203	2212	# Cleanup the build dir

buildstream/sandbox/_mount.py

---

...	...	@@ -31,7 +31,6 @@ from .._fuse import SafeHardlinks
31	31	#
32	32	class Mount():
33	33	def __init__(self, sandbox, mount_point, safe_hardlinks, fuse_mount_options=None):
34		- scratch_directory = sandbox._get_scratch_directory()
35	34	# Getting _get_underlying_directory() here is acceptable as
36	35	# we're part of the sandbox code. This will fail if our
37	36	# directory is CAS-based.
...	...	@@ -51,6 +50,7 @@ class Mount():
51	50	# a regular mount point within the parent's redirected mount.
52	51	#
53	52	if self.safe_hardlinks:
	53	+ scratch_directory = sandbox._get_scratch_directory()
54	54	# Redirected mount
55	55	self.mount_origin = os.path.join(root_directory, mount_point.lstrip(os.sep))
56	56	self.mount_base = os.path.join(scratch_directory, utils.url_directory_name(mount_point))

buildstream/sandbox/sandbox.py

---

...	...	@@ -98,16 +98,23 @@ class Sandbox():
98	98	self.__config = kwargs['config']
99	99	self.__stdout = kwargs['stdout']
100	100	self.__stderr = kwargs['stderr']
	101	+ self.__bare_directory = kwargs['bare_directory']
101	102
102	103	# Setup the directories. Root and output_directory should be
103	104	# available to subclasses, hence being single-underscore. The
104	105	# others are private to this class.
105		- self._root = os.path.join(directory, 'root')
	106	+ # If the directory is bare, it probably doesn't need scratch
	107	+ if self.__bare_directory:
	108	+ self._root = directory
	109	+ self.__scratch = None
	110	+ os.makedirs(self._root, exist_ok=True)
	111	+ else:
	112	+ self._root = os.path.join(directory, 'root')
	113	+ self.__scratch = os.path.join(directory, 'scratch')
	114	+ for directory_ in [self._root, self.__scratch]:
	115	+ os.makedirs(directory_, exist_ok=True)
	116	+
106	117	self._output_directory = None
107		- self.__directory = directory
108		- self.__scratch = os.path.join(self.__directory, 'scratch')
109		- for directory_ in [self._root, self.__scratch]:
110		- os.makedirs(directory_, exist_ok=True)
111	118	self._vdir = None
112	119
113	120	# This is set if anyone requests access to the underlying
...	...	@@ -334,6 +341,7 @@ class Sandbox():
334	341	# Returns:
335	342	# (str): The sandbox scratch directory
336	343	def _get_scratch_directory(self):
	344	+ assert not self.__bare_directory, "Scratch is not going to work with bare directories"
337	345	return self.__scratch
338	346
339	347	# _get_output()

tests/integration/build-tree.py

---

	1	+import os
	2	+import pytest
	3	+import shutil
	4	+
	5	+from tests.testutils import cli, cli_integration, create_artifact_share
	6	+from buildstream._exceptions import ErrorDomain
	7	+
	8	+
	9	+pytestmark = pytest.mark.integration
	10	+
	11	+
	12	+DATA_DIR = os.path.join(
	13	+ os.path.dirname(os.path.realpath(__file__)),
	14	+ "project"
	15	+)
	16	+
	17	+
	18	+@pytest.mark.datafiles(DATA_DIR)
	19	+def test_buildtree_staged(cli_integration, tmpdir, datafiles):
	20	+ # i.e. tests that cached build trees are staged by `bst shell --build`
	21	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	22	+ element_name = 'build-shell/buildtree.bst'
	23	+
	24	+ res = cli_integration.run(project=project, args=['build', element_name])
	25	+ res.assert_success()
	26	+
	27	+ res = cli_integration.run(project=project, args=[
	28	+ 'shell', '--build', element_name, '--', 'grep', '-q', 'Hi', 'test'
	29	+ ])
	30	+ res.assert_success()
	31	+
	32	+
	33	+@pytest.mark.datafiles(DATA_DIR)
	34	+def test_buildtree_from_failure(cli_integration, tmpdir, datafiles):
	35	+ # i.e. test that on a build failure, we can still shell into it
	36	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	37	+ element_name = 'build-shell/buildtree-fail.bst'
	38	+
	39	+ res = cli_integration.run(project=project, args=['build', element_name])
	40	+ res.assert_main_error(ErrorDomain.STREAM, None)
	41	+
	42	+ # Assert that file has expected contents
	43	+ res = cli_integration.run(project=project, args=[
	44	+ 'shell', '--build', element_name, '--', 'cat', 'test'
	45	+ ])
	46	+ res.assert_success()
	47	+ assert 'Hi' in res.output
	48	+
	49	+
	50	+# Check that build shells work when pulled from a remote cache
	51	+# This is to roughly simulate remote execution
	52	+@pytest.mark.datafiles(DATA_DIR)
	53	+def test_buildtree_pulled(cli, tmpdir, datafiles):
	54	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	55	+ element_name = 'build-shell/buildtree.bst'
	56	+
	57	+ with create_artifact_share(os.path.join(str(tmpdir), 'artifactshare')) as share:
	58	+ # Build the element to push it to cache
	59	+ cli.configure({
	60	+ 'artifacts': {'url': share.repo, 'push': True}
	61	+ })
	62	+ result = cli.run(project=project, args=['build', element_name])
	63	+ result.assert_success()
	64	+ assert cli.get_element_state(project, element_name) == 'cached'
	65	+
	66	+ # Discard the cache
	67	+ cli.configure({
	68	+ 'artifacts': {'url': share.repo, 'push': True},
	69	+ 'artifactdir': os.path.join(cli.directory, 'artifacts2')
	70	+ })
	71	+ assert cli.get_element_state(project, element_name) != 'cached'
	72	+
	73	+ # Pull from cache
	74	+ result = cli.run(project=project, args=['pull', '--deps', 'all', element_name])
	75	+ result.assert_success()
	76	+
	77	+ # Check it's using the cached build tree
	78	+ res = cli.run(project=project, args=[
	79	+ 'shell', '--build', element_name, '--', 'grep', '-q', 'Hi', 'test'
	80	+ ])
	81	+ res.assert_success()

tests/integration/project/elements/build-shell/buildtree-fail.bst

---

	1	+kind: manual
	2	+description: |
	3	+ Puts a file in the build tree so that build tree caching and staging can be tested,
	4	+ then deliberately failing to build so we can check the output.
	5	+
	6	+depends:
	7	+ - filename: base.bst
	8	+ type: build
	9	+
	10	+config:
	11	+ build-commands:
	12	+ - "echo 'Hi' > %{build-root}/test"
	13	+ - "false"

tests/integration/project/elements/build-shell/buildtree.bst

---

	1	+kind: manual
	2	+description: |
	3	+ Puts a file in the build tree so that build tree caching and staging can be tested.
	4	+
	5	+depends:
	6	+ - filename: base.bst
	7	+ type: build
	8	+
	9	+config:
	10	+ build-commands:
	11	+ - "echo 'Hi' > %{build-root}/test"

tests/integration/shell.py

---

...	...	@@ -302,46 +302,33 @@ def test_workspace_visible(cli, tmpdir, datafiles):
302	302	assert result.output == workspace_hello
303	303
304	304
305		-# Test that we can see the workspace files in a shell
306		-@pytest.mark.integration
	305	+# Test that '--sysroot' works
307	306	@pytest.mark.datafiles(DATA_DIR)
308		-def test_sysroot_workspace_visible(cli, tmpdir, datafiles):
	307	+def test_sysroot(cli, tmpdir, datafiles):
309	308	project = os.path.join(datafiles.dirname, datafiles.basename)
310		- workspace = os.path.join(cli.directory, 'workspace')
311		- element_name = 'workspace/workspace-mount-fail.bst'
312		-
313		- # Open a workspace on our build failing element
314		- #
315		- res = cli.run(project=project, args=['workspace', 'open', element_name, workspace])
316		- assert res.exit_code == 0
317		-
318		- # Ensure the dependencies of our build failing element are built
319		- result = cli.run(project=project, args=['build', element_name])
320		- result.assert_main_error(ErrorDomain.STREAM, None)
321		-
322		- # Discover the sysroot of the failed build directory, after one
323		- # failed build, there should be only one directory there.
324		- #
325		- build_base = os.path.join(cli.directory, 'build')
326		- build_dirs = os.listdir(path=build_base)
327		- assert len(build_dirs) == 1
328		- build_dir = os.path.join(build_base, build_dirs[0])
329		-
330		- # Obtain a copy of the hello.c content from the workspace
331		- #
332		- workspace_hello_path = os.path.join(cli.directory, 'workspace', 'hello.c')
333		- assert os.path.exists(workspace_hello_path)
334		- with open(workspace_hello_path, 'r') as f:
335		- workspace_hello = f.read()
336		-
337		- # Cat the hello.c file from a bst shell command, and assert
338		- # that we got the same content here
339		- #
340		- result = cli.run(project=project, args=[
341		- 'shell', '--build', '--sysroot', build_dir, element_name, '--', 'cat', 'hello.c'
	309	+ base_element = "base/base-alpine.bst"
	310	+ # test element only needs to be something lightweight for this test
	311	+ test_element = "script/script.bst"
	312	+ checkout_dir = os.path.join(str(tmpdir), 'alpine-sysroot')
	313	+ test_file = 'hello'
	314	+
	315	+ # Build and check out a sysroot
	316	+ res = cli.run(project=project, args=['build', base_element])
	317	+ res.assert_success()
	318	+ res = cli.run(project=project, args=['checkout', base_element, checkout_dir])
	319	+ res.assert_success()
	320	+
	321	+ # Mutate the sysroot
	322	+ test_path = os.path.join(checkout_dir, test_file)
	323	+ with open(test_path, 'w') as f:
	324	+ f.write('hello\n')
	325	+
	326	+ # Shell into the sysroot and check the test file exists
	327	+ res = cli.run(project=project, args=[
	328	+ 'shell', '--build', '--sysroot', checkout_dir, test_element, '--',
	329	+ 'grep', '-q', 'hello', '/' + test_file
342	330	])
343		- assert result.exit_code == 0
344		- assert result.output == workspace_hello
	331	+ res.assert_success()
345	332
346	333
347	334	# Test system integration commands can access devices in /dev

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