[Notes] [Git][BuildStream/buildstream][jonathan/debug-remote-failed-buil

Jonathan Maw pushed to branch jonathan/debug-remote-failed-builds at BuildStream / buildstream

Commits:

f1aa0974

by Javier Jardón at 2018-10-18T14:13:35Z

Fix python warnings: Use collections.abc instead collections

Since python 3.3, collections has been moved to collections.abc module.
For backwards compatibility, they continue to be visible in this module through Python 3.7.
Subsequently, they will be removed entirely.

See https://docs.python.org/3/library/collections.html

f1550c8e

by Valentin David at 2018-10-18T16:02:45Z

Merge branch 'jjardon/warnings_collections' into 'master'

Fix python warnings: Use collections.abc instead collections

See merge request BuildStream/buildstream!883

89992cb1

by Jonathan Maw at 2018-10-19T11:50:52Z

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

95c1956d

by Jonathan Maw at 2018-10-19T11:50:53Z

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.
* Always allow producing a failure shell.

Changes in element.py are:
* Errors caused by building don't store the failed build sysroot.
* When staging sources, will stage the element's cached build tree if it
  exists.

This fixes part of #539

07081b6a

by Jonathan Maw at 2018-10-19T11:50:53Z

NEWS: Add item for cached buildtree behaviour

c5a25f8e

by Jonathan Maw at 2018-10-19T11:50:53Z

tests: Add test that cached build trees are staged in build shells

This is related to #539

e6482060

by Jonathan Maw at 2018-10-19T11:51:26Z

tests: Add a test for the interactive failure shell

This test checks that the interactive failure shell stages the cached
build tree.

75ebc1b6

by Jonathan Maw at 2018-10-19T11:51:30Z

sandbox.py: Remove redundant Sandbox.__directory

dcf14ebb

by Jonathan Maw at 2018-10-19T11:51:30Z

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

88348218

by Jonathan Maw at 2018-10-19T11:51:30Z

NEWS: Add item for bst shell --sysroot changes

d9fecdb4

by Jonathan Maw at 2018-10-19T11:51:30Z

tests: Add tests for 'bst shell --sysroot'

This is related to #539

20 changed files:

NEWS
buildstream/_artifactcache/artifactcache.py
buildstream/_context.py
buildstream/_frontend/app.py
buildstream/_includes.py
buildstream/_loader/loadelement.py
buildstream/_loader/loader.py
buildstream/_options/optionpool.py
buildstream/_project.py
buildstream/element.py
buildstream/plugins/elements/junction.py
buildstream/plugins/sources/git.py
buildstream/sandbox/_mount.py
buildstream/sandbox/sandbox.py
buildstream/source.py
dev-requirements.txt
+ tests/integration/build-tree.py
+ tests/integration/project/elements/build-shell/buildtree.bst
tests/integration/shell.py
tests/yaml/yaml.py

Changes:

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, and *possible* to debug remote 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/_artifactcache/artifactcache.py

@@ -19,7 +19,8 @@
  import os
  import string
 -from collections import Mapping, namedtuple
 +from collections import namedtuple
 +from collections.abc import Mapping
  from ..types import _KeyStrength
  from .._exceptions import ArtifactError, ImplError, LoadError, LoadErrorReason

buildstream/_context.py

@@ -19,7 +19,8 @@
  import os
  import datetime
 -from collections import deque, Mapping
 +from collections import deque
 +from collections.abc import Mapping
  from contextlib import contextmanager
  from . import utils
  from . import _cachekey

buildstream/_frontend/app.py

@@ -564,18 +564,15 @@ class App():
                         "  (c)ontinue  - Continue queueing jobs as much as possible\n" +
                         "  (q)uit      - Exit after all ongoing jobs complete\n" +
                         "  (t)erminate - Terminate any ongoing jobs and exit\n" +
 -                       "  (r)etry     - Retry this job\n")
 +                       "  (r)etry     - Retry this job\n" +
 +                       "  (s)hell     - Drop into a shell in the failed build sandbox\n")
              if failure.logfile:
                  summary += "  (l)og       - View the full log file\n"
 -            if failure.sandbox:
 -                summary += "  (s)hell     - Drop into a shell in the failed build sandbox\n"
              summary += "\nPressing ^C will terminate jobs and exit\n"
 -            choices = ['continue', 'quit', 'terminate', 'retry']
 +            choices = ['continue', 'quit', 'terminate', 'retry', 'shell']
              if failure.logfile:
                  choices += ['log']
 -            if failure.sandbox:
 -                choices += ['shell']
              choice = ''
              while choice not in ['continue', 'quit', 'terminate', 'retry']:
@@ -595,10 +592,10 @@ class App():
                  # Handle choices which you can come back from
+                 #
                  if choice == 'shell':
 -                    click.echo("\nDropping into an interactive shell in the failed build sandbox\n", err=True)
 +                    click.echo("\nDropping into an interactive shell in the failed build tree\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/_includes.py

  import os
 -from collections import Mapping
 +from collections.abc import Mapping
  from . import _yaml
  from ._exceptions import LoadError, LoadErrorReason

buildstream/_loader/loadelement.py

@@ -18,7 +18,7 @@
  #        Tristan Van Berkom <tristan vanberkom codethink co uk>
  # System imports
 -from collections import Mapping
 +from collections.abc import Mapping
  # BuildStream toplevel imports
  from .._exceptions import LoadError, LoadErrorReason

buildstream/_loader/loader.py

@@ -19,7 +19,8 @@
  import os
  from functools import cmp_to_key
 -from collections import Mapping, namedtuple
 +from collections import namedtuple
 +from collections.abc import Mapping
  import tempfile
  import shutil

buildstream/_options/optionpool.py

@@ -18,7 +18,7 @@
  #        Tristan Van Berkom <tristan vanberkom codethink co uk>
+ #
 -from collections import Mapping
 +from collections.abc import Mapping
  import jinja2
  from .. import _yaml

buildstream/_project.py

@@ -19,7 +19,8 @@
  #        Tiago Gomes <tiago gomes codethink co uk>
  import os
 -from collections import Mapping, OrderedDict
 +from collections import OrderedDict
 +from collections.abc import Mapping
  from pluginbase import PluginBase
  from . import utils
  from . import _cachekey

buildstream/element.py

@@ -76,7 +76,8 @@ import os
  import re
  import stat
  import copy
 -from collections import Mapping, OrderedDict
 +from collections import OrderedDict
 +from collections.abc import Mapping
  from contextlib import contextmanager
  import tempfile
  import shutil
@@ -1317,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=True if directory else False) as sandbox:
              # Configure always comes first, and we need it.
              self.configure_sandbox(sandbox)
@@ -1384,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()
@@ -1394,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
@@ -1565,10 +1573,6 @@ class Element(Plugin):
                      collect = self.assemble(sandbox)
                      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
+-
                      # If there is a workspace open on this element, it will have
                      # been mounted for sandbox invocations instead of being staged.
+                     #
@@ -1682,8 +1686,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
@@ -2151,12 +2155,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()
@@ -2187,6 +2193,7 @@ class Element(Plugin):
                                                stdout=stdout,
                                                stderr=stderr,
                                                config=config,
 +                                              bare_directory=bare_directory,
                                                allow_real_directory=not self.BST_VIRTUAL_DIRECTORY)
              yield sandbox
@@ -2196,7 +2203,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/plugins/elements/junction.py

@@ -124,7 +124,7 @@ the user to resolve possibly conflicting nested junctions by creating a junction
  with the same name in the top-level project, which then takes precedence.
  """
 -from collections import Mapping
 +from collections.abc import Mapping
  from buildstream import Element
  from buildstream._pipeline import PipelineError

buildstream/plugins/sources/git.py

@@ -89,7 +89,7 @@ import os
  import errno
  import re
  import shutil
 -from collections import Mapping
 +from collections.abc import Mapping
  from io import StringIO
  from configparser import RawConfigParser

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={}):
 -        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/source.py

@@ -155,7 +155,7 @@ Class Reference
  """
  import os
 -from collections import Mapping
 +from collections.abc import Mapping
  from contextlib import contextmanager
  from . import Plugin, Consistency

dev-requirements.txt

@@ -9,3 +9,4 @@ pytest-pep8
  pytest-pylint
  pytest-xdist
  pytest-timeout
 +pexpect

tests/integration/build-tree.py

 +import os
 +import pytest
 +import shutil
 +import pexpect
++
 +from tests.testutils import cli, cli_integration, create_artifact_share
++
++
 +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()
++
++
 +# Check that a failed build causes a shell that stages the buildtree
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_buildtree_staged_interactive(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-fail.bst'
++
 +    # Start an interactive session and intercept the interactive prompt
 +    child = pexpect.spawn('bst', ['--no-colors', 'build', element_name], cwd=project)
++
 +    # Ensure starting a shell is a valid option
 +    child.expect('\(s\)hell')
++
 +    # Wait for the interactive prompt
 +    child.expect('Choice: \[continue\]:')
++
 +    # Start a shell and wait for the shell's prompt
 +    child.sendline('s')
 +    child.expect('\]\$')
++
 +    # Read the contents of the file we added
 +    child.sendline('cat test')
 +    child.expect('Hi')
++
++
 +# 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/yaml/yaml.py

  import os
  import pytest
  import tempfile
 -from collections import Mapping
 +from collections.abc import Mapping
  from buildstream import _yaml
  from buildstream._exceptions import LoadError, LoadErrorReason

[Notes] [Git][BuildStream/buildstream][jonathan/debug-remote-failed-builds] 11 commits: Fix python warnings: Use collections.abc instead collections

Jonathan Maw pushed to branch jonathan/debug-remote-failed-builds at BuildStream / buildstream

Commits:

20 changed files:

Changes: