[Notes] [Git][BuildStream/buildstream][Qinusty/526-fail-on-warnings] 13

Qinusty pushed to branch Qinusty/526-fail-on-warnings at BuildStream / buildstream

Commits:

2d061173

by Javier Jardón at 2018-08-09T16:31:15Z

buildstream/_project.py:

Multiprocessing with n_jobs given by multiprocessing.cpu_count() is not optimal for
systems where not all CPU can be used (in particular, Docker, CI etc).

As suggested in the multiprocessing docs
(http://docs.python.org/3/library/multiprocessing.html#multiprocessing.cpu_count)
using len(os.sched_getaffinity(0)) might be better.

4e1488ee

by Javier Jardón at 2018-08-09T16:31:15Z

buildstream/_project.py: Restrict max-jobs

even if the machine have mores cores available

Patch taken from YBD: https://gitlab.com/baserock/ybd/blob/master/ybd/app.py#L227

de955834

by Javier Jardón at 2018-08-09T17:22:24Z

Merge branch 'jjardon/max-jobs' into 'master'

buildstream/_project.py: Restrict max-jobs

See merge request BuildStream/buildstream!620

97595585
by Jim MacArthur at 2018-08-09T17:35:19Z
```
setup.py: Pin ruamel.yaml version to <= 0.15
```

a602365c

by Tristan Van Berkom at 2018-08-09T20:03:52Z

Merge branch 'jmac/pin-ruamel-version' into 'master'

Pin ruamel.yaml version to <= 0.15

See merge request BuildStream/buildstream!630

db0478ab

by Phillip Smyth at 2018-08-10T10:17:47Z

Implementing relative workspaces

This fixes #191

A note has been added to NEWS explaining backwards
compatibility issues

04cee9a9

by Phillip Smyth at 2018-08-10T11:12:31Z

Merge branch 'relative_workspaces' into 'master'

Patch for issue #191 support relative workspaces

Closes #191

See merge request BuildStream/buildstream!504

5dcecbad

by Valentin David at 2018-08-10T12:18:55Z

Set environment in bwrap command line instead of its environment

Fixes #498

2e8db54e

by Valentin David at 2018-08-10T12:53:19Z

Merge branch 'valentindavid/498_bwrap_environment' into 'master'

Set environment in bwrap command line instead of its environment

Closes #498

See merge request BuildStream/buildstream!565

abe49b10

by Josh Smith at 2018-08-10T13:04:13Z

_project.py: Add fatal-warnings configuration item

This allows for users to configure fatal-warnings to be either a list of
warnings or simply True to consider all warnings as errors.

This commit deprecates the use of fail-on-overlap within project.conf,
this will now use the fatal-warnings configuration item.

tests: This modifys the tests/frontend/overlaps.py tests to support the
new fatal-warnings configuration. Backwards compatibility is also
tested for `fail-on-overlap`

_versions.py: BST_FORMAT_VERSION bumped to 12 for fatal-warnings

Fixes: #526

37334d1d

by Josh Smith at 2018-08-10T13:04:13Z

git.py: Add configurable warning for inconsistent-submodule

This follows the implementation of configurable warnings.

e3be4699

by Josh Smith at 2018-08-10T13:04:13Z

docs: Add documentation for Configurable Warnings

This includes detailing the use of `fatal-warnings` within project.conf

This also adds a deprecation notice to the fail on overlaps section.

3eb2bf6f

by Josh Smith at 2018-08-10T13:04:13Z

tests: Add tests for configurable warnings

This adds multiple tests for custom plugin warnings and core warnings,
providing checks for both cases which should cause warnings and errors
when configured as fatal.

23 changed files:

NEWS
buildstream/_frontend/widget.py
buildstream/_project.py
buildstream/_stream.py
buildstream/_versions.py
buildstream/_workspaces.py
buildstream/data/projectconfig.yaml
buildstream/element.py
buildstream/plugin.py
buildstream/plugins/sources/git.py
buildstream/sandbox/_sandboxbwrap.py
doc/source/format_project.rst
setup.py
+ tests/frontend/configurable_warnings.py
+ tests/frontend/configuredwarning/elements/corewarn.bst
+ tests/frontend/configuredwarning/elements/warninga.bst
+ tests/frontend/configuredwarning/elements/warningb.bst
+ tests/frontend/configuredwarning/plugins/corewarn.py
+ tests/frontend/configuredwarning/plugins/warninga.py
+ tests/frontend/configuredwarning/plugins/warningb.py
+ tests/frontend/configuredwarning/project.conf
tests/frontend/overlaps.py
tests/frontend/workspace.py

Changes:

NEWS

@@ -17,6 +17,10 @@ buildstream 1.1.5
      to not be scheduled and fail during artifact assembly,
      and display the retry prompt during an interactive session.
 +  o Due to enabling the use of relative workspaces, "Legacy" workspaces
 +    may need to be closed and remade before the changes will affect them.
 +    Downgrading after using this feature may result in workspaces
 +    not functioning correctly
  =================
  buildstream 1.1.4

buildstream/_frontend/widget.py

@@ -418,7 +418,9 @@ class LogLine(Widget):
              if "%{workspace-dirs" in format_:
                  workspace = element._get_workspace()
                  if workspace is not None:
 -                    path = workspace.path.replace(os.getenv('HOME', '/root'), '~')
 +                    path = workspace.get_absolute_path()
 +                    if path.startswith("~/"):
 +                        path = os.path.join(os.getenv('HOME', '/root'), path[2:])
                      line = p.fmt_subst(line, 'workspace-dirs', "Workspace: {}".format(path))
                  else:
                      line = p.fmt_subst(

buildstream/_project.py

@@ -19,7 +19,6 @@
  #        Tiago Gomes <tiago gomes codethink co uk>
  import os
 -import multiprocessing  # for cpu_count()
  from collections import Mapping, OrderedDict
  from pluginbase import PluginBase
  from . import utils
@@ -32,6 +31,7 @@ from ._options import OptionPool
  from ._artifactcache import ArtifactCache
  from ._elementfactory import ElementFactory
  from ._sourcefactory import SourceFactory
 +from .plugin import CoreWarnings
  from ._projectrefs import ProjectRefs, ProjectRefStorage
  from ._versions import BST_FORMAT_VERSION
  from ._loader import Loader
@@ -106,7 +106,7 @@ class Project():
          self.first_pass_config = ProjectConfig()
          self.junction = junction                 # The junction Element object, if this is a subproject
 -        self.fail_on_overlap = False             # Whether overlaps are treated as errors
++
          self.ref_storage = None                  # ProjectRefStorage setting
          self.base_environment = {}               # The base set of environment variables
          self.base_env_nocache = None             # The base nocache mask (list) for the environment
@@ -121,6 +121,9 @@ class Project():
          self._cli_options = cli_options
          self._cache_key = None
 +        self._fatal_warnings = []             # A list of warnings which should trigger an error
 +        self._all_warnings_are_fatal = False  # A flag set to true if all warnings should be considered fatal
++
          self._shell_command = []      # The default interactive shell command
          self._shell_environment = {}  # Statically set environment vars
          self._shell_host_files = []   # A list of HostMount objects
@@ -457,7 +460,7 @@ class Project():
              'split-rules', 'elements', 'plugins',
              'aliases', 'name',
              'artifacts', 'options',
 -            'fail-on-overlap', 'shell',
 +            'fail-on-overlap', 'shell', 'fatal-warnings',
              'ref-storage', 'sandbox', 'mirrors'
          ])
@@ -479,8 +482,38 @@ class Project():
          # Load project split rules
          self._splits = _yaml.node_get(config, Mapping, 'split-rules')
 -        # Fail on overlap
 -        self.fail_on_overlap = _yaml.node_get(config, bool, 'fail-on-overlap')
 +        # Fatal warnings
 +        p = _yaml.node_get_provenance(config, 'fatal-warnings')
 +        try:  # Check for bool type
 +            fatal_warnings = _yaml.node_get(config, bool, 'fatal-warnings', default_value=False)
 +        except LoadError:
 +            try:  # Check for list type
 +                fatal_warnings = _yaml.node_get(config, list, 'fatal-warnings', default_value=[])
 +            except LoadError:
 +                raise LoadError(LoadErrorReason.INVALID_DATA,
 +                                "{}: Invalid value specified for 'fatal-warnings', ".format(p) +
 +                                "must be list or bool.")
++
 +        # Set fatal warnings
 +        self._set_fatal_warnings(fatal_warnings)
++
 +        # Support backwards compatibility for fail-on-overlap
++
 +        fail_on_overlap = _yaml.node_get(config, bool, 'fail-on-overlap', default_value=None)
++
 +        if CoreWarnings.OVERLAPS not in self._fatal_warnings and fail_on_overlap:
 +            self._fatal_warnings.append(CoreWarnings.OVERLAPS)
++
 +        # Deprecation check
 +        if fail_on_overlap is not None:
 +            self._context.message(
 +                Message(
 +                    None,
 +                    MessageType.WARN,
 +                    "Use of fail-on-overlap within project.conf " +
 +                    "is deprecated. Consider using fatal-warnings instead."
 +                )
 +            )
          # Load project.refs if it exists, this may be ignored.
          if self.ref_storage == ProjectRefStorage.PROJECT_REFS:
@@ -572,7 +605,10 @@ class Project():
          # Extend variables with automatic variables and option exports
          # Initialize it as a string as all variables are processed as strings.
 -        output.base_variables['max-jobs'] = str(multiprocessing.cpu_count())
 +        # Based on some testing (mainly on AWS), maximum effective
 +        # max-jobs value seems to be around 8-10 if we have enough cores
 +        # users should set values based on workload and build infrastructure
 +        output.base_variables['max-jobs'] = str(min(len(os.sched_getaffinity(0)), 8))
          # Export options into variables, if that was requested
          output.options.export_variables(output.base_variables)
@@ -710,3 +746,36 @@ class Project():
                  # paths are passed in relative to the project, but must be absolute
                  origin_dict['path'] = os.path.join(self.directory, path)
              destination.append(origin_dict)
++
 +    # _warning_is_fatal():
 +    #
 +    # Returns true if the warning in question should be considered fatal based on
 +    # the project configuration.
 +    #
 +    # Args:
 +    #   warning_str (str): The warning configuration string to check against
 +    #
 +    # Returns:
 +    #    (bool): True if the warning should be considered fatal and cause an error.
 +    #
 +    def _warning_is_fatal(self, warning_str):
 +        return self._all_warnings_are_fatal or (warning_str in self._fatal_warnings)
++
 +    # _set_fatal_warnings():
 +    #
 +    # Sets self._fatal_warnings appropriately. If warnings is of type bool self._all_warnings_are_fatal
 +    # wil be set to its value.
 +    #
 +    # Args:
 +    #   warnings (list|bool): The warnings to set self._fatal_warnings to.
 +    #                         If bool, self._all_warnings_are_fatal is set to the boolean value.
 +    #   provenance (str): The provenance assosciated with the warnings parameter.
 +    #
 +    def _set_fatal_warnings(self, warnings):
 +        if warnings is None:
 +            return
 +        elif isinstance(warnings, bool):
 +            self._fatal_warnings = []
 +            self._all_warnings_are_fatal = warnings
 +        else:
 +            self._fatal_warnings = warnings

buildstream/_stream.py

@@ -460,7 +460,7 @@ class Stream():
                                                selection=PipelineSelection.REDIRECT,
                                                track_selection=PipelineSelection.REDIRECT)
          target = elements[0]
 -        workdir = os.path.abspath(directory)
 +        directory = os.path.abspath(directory)
          if not list(target.sources()):
              build_depends = [x.name for x in target.dependencies(Scope.BUILD, recurse=False)]
@@ -476,7 +476,7 @@ class Stream():
          workspace = workspaces.get_workspace(target._get_full_name())
          if workspace and not force:
              raise StreamError("Workspace '{}' is already defined at: {}"
 -                              .format(target.name, workspace.path))
 +                              .format(target.name, workspace.get_absolute_path()))
          # If we're going to checkout, we need at least a fetch,
          # if we were asked to track first, we're going to fetch anyway.
@@ -502,7 +502,7 @@ class Stream():
          except OSError as e:
              raise StreamError("Failed to create workspace directory: {}".format(e)) from e
 -        workspaces.create_workspace(target._get_full_name(), workdir)
 +        workspaces.create_workspace(target._get_full_name(), directory)
          if not no_checkout:
              with target.timed_activity("Staging sources to {}".format(directory)):
@@ -526,12 +526,12 @@ class Stream():
          # Remove workspace directory if prompted
          if remove_dir:
              with self._context.timed_activity("Removing workspace directory {}"
 -                                              .format(workspace.path)):
 +                                              .format(workspace.get_absolute_path())):
                  try:
 -                    shutil.rmtree(workspace.path)
 +                    shutil.rmtree(workspace.get_absolute_path())
                  except OSError as e:
                      raise StreamError("Could not remove  '{}': {}"
 -                                      .format(workspace.path, e)) from e
 +                                      .format(workspace.get_absolute_path(), e)) from e
          # Delete the workspace and save the configuration
          workspaces.delete_workspace(element_name)
@@ -574,28 +574,30 @@ class Stream():
          for element in elements:
              workspace = workspaces.get_workspace(element._get_full_name())
+-
 +            workspace_path = workspace.get_absolute_path()
              if soft:
                  workspace.prepared = False
                  self._message(MessageType.INFO, "Reset workspace state for {} at: {}"
 -                              .format(element.name, workspace.path))
 +                              .format(element.name, workspace_path))
                  continue
              with element.timed_activity("Removing workspace directory {}"
 -                                        .format(workspace.path)):
 +                                        .format(workspace_path)):
                  try:
 -                    shutil.rmtree(workspace.path)
 +                    shutil.rmtree(workspace_path)
                  except OSError as e:
                      raise StreamError("Could not remove  '{}': {}"
 -                                      .format(workspace.path, e)) from e
 +                                      .format(workspace_path, e)) from e
              workspaces.delete_workspace(element._get_full_name())
 -            workspaces.create_workspace(element._get_full_name(), workspace.path)
 +            workspaces.create_workspace(element._get_full_name(), workspace_path)
 -            with element.timed_activity("Staging sources to {}".format(workspace.path)):
 +            with element.timed_activity("Staging sources to {}".format(workspace_path)):
                  element._open_workspace()
 -            self._message(MessageType.INFO, "Reset workspace for {} at: {}".format(element.name, workspace.path))
 +            self._message(MessageType.INFO,
 +                          "Reset workspace for {} at: {}".format(element.name,
 +                                                                 workspace_path))
          workspaces.save_config()
@@ -632,7 +634,7 @@ class Stream():
          for element_name, workspace_ in self._context.get_workspaces().list():
              workspace_detail = {
                  'element': element_name,
 -                'directory': workspace_.path,
 +                'directory': workspace_.get_absolute_path(),
+             }
              workspaces.append(workspace_detail)

buildstream/_versions.py

@@ -23,7 +23,7 @@
  # This version is bumped whenever enhancements are made
  # to the `project.conf` format or the core element format.
+ #
 -BST_FORMAT_VERSION = 13
 +BST_FORMAT_VERSION = 14
  # The base BuildStream artifact version

buildstream/_workspaces.py

@@ -26,14 +26,6 @@ from ._exceptions import LoadError, LoadErrorReason
  BST_WORKSPACE_FORMAT_VERSION = 3
 -# Hold on to a list of members which get serialized
 -_WORKSPACE_MEMBERS = [
 -    'prepared',
 -    'path',
 -    'last_successful',
 -    'running_files'
 -]
+-
  # Workspace()
+ #
@@ -56,7 +48,7 @@ class Workspace():
      def __init__(self, toplevel_project, *, last_successful=None, path=None, prepared=False, running_files=None):
          self.prepared = prepared
          self.last_successful = last_successful
 -        self.path = path
 +        self._path = path
          self.running_files = running_files if running_files is not None else {}
          self._toplevel_project = toplevel_project
@@ -64,14 +56,20 @@ class Workspace():
      # to_dict()
+     #
 -    # Convert this object to a dict for serialization purposes
 +    # Convert a list of members which get serialized to a dict for serialization purposes
+     #
      # Returns:
      #     (dict) A dict representation of the workspace
+     #
      def to_dict(self):
 -        return {key: val for key, val in self.__dict__.items()
 -                if key in _WORKSPACE_MEMBERS and val is not None}
 +        ret = {
 +            'prepared': self.prepared,
 +            'path': self._path,
 +            'running_files': self.running_files
 +        }
 +        if self.last_successful is not None:
 +            ret["last_successful"] = self.last_successful
 +        return ret
      # from_dict():
+     #
@@ -103,15 +101,7 @@ class Workspace():
      #    True if the workspace differs from 'other', otherwise False
+     #
      def differs(self, other):
+-
 -        for member in _WORKSPACE_MEMBERS:
 -            member_a = getattr(self, member)
 -            member_b = getattr(other, member)
+-
 -            if member_a != member_b:
 -                return True
+-
 -        return False
 +        return self.to_dict() != other.to_dict()
      # invalidate_key()
+     #
@@ -133,7 +123,7 @@ class Workspace():
          if os.path.isdir(fullpath):
              utils.copy_files(fullpath, directory)
          else:
 -            destfile = os.path.join(directory, os.path.basename(self.path))
 +            destfile = os.path.join(directory, os.path.basename(self.get_absolute_path()))
              utils.safe_copy(fullpath, destfile)
      # add_running_files()
@@ -189,7 +179,7 @@ class Workspace():
                  filelist = utils.list_relative_paths(fullpath)
                  filelist = [(relpath, os.path.join(fullpath, relpath)) for relpath in filelist]
              else:
 -                filelist = [(self.path, fullpath)]
 +                filelist = [(self.get_absolute_path(), fullpath)]
              self._key = [(relpath, unique_key(fullpath)) for relpath, fullpath in filelist]
@@ -200,7 +190,7 @@ class Workspace():
      # Returns: The absolute path of the element's workspace.
+     #
      def get_absolute_path(self):
 -        return os.path.join(self._toplevel_project.directory, self.path)
 +        return os.path.join(self._toplevel_project.directory, self._path)
  # Workspaces()
@@ -236,6 +226,9 @@ class Workspaces():
      #    path (str) - The path in which the workspace should be kept
+     #
      def create_workspace(self, element_name, path):
 +        if path.startswith(self._toplevel_project.directory):
 +            path = os.path.relpath(path, self._toplevel_project.directory)
++
          self._workspaces[element_name] = Workspace(self._toplevel_project, path=path)
          return self._workspaces[element_name]

buildstream/data/projectconfig.yaml

@@ -14,7 +14,15 @@ element-path: .
  ref-storage: inline
  # Overlaps are just warnings
 -fail-on-overlap: False
 +# This has been DEPRECATED in favour of fatal-warnings
 +#fail-on-overlap: False
++
 +# Allows a collection of warnings to be configured to be raised as errors.
 +# Setting this value to true will enable all possible fatal-warnings
 +# fatal-warnings: True
++
 +# fatal-warnings:
 +# - overlaps
  # Variable Configuration

buildstream/element.py

@@ -94,6 +94,7 @@ from . import _cachekey
  from . import _signals
  from . import _site
  from ._platform import Platform
 +from .plugin import CoreWarnings
  from .sandbox._config import SandboxConfig
  from .storage.directory import Directory
@@ -746,32 +747,23 @@ class Element(Plugin):
                  ignored[dep.name] = result.ignored
          if overlaps:
 -            overlap_error = overlap_warning = False
 -            error_detail = warning_detail = "Staged files overwrite existing files in staging area:\n"
 +            overlap_warning = False
 +            warning_detail = "Staged files overwrite existing files in staging area:\n"
              for f, elements in overlaps.items():
 -                overlap_error_elements = []
                  overlap_warning_elements = []
                  # The bottom item overlaps nothing
                  overlapping_elements = elements[1:]
                  for elm in overlapping_elements:
                      element = self.search(scope, elm)
 -                    element_project = element._get_project()
                      if not element.__file_is_whitelisted(f):
 -                        if element_project.fail_on_overlap:
 -                            overlap_error_elements.append(elm)
 -                            overlap_error = True
 -                        else:
 -                            overlap_warning_elements.append(elm)
 -                            overlap_warning = True
 +                        overlap_warning_elements.append(elm)
 +                        overlap_warning = True
                  warning_detail += _overlap_error_detail(f, overlap_warning_elements, elements)
 -                error_detail += _overlap_error_detail(f, overlap_error_elements, elements)
              if overlap_warning:
 -                self.warn("Non-whitelisted overlaps detected", detail=warning_detail)
 -            if overlap_error:
 -                raise ElementError("Non-whitelisted overlaps detected and fail-on-overlaps is set",
 -                                   detail=error_detail, reason="overlap-error")
 +                self.warn("Non-whitelisted overlaps detected", detail=warning_detail,
 +                          warning_token=CoreWarnings.OVERLAPS)
          if ignored:
              detail = "Not staging files which would replace non-empty directories:\n"
@@ -1403,7 +1395,8 @@ class Element(Plugin):
                      # If mount_workspaces is set and we're doing incremental builds,
                      # the workspace is already mounted into the sandbox.
                      if not (mount_workspaces and self.__can_build_incrementally()):
 -                        with self.timed_activity("Staging local files at {}".format(workspace.path)):
 +                        with self.timed_activity("Staging local files at {}"
 +                                                 .format(workspace.get_absolute_path())):
                              workspace.stage(temp_staging_directory)
                  else:
                      # No workspace, stage directly
@@ -1566,7 +1559,7 @@ class Element(Plugin):
                          path_components = self.__staged_sources_directory.lstrip(os.sep).split(os.sep)
                          sandbox_vpath = sandbox_vroot.descend(path_components)
                          try:
 -                            sandbox_vpath.import_files(workspace.path)
 +                            sandbox_vpath.import_files(workspace.get_absolute_path())
                          except UtilError as e:
                              self.warn("Failed to preserve workspace state for failed build sysroot: {}"
                                        .format(e))
@@ -1893,7 +1886,7 @@ class Element(Plugin):
                  source._init_workspace(temp)
              # Now hardlink the files into the workspace target.
 -            utils.link_files(temp, workspace.path)
 +            utils.link_files(temp, workspace.get_absolute_path())
      # _get_workspace():
+     #
@@ -2054,7 +2047,7 @@ class Element(Plugin):
+             }
              # fail-on-overlap setting cannot affect elements without dependencies
 -            if project.fail_on_overlap and dependencies:
 +            if project._warning_is_fatal(CoreWarnings.OVERLAPS) and dependencies:
                  self.__cache_key_dict['fail-on-overlap'] = True
          cache_key_dict = self.__cache_key_dict.copy()

buildstream/plugin.py

@@ -47,6 +47,23 @@ it is mandatory to implement the following abstract methods:
    Once all configuration has been loaded and preflight checks have passed,
    this method is used to inform the core of a plugin's unique configuration.
 +Configurable Warnings
 +---------------------
 +Warnings raised through calling :func:`Plugin.warn() <buildstream.plugin.Plugin.warn>` can provide an optional
 +parameter ``warning_token``, this will raise a :class:`PluginError` if the warning is configured as fatal within
 +the project configuration.
++
 +Configurable warnings will be prefixed with :func:`Plugin.get_kind() <buildstream.plugin.Plugin.get_kind>`
 +within buildstream and must be prefixed as such in project configurations. For more detail on project configuration
 +see :ref:`Configurable Warnings <configurable_warnings>`.
++
 +It is important to document these warnings in your plugin documentation to allow users to make full use of them
 +while configuring their projects.
++
 +Example
 +~~~~~~~
 +If the ``git.py`` plugin uses the warning ``"inconsistent-submodule"`` then it could be referenced in project
 +configuration as ``"git:inconsistent-submodule"``.
  Plugin Structure
  ----------------
@@ -103,6 +120,18 @@ from ._exceptions import PluginError, ImplError
  from ._message import Message, MessageType
 +class CoreWarnings():
 +    OVERLAPS = "overlaps"
 +    REF_NOT_IN_TRACK = "ref-not-in-track"
++
++
 +CORE_WARNINGS = [
 +    value
 +    for name, value in CoreWarnings.__dict__.items()
 +    if not name.startswith("__")
 +]
++
++
  class Plugin():
      """Plugin()
@@ -166,7 +195,6 @@ class Plugin():
          # Infer the kind identifier
          modulename = type(self).__module__
          self.__kind = modulename.split('.')[-1]
+-
          self.debug("Created: {}".format(self))
      def __del__(self):
@@ -473,14 +501,27 @@ class Plugin():
          """
          self.__message(MessageType.INFO, brief, detail=detail)
 -    def warn(self, brief, *, detail=None):
 -        """Print a warning message
 +    def warn(self, brief, *, detail=None, warning_token=None):
 +        """Print a warning message, checks warning_token against project configuration
          Args:
             brief (str): The brief message
             detail (str): An optional detailed message, can be multiline output
 +           warning_token (str): An optional configurable warning assosciated with this warning,
 +                                this will cause PluginError to be raised if this warning is configured as fatal.
 +                                (*Since 1.4*)
++
 +        Raises:
 +           (:class:`.PluginError`): When warning_token is considered fatal by the project configuration
          """
 -        self.__message(MessageType.WARN, brief, detail=detail)
 +        if warning_token:
 +            warning_token = _prefix_warning(self, warning_token)
 +            brief = "[{}]: {}".format(warning_token, brief)
++
 +            if self.__warning_is_fatal(warning_token):
 +                raise PluginError(message="{}\n{}".format(brief, detail), reason=warning_token)
++
 +        self.__message(MessageType.WARN, brief=brief, detail=detail)
      def log(self, brief, *, detail=None):
          """Log a message into the plugin's log file
@@ -708,6 +749,9 @@ class Plugin():
          else:
              return self.name
 +    def __warning_is_fatal(self, warning):
 +        return self._get_project()._warning_is_fatal(warning)
++
  # Hold on to a lookup table by counter of all instantiated plugins.
  # We use this to send the id back from child processes so we can lookup
@@ -738,6 +782,22 @@ def _plugin_lookup(unique_id):
      assert unique_id in __PLUGINS_TABLE, "Could not find plugin with ID {}".format(unique_id)
      return __PLUGINS_TABLE[unique_id]
 +# _prefix_warning():
 +#
 +# Prefix a warning with the plugin kind. CoreWarnings are not prefixed.
 +#
 +# Args:
 +#   plugin (Plugin): The plugin which raised the warning
 +#   warning (str): The warning to prefix
 +#
 +# Returns:
 +#    (str): A prefixed warning
 +#
 +def _prefix_warning(plugin, warning):
 +    if any([warning is core_warning for core_warning in CORE_WARNINGS]):
 +        return warning
 +    return "{}:{}".format(plugin.get_kind(), warning)
++
  # No need for unregister, WeakValueDictionary() will remove entries
  # in itself when the referenced plugins are garbage collected.

buildstream/plugins/sources/git.py

@@ -68,6 +68,12 @@ git - stage files from a git repository
         url: upstream:baz.git
         checkout: False
 +**Configurable Warnings:**
++
 +This plugin provides the following configurable warnings:
++
 +- 'git:inconsistent-submodule' - A submodule was found to be missing from the underlying git repository.
++
  """
  import os
@@ -84,6 +90,9 @@ from buildstream import utils
  GIT_MODULES = '.gitmodules'
 +# Warnings
 +INCONSISTENT_SUBMODULE = "inconsistent-submodules"
++
  # Because of handling of submodules, we maintain a GitMirror
  # for the primary git source and also for each submodule it
@@ -283,7 +292,7 @@ class GitMirror(SourceFetcher):
                       "underlying git repository with `git submodule add`."
              self.source.warn("{}: Ignoring inconsistent submodule '{}'"
 -                             .format(self.source, submodule), detail=detail)
 +                             .format(self.source, submodule), detail=detail, warning_token=INCONSISTENT_SUBMODULE)
              return None
@@ -350,6 +359,9 @@ class GitSource(Source):
              return Consistency.RESOLVED
          return Consistency.INCONSISTENT
 +    def get_warnings(self):
 +        return [INCONSISTENT_SUBMODULE]
++
      def load_ref(self, node):
          self.mirror.ref = self.node_get_member(node, str, 'ref', None)

buildstream/sandbox/_sandboxbwrap.py

@@ -89,6 +89,11 @@ class SandboxBwrap(Sandbox):
          # Grab the full path of the bwrap binary
          bwrap_command = [utils.get_host_tool('bwrap')]
 +        for k, v in env.items():
 +            bwrap_command += ['--setenv', k, v]
 +        for k in os.environ.keys() - env.keys():
 +            bwrap_command += ['--unsetenv', k]
++
          # Create a new pid namespace, this also ensures that any subprocesses
          # are cleaned up when the bwrap process exits.
          bwrap_command += ['--unshare-pid']
@@ -194,7 +199,7 @@ class SandboxBwrap(Sandbox):
                  stdin = stack.enter_context(open(os.devnull, "r"))
              # Run bubblewrap !
 -            exit_code = self.run_bwrap(bwrap_command, stdin, stdout, stderr, env,
 +            exit_code = self.run_bwrap(bwrap_command, stdin, stdout, stderr,
                                         (flags & SandboxFlags.INTERACTIVE))
              # Cleanup things which bwrap might have left behind, while
@@ -245,7 +250,7 @@ class SandboxBwrap(Sandbox):
          return exit_code
 -    def run_bwrap(self, argv, stdin, stdout, stderr, env, interactive):
 +    def run_bwrap(self, argv, stdin, stdout, stderr, interactive):
          # Wrapper around subprocess.Popen() with common settings.
+         #
          # This function blocks until the subprocess has terminated.
@@ -321,7 +326,6 @@ 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,
 -                env=env,
                  stdin=stdin,
                  stdout=stdout,
                  stderr=stderr,

doc/source/format_project.rst

@@ -126,23 +126,74 @@ following to your ``project.conf``:
     The ``ref-storage`` configuration is available since :ref:`format version 8 <project_format_version>`
 +.. _configurable_warnings:
++
 +Configurable Warnings
 +~~~~~~~~~~~~~~~~~~~~~
 +Warnings can be configured as fatal using the ``fatal-warnings`` configuration item.
 +When a warning is configured as fatal, where a warning would usually be thrown instead an error will be thrown
 +causing the build to fail.
++
 +When ``fatal-warnings`` is True, all configurable fatal warnings will be set as fatal. Individual warnings
 +can also be set by setting ``fatal-warnings`` to a list of warnings.
++
 +.. code::
++
 +  fatal-warnings:
 +  - core:overlaps
 +  - core:ref-not-in-track
 +  - <plugin>:<warning>
++
 +Core Configurable warnings include:
++
 +- :ref:`core:overlaps <fail_on_overlaps>`
 +- :ref:`core:ref-not-in-track <ref_not_in_track>`
++
 +.. note::
++
 +  The ``ref-storage`` configuration is available since :ref:`format version 12 <project_format_version>`
++
 +.. note::
++
 +  Other configurable warnings are plugin specific and should be noted within their individual documentation.
++
 +.. _fail_on_overlaps:
++
  Fail on overlaps
  ~~~~~~~~~~~~~~~~
  When multiple elements are staged, there's a possibility that different
  elements will try and stage different versions of the same file.
 -When ``fail-on-overlap`` is true, if an overlap is detected
 -that hasn't been allowed by the element's
 -:ref:`overlap whitelist<public_overlap_whitelist>`,
 -then an error will be raised and the build will fail.
 +.. deprecated:: 1.4
++
 -otherwise, a warning will be raised indicating which files had overlaps,
 -and the order that the elements were overlapped.
 +  When ``fail-on-overlap`` is true, if an overlap is detected
 +  that hasn't been allowed by the element's
 +  :ref:`overlap whitelist<public_overlap_whitelist>`,
 +  then an error will be raised and the build will fail.
++
 +  Otherwise, a warning will be raised indicating which files had overlaps,
 +  and the order that the elements were overlapped.
  .. code:: yaml
 +  # Deprecated
    fail-on-overlap: true
 +.. note::
++
 +  Since deprecation in :ref:`format version 12 <project_format_version>` the recommended
 +  solution to this is :ref:`Configurable Warnings <configurable_warnings>`
++
 +  When used in combination with ``fatal-warnings``, setting ``fail-on-overlap: False`` can be overriden by ``fatal-warnings``.
++
++
 +.. _ref_not_in_track:
++
 +Ref not in track
 +~~~~~~~~~~~~~~~~
 +The configured ref is not valid for the configured track.
++
  .. _project_source_aliases:

setup.py

@@ -251,7 +251,7 @@ setup(name='BuildStream',
        install_requires=[
            'setuptools',
            'psutil',
 -          'ruamel.yaml',
 +          'ruamel.yaml <= 0.15',
            'pluginbase',
            'Click',
            'blessings',

tests/frontend/configurable_warnings.py

 +import pytest
 +import os
++
 +from buildstream.plugin import CoreWarnings
 +from buildstream._exceptions import ErrorDomain, LoadErrorReason
 +from buildstream import _yaml
 +from tests.testutils.runcli import cli
++
 +TOP_DIR = os.path.join(
 +    os.path.dirname(os.path.realpath(__file__)),
 +    "configuredwarning"
 +)
++
++
 +def get_project(fatal_warnings):
 +    return {
 +        "name": "test",
 +        "element-path": "elements",
 +        "plugins": [
 +            {
 +                "origin": "local",
 +                "path": "plugins",
 +                "elements": {
 +                    "warninga": 0,
 +                    "warningb": 0,
 +                    "corewarn": 0,
 +                }
 +            }
 +        ],
 +        "fatal-warnings": fatal_warnings
 +    }
++
++
 +def build_project(datafiles, fatal_warnings):
 +    project_path = os.path.join(datafiles.dirname, datafiles.basename)
++
 +    project = get_project(fatal_warnings)
++
 +    _yaml.dump(project, os.path.join(project_path, "project.conf"))
++
 +    return project_path
++
++
 +@pytest.mark.datafiles(TOP_DIR)
 +@pytest.mark.parametrize("element_name, fatal_warnings, expect_fatal, error_domain", [
 +    ("corewarn.bst", [CoreWarnings.OVERLAPS], True, ErrorDomain.STREAM),
 +    ("warninga.bst", ["warninga:warning-a"], True, ErrorDomain.STREAM),
 +    ("warningb.bst", ["warningb:warning-b"], True, ErrorDomain.STREAM),
 +    ("corewarn.bst", [], False, None),
 +    ("warninga.bst", [], False, None),
 +    ("warningb.bst", [], False, None),
 +    ("corewarn.bst", "true", True, ErrorDomain.STREAM),
 +    ("warninga.bst", "true", True, ErrorDomain.STREAM),
 +    ("warningb.bst", "true", True, ErrorDomain.STREAM),
 +    ("warninga.bst", [CoreWarnings.OVERLAPS], False, None),
 +    ("warningb.bst", [CoreWarnings.OVERLAPS], False, None),
 +])
 +def test_fatal_warnings(cli, datafiles, element_name,
 +                        fatal_warnings, expect_fatal, error_domain):
 +    project_path = build_project(datafiles, fatal_warnings)
++
 +    result = cli.run(project=project_path, args=["build", element_name])
 +    if expect_fatal:
 +        result.assert_main_error(error_domain, None, "Expected fatal execution")
 +    else:
 +        result.assert_success("Unexpected fatal execution")

tests/frontend/configuredwarning/elements/corewarn.bst

	1	+kind: corewarn
		\ No newline at end of file

tests/frontend/configuredwarning/elements/warninga.bst

+kind: warninga

tests/frontend/configuredwarning/elements/warningb.bst

+kind: warningb

tests/frontend/configuredwarning/plugins/corewarn.py

 +from buildstream import Element
 +from buildstream.plugin import CoreWarnings
++
++
 +class CoreWarn(Element):
 +    def configure(self, node):
 +        pass
++
 +    def preflight(self):
 +        pass
++
 +    def get_unique_key(self):
 +        pass
++
 +    def get_warnings(self):
 +        return []  # CoreWarnings should be included regardless of plugins.
++
 +    def configure_sandbox(self, sandbox):
 +        pass
++
 +    def stage(self, sandbox):
 +        pass
++
 +    def assemble(self, sandbox):
 +        self.warn("Testing: CoreWarning produced during assemble",
 +                  warning_token=CoreWarnings.OVERLAPS)
++
++
 +def setup():
 +    return CoreWarn

tests/frontend/configuredwarning/plugins/warninga.py

 +from buildstream import Element
++
 +WARNING_A = "warning-a"
++
++
 +class WarningA(Element):
 +    def configure(self, node):
 +        pass
++
 +    def preflight(self):
 +        pass
++
 +    def get_unique_key(self):
 +        pass
++
 +    def configure_sandbox(self, sandbox):
 +        pass
++
 +    def stage(self, sandbox):
 +        pass
++
 +    def assemble(self, sandbox):
 +        self.warn("Testing: warning-a produced during assemble", warning_token=WARNING_A)
++
++
 +def setup():
 +    return WarningA

tests/frontend/configuredwarning/plugins/warningb.py

 +from buildstream import Element
++
 +WARNING_B = "warning-b"
++
++
 +class WarningB(Element):
 +    def configure(self, node):
 +        pass
++
 +    def preflight(self):
 +        pass
++
 +    def get_unique_key(self):
 +        pass
++
 +    def configure_sandbox(self, sandbox):
 +        pass
++
 +    def stage(self, sandbox):
 +        pass
++
 +    def assemble(self, sandbox):
 +        self.warn("Testing: warning-b produced during assemble", warning_token=WARNING_B)
++
++
 +def setup():
 +    return WarningB

tests/frontend/configuredwarning/project.conf

 +name: test
 +element-path: elements
 +plugins:
 +- origin: local
 +  path: element_plugins
 +  elements:
 +    warninga: 0
 +    warningb: 0

tests/frontend/overlaps.py

@@ -3,6 +3,7 @@ import pytest
  from tests.testutils.runcli import cli
  from buildstream._exceptions import ErrorDomain
  from buildstream import _yaml
 +from buildstream.plugin import CoreWarnings
  # Project directory
  DATA_DIR = os.path.join(
@@ -16,30 +17,35 @@ project_template = {
+ }
 -def gen_project(project_dir, fail_on_overlap):
 +def gen_project(project_dir, fail_on_overlap, use_fatal_warnings=True):
      template = dict(project_template)
 -    template["fail-on-overlap"] = fail_on_overlap
 +    if use_fatal_warnings:
 +        template["fatal-warnings"] = [CoreWarnings.OVERLAPS] if fail_on_overlap else []
 +    else:
 +        template["fail-on-overlap"] = fail_on_overlap
      projectfile = os.path.join(project_dir, "project.conf")
      _yaml.dump(template, projectfile)
  @pytest.mark.datafiles(DATA_DIR)
 -def test_overlaps(cli, datafiles):
 +@pytest.mark.parametrize("use_fatal_warnings", [True, False])
 +def test_overlaps(cli, datafiles, use_fatal_warnings):
      project_dir = str(datafiles)
 -    gen_project(project_dir, False)
 +    gen_project(project_dir, False, use_fatal_warnings)
      result = cli.run(project=project_dir, silent=True, args=[
          'build', 'collect.bst'])
      result.assert_success()
  @pytest.mark.datafiles(DATA_DIR)
 -def test_overlaps_error(cli, datafiles):
 +@pytest.mark.parametrize("use_fatal_warnings", [True, False])
 +def test_overlaps_error(cli, datafiles, use_fatal_warnings):
      project_dir = str(datafiles)
 -    gen_project(project_dir, True)
 +    gen_project(project_dir, True, use_fatal_warnings)
      result = cli.run(project=project_dir, silent=True, args=[
          'build', 'collect.bst'])
      result.assert_main_error(ErrorDomain.STREAM, None)
 -    result.assert_task_error(ErrorDomain.ELEMENT, "overlap-error")
 +    result.assert_task_error(ErrorDomain.PLUGIN, CoreWarnings.OVERLAPS)
  @pytest.mark.datafiles(DATA_DIR)
@@ -70,15 +76,16 @@ def test_overlaps_whitelist_on_overlapper(cli, datafiles):
      result = cli.run(project=project_dir, silent=True, args=[
          'build', 'collect-partially-whitelisted.bst'])
      result.assert_main_error(ErrorDomain.STREAM, None)
 -    result.assert_task_error(ErrorDomain.ELEMENT, "overlap-error")
 +    result.assert_task_error(ErrorDomain.PLUGIN, CoreWarnings.OVERLAPS)
  @pytest.mark.datafiles(DATA_DIR)
 -def test_overlaps_script(cli, datafiles):
 +@pytest.mark.parametrize("use_fatal_warnings", [True, False])
 +def test_overlaps_script(cli, datafiles, use_fatal_warnings):
      # Test overlaps with script element to test
      # Element.stage_dependency_artifacts() with Scope.RUN
      project_dir = str(datafiles)
 -    gen_project(project_dir, False)
 +    gen_project(project_dir, False, use_fatal_warnings)
      result = cli.run(project=project_dir, silent=True, args=[
          'build', 'script.bst'])
      result.assert_success()

tests/frontend/workspace.py

@@ -18,12 +18,13 @@ DATA_DIR = os.path.join(
+ )
 -def open_workspace(cli, tmpdir, datafiles, kind, track, suffix=''):
 -    project = os.path.join(datafiles.dirname, datafiles.basename)
 -    bin_files_path = os.path.join(project, 'files', 'bin-files')
 -    element_path = os.path.join(project, 'elements')
 +def open_workspace(cli, tmpdir, datafiles, kind, track, suffix='', workspace_dir=None):
 +    if not workspace_dir:
 +        workspace_dir = os.path.join(str(tmpdir), 'workspace{}'.format(suffix))
 +    project_path = os.path.join(datafiles.dirname, datafiles.basename)
 +    bin_files_path = os.path.join(project_path, 'files', 'bin-files')
 +    element_path = os.path.join(project_path, 'elements')
      element_name = 'workspace-test-{}{}.bst'.format(kind, suffix)
 -    workspace = os.path.join(str(tmpdir), 'workspace{}'.format(suffix))
      # Create our repo object of the given source type with
      # the bin files, and then collect the initial ref.
@@ -45,7 +46,7 @@ def open_workspace(cli, tmpdir, datafiles, kind, track, suffix=''):
                              element_name))
      # Assert that there is no reference, a track & fetch is needed
 -    state = cli.get_element_state(project, element_name)
 +    state = cli.get_element_state(project_path, element_name)
      if track:
          assert state == 'no reference'
      else:
@@ -56,20 +57,20 @@ def open_workspace(cli, tmpdir, datafiles, kind, track, suffix=''):
      args = ['workspace', 'open']
      if track:
          args.append('--track')
 -    args.extend([element_name, workspace])
 +    args.extend([element_name, workspace_dir])
 +    result = cli.run(project=project_path, args=args)
 -    result = cli.run(project=project, args=args)
      result.assert_success()
      # Assert that we are now buildable because the source is
      # now cached.
 -    assert cli.get_element_state(project, element_name) == 'buildable'
 +    assert cli.get_element_state(project_path, element_name) == 'buildable'
      # Check that the executable hello file is found in the workspace
 -    filename = os.path.join(workspace, 'usr', 'bin', 'hello')
 +    filename = os.path.join(workspace_dir, 'usr', 'bin', 'hello')
      assert os.path.exists(filename)
 -    return (element_name, project, workspace)
 +    return (element_name, project_path, workspace_dir)
  @pytest.mark.datafiles(DATA_DIR)
@@ -190,6 +191,46 @@ def test_close(cli, tmpdir, datafiles, kind):
      assert not os.path.exists(workspace)
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_close_external_after_move_project(cli, tmpdir, datafiles):
 +    tmp_parent = os.path.dirname(str(tmpdir))
 +    workspace_dir = os.path.join(tmp_parent, "workspace")
 +    element_name, project_path, _ = open_workspace(cli, tmpdir, datafiles, 'git', False, "", workspace_dir)
 +    assert os.path.exists(workspace_dir)
 +    tmp_dir = os.path.join(tmp_parent, 'external_project')
 +    shutil.move(project_path, tmp_dir)
 +    assert os.path.exists(tmp_dir)
++
 +    # Close the workspace
 +    result = cli.run(configure=False, project=tmp_dir, args=[
 +        'workspace', 'close', '--remove-dir', element_name
 +    ])
 +    result.assert_success()
++
 +    # Assert the workspace dir has been deleted
 +    assert not os.path.exists(workspace_dir)
 +    # Move directory back inside tmp directory so it can be recognised
 +    shutil.move(tmp_dir, project_path)
++
++
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_close_internal_after_move_project(cli, tmpdir, datafiles):
 +    element_name, project, _ = open_workspace(cli, tmpdir, datafiles, 'git', False)
 +    tmp_dir = os.path.join(os.path.dirname(str(tmpdir)), 'external_project')
 +    shutil.move(str(tmpdir), tmp_dir)
 +    assert os.path.exists(tmp_dir)
++
 +    # Close the workspace
 +    result = cli.run(configure=False, project=tmp_dir, args=[
 +        'workspace', 'close', '--remove-dir', element_name
 +    ])
 +    result.assert_success()
++
 +    # Assert the workspace dir has been deleted
 +    workspace = os.path.join(tmp_dir, 'workspace')
 +    assert not os.path.exists(workspace)
++
++
  @pytest.mark.datafiles(DATA_DIR)
  def test_close_removed(cli, tmpdir, datafiles):
      element_name, project, workspace = open_workspace(cli, tmpdir, datafiles, 'git', False)

[Notes] [Git][BuildStream/buildstream][Qinusty/526-fail-on-warnings] 13 commits: buildstream/_project.py:

Qinusty pushed to branch Qinusty/526-fail-on-warnings at BuildStream / buildstream

Commits:

23 changed files:

Changes: