Merge branch 'tristan/source-fetcher-changes-1.2' into 'bst-1.2'

Source fetcher changes 1.2

See merge request BuildStream/buildstream!773

5 files changed, 149 insertions, 32 deletions

diff --git a/buildstream/element.py b/buildstream/element.py
index 70e12313c..6a6d1a9b8 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -262,7 +262,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)
diff --git a/buildstream/plugin.py b/buildstream/plugin.py
index 836b60834..f57c0e15c 100644
--- a/buildstream/plugin.py
+++ b/buildstream/plugin.py
@@ -162,6 +162,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__
@@ -651,7 +652,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
     #
@@ -659,6 +685,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()
 
diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py
index 354f80b5d..065870169 100644
--- a/buildstream/plugins/sources/git.py
+++ b/buildstream/plugins/sources/git.py
@@ -90,13 +90,14 @@ GIT_MODULES = '.gitmodules'
 #
 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)
 
@@ -114,7 +115,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)
@@ -136,7 +138,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)
@@ -294,7 +298,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.
@@ -314,6 +318,11 @@ 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')
diff --git a/buildstream/source.py b/buildstream/source.py
index 9f8f4ffdb..f546258e6 100644
--- a/buildstream/source.py
+++ b/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.
 
@@ -149,6 +161,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
@@ -171,7 +190,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                      #
@@ -216,8 +235,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.
@@ -228,7 +250,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
@@ -290,10 +312,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::
 
@@ -317,8 +339,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
@@ -362,7 +384,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.
@@ -380,8 +402,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*
         """
@@ -404,17 +433,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:
@@ -433,25 +472,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
diff --git a/tests/frontend/project/sources/fetch_source.py b/tests/frontend/project/sources/fetch_source.py
index ebd3fe757..10e89960c 100644
--- a/tests/frontend/project/sources/fetch_source.py
+++ b/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