NEWS

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

buildstream/_exceptions.py

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

buildstream/_frontend/app.py

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

buildstream/_stream.py

@@ -423,9 +423,16 @@ class Stream():
                 else:
                     if location == '-':
                         with target.timed_activity("Creating tarball"):
-                            with os.fdopen(sys.stdout.fileno(), 'wb') as fo:
-                                with tarfile.open(fileobj=fo, mode="w|") as tf:
-                                    sandbox_vroot.export_to_tar(tf, '.')
+                            # Save the stdout FD to restore later
+                            saved_fd = os.dup(sys.stdout.fileno())
+                            try:
+                                with os.fdopen(sys.stdout.fileno(), 'wb') as fo:
+                                    with tarfile.open(fileobj=fo, mode="w|") as tf:
+                                        sandbox_vroot.export_to_tar(tf, '.')
+                            finally:
+                                # No matter what, restore stdout for further use
+                                os.dup2(saved_fd, sys.stdout.fileno())
+                                os.close(saved_fd)
                     else:
                         with target.timed_activity("Creating tarball '{}'"
                                                    .format(location)):

buildstream/_yaml.py

@@ -335,16 +335,9 @@ def node_get_provenance(node, key=None, indices=None):
     return provenance
 
 
-# Helper to use utils.sentinel without unconditional utils import,
-# which causes issues for completion.
-#
-# Local private, but defined here because sphinx appears to break if
-# it's not defined before any functions calling it in default kwarg
-# values.
-#
-def _get_sentinel():
-    from .utils import _sentinel
-    return _sentinel
+# A sentinel to be used as a default argument for functions that need
+# to distinguish between a kwarg set to None and an unset kwarg.
+_sentinel = object()
 
 
 # node_get()
@@ -368,10 +361,10 @@ def _get_sentinel():
 # Note:
 #    Returned strings are stripped of leading and trailing whitespace
 #
-def node_get(node, expected_type, key, indices=None, default_value=_get_sentinel()):
+def node_get(node, expected_type, key, indices=None, default_value=_sentinel):
     value = node.get(key, default_value)
     provenance = node_get_provenance(node)
-    if value is _get_sentinel():
+    if value is _sentinel:
         raise LoadError(LoadErrorReason.INVALID_DATA,
                         "{}: Dictionary did not contain expected key '{}'".format(provenance, key))
 

buildstream/element.py

@@ -451,7 +451,7 @@ class Element(Plugin):
 
         return None
 
-    def node_subst_member(self, node, member_name, default=utils._sentinel):
+    def node_subst_member(self, node, member_name, default=_yaml._sentinel):
         """Fetch the value of a string node member, substituting any variables
         in the loaded value with the element contextual variables.
 
@@ -1318,7 +1318,9 @@ class Element(Plugin):
     @contextmanager
     def _prepare_sandbox(self, scope, directory, deps='run', integrate=True):
         # bst shell and bst checkout require a local sandbox.
-        with self.__sandbox(directory, config=self.__sandbox_config, allow_remote=False) as sandbox:
+        bare_directory = True if directory else False
+        with self.__sandbox(directory, config=self.__sandbox_config, allow_remote=False,
+                            bare_directory=bare_directory) as sandbox:
 
             # Configure always comes first, and we need it.
             self.configure_sandbox(sandbox)
@@ -1385,6 +1387,7 @@ class Element(Plugin):
             # the same filing system as the rest of our cache.
             temp_staging_location = os.path.join(self._get_context().artifactdir, "staging_temp")
             temp_staging_directory = tempfile.mkdtemp(prefix=temp_staging_location)
+            import_dir = temp_staging_directory
 
             try:
                 workspace = self._get_workspace()
@@ -1395,12 +1398,16 @@ class Element(Plugin):
                         with self.timed_activity("Staging local files at {}"
                                                  .format(workspace.get_absolute_path())):
                             workspace.stage(temp_staging_directory)
+                elif self._cached():
+                    # We have a cached buildtree to use, instead
+                    artifact_base, _ = self.__extract()
+                    import_dir = os.path.join(artifact_base, 'buildtree')
                 else:
                     # No workspace, stage directly
                     for source in self.sources():
                         source._stage(temp_staging_directory)
 
-                vdirectory.import_files(temp_staging_directory)
+                vdirectory.import_files(import_dir)
 
             finally:
                 # Staging may produce directories with less than 'rwx' permissions
@@ -1566,9 +1573,8 @@ class Element(Plugin):
                     collect = self.assemble(sandbox)  # pylint: disable=assignment-from-no-return
                     self.__set_build_result(success=True, description="succeeded")
                 except BstError as e:
-                    # If an error occurred assembling an element in a sandbox,
-                    # then tack on the sandbox directory to the error
-                    e.sandbox = rootdir
+                    # Shelling into a sandbox is useful to debug this error
+                    e.sandbox = True
 
                     # If there is a workspace open on this element, it will have
                     # been mounted for sandbox invocations instead of being staged.
@@ -1683,8 +1689,8 @@ class Element(Plugin):
                             "unable to collect artifact contents"
                             .format(collect))
 
-            # Finally cleanup the build dir
-            cleanup_rootdir()
+                    # Finally cleanup the build dir
+                    cleanup_rootdir()
 
         return artifact_size
 
@@ -2152,12 +2158,14 @@ class Element(Plugin):
     #    stderr (fileobject): The stream for stderr for the sandbox
     #    config (SandboxConfig): The SandboxConfig object
     #    allow_remote (bool): Whether the sandbox is allowed to be remote
+    #    bare_directory (bool): Whether the directory is bare i.e. doesn't have
+    #                           a separate 'root' subdir
     #
     # Yields:
     #    (Sandbox): A usable sandbox
     #
     @contextmanager
-    def __sandbox(self, directory, stdout=None, stderr=None, config=None, allow_remote=True):
+    def __sandbox(self, directory, stdout=None, stderr=None, config=None, allow_remote=True, bare_directory=False):
         context = self._get_context()
         project = self._get_project()
         platform = Platform.get_platform()
@@ -2188,6 +2196,7 @@ class Element(Plugin):
                                               stdout=stdout,
                                               stderr=stderr,
                                               config=config,
+                                              bare_directory=bare_directory,
                                               allow_real_directory=not self.BST_VIRTUAL_DIRECTORY)
             yield sandbox
 
@@ -2197,7 +2206,7 @@ class Element(Plugin):
 
             # Recursive contextmanager...
             with self.__sandbox(rootdir, stdout=stdout, stderr=stderr, config=config,
-                                allow_remote=allow_remote) as sandbox:
+                                allow_remote=allow_remote, bare_directory=False) as sandbox:
                 yield sandbox
 
             # Cleanup the build dir

buildstream/plugin.py

@@ -321,7 +321,7 @@ class Plugin():
         provenance = _yaml.node_get_provenance(node, key=member_name)
         return str(provenance)
 
-    def node_get_member(self, node, expected_type, member_name, default=utils._sentinel):
+    def node_get_member(self, node, expected_type, member_name, default=_yaml._sentinel):
         """Fetch the value of a node member, raising an error if the value is
         missing or incorrectly typed.
 

buildstream/sandbox/_mount.py

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

buildstream/sandbox/sandbox.py

@@ -98,16 +98,23 @@ class Sandbox():
         self.__config = kwargs['config']
         self.__stdout = kwargs['stdout']
         self.__stderr = kwargs['stderr']
+        self.__bare_directory = kwargs['bare_directory']
 
         # Setup the directories. Root and output_directory should be
         # available to subclasses, hence being single-underscore. The
         # others are private to this class.
-        self._root = os.path.join(directory, 'root')
+        # If the directory is bare, it probably doesn't need scratch
+        if self.__bare_directory:
+            self._root = directory
+            self.__scratch = None
+            os.makedirs(self._root, exist_ok=True)
+        else:
+            self._root = os.path.join(directory, 'root')
+            self.__scratch = os.path.join(directory, 'scratch')
+            for directory_ in [self._root, self.__scratch]:
+                os.makedirs(directory_, exist_ok=True)
+
         self._output_directory = None
-        self.__directory = directory
-        self.__scratch = os.path.join(self.__directory, 'scratch')
-        for directory_ in [self._root, self.__scratch]:
-            os.makedirs(directory_, exist_ok=True)
         self._vdir = None
 
         # This is set if anyone requests access to the underlying
@@ -334,6 +341,7 @@ class Sandbox():
     # Returns:
     #    (str): The sandbox scratch directory
     def _get_scratch_directory(self):
+        assert not self.__bare_directory, "Scratch is not going to work with bare directories"
         return self.__scratch
 
     # _get_output()

buildstream/utils.py

@@ -654,10 +654,6 @@ def _pretty_size(size, dec_places=0):
     return "{size:g}{unit}".format(size=round(psize, dec_places), unit=unit)
 
 
-# A sentinel to be used as a default argument for functions that need
-# to distinguish between a kwarg set to None and an unset kwarg.
-_sentinel = object()
-
 # Main process pid
 _main_pid = os.getpid()
 

tests/frontend/buildcheckout.py

@@ -128,7 +128,6 @@ def test_build_checkout_tarball(datafiles, cli):
     assert os.path.join('.', 'usr', 'include', 'pony.h') in tar.getnames()
 
 
-@pytest.mark.skip(reason="Capturing the binary output is causing a stacktrace")
 @pytest.mark.datafiles(DATA_DIR)
 def test_build_checkout_tarball_stdout(datafiles, cli):
     project = os.path.join(datafiles.dirname, datafiles.basename)
@@ -143,7 +142,7 @@ def test_build_checkout_tarball_stdout(datafiles, cli):
 
     checkout_args = ['checkout', '--tar', 'target.bst', '-']
 
-    result = cli.run(project=project, args=checkout_args)
+    result = cli.run(project=project, args=checkout_args, binary_capture=True)
     result.assert_success()
 
     with open(tarball, 'wb') as f:

tests/frontend/yamlcache.py

@@ -103,7 +103,7 @@ def test_yamlcache_used(cli, tmpdir, ref_storage, with_junction, move_project):
         yc.put_from_key(prj, element_path, key, contents)
 
     # Show that a variable has been added
-    result = cli.run(project=project, args=['show', '--format', '%{vars}', 'test.bst'])
+    result = cli.run(project=project, args=['show', '--deps', 'none', '--format', '%{vars}', 'test.bst'])
     result.assert_success()
     data = yaml.safe_load(result.output)
     assert 'modified' in data
@@ -135,7 +135,7 @@ def test_yamlcache_changed_file(cli, tmpdir, ref_storage, with_junction):
         _yaml.load(element_path, copy_tree=False, project=prj, yaml_cache=yc)
 
     # Show that a variable has been added
-    result = cli.run(project=project, args=['show', '--format', '%{vars}', 'test.bst'])
+    result = cli.run(project=project, args=['show', '--deps', 'none', '--format', '%{vars}', 'test.bst'])
     result.assert_success()
     data = yaml.safe_load(result.output)
     assert 'modified' in data

tests/integration/build-tree.py

+import os
+import pytest
+import shutil
+
+from tests.testutils import cli, cli_integration, create_artifact_share
+from buildstream._exceptions import ErrorDomain
+
+
+pytestmark = pytest.mark.integration
+
+
+DATA_DIR = os.path.join(
+    os.path.dirname(os.path.realpath(__file__)),
+    "project"
+)
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_buildtree_staged(cli_integration, tmpdir, datafiles):
+    # i.e. tests that cached build trees are staged by `bst shell --build`
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    element_name = 'build-shell/buildtree.bst'
+
+    res = cli_integration.run(project=project, args=['build', element_name])
+    res.assert_success()
+
+    res = cli_integration.run(project=project, args=[
+        'shell', '--build', element_name, '--', 'grep', '-q', 'Hi', 'test'
+    ])
+    res.assert_success()
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_buildtree_from_failure(cli_integration, tmpdir, datafiles):
+    # i.e. test that on a build failure, we can still shell into it
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    element_name = 'build-shell/buildtree-fail.bst'
+
+    res = cli_integration.run(project=project, args=['build', element_name])
+    res.assert_task_error(ErrorDomain.ELEMENT, None)
+
+    # Assert that file has expected contents
+    res = cli_integration.run(project=project, args=[
+        'shell', '--build', element_name, '--', 'cat', 'test'
+    ])
+    res.assert_success()
+    assert 'Hi' in res.output
+
+
+# Check that build shells work when pulled from a remote cache
+# This is to roughly simulate remote execution
+@pytest.mark.datafiles(DATA_DIR)
+def test_buildtree_pulled(cli, tmpdir, datafiles):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    element_name = 'build-shell/buildtree.bst'
+
+    with create_artifact_share(os.path.join(str(tmpdir), 'artifactshare')) as share:
+        # Build the element to push it to cache
+        cli.configure({
+            'artifacts': {'url': share.repo, 'push': True}
+        })
+        result = cli.run(project=project, args=['build', element_name])
+        result.assert_success()
+        assert cli.get_element_state(project, element_name) == 'cached'
+
+        # Discard the cache
+        cli.configure({
+            'artifacts': {'url': share.repo, 'push': True},
+            'artifactdir': os.path.join(cli.directory, 'artifacts2')
+        })
+        assert cli.get_element_state(project, element_name) != 'cached'
+
+        # Pull from cache
+        result = cli.run(project=project, args=['pull', '--deps', 'all', element_name])
+        result.assert_success()
+
+        # Check it's using the cached build tree
+        res = cli.run(project=project, args=[
+            'shell', '--build', element_name, '--', 'grep', '-q', 'Hi', 'test'
+        ])
+        res.assert_success()

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

+kind: manual
+description: |
+  Puts a file in the build tree so that build tree caching and staging can be tested.
+
+depends:
+  - filename: base.bst
+    type: build
+
+config:
+  build-commands:
+    - "echo 'Hi' > %{build-root}/test"

tests/integration/shell.py

@@ -302,46 +302,33 @@ def test_workspace_visible(cli, tmpdir, datafiles):
     assert result.output == workspace_hello
 
 
-# Test that we can see the workspace files in a shell
-@pytest.mark.integration
+# Test that '--sysroot' works
 @pytest.mark.datafiles(DATA_DIR)
-def test_sysroot_workspace_visible(cli, tmpdir, datafiles):
+def test_sysroot(cli, tmpdir, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename)
-    workspace = os.path.join(cli.directory, 'workspace')
-    element_name = 'workspace/workspace-mount-fail.bst'
-
-    # Open a workspace on our build failing element
-    #
-    res = cli.run(project=project, args=['workspace', 'open', element_name, workspace])
-    assert res.exit_code == 0
-
-    # Ensure the dependencies of our build failing element are built
-    result = cli.run(project=project, args=['build', element_name])
-    result.assert_main_error(ErrorDomain.STREAM, None)
-
-    # Discover the sysroot of the failed build directory, after one
-    # failed build, there should be only one directory there.
-    #
-    build_base = os.path.join(cli.directory, 'build')
-    build_dirs = os.listdir(path=build_base)
-    assert len(build_dirs) == 1
-    build_dir = os.path.join(build_base, build_dirs[0])
-
-    # Obtain a copy of the hello.c content from the workspace
-    #
-    workspace_hello_path = os.path.join(cli.directory, 'workspace', 'hello.c')
-    assert os.path.exists(workspace_hello_path)
-    with open(workspace_hello_path, 'r') as f:
-        workspace_hello = f.read()
-
-    # Cat the hello.c file from a bst shell command, and assert
-    # that we got the same content here
-    #
-    result = cli.run(project=project, args=[
-        'shell', '--build', '--sysroot', build_dir, element_name, '--', 'cat', 'hello.c'
+    base_element = "base/base-alpine.bst"
+    # test element only needs to be something lightweight for this test
+    test_element = "script/script.bst"
+    checkout_dir = os.path.join(str(tmpdir), 'alpine-sysroot')
+    test_file = 'hello'
+
+    # Build and check out a sysroot
+    res = cli.run(project=project, args=['build', base_element])
+    res.assert_success()
+    res = cli.run(project=project, args=['checkout', base_element, checkout_dir])
+    res.assert_success()
+
+    # Mutate the sysroot
+    test_path = os.path.join(checkout_dir, test_file)
+    with open(test_path, 'w') as f:
+        f.write('hello\n')
+
+    # Shell into the sysroot and check the test file exists
+    res = cli.run(project=project, args=[
+        'shell', '--build', '--sysroot', checkout_dir, test_element, '--',
+        'grep', '-q', 'hello', '/' + test_file
     ])
-    assert result.exit_code == 0
-    assert result.output == workspace_hello
+    res.assert_success()
 
 
 # Test system integration commands can access devices in /dev

tests/testutils/runcli.py

@@ -17,7 +17,7 @@ import pytest
 # CliRunner convenience API (click.testing module) does not support
 # separation of stdout/stderr.
 #
-from _pytest.capture import MultiCapture, FDCapture
+from _pytest.capture import MultiCapture, FDCapture, FDCaptureBinary
 
 # Import the main cli entrypoint
 from buildstream._frontend import cli as bst_cli
@@ -234,9 +234,10 @@ class Cli():
     #    silent (bool): Whether to pass --no-verbose
     #    env (dict): Environment variables to temporarily set during the test
     #    args (list): A list of arguments to pass buildstream
+    #    binary_capture (bool): Whether to capture the stdout/stderr as binary
     #
     def run(self, configure=True, project=None, silent=False, env=None,
-            cwd=None, options=None, args=None):
+            cwd=None, options=None, args=None, binary_capture=False):
         if args is None:
             args = []
         if options is None:
@@ -278,7 +279,7 @@ class Cli():
             except ValueError:
                 sys.__stdout__ = open('/dev/stdout', 'w')
 
-            result = self.invoke(bst_cli, bst_args)
+            result = self.invoke(bst_cli, bst_args, binary_capture=binary_capture)
 
         # Some informative stdout we can observe when anything fails
         if self.verbose:
@@ -295,7 +296,7 @@ class Cli():
 
         return result
 
-    def invoke(self, cli, args=None, color=False, **extra):
+    def invoke(self, cli, args=None, color=False, binary_capture=False, **extra):
         exc_info = None
         exception = None
         exit_code = 0
@@ -305,8 +306,8 @@ class Cli():
         old_stdin = sys.stdin
         with open(os.devnull) as devnull:
             sys.stdin = devnull
-
-            capture = MultiCapture(out=True, err=True, in_=False, Capture=FDCapture)
+            capture_kind = FDCaptureBinary if binary_capture else FDCapture
+            capture = MultiCapture(out=True, err=True, in_=False, Capture=capture_kind)
             capture.start_capturing()
 
             try: