[Notes] [Git][BuildStream/buildstream][tpollard/483] 23 commits: Report

Tom Pollard pushed to branch tpollard/483 at BuildStream / buildstream

Commits:

a38d9163

by Valentin David at 2018-08-30T10:26:57Z

Report processing errors from tracking

Failures to write files when tracking were not reported.

Fixes #533.

02138181

by Tristan Van Berkom at 2018-08-30T11:36:00Z

Merge branch 'valentindavid/post_tracking_errors' into 'master'

Report processing errors from tracking

Closes #533

See merge request BuildStream/buildstream!747

d23d4f3e

by Tristan Van Berkom at 2018-08-30T12:02:27Z

_artifactcache/artifactcache.py: Write the cache_size file atomically

This is causing issues while the size file is being read and written
simultaneously.

The proper fix will be to read/add/save the file atomically and that
will require locking, but this fix is a good stop gap to existing crashes.

bb6aa5a5

by Tristan Van Berkom at 2018-08-30T12:27:55Z

Merge branch 'tristan/atomic-cache-size-file' into 'master'

_artifactcache/artifactcache.py: Write the cache_size file atomically

See merge request BuildStream/buildstream!762

680e4fe1

by knownexus at 2018-08-30T13:37:55Z

Replacing string 'bzr' with value from host tools

8cd719eb

by Phillip Smyth at 2018-08-30T15:47:06Z

Merge branch 'bzr_fix' into 'master'

Replacing string 'bzr' with value from host tools

See merge request BuildStream/buildstream!764

18d0bfb4

by Tom Pollard at 2018-08-30T19:14:33Z

buildstream/_project.py: Report if project.conf is missing name

Explicitly check that project.conf contains a name. This resolves
the issue of the provenance check from _yaml.py incorrectly reporting
the offending file as the default_config_node projectconfig.yaml
when no name key exists in the pre_config_node dict.

95121148

by Tiago Gomes at 2018-08-30T20:52:29Z

Merge branch 'tpollard/591' into 'master'

buildstream/_project.py: Report if project.conf is missing name

See merge request BuildStream/buildstream!680

775d3fca

by Tristan Van Berkom at 2018-08-31T06:05:32Z

_project.py: Adding FIXME comment to address #591 properly

0682aa1c

by Josh Smith at 2018-08-31T10:58:12Z

job.py: Modify retry messages to be FAIL

This adjusts the message handler for the child processes to no longer
override the message type.

This also removes the ability for unhandled non BstError exceptions to
retry.

68eff9ba

by Josh Smith at 2018-08-31T10:58:20Z

tests: Modify tests to ensure retry FAILURE

9b18e1bf

by Tristan Van Berkom at 2018-08-31T11:22:51Z

Merge branch 'Qinusty/retries-should-fail' into 'master'

Retries log as failures

See merge request BuildStream/buildstream!766

e2d8fedf

by Josh Smith at 2018-08-31T15:53:43Z

scheduler.py: Move cache_size logs into folder

This prevents the cache_size.pid.log files from cluttering the root
log directory.

6805a2ab

by Tristan Van Berkom at 2018-09-01T08:24:05Z

Merge branch 'Qinusty/cache-size-directory' into 'master'

Move cache_size.pid.log files into a subdirectory of logs

See merge request BuildStream/buildstream!769

f8b06acc

by Tristan Van Berkom at 2018-09-02T09:33:39Z

source.py: Document Source.get_source_fetchers() to return an iterable

Also highlight the fact that the plugin can rely on the fetcher's
fetch() method getting called before consuming the next item in the
list, which is the magick behavior that the git plugin relies on.

This is a part of #620

559b6fbc

by Tristan Van Berkom at 2018-09-02T09:34:10Z

source.py: Fixing docs link formatting to be consistent.

43ad22d7

by Tristan Van Berkom at 2018-09-02T09:34:10Z

source.py: Documenting that marking download URLs is mandatory

A download URL must be interpreted by the core at `Plugin.configure()`
time, even if only employed later on.

This is a part of #620

16462e9c

by Tristan Van Berkom at 2018-09-02T09:36:49Z

plugin.py: Added _configure() and _get_configuring() private APIs

Keeps track of whether the plugin is currently being configured.

Adjusted Element and Source classes to call _configure() in place
of calling configure() directly.

This is a part of #620

1f9c4147

by Tristan Van Berkom at 2018-09-02T09:37:12Z

source.py: Added `primary` argument to URL marking APIs

The Source must now mention whether the marked or translated
URL is "primary" or not. Even when a Source may have multiple
URLs, the auxilliary URLs are derived from the primary one, not
only is this true for git, but it is mandated by our tracking
API which assumes there is a primary URL.

This adjusts the `git` source and the test `fetch_source.py` source
to behave properly and advertize it's primary URL properly.

This is a part of #620

f9b2f1a9

by Tristan Van Berkom at 2018-09-02T09:37:21Z

source.py: Track marked URLs and assert they are marked during Plugin.configure()

This cannot test for unaliased URLs, as those can be discovered later
on outside of user provided element configuration; at least we
assert that if an alias was used, we have seen it at load time.

This will cause a BUG to occur for a plugin which marks an aliased
URL (or attempts to translate one) outside of `Plugin.configure()`,
if that URL was not previously seen.

This is a part of #620

88460cd2

by Tristan Van Berkom at 2018-09-03T06:23:38Z

Merge branch 'tristan/source-fetcher-changes' into 'master'

Source fetcher changes

See merge request BuildStream/buildstream!772

2339f0c4

by Tom Pollard at 2018-09-03T09:44:36Z

plugins/git.py: Warn if ref is not in given track

Add a helper function assert_ref_in_track to git.py GitMirror()
which is used when staging & initing the source workspace. It
checks the element's ref exists in the track (branch/tag) if it
has been specified, raising a warning if necessary. The warning makes
use of the warning token 'REF_NOT_IN_TRACK' from the configurable
CoreWarnings. If the element has been tracked with bst, it is assumed
that the value of ref exists in the track as it was generated from it
& as such is not asserted.

c96fec5d

by Tom Pollard at 2018-09-03T09:44:36Z

tests/sources/git.py: Add tests for REF_NOT_IN_TRACK

Add tests that cover assert_ref_in_track & the configurable
CoreWarnings REF_NOT_IN_TRACK warnings token.

16 changed files:

buildstream/_artifactcache/artifactcache.py
buildstream/_project.py
buildstream/_scheduler/jobs/job.py
buildstream/_scheduler/queues/trackqueue.py
buildstream/_scheduler/scheduler.py
buildstream/element.py
buildstream/plugin.py
buildstream/plugins/sources/git.py
buildstream/source.py
tests/frontend/project/sources/fetch_source.py
tests/frontend/track.py
tests/sources/deb.py
tests/sources/git.py
tests/sources/tar.py
tests/sources/zip.py
tests/testutils/repo/bzr.py

Changes:

buildstream/_artifactcache/artifactcache.py

@@ -571,7 +571,7 @@ class ArtifactCache():
      def _write_cache_size(self, size):
          assert isinstance(size, int)
          size_file_path = os.path.join(self.context.artifactdir, CACHE_SIZE_FILE)
 -        with open(size_file_path, "w") as f:
 +        with utils.save_file_atomic(size_file_path, "w") as f:
              f.write(str(size))
      # _read_cache_size()

buildstream/_project.py

@@ -398,6 +398,17 @@ class Project():
                  "Project requested format version {}, but BuildStream {}.{} only supports up until format version {}"
                  .format(format_version, major, minor, BST_FORMAT_VERSION))
 +        # FIXME:
 +        #
 +        #   Performing this check manually in the absense
 +        #   of proper support from _yaml.node_get(), this should
 +        #   be removed in favor of a proper accessor function
 +        #   from the _yaml module when #591 is fixed.
 +        #
 +        if self._project_conf.get('name') is None:
 +            raise LoadError(LoadErrorReason.INVALID_DATA,
 +                            "{}: project.conf does not contain expected key '{}'".format(projectfile, 'name'))
++
          # The project name, element path and option declarations
          # are constant and cannot be overridden by option conditional statements
          self.name = _yaml.node_get(pre_config_node, str, 'name')

buildstream/_scheduler/jobs/job.py

@@ -403,7 +403,7 @@ class Job():
                  if self._retry_flag and (self._tries <= self._max_retries):
                      self.message(MessageType.FAIL,
                                   "Try #{} failed, retrying".format(self._tries),
 -                                 elapsed=elapsed)
 +                                 elapsed=elapsed, logfile=filename)
                  else:
                      self.message(MessageType.FAIL, str(e),
                                   elapsed=elapsed, detail=e.detail,
@@ -430,7 +430,8 @@ class Job():
                  self.message(MessageType.BUG, self.action_name,
                               elapsed=elapsed, detail=detail,
                               logfile=filename)
 -                self._child_shutdown(RC_FAIL)
 +                # Unhandled exceptions should permenantly fail
 +                self._child_shutdown(RC_PERM_FAIL)
              else:
                  # No exception occurred in the action
@@ -509,11 +510,6 @@ class Job():
          message.action_name = self.action_name
          message.task_id = self._task_id
 -        if (message.message_type == MessageType.FAIL and
 -                self._tries <= self._max_retries and self._retry_flag):
 -            # Job will be retried, display failures as warnings in the frontend
 -            message.message_type = MessageType.WARN
+-
          # Send to frontend if appropriate
          if context.silent_messages() and (message.message_type not in unconditional_messages):
              return

buildstream/_scheduler/queues/trackqueue.py

@@ -58,18 +58,9 @@ class TrackQueue(Queue):
          # Set the new refs in the main process one by one as they complete
          for unique_id, new_ref in result:
              source = _plugin_lookup(unique_id)
 -            try:
 -                # We appear processed if at least one source has changed
 -                if source._save_ref(new_ref):
 -                    changed = True
 -            except SourceError as e:
 -                # FIXME: We currently dont have a clear path to
 -                #        fail the scheduler from the main process, so
 -                #        this will just warn and BuildStream will exit
 -                #        with a success code.
 -                #
 -                source.warn("Failed to update project file",
 -                            detail="{}".format(e))
 +            # We appear processed if at least one source has changed
 +            if source._save_ref(new_ref):
 +                changed = True
          element._tracking_done()

buildstream/_scheduler/scheduler.py

@@ -329,7 +329,7 @@ class Scheduler():
          self.schedule_jobs([job])
      def _check_cache_size_real(self):
 -        job = CacheSizeJob(self, 'cache_size', 'cache_size',
 +        job = CacheSizeJob(self, 'cache_size', 'cache_size/cache_size',
                             resources=[ResourceType.CACHE,
                                        ResourceType.PROCESS],
                             exclusive_resources=[ResourceType.CACHE],

buildstream/element.py

@@ -245,7 +245,7 @@ class Element(Plugin):
          # Collect the composited element configuration and
          # ask the element to configure itself.
          self.__config = self.__extract_config(meta)
 -        self.configure(self.__config)
 +        self._configure(self.__config)
          # Extract Sandbox config
          self.__sandbox_config = self.__extract_sandbox_config(meta)

buildstream/plugin.py

@@ -179,6 +179,7 @@ class Plugin():
          self.__provenance = provenance  # The Provenance information
          self.__type_tag = type_tag      # The type of plugin (element or source)
          self.__unique_id = _plugin_register(self)  # Unique ID
 +        self.__configuring = False      # Whether we are currently configuring
          # Infer the kind identifier
          modulename = type(self).__module__
@@ -682,7 +683,32 @@ class Plugin():
          else:
              yield log
 +    # _configure():
 +    #
 +    # Calls configure() for the plugin, this must be called by
 +    # the core instead of configure() directly, so that the
 +    # _get_configuring() state is up to date.
 +    #
 +    # Args:
 +    #    node (dict): The loaded configuration dictionary
 +    #
 +    def _configure(self, node):
 +        self.__configuring = True
 +        self.configure(node)
 +        self.__configuring = False
++
 +    # _get_configuring():
 +    #
 +    # Checks whether the plugin is in the middle of having
 +    # its Plugin.configure() method called
 +    #
 +    # Returns:
 +    #    (bool): Whether we are currently configuring
 +    def _get_configuring(self):
 +        return self.__configuring
++
      # _preflight():
 +    #
      # Calls preflight() for the plugin, and allows generic preflight
      # checks to be added
+     #
@@ -690,6 +716,7 @@ class Plugin():
      #    SourceError: If it's a Source implementation
      #    ElementError: If it's an Element implementation
      #    ProgramNotFoundError: If a required host tool is not found
 +    #
      def _preflight(self):
          self.preflight()

buildstream/plugins/sources/git.py

@@ -74,6 +74,9 @@ This plugin provides the following configurable warnings:
  - 'git:inconsistent-submodule' - A submodule was found to be missing from the underlying git repository.
 +This plugin also utilises the following configurable core plugin warnings:
++
 +- 'ref-not-in-track' - The provided ref was not found in the provided track in the element's git repository.
  """
  import os
@@ -87,6 +90,7 @@ from configparser import RawConfigParser
  from buildstream import Source, SourceError, Consistency, SourceFetcher
  from buildstream import utils
 +from buildstream.plugin import CoreWarnings
  GIT_MODULES = '.gitmodules'
@@ -100,13 +104,14 @@ INCONSISTENT_SUBMODULE = "inconsistent-submodules"
+ #
  class GitMirror(SourceFetcher):
 -    def __init__(self, source, path, url, ref):
 +    def __init__(self, source, path, url, ref, *, primary=False):
          super().__init__()
          self.source = source
          self.path = path
          self.url = url
          self.ref = ref
 +        self.primary = primary
          self.mirror = os.path.join(source.get_mirror_directory(), utils.url_directory_name(url))
          self.mark_download_url(url)
@@ -124,7 +129,8 @@ class GitMirror(SourceFetcher):
              # system configured tmpdir is not on the same partition.
+             #
              with self.source.tempdir() as tmpdir:
 -                url = self.source.translate_url(self.url, alias_override=alias_override)
 +                url = self.source.translate_url(self.url, alias_override=alias_override,
 +                                                primary=self.primary)
                  self.source.call([self.source.host_git, 'clone', '--mirror', '-n', url, tmpdir],
                                   fail="Failed to clone git repository {}".format(url),
                                   fail_temporarily=True)
@@ -146,7 +152,9 @@ class GitMirror(SourceFetcher):
                                            .format(self.source, url, tmpdir, self.mirror, e)) from e
      def _fetch(self, alias_override=None):
 -        url = self.source.translate_url(self.url, alias_override=alias_override)
 +        url = self.source.translate_url(self.url,
 +                                        alias_override=alias_override,
 +                                        primary=self.primary)
          if alias_override:
              remote_name = utils.url_directory_name(alias_override)
@@ -199,7 +207,7 @@ class GitMirror(SourceFetcher):
              cwd=self.mirror)
          return output.rstrip('\n')
 -    def stage(self, directory):
 +    def stage(self, directory, track=None):
          fullpath = os.path.join(directory, self.path)
          # Using --shared here avoids copying the objects into the checkout, in any
@@ -213,10 +221,14 @@ class GitMirror(SourceFetcher):
                           fail="Failed to checkout git ref {}".format(self.ref),
                           cwd=fullpath)
 +        # Check that the user specified ref exists in the track if provided & not already tracked
 +        if track:
 +            self.assert_ref_in_track(fullpath, track)
++
          # Remove .git dir
          shutil.rmtree(os.path.join(fullpath, ".git"))
 -    def init_workspace(self, directory):
 +    def init_workspace(self, directory, track=None):
          fullpath = os.path.join(directory, self.path)
          url = self.source.translate_url(self.url)
@@ -232,6 +244,10 @@ class GitMirror(SourceFetcher):
                           fail="Failed to checkout git ref {}".format(self.ref),
                           cwd=fullpath)
 +        # Check that the user specified ref exists in the track if provided & not already tracked
 +        if track:
 +            self.assert_ref_in_track(fullpath, track)
++
      # List the submodules (path/url tuples) present at the given ref of this repo
      def submodule_list(self):
          modules = "{}:{}".format(self.ref, GIT_MODULES)
@@ -296,6 +312,28 @@ class GitMirror(SourceFetcher):
              return None
 +    # Assert that ref exists in track, if track has been specified.
 +    def assert_ref_in_track(self, fullpath, track):
 +        _, branch = self.source.check_output([self.source.host_git, 'branch', '--list', track,
 +                                              '--contains', self.ref],
 +                                             cwd=fullpath,)
 +        if branch:
 +            return True
 +        else:
 +            _, tag = self.source.check_output([self.source.host_git, 'tag', '--list', track,
 +                                               '--contains', self.ref],
 +                                              cwd=fullpath,)
 +            if tag:
 +                return True
++
 +        detail = "The ref provided for the element does not exist locally in the provided track branch / tag " + \
 +                 "'{}'.\nYou may wish to track the element to update the ref from '{}' ".format(track, track) + \
 +                 "with `bst track`,\nor examine the upstream at '{}' for the specific ref.".format(self.url)
++
 +        self.source.warn("{}: expected ref '{}' was not found in given track '{}' for staged repository: '{}'\n"
 +                         .format(self.source, self.ref, track, self.url),
 +                         detail=detail, warning_token=CoreWarnings.REF_NOT_IN_TRACK)
++
  class GitSource(Source):
      # pylint: disable=attribute-defined-outside-init
@@ -307,7 +345,7 @@ class GitSource(Source):
          self.node_validate(node, config_keys + Source.COMMON_CONFIG_KEYS)
          self.original_url = self.node_get_member(node, str, 'url')
 -        self.mirror = GitMirror(self, '', self.original_url, ref)
 +        self.mirror = GitMirror(self, '', self.original_url, ref, primary=True)
          self.tracking = self.node_get_member(node, str, 'track', None)
          # At this point we now know if the source has a ref and/or a track.
@@ -327,12 +365,18 @@ class GitSource(Source):
          for path, _ in self.node_items(modules):
              submodule = self.node_get_member(modules, Mapping, path)
              url = self.node_get_member(submodule, str, 'url', None)
++
 +            # Make sure to mark all URLs that are specified in the configuration
 +            if url:
 +                self.mark_download_url(url, primary=False)
++
              self.submodule_overrides[path] = url
              if 'checkout' in submodule:
                  checkout = self.node_get_member(submodule, bool, 'checkout')
                  self.submodule_checkout_overrides[path] = checkout
          self.mark_download_url(self.original_url)
 +        self.tracked = False
      def preflight(self):
          # Check if git is installed, get the binary at the same time
@@ -398,6 +442,8 @@ class GitSource(Source):
              # Update self.mirror.ref and node.ref from the self.tracking branch
              ret = self.mirror.latest_commit(self.tracking)
 +        # Set tracked attribute, parameter for if self.mirror.assert_ref_in_track is needed
 +        self.tracked = True
          return ret
      def init_workspace(self, directory):
@@ -405,7 +451,7 @@ class GitSource(Source):
          self.refresh_submodules()
          with self.timed_activity('Setting up workspace "{}"'.format(directory), silent_nested=True):
 -            self.mirror.init_workspace(directory)
 +            self.mirror.init_workspace(directory, track=(self.tracking if not self.tracked else None))
              for mirror in self.submodules:
                  mirror.init_workspace(directory)
@@ -421,7 +467,7 @@ class GitSource(Source):
          # Stage the main repo in the specified directory
+         #
          with self.timed_activity("Staging {}".format(self.mirror.url), silent_nested=True):
 -            self.mirror.stage(directory)
 +            self.mirror.stage(directory, track=(self.tracking if not self.tracked else None))
              for mirror in self.submodules:
                  if mirror.path in self.submodule_checkout_overrides:
                      checkout = self.submodule_checkout_overrides[mirror.path]

buildstream/source.py

@@ -28,6 +28,18 @@ Abstract Methods
  For loading and configuration purposes, Sources must implement the
  :ref:`Plugin base class abstract methods <core_plugin_abstract_methods>`.
 +.. attention::
++
 +   In order to ensure that all configuration data is processed at
 +   load time, it is important that all URLs have been processed during
 +   :func:`Plugin.configure() <buildstream.plugin.Plugin.configure>`.
++
 +   Source implementations *must* either call
 +   :func:`Source.translate_url() <buildstream.source.Source.translate_url>` or
 +   :func:`Source.mark_download_url() <buildstream.source.Source.mark_download_url>`
 +   for every URL that has been specified in the configuration during
 +   :func:`Plugin.configure() <buildstream.plugin.Plugin.configure>`
++
  Sources expose the following abstract methods. Unless explicitly mentioned,
  these methods are mandatory to implement.
@@ -184,6 +196,13 @@ class SourceFetcher():
      fetching and substituting aliases.
      *Since: 1.2*
++
 +    .. attention::
++
 +       When implementing a SourceFetcher, remember to call
 +       :func:`Source.mark_download_url() <buildstream.source.Source.mark_download_url>`
 +       for every URL found in the configuration data at
 +       :func:`Plugin.configure() <buildstream.plugin.Plugin.configure>` time.
      """
      def __init__(self):
          self.__alias = None
@@ -206,7 +225,7 @@ class SourceFetcher():
          Implementors should raise :class:`.SourceError` if the there is some
          network error or if the source reference could not be matched.
          """
 -        raise ImplError("Source fetcher '{}' does not implement fetch()".format(type(self)))
 +        raise ImplError("SourceFetcher '{}' does not implement fetch()".format(type(self)))
      #############################################################
      #                       Public Methods                      #
@@ -277,8 +296,11 @@ class Source(Plugin):
          self.__element_kind = meta.element_kind         # The kind of the element owning this source
          self.__directory = meta.directory               # Staging relative directory
          self.__consistency = Consistency.INCONSISTENT   # Cached consistency state
++
 +        # The alias_override is only set on a re-instantiated Source
          self.__alias_override = alias_override          # Tuple of alias and its override to use instead
 -        self.__expected_alias = None                    # A hacky way to store the first alias used
 +        self.__expected_alias = None                    # The primary alias
 +        self.__marked_urls = set()                      # Set of marked download URLs
          # FIXME: Reconstruct a MetaSource from a Source instead of storing it.
          self.__meta = meta                              # MetaSource stored so we can copy this source later.
@@ -289,7 +311,7 @@ class Source(Plugin):
          self.__config = self.__extract_config(meta)
          self.__first_pass = meta.first_pass
 -        self.configure(self.__config)
 +        self._configure(self.__config)
      COMMON_CONFIG_KEYS = ['kind', 'directory']
      """Common source config keys
@@ -351,10 +373,10 @@ class Source(Plugin):
          Args:
             ref (simple object): The internal source reference to set, or ``None``
             node (dict): The same dictionary which was previously passed
 -                        to :func:`~buildstream.source.Source.configure`
 +                        to :func:`Plugin.configure() <buildstream.plugin.Plugin.configure>`
 -        See :func:`~buildstream.source.Source.get_ref` for a discussion on
 -        the *ref* parameter.
 +        See :func:`Source.get_ref() <buildstream.source.Source.get_ref>`
 +        for a discussion on the *ref* parameter.
          .. note::
@@ -384,8 +406,8 @@ class Source(Plugin):
          backend store allows one to query for a new ref from a symbolic
          tracking data without downloading then that is desirable.
 -        See :func:`~buildstream.source.Source.get_ref` for a discussion on
 -        the *ref* parameter.
 +        See :func:`Source.get_ref() <buildstream.source.Source.get_ref>`
 +        for a discussion on the *ref* parameter.
          """
          # Allow a non implementation
          return None
@@ -435,7 +457,7 @@ class Source(Plugin):
             :class:`.SourceError`
          Default implementation is to call
 -        :func:`~buildstream.source.Source.stage`.
 +        :func:`Source.stage() <buildstream.source.Source.stage>`.
          Implementors overriding this method should assume that *directory*
          already exists.
@@ -453,8 +475,15 @@ class Source(Plugin):
          is recommended.
          Returns:
 -           list: A list of SourceFetchers. If SourceFetchers are not supported,
 -                 this will be an empty list.
 +           iterable: The Source's SourceFetchers, if any.
++
 +        .. note::
++
 +           Implementors can implement this as a generator.
++
 +           The :func:`SourceFetcher.fetch() <buildstream.source.SourceFetcher.fetch>`
 +           method will be called on the returned fetchers one by one,
 +           before consuming the next fetcher in the list.
          *Since: 1.2*
          """
@@ -477,17 +506,27 @@ class Source(Plugin):
          os.makedirs(directory, exist_ok=True)
          return directory
 -    def translate_url(self, url, *, alias_override=None):
 +    def translate_url(self, url, *, alias_override=None, primary=True):
          """Translates the given url which may be specified with an alias
          into a fully qualified url.
          Args:
 -           url (str): A url, which may be using an alias
 +           url (str): A URL, which may be using an alias
             alias_override (str): Optionally, an URI to override the alias with. (*Since: 1.2*)
 +           primary (bool): Whether this is the primary URL for the source. (*Since: 1.2*)
          Returns:
 -           str: The fully qualified url, with aliases resolved
 +           str: The fully qualified URL, with aliases resolved
 +        .. note::
++
 +           This must be called for every URL in the configuration during
 +           :func:`Plugin.configure() <buildstream.plugin.Plugin.configure>` if
 +           :func:`Source.mark_download_url() <buildstream.source.Source.mark_download_url>`
 +           is not called.
          """
 +        # Ensure that the download URL is also marked
 +        self.mark_download_url(url, primary=primary)
++
          # Alias overriding can happen explicitly (by command-line) or
          # implicitly (the Source being constructed with an __alias_override).
          if alias_override or self.__alias_override:
@@ -506,25 +545,55 @@ class Source(Plugin):
                          url = override_url + url_body
              return url
          else:
 -            # Sneakily store the alias if it hasn't already been stored
 -            if not self.__expected_alias and url and utils._ALIAS_SEPARATOR in url:
 -                self.mark_download_url(url)
+-
              project = self._get_project()
              return project.translate_url(url, first_pass=self.__first_pass)
 -    def mark_download_url(self, url):
 +    def mark_download_url(self, url, *, primary=True):
          """Identifies the URL that this Source uses to download
 -        This must be called during :func:`~buildstream.plugin.Plugin.configure` if
 -        :func:`~buildstream.source.Source.translate_url` is not called.
+-
          Args:
 -           url (str): The url used to download
 +           url (str): The URL used to download
 +           primary (bool): Whether this is the primary URL for the source
++
 +        .. note::
++
 +           This must be called for every URL in the configuration during
 +           :func:`Plugin.configure() <buildstream.plugin.Plugin.configure>` if
 +           :func:`Source.translate_url() <buildstream.source.Source.translate_url>`
 +           is not called.
          *Since: 1.2*
          """
 -        self.__expected_alias = _extract_alias(url)
 +        # Only mark the Source level aliases on the main instance, not in
 +        # a reinstantiated instance in mirroring.
 +        if not self.__alias_override:
 +            if primary:
 +                expected_alias = _extract_alias(url)
++
 +                assert (self.__expected_alias is None or
 +                        self.__expected_alias == expected_alias), \
 +                    "Primary URL marked twice with different URLs"
++
 +                self.__expected_alias = expected_alias
++
 +        # Enforce proper behaviour of plugins by ensuring that all
 +        # aliased URLs have been marked at Plugin.configure() time.
 +        #
 +        if self._get_configuring():
 +            # Record marked urls while configuring
 +            #
 +            self.__marked_urls.add(url)
 +        else:
 +            # If an unknown aliased URL is seen after configuring,
 +            # this is an error.
 +            #
 +            # It is still possible that a URL that was not mentioned
 +            # in the element configuration can be marked, this is
 +            # the case for git submodules which might be automatically
 +            # discovered.
 +            #
 +            assert (url in self.__marked_urls or not _extract_alias(url)), \
 +                "URL was not seen at configure time: {}".format(url)
      def get_project_directory(self):
          """Fetch the project base directory

tests/frontend/project/sources/fetch_source.py

@@ -15,14 +15,17 @@ from buildstream import Source, Consistency, SourceError, SourceFetcher
  class FetchFetcher(SourceFetcher):
 -    def __init__(self, source, url):
 +    def __init__(self, source, url, primary=False):
          super().__init__()
          self.source = source
          self.original_url = url
 +        self.primary = primary
          self.mark_download_url(url)
      def fetch(self, alias_override=None):
 -        url = self.source.translate_url(self.original_url, alias_override=alias_override)
 +        url = self.source.translate_url(self.original_url,
 +                                        alias_override=alias_override,
 +                                        primary=self.primary)
          with open(self.source.output_file, "a") as f:
              success = url in self.source.fetch_succeeds and self.source.fetch_succeeds[url]
              message = "Fetch {} {} from {}\n".format(self.original_url,
@@ -37,12 +40,21 @@ class FetchSource(Source):
      # Read config to know which URLs to fetch
      def configure(self, node):
          self.original_urls = self.node_get_member(node, list, 'urls')
 -        self.fetchers = [FetchFetcher(self, url) for url in self.original_urls]
          self.output_file = self.node_get_member(node, str, 'output-text')
          self.fetch_succeeds = {}
          if 'fetch-succeeds' in node:
              self.fetch_succeeds = {x[0]: x[1] for x in self.node_items(node['fetch-succeeds'])}
 +        # First URL is the primary one for this test
 +        #
 +        primary = True
 +        self.fetchers = []
 +        for url in self.original_urls:
 +            self.mark_download_url(url, primary=primary)
 +            fetcher = FetchFetcher(self, url, primary=primary)
 +            self.fetchers.append(fetcher)
 +            primary = False
++
      def get_source_fetchers(self):
          return self.fetchers

tests/frontend/track.py

 +import stat
  import os
  import pytest
  from tests.testutils import cli, create_repo, ALL_REPO_KINDS, generate_junction
@@ -634,3 +635,36 @@ def test_track_junction_included(cli, tmpdir, datafiles, ref_storage, kind):
      result = cli.run(project=project, args=['track', 'junction.bst'])
      result.assert_success()
++
++
 +@pytest.mark.datafiles(DATA_DIR)
 +@pytest.mark.parametrize("kind", [(kind) for kind in ALL_REPO_KINDS])
 +def test_track_error_cannot_write_file(cli, tmpdir, datafiles, kind):
 +    if os.geteuid() == 0:
 +        pytest.skip("This is not testable with root permissions")
++
 +    project = str(datafiles)
 +    dev_files_path = os.path.join(project, 'files', 'dev-files')
 +    element_path = os.path.join(project, 'elements')
 +    element_name = 'track-test-{}.bst'.format(kind)
++
 +    configure_project(project, {
 +        'ref-storage': 'inline'
 +    })
++
 +    repo = create_repo(kind, str(tmpdir))
 +    ref = repo.create(dev_files_path)
++
 +    element_full_path = os.path.join(element_path, element_name)
 +    generate_element(repo, element_full_path)
++
 +    st = os.stat(element_path)
 +    try:
 +        read_mask = stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH
 +        os.chmod(element_path, stat.S_IMODE(st.st_mode) & ~read_mask)
++
 +        result = cli.run(project=project, args=['track', element_name])
 +        result.assert_main_error(ErrorDomain.STREAM, None)
 +        result.assert_task_error(ErrorDomain.SOURCE, 'save-ref-error')
 +    finally:
 +        os.chmod(element_path, stat.S_IMODE(st.st_mode))

tests/sources/deb.py

@@ -56,7 +56,7 @@ def test_fetch_bad_url(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=[
          'fetch', 'target.bst'
      ])
 -    assert "Try #" in result.stderr
 +    assert "FAILURE Try #" in result.stderr
      result.assert_main_error(ErrorDomain.STREAM, None)
      result.assert_task_error(ErrorDomain.SOURCE, None)

tests/sources/git.py

@@ -25,6 +25,7 @@ import pytest
  from buildstream._exceptions import ErrorDomain
  from buildstream import _yaml
 +from buildstream.plugin import CoreWarnings
  from tests.testutils import cli, create_repo
  from tests.testutils.site import HAVE_GIT
@@ -408,3 +409,70 @@ def test_submodule_track_no_ref_or_track(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=['show', 'target.bst'])
      result.assert_main_error(ErrorDomain.SOURCE, "missing-track-and-ref")
      result.assert_task_error(None, None)
++
++
 +@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
 +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
 +def test_ref_not_in_track_warn(cli, tmpdir, datafiles):
 +    project = os.path.join(datafiles.dirname, datafiles.basename)
++
 +    # Create the repo from 'repofiles', create a branch without latest commit
 +    repo = create_repo('git', str(tmpdir))
 +    ref = repo.create(os.path.join(project, 'repofiles'))
++
 +    gitsource = repo.source_config(ref=ref)
++
 +    # Overwrite the track value to the added branch
 +    gitsource['track'] = 'foo'
++
 +    # Write out our test target
 +    element = {
 +        'kind': 'import',
 +        'sources': [
 +            gitsource
 +        ]
 +    }
 +    _yaml.dump(element, os.path.join(project, 'target.bst'))
++
 +    # Assert the warning is raised as ref is not in branch foo.
 +    # Assert warning not error to the user, when not set as fatal.
 +    result = cli.run(project=project, args=['build', 'target.bst'])
 +    assert "The ref provided for the element does not exist locally" in result.stderr
++
++
 +@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
 +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
 +def test_ref_not_in_track_warn_error(cli, tmpdir, datafiles):
 +    project = os.path.join(datafiles.dirname, datafiles.basename)
++
 +    # Add fatal-warnings ref-not-in-track to project.conf
 +    project_template = {
 +        "name": "foo",
 +        "fatal-warnings": [CoreWarnings.REF_NOT_IN_TRACK]
 +    }
++
 +    _yaml.dump(project_template, os.path.join(project, 'project.conf'))
++
 +    # Create the repo from 'repofiles', create a branch without latest commit
 +    repo = create_repo('git', str(tmpdir))
 +    ref = repo.create(os.path.join(project, 'repofiles'))
++
 +    gitsource = repo.source_config(ref=ref)
++
 +    # Overwrite the track value to the added branch
 +    gitsource['track'] = 'foo'
++
 +    # Write out our test target
 +    element = {
 +        'kind': 'import',
 +        'sources': [
 +            gitsource
 +        ]
 +    }
 +    _yaml.dump(element, os.path.join(project, 'target.bst'))
++
 +    # Assert that build raises a warning here that is captured
 +    # as plugin error, due to the fatal warning being set
 +    result = cli.run(project=project, args=['build', 'target.bst'])
 +    result.assert_main_error(ErrorDomain.STREAM, None)
 +    result.assert_task_error(ErrorDomain.PLUGIN, CoreWarnings.REF_NOT_IN_TRACK)

tests/sources/tar.py

@@ -67,7 +67,7 @@ def test_fetch_bad_url(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=[
          'fetch', 'target.bst'
      ])
 -    assert "Try #" in result.stderr
 +    assert "FAILURE Try #" in result.stderr
      result.assert_main_error(ErrorDomain.STREAM, None)
      result.assert_task_error(ErrorDomain.SOURCE, None)

tests/sources/zip.py

@@ -53,7 +53,7 @@ def test_fetch_bad_url(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=[
          'fetch', 'target.bst'
      ])
 -    assert "Try #" in result.stderr
 +    assert "FAILURE Try #" in result.stderr
      result.assert_main_error(ErrorDomain.STREAM, None)
      result.assert_task_error(ErrorDomain.SOURCE, None)

tests/testutils/repo/bzr.py

@@ -2,6 +2,7 @@ import os
  import subprocess
  import pytest
 +from buildstream import utils
  from .repo import Repo
  from ..site import HAVE_BZR
@@ -16,15 +17,16 @@ class Bzr(Repo):
          if not HAVE_BZR:
              pytest.skip("bzr is not available")
          super(Bzr, self).__init__(directory, subdir)
 +        self.bzr = utils.get_host_tool('bzr')
      def create(self, directory):
          branch_dir = os.path.join(self.repo, 'trunk')
 -        subprocess.call(['bzr', 'init-repo', self.repo], env=BZR_ENV)
 -        subprocess.call(['bzr', 'init', branch_dir], env=BZR_ENV)
 +        subprocess.call([self.bzr, 'init-repo', self.repo], env=BZR_ENV)
 +        subprocess.call([self.bzr, 'init', branch_dir], env=BZR_ENV)
          self.copy_directory(directory, branch_dir)
 -        subprocess.call(['bzr', 'add', '.'], env=BZR_ENV, cwd=branch_dir)
 -        subprocess.call(['bzr', 'commit', '--message="Initial commit"'],
 +        subprocess.call([self.bzr, 'add', '.'], env=BZR_ENV, cwd=branch_dir)
 +        subprocess.call([self.bzr, 'commit', '--message="Initial commit"'],
                          env=BZR_ENV, cwd=branch_dir)
          return self.latest_commit()
@@ -42,7 +44,7 @@ class Bzr(Repo):
      def latest_commit(self):
          output = subprocess.check_output([
 -            'bzr', 'version-info',
 +            self.bzr, 'version-info',
              '--custom', '--template={revno}',
              os.path.join(self.repo, 'trunk')
          ], env=BZR_ENV)

[Notes] [Git][BuildStream/buildstream][tpollard/483] 23 commits: Report processing errors from tracking

Tom Pollard pushed to branch tpollard/483 at BuildStream / buildstream

Commits:

16 changed files:

Changes: