[Notes] [Git][BuildStream/buildstream][richardmaw/distinguish-sandboxing

richardmaw-codethink pushed to branch richardmaw/distinguish-sandboxing-build-fail at BuildStream / buildstream

Commits:

350c6796

by Tristan Van Berkom at 2018-10-08T10:44:47Z

CONTRIBUTING.rst: Added section in PEP-8 coding style about line lengths.

We have a long line limit in order to handle the exceptions without making
code unreadable as a result, this long line length limit is not an invitation
to fill up the limit as much as possible.

3ca487b8

by Chandan Singh at 2018-10-08T18:25:39Z

bst-docker-import: Consistently use stderr for all logs

In !857, we added `contrib/bst-docker-import` script but it has a small
issue that some of logs go to stdout while others go to stderr. Fix it
so that all logging is done on stderr.

49df3d75

by Chandan Singh at 2018-10-08T18:58:54Z

Merge branch 'chandan/fix-bst-docker-import-logging' into 'master'

bst-docker-import: Consistently use stderr for all logs

See merge request BuildStream/buildstream!863

d827dfae
by Angelos Evripiotis at 2018-10-09T09:12:45Z
```
contributing.rst: end lines with punctuation
```

9590e8ae

by Angelos Evripiotis at 2018-10-09T09:12:45Z

contributing.rst: fix whitespace

No tabs, no lines of only horizontal whitespace.

6d02e269
by Angelos Evripiotis at 2018-10-09T09:12:45Z
```
contributing.rst: fix typo of 'get_count'
```

3ed26a47

by Angelos Evripiotis at 2018-10-09T09:12:45Z

contributing.rst: de-paren subclass example

It seems easier to read when separated into two sentences.

5b22d850
by Angelos Evripiotis at 2018-10-09T09:12:45Z
```
contributing.rst: no spaces before '?'
```

ac0776f8

by Tristan Van Berkom at 2018-10-09T09:36:33Z

Merge branch 'aevri/contributing_fixups' into 'master'

Minor fixups to contributing.rst

See merge request BuildStream/buildstream!866

885bd946

by Tristan Van Berkom at 2018-10-09T09:39:24Z

CONTRIBUTING.rst: Added note about sphinx supporting docstrings on instance variables

Python does not natively support this, but sphinx does parse them and includes
these in the generated documentation.

120d8c73

by Tristan Van Berkom at 2018-10-09T09:45:23Z

CONTRIBUTING.rst: Fix typos in previous commit.

Ooops, that was done far too quickly.

0e11a8d0

by Richard Maw at 2018-10-09T16:23:25Z

buildstream/sandbox/_sandboxbwrap.py: Distinguish sandbox failure from command failure

If `bwrap` fails to set up the sandbox and start the payload command
it won't write an exit-code in --json-status-fd,
so we can report if it was a sandboxing failure if we don't get exit-code status
and a payload command failure if we do and it's non-zero.

Closes https://gitlab.com/BuildStream/buildstream/issues/286

e93d7112

by Richard Maw at 2018-10-09T16:23:25Z

tests/testutils/site.py: Check for bwrap supporting --json-status-fd

d850d1ee

by Richard Maw at 2018-10-09T16:23:25Z

tests/integration/sandbox-bwrap.py: Test distinguishing sandbox exit code from command

Changes:

CONTRIBUTING.rst

@@ -14,7 +14,7 @@ if no issue already exists.
  For policies on how to submit an issue and how to use our project labels,
  we recommend that you read the `policies guide
 -<https://gitlab.com/BuildStream/nosoftware/alignment/blob/master/BuildStream_policies.md>`_
 +<https://gitlab.com/BuildStream/nosoftware/alignment/blob/master/BuildStream_policies.md>`_.
  .. _contributing_fixing_bugs:
@@ -192,6 +192,21 @@ code readability by being overly restrictive on line length for instance.
  The pep8 linter will run automatically when :ref:`running the test suite <contributing_testing>`.
 +Line lengths
 +''''''''''''
 +Regarding laxness on the line length in our linter settings, it should be clarified
 +that the line length limit is a hard limit which causes the linter to bail out
 +and reject commits which break the high limit - not an invitation to write exceedingly
 +long lines of code, comments, or API documenting docstrings.
++
 +Code, comments and docstrings should strive to remain written for approximately 80
 +or 90 character lines, where exceptions can be made when code would be less readable
 +when exceeding 80 or 90 characters (often this happens in conditional statements
 +when raising an exception, for example). Or, when comments contain a long link that
 +causes the given line to to exceed 80 or 90 characters, we don't want this to cause
 +the linter to refuse the commit.
++
++
  .. _contributing_documenting_symbols:
  Documenting symbols
@@ -255,6 +270,11 @@ comments and docstrings.
      *Since: 1.2*
      """
 +.. note::
++
 +   Python does not support docstrings on instance variables, but sphinx does
 +   pick them up and includes them in the generated documentation.
++
  **Internal instance variable**::
    def __init__(self, context, element):
@@ -389,7 +409,7 @@ on a Python class in BuildStream::
           # Implementation of the "frobbish" abstract method
           # defined by the parent Bar class.
+          #
 -	 return True
 +         return True
        ################################################
        #                 Public Methods               #
@@ -430,7 +450,7 @@ on a Python class in BuildStream::
        # Returns:
        #    (int): The count of this foo
+       #
 -      def set_count(self, count):
 +      def get_count(self, count):
            return self._count
@@ -444,7 +464,7 @@ on a Python class in BuildStream::
        #       Even though these are private implementation
        #       details, they still MUST have API documenting
        #       comments on them.
+-
++
        # _do_frobbing()
+       #
        # Does the actual frobbing
@@ -479,10 +499,10 @@ reference on how the PEP-8 defines public and non-public.
    A private symbol must be denoted by a leading underscore.
 -* When a class can have subclasses (for example, the ``Sandbox`` or ``Platform``
 +* When a class can have subclasses, then private symbols should be denoted
 +  by two leading underscores. For example, the ``Sandbox`` or ``Platform``
    classes which have various implementations, or the ``Element`` and ``Source``
 -  classes which plugins derive from), then private symbols should be denoted
 -  by two leading underscores.
 +  classes which plugins derive from.
    The double leading underscore naming convention invokes Python's name
    mangling algorithm which helps prevent namespace collisions in the case
@@ -521,7 +541,7 @@ In order to disambiguate between:
  * Symbols which are publicly accessible details of the ``Element`` class, can
    be accessed by BuildStream internals, but must remain hidden from the
 -  *"Public API Surface"*
 +  *"Public API Surface"*.
  * Symbols which are private to the ``Element`` class, and cannot be accessed
    from outside of the ``Element`` class at all.
@@ -571,7 +591,7 @@ is found at ``_artifactcache/artifactcache.py``.
  Imports
  ~~~~~~~
 -Module imports inside BuildStream are done with relative ``.`` notation
 +Module imports inside BuildStream are done with relative ``.`` notation:
  **Good**::
@@ -613,12 +633,12 @@ which exposes an instance variable is the only one in control of the value of th
  variable.
  * If an instance variable is public and must be modified; then it must be
 -  modified using a :ref:`mutator <contributing_accessor_mutator>`
 +  modified using a :ref:`mutator <contributing_accessor_mutator>`.
  * Ideally for better encapsulation, all object state is declared as
    :ref:`private instance variables <contributing_public_and_private>` and can
    only be accessed by external classes via public :ref:`accessors and mutators
 -  <contributing_accessor_mutator>`
 +  <contributing_accessor_mutator>`.
  .. note::
@@ -705,10 +725,10 @@ In BuildStream, we use the term *"Abstract Method"*, to refer to
  a method which **can** be overridden by a subclass, whereas it
  is **illegal** to override any other method.
 -* Abstract methods are allowed to have default implementations
 +* Abstract methods are allowed to have default implementations.
  * Subclasses are not allowed to redefine the calling signature
 -  of an abstract method, or redefine the API contract in any way
 +  of an abstract method, or redefine the API contract in any way.
  * Subclasses are not allowed to override any other methods.
@@ -783,7 +803,7 @@ BstError parameters
  When raising ``BstError`` class exceptions, there are some common properties
  which can be useful to know about:
 -* **message:** The brief human readable error, will be formatted on one line in the frontend
 +* **message:** The brief human readable error, will be formatted on one line in the frontend.
  * **detail:** An optional detailed human readable message to accompany the **message** summary
    of the error. This is often used to recommend the user some course of action, or to provide
@@ -959,9 +979,9 @@ symbols to a minimum, this is important for both
  When anyone visits a file, there are two levels of comprehension:
 -* What do I need to know in order to *use* this object
 +* What do I need to know in order to *use* this object.
 -* What do I need to know in order to *modify* this object
 +* What do I need to know in order to *modify* this object.
  For the former, we want the reader to understand with as little effort
  as possible, what the public API contract is for a given object and consequently,
@@ -986,9 +1006,9 @@ well documented and minimal.
  When adding new API to a given object for a new purpose, consider whether
  the new API is in any way redundant with other API (should this value now
 -go into the constructor, since we use it more than once ? could this
 +go into the constructor, since we use it more than once? could this
  value be passed along with another function, and the other function renamed,
 -to better suit the new purposes of this module/object ?) and repurpose
 +to better suit the new purposes of this module/object?) and repurpose
  the outward facing API of an object as a whole every time.
@@ -1168,7 +1188,7 @@ The BuildStream documentation style is as follows:
  * Cross references should be of the form ``:role:`target```.
    * Explicit anchors can be declared as ``.. _anchor_name:`` on a line by itself.
+-
++
    * To cross reference arbitrary locations with, for example, the anchor ``anchor_name``,
      always provide some explicit text in the link instead of deriving the text from
      the target, e.g.: ``:ref:`Link text <anchor_name>```.
@@ -1251,23 +1271,23 @@ Documentation Examples
  The examples section of the documentation contains a series of standalone
  examples, here are the criteria for an example addition.
 -* The example has a ``${name}``
 +* The example has a ``${name}``.
 -* The example has a project users can copy and use
 +* The example has a project users can copy and use.
 -  * This project is added in the directory ``doc/examples/${name}``
 +  * This project is added in the directory ``doc/examples/${name}``.
 -* The example has a documentation component
 +* The example has a documentation component.
    * This is added at ``doc/source/examples/${name}.rst``
    * A reference to ``examples/${name}`` is added to the toctree in ``doc/source/examples.rst``
    * This documentation discusses the project elements declared in the project and may
 -    provide some BuildStream command examples
 -  * This documentation links out to the reference manual at every opportunity
 +    provide some BuildStream command examples.
 +  * This documentation links out to the reference manual at every opportunity.
 -* The example has a CI test component
 +* The example has a CI test component.
 -  * This is an integration test added at ``tests/examples/${name}``
 +  * This is an integration test added at ``tests/examples/${name}``.
    * This test runs BuildStream in the ways described in the example
      and assert that we get the results which we advertize to users in
      the said examples.
@@ -1294,17 +1314,17 @@ The ``.run`` file format is just another YAML dictionary which consists of a
  Each *command* is a dictionary, the members of which are listed here:
 -* ``directory``: The input file relative project directory
 +* ``directory``: The input file relative project directory.
 -* ``output``: The input file relative output html file to generate (optional)
 +* ``output``: The input file relative output html file to generate (optional).
  * ``fake-output``: Don't really run the command, just pretend to and pretend
    this was the output, an empty string will enable this too.
 -* ``command``: The command to run, without the leading ``bst``
 +* ``command``: The command to run, without the leading ``bst``.
  * ``shell``: Specifying ``True`` indicates that ``command`` should be run as
 -  a shell command from the project directory, instead of a bst command (optional)
 +  a shell command from the project directory, instead of a bst command (optional).
  When adding a new ``.run`` file, one should normally also commit the new
  resulting generated ``.html`` file(s) into the ``doc/source/sessions-stored/``
@@ -1402,7 +1422,7 @@ a subdirectory beside your test in which to store data.
  When creating a test that needs data, use the datafiles extension
  to decorate your test case (again, examples exist in the existing
  tests for this), documentation on the datafiles extension can
 -be found here: https://pypi.python.org/pypi/pytest-datafiles
 +be found here: https://pypi.python.org/pypi/pytest-datafiles.
  Tests that run a sandbox should be decorated with::

buildstream/_platform/linux.py

@@ -44,6 +44,7 @@ class Linux(Platform):
          self._local_sandbox_available = self._have_fuse and self._have_good_bwrap
          self._die_with_parent_available = _site.check_bwrap_version(0, 1, 8)
 +        self._json_status_available = _site.check_bwrap_version(0, 3, 2)
          if self._local_sandbox_available:
              self._user_ns_available = self._check_user_ns_available()
@@ -91,6 +92,7 @@ class Linux(Platform):
          # Inform the bubblewrap sandbox as to whether it can use user namespaces or not
          kwargs['user_ns_available'] = self._user_ns_available
          kwargs['die_with_parent_available'] = self._die_with_parent_available
 +        kwargs['json_status_available'] = self._json_status_available
          return SandboxBwrap(*args, **kwargs)
      def _check_user_ns_available(self):

buildstream/sandbox/_sandboxbwrap.py

@@ -17,6 +17,8 @@
  #  Authors:
  #        Andrew Leeming <andrew leeming codethink co uk>
  #        Tristan Van Berkom <tristan vanberkom codethink co uk>
 +import collections
 +import json
  import os
  import sys
  import time
@@ -24,7 +26,8 @@ import errno
  import signal
  import subprocess
  import shutil
 -from contextlib import ExitStack
 +from contextlib import ExitStack, suppress
 +from tempfile import TemporaryFile
  import psutil
@@ -53,6 +56,7 @@ class SandboxBwrap(Sandbox):
          super().__init__(*args, **kwargs)
          self.user_ns_available = kwargs['user_ns_available']
          self.die_with_parent_available = kwargs['die_with_parent_available']
 +        self.json_status_available = kwargs['json_status_available']
      def run(self, command, flags, *, cwd=None, env=None):
          stdout, stderr = self._get_output()
@@ -159,24 +163,31 @@ class SandboxBwrap(Sandbox):
                  gid = self._get_config().build_gid
                  bwrap_command += ['--uid', str(uid), '--gid', str(gid)]
 -        # Add the command
 -        bwrap_command += command
+-
 -        # bwrap might create some directories while being suid
 -        # and may give them to root gid, if it does, we'll want
 -        # to clean them up after, so record what we already had
 -        # there just in case so that we can safely cleanup the debris.
 -        #
 -        existing_basedirs = {
 -            directory: os.path.exists(os.path.join(root_directory, directory))
 -            for directory in ['tmp', 'dev', 'proc']
 -        }
+-
 -        # Use the MountMap context manager to ensure that any redirected
 -        # mounts through fuse layers are in context and ready for bwrap
 -        # to mount them from.
 -        #
          with ExitStack() as stack:
 +            pass_fds = ()
 +            # Improve error reporting with json-status if available
 +            if self.json_status_available:
 +                json_status_file = stack.enter_context(TemporaryFile())
 +                pass_fds = (json_status_file.fileno(),)
 +                bwrap_command += ['--json-status-fd', str(json_status_file.fileno())]
++
 +            # Add the command
 +            bwrap_command += command
++
 +            # bwrap might create some directories while being suid
 +            # and may give them to root gid, if it does, we'll want
 +            # to clean them up after, so record what we already had
 +            # there just in case so that we can safely cleanup the debris.
 +            #
 +            existing_basedirs = {
 +                directory: os.path.exists(os.path.join(root_directory, directory))
 +                for directory in ['tmp', 'dev', 'proc']
 +            }
++
 +            # Use the MountMap context manager to ensure that any redirected
 +            # mounts through fuse layers are in context and ready for bwrap
 +            # to mount them from.
 +            #
              stack.enter_context(mount_map.mounted(self))
              # Ensure the cwd exists
@@ -194,7 +205,7 @@ class SandboxBwrap(Sandbox):
              # Run bubblewrap !
              exit_code = self.run_bwrap(bwrap_command, stdin, stdout, stderr,
 -                                       (flags & SandboxFlags.INTERACTIVE))
 +                                       (flags & SandboxFlags.INTERACTIVE), pass_fds)
              # Cleanup things which bwrap might have left behind, while
              # everything is still mounted because bwrap can be creating
@@ -242,10 +253,24 @@ class SandboxBwrap(Sandbox):
                          # a bug, bwrap mounted a tempfs here and when it exits, that better be empty.
                          pass
 +            if self.json_status_available:
 +                json_status_file.seek(0, 0)
 +                child_exit_code = None
 +                for line in json_status_file:
 +                    with suppress(json.decoder.JSONDecodeError):
 +                        o = json.loads(line)
 +                        if isinstance(o, collections.abc.Mapping) and 'exit-code' in o:
 +                            child_exit_code = o['exit-code']
 +                            break
 +                if child_exit_code is None:
 +                    raise SandboxError("`bwrap' terminated during sandbox setup with exitcode {}".format(exit_code),
 +                                       reason="bwrap-sandbox-fail")
 +                exit_code = child_exit_code
++
          self._vdir._mark_changed()
          return exit_code
 -    def run_bwrap(self, argv, stdin, stdout, stderr, interactive):
 +    def run_bwrap(self, argv, stdin, stdout, stderr, interactive, pass_fds):
          # Wrapper around subprocess.Popen() with common settings.
+         #
          # This function blocks until the subprocess has terminated.
@@ -321,6 +346,7 @@ class SandboxBwrap(Sandbox):
                  # The default is to share file descriptors from the parent process
                  # to the subprocess, which is rarely good for sandboxing.
                  close_fds=True,
 +                pass_fds=pass_fds,
                  stdin=stdin,
                  stdout=stdout,
                  stderr=stderr,

contrib/bst-docker-import

@@ -93,10 +93,10 @@ echo "INFO: Checking out $element ..." >&2
  $bst_cmd checkout --tar "$element" "$checkout_tar" || die "Failed to checkout $element"
  echo "INFO: Successfully checked out $element" >&2
 -echo "INFO: Importing Docker image ..."
 +echo "INFO: Importing Docker image ..." >&2
  "${docker_import_cmd[@]}" "$checkout_tar" "$docker_image_tag" || die "Failed to import Docker image from tarball"
 -echo "INFO: Successfully import Docker image $docker_image_tag"
 +echo "INFO: Successfully import Docker image $docker_image_tag" >&2
 -echo "INFO: Cleaning up ..."
 +echo "INFO: Cleaning up ..." >&2
  rm "$checkout_tar" || die "Failed to remove $checkout_tar"
 -echo "INFO: Clean up finished"
 +echo "INFO: Clean up finished" >&2

tests/integration/sandbox-bwrap.py

  import os
  import pytest
 +from buildstream._exceptions import ErrorDomain
++
  from tests.testutils import cli_integration as cli
  from tests.testutils.integration import assert_contains
 -from tests.testutils.site import HAVE_BWRAP
 +from tests.testutils.site import HAVE_BWRAP, HAVE_BWRAP_JSON_STATUS
  pytestmark = pytest.mark.integration
@@ -29,3 +31,32 @@ def test_sandbox_bwrap_cleanup_build(cli, tmpdir, datafiles):
      # Here, BuildStream should not attempt any rmdir etc.
      result = cli.run(project=project, args=['build', element_name])
      assert result.exit_code == 0
++
++
 +@pytest.mark.skipif(not HAVE_BWRAP, reason='Only available with bubblewrap')
 +@pytest.mark.skipif(not HAVE_BWRAP_JSON_STATUS, reason='Only available with bubblewrap supporting --json-status-fd')
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_sandbox_bwrap_distinguish_setup_error(cli, tmpdir, datafiles):
 +    project = os.path.join(datafiles.dirname, datafiles.basename)
 +    element_name = 'sandbox-bwrap/non-executable-shell.bst'
++
 +    result = cli.run(project=project, args=['build', element_name])
 +    result.assert_task_error(error_domain=ErrorDomain.SANDBOX, error_reason="bwrap-sandbox-fail")
++
++
 +@pytest.mark.integration
 +@pytest.mark.skipif(not HAVE_BWRAP, reason='Only available with bubblewrap')
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_sandbox_bwrap_return_subprocess(cli, tmpdir, datafiles):
 +    project = os.path.join(datafiles.dirname, datafiles.basename)
 +    element_name = 'sandbox-bwrap/command-exit-42.bst'
++
 +    cli.configure({
 +        "logging": {
 +            "message-format": "%{element}|%{message}",
 +        },
 +    })
++
 +    result = cli.run(project=project, args=['build', element_name])
 +    result.assert_task_error(error_domain=ErrorDomain.ELEMENT, error_reason=None)
 +    assert "sandbox-bwrap/command-exit-42.bst|Command 'exit 42' failed with exitcode 42" in result.stderr

tests/testutils/site.py

@@ -4,7 +4,7 @@
  import os
  import sys
 -from buildstream import utils, ProgramNotFoundError
 +from buildstream import _site, utils, ProgramNotFoundError
  try:
      utils.get_host_tool('bzr')
@@ -33,8 +33,10 @@ except (ImportError, ValueError):
  try:
      utils.get_host_tool('bwrap')
      HAVE_BWRAP = True
 +    HAVE_BWRAP_JSON_STATUS = _site.check_bwrap_version(0, 3, 2)
  except ProgramNotFoundError:
      HAVE_BWRAP = False
 +    HAVE_BWRAP_JSON_STATUS = False
  try:
      utils.get_host_tool('lzip')

[Notes] [Git][BuildStream/buildstream][richardmaw/distinguish-sandboxing-build-fail] 14 commits: CONTRIBUTING.rst: Added section in PEP-8 coding style about line lengths.

richardmaw-codethink pushed to branch richardmaw/distinguish-sandboxing-build-fail at BuildStream / buildstream

Commits:

6 changed files:

Changes: