Merge branch 'tristan/bst-1/submodule-warnings' into 'bst-1'

Backport git submodule warnings

See merge request BuildStream/buildstream!1515

6 files changed, 344 insertions, 46 deletions

diff --git a/NEWS b/NEWS
index fdf3b403c..6227faa17 100644
--- a/NEWS
+++ b/NEWS
@@ -23,6 +23,12 @@ buildstream 1.3.2
       o git:inconsistent-submodule: A .gitmodules file is present but the
                                     submodule was never added to the repo.
 
+      o git:unlisted-submodule: A submodule exists but is not specified
+                                in the YAML declaration.
+
+      o git:invalid-submodule: A submodule is specified in the YAML
+                               declaration but does not exist at the
+			       given ref in the git repository.
 
 =================
 buildstream 1.3.1
diff --git a/buildstream/_versions.py b/buildstream/_versions.py
index e4d503929..e55296513 100644
--- a/buildstream/_versions.py
+++ b/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 = 16
+BST_FORMAT_VERSION = 17
 
 
 # The base BuildStream artifact version
diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py
index 4b099b5ab..542fc8011 100644
--- a/buildstream/plugins/sources/git.py
+++ b/buildstream/plugins/sources/git.py
@@ -72,7 +72,22 @@ git - stage files from a git repository
 
 This plugin provides the following configurable warnings:
 
-- 'git:inconsistent-submodule' - A submodule was found to be missing from the underlying git repository.
+- ``git:inconsistent-submodule`` - A submodule present in the git repository's .gitmodules was never
+  added with `git submodule add`.
+
+- ``git:unlisted-submodule`` - A submodule is present in the git repository but was not specified in
+  the source configuration and was not disabled for checkout.
+
+  .. note::
+
+     The ``git:unlisted-submodule`` warning is available since :ref:`format version 20 <project_format_version>`
+
+- ``git:invalid-submodule`` - A submodule is specified in the source configuration but does not exist
+  in the repository.
+
+  .. note::
+
+     The ``git:invalid-submodule`` warning is available since :ref:`format version 20 <project_format_version>`
 
 This plugin also utilises the following configurable core plugin warnings:
 
@@ -94,7 +109,9 @@ from buildstream.plugin import CoreWarnings
 GIT_MODULES = '.gitmodules'
 
 # Warnings
-INCONSISTENT_SUBMODULE = "inconsistent-submodules"
+WARN_INCONSISTENT_SUBMODULE = "inconsistent-submodule"
+WARN_UNLISTED_SUBMODULE = "unlisted-submodule"
+WARN_INVALID_SUBMODULE = "invalid-submodule"
 
 
 # Because of handling of submodules, we maintain a GitMirror
@@ -214,7 +231,7 @@ class GitMirror(SourceFetcher):
             cwd=self.mirror)
         return output.rstrip('\n')
 
-    def stage(self, directory, track=None):
+    def stage(self, directory):
         fullpath = os.path.join(directory, self.path)
 
         # We need to pass '--no-hardlinks' because there's nothing to
@@ -228,11 +245,7 @@ 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)
-
-    def init_workspace(self, directory, track=None):
+    def init_workspace(self, directory):
         fullpath = os.path.join(directory, self.path)
         url = self.source.translate_url(self.url)
 
@@ -248,10 +261,6 @@ 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)
@@ -312,32 +321,10 @@ class GitMirror(SourceFetcher):
                      "underlying git repository with `git submodule add`."
 
             self.source.warn("{}: Ignoring inconsistent submodule '{}'"
-                             .format(self.source, submodule), detail=detail, warning_token=INCONSISTENT_SUBMODULE)
+                             .format(self.source, submodule), detail=detail, warning_token=WARN_INCONSISTENT_SUBMODULE)
 
             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
@@ -380,7 +367,6 @@ class GitSource(Source):
                 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
@@ -440,8 +426,6 @@ 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):
@@ -449,7 +433,7 @@ class GitSource(Source):
         self.refresh_submodules()
 
         with self.timed_activity('Setting up workspace "{}"'.format(directory), silent_nested=True):
-            self.mirror.init_workspace(directory, track=(self.tracking if not self.tracked else None))
+            self.mirror.init_workspace(directory)
             for mirror in self.submodules:
                 mirror.init_workspace(directory)
 
@@ -465,7 +449,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, track=(self.tracking if not self.tracked else None))
+            self.mirror.stage(directory)
             for mirror in self.submodules:
                 mirror.stage(directory)
 
@@ -475,6 +459,74 @@ class GitSource(Source):
         for submodule in self.submodules:
             yield submodule
 
+    def validate_cache(self):
+        discovered_submodules = {}
+        unlisted_submodules = []
+        invalid_submodules = []
+
+        for path, url in self.mirror.submodule_list():
+            discovered_submodules[path] = url
+            if self.ignore_submodule(path):
+                continue
+
+            override_url = self.submodule_overrides.get(path)
+            if not override_url:
+                unlisted_submodules.append((path, url))
+
+        # Warn about submodules which are explicitly configured but do not exist
+        for path, url in self.submodule_overrides.items():
+            if path not in discovered_submodules:
+                invalid_submodules.append((path, url))
+
+        if invalid_submodules:
+            detail = []
+            for path, url in invalid_submodules:
+                detail.append("  Submodule URL '{}' at path '{}'".format(url, path))
+
+            self.warn("{}: Invalid submodules specified".format(self),
+                      warning_token=WARN_INVALID_SUBMODULE,
+                      detail="The following submodules are specified in the source "
+                      "description but do not exist according to the repository\n\n" +
+                      "\n".join(detail))
+
+        # Warn about submodules which exist but have not been explicitly configured
+        if unlisted_submodules:
+            detail = []
+            for path, url in unlisted_submodules:
+                detail.append("  Submodule URL '{}' at path '{}'".format(url, path))
+
+            self.warn("{}: Unlisted submodules exist".format(self),
+                      warning_token=WARN_UNLISTED_SUBMODULE,
+                      detail="The following submodules exist but are not specified " +
+                      "in the source description\n\n" +
+                      "\n".join(detail))
+
+        # Assert that the ref exists in the track tag/branch, if track has been specified.
+        ref_in_track = False
+        if self.tracking:
+            _, branch = self.check_output([self.host_git, 'branch', '--list', self.tracking,
+                                           '--contains', self.mirror.ref],
+                                          cwd=self.mirror.mirror)
+            if branch:
+                ref_in_track = True
+            else:
+                _, tag = self.check_output([self.host_git, 'tag', '--list', self.tracking,
+                                            '--contains', self.mirror.ref],
+                                           cwd=self.mirror.mirror)
+                if tag:
+                    ref_in_track = True
+
+            if not ref_in_track:
+                detail = "The ref provided for the element does not exist locally " + \
+                         "in the provided track branch / tag '{}'.\n".format(self.tracking) + \
+                         "You may wish to track the element to update the ref from '{}' ".format(self.tracking) + \
+                         "with `bst track`,\n" + \
+                         "or examine the upstream at '{}' for the specific ref.".format(self.mirror.url)
+
+                self.warn("{}: expected ref '{}' was not found in given track '{}' for staged repository: '{}'\n"
+                          .format(self, self.mirror.ref, self.tracking, self.mirror.url),
+                          detail=detail, warning_token=CoreWarnings.REF_NOT_IN_TRACK)
+
     ###########################################################
     #                     Local Functions                     #
     ###########################################################
@@ -499,10 +551,6 @@ class GitSource(Source):
         self.mirror.ensure()
         submodules = []
 
-        # XXX Here we should issue a warning if either:
-        #   A.) A submodule exists but is not defined in the element configuration
-        #   B.) The element configuration configures submodules which dont exist at the current ref
-        #
         for path, url in self.mirror.submodule_list():
 
             # Completely ignore submodules which are disabled for checkout
@@ -532,6 +580,16 @@ class GitSource(Source):
 
         return not checkout
 
+    # Checks whether the plugin configuration has explicitly
+    # configured this submodule to be ignored
+    def ignore_submodule(self, path):
+        try:
+            checkout = self.submodule_checkout_overrides[path]
+        except KeyError:
+            checkout = self.checkout_submodules
+
+        return not checkout
+
 
 # Plugin entry point
 def setup():
diff --git a/buildstream/source.py b/buildstream/source.py
index ed4dd9617..24e5b6c5b 100644
--- a/buildstream/source.py
+++ b/buildstream/source.py
@@ -87,6 +87,11 @@ these methods are mandatory to implement.
   submodules). For details on how to define a SourceFetcher, see
   :ref:`SourceFetcher <core_source_fetcher>`.
 
+* :func:`Source.validate_cache() <buildstream.source.Source.validate_cache>`
+
+  Perform any validations which require the sources to be cached.
+
+  **Optional**: This is completely optional and will do nothing if left unimplemented.
 
 Accessing previous sources
 --------------------------
@@ -487,9 +492,22 @@ class Source(Plugin):
 
         *Since: 1.2*
         """
-
         return []
 
+    def validate_cache(self):
+        """Implement any validations once we know the sources are cached
+
+        This is guaranteed to be called only once for a given session
+        once the sources are known to be
+        :attr:`Consistency.CACHED <buildstream.types.Consistency.CACHED>`,
+        if source tracking is enabled in the session for this source,
+        then this will only be called if the sources become cached after
+        tracking completes.
+
+        *Since: 1.4*
+        """
+        pass
+
     #############################################################
     #                       Public Methods                      #
     #############################################################
@@ -649,6 +667,11 @@ class Source(Plugin):
             with context.silence():
                 self.__consistency = self.get_consistency()
 
+                # Give the Source an opportunity to validate the cached
+                # sources as soon as the Source becomes Consistency.CACHED.
+                if self.__consistency == Consistency.CACHED:
+                    self.validate_cache()
+
     # Return cached consistency
     #
     def _get_consistency(self):
diff --git a/tests/sources/git.py b/tests/sources/git.py
index db433f813..e7daa95a7 100644
--- a/tests/sources/git.py
+++ b/tests/sources/git.py
@@ -481,6 +481,211 @@ def test_ref_not_in_track_warn_error(cli, tmpdir, datafiles):
 
 @pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
 @pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
+@pytest.mark.parametrize("fail", ['warn', 'error'])
+def test_unlisted_submodule(cli, tmpdir, datafiles, fail):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+
+    # Make the warning an error if we're testing errors
+    if fail == 'error':
+        project_template = {
+            "name": "foo",
+            "fatal-warnings": ['git:unlisted-submodule']
+        }
+        _yaml.dump(project_template, os.path.join(project, 'project.conf'))
+
+    # Create the submodule first from the 'subrepofiles' subdir
+    subrepo = create_repo('git', str(tmpdir), 'subrepo')
+    subrepo.create(os.path.join(project, 'subrepofiles'))
+
+    # Create the repo from 'repofiles' subdir
+    repo = create_repo('git', str(tmpdir))
+    ref = repo.create(os.path.join(project, 'repofiles'))
+
+    # Add a submodule pointing to the one we created
+    ref = repo.add_submodule('subdir', 'file://' + subrepo.repo)
+
+    # Create the source, and delete the explicit configuration
+    # of the submodules.
+    #
+    # We expect this to cause an unlisted submodule warning
+    # after the source has been fetched.
+    #
+    gitsource = repo.source_config(ref=ref)
+    del gitsource['submodules']
+
+    # Write out our test target
+    element = {
+        'kind': 'import',
+        'sources': [
+            gitsource
+        ]
+    }
+    _yaml.dump(element, os.path.join(project, 'target.bst'))
+
+    # We will not see the warning or error before the first fetch, because
+    # we don't have the repository yet and so we have no knowledge of
+    # the unlisted submodule.
+    result = cli.run(project=project, args=['show', 'target.bst'])
+    result.assert_success()
+    assert "git:unlisted-submodule" not in result.stderr
+
+    # We will notice this directly in fetch, as it will try to fetch
+    # the submodules it discovers as a result of fetching the primary repo.
+    result = cli.run(project=project, args=['fetch', 'target.bst'])
+
+    # Assert a warning or an error depending on what we're checking
+    if fail == 'error':
+        result.assert_main_error(ErrorDomain.STREAM, None)
+        result.assert_task_error(ErrorDomain.PLUGIN, 'git:unlisted-submodule')
+    else:
+        result.assert_success()
+        assert "git:unlisted-submodule" in result.stderr
+
+    # Now that we've fetched it, `bst show` will discover the unlisted submodule too
+    result = cli.run(project=project, args=['show', 'target.bst'])
+
+    # Assert a warning or an error depending on what we're checking
+    if fail == 'error':
+        result.assert_main_error(ErrorDomain.PLUGIN, 'git:unlisted-submodule')
+    else:
+        result.assert_success()
+        assert "git:unlisted-submodule" in result.stderr
+
+
+@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
+@pytest.mark.parametrize("fail", ['warn', 'error'])
+def test_invalid_submodule(cli, tmpdir, datafiles, fail):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+
+    # Make the warning an error if we're testing errors
+    if fail == 'error':
+        project_template = {
+            "name": "foo",
+            "fatal-warnings": ['git:invalid-submodule']
+        }
+        _yaml.dump(project_template, os.path.join(project, 'project.conf'))
+
+    # Create the repo from 'repofiles' subdir
+    repo = create_repo('git', str(tmpdir))
+    ref = repo.create(os.path.join(project, 'repofiles'))
+
+    # Create the source without any submodules, and add
+    # an invalid submodule configuration to it.
+    #
+    # We expect this to cause an invalid submodule warning
+    # after the source has been fetched and we know what
+    # the real submodules actually are.
+    #
+    gitsource = repo.source_config(ref=ref)
+    gitsource['submodules'] = {
+        'subdir': {
+            'url': 'https://pony.org/repo.git'
+        }
+    }
+
+    # Write out our test target
+    element = {
+        'kind': 'import',
+        'sources': [
+            gitsource
+        ]
+    }
+    _yaml.dump(element, os.path.join(project, 'target.bst'))
+
+    # We will not see the warning or error before the first fetch, because
+    # we don't have the repository yet and so we have no knowledge of
+    # the unlisted submodule.
+    result = cli.run(project=project, args=['show', 'target.bst'])
+    result.assert_success()
+    assert "git:invalid-submodule" not in result.stderr
+
+    # We will notice this directly in fetch, as it will try to fetch
+    # the submodules it discovers as a result of fetching the primary repo.
+    result = cli.run(project=project, args=['fetch', 'target.bst'])
+
+    # Assert a warning or an error depending on what we're checking
+    if fail == 'error':
+        result.assert_main_error(ErrorDomain.STREAM, None)
+        result.assert_task_error(ErrorDomain.PLUGIN, 'git:invalid-submodule')
+    else:
+        result.assert_success()
+        assert "git:invalid-submodule" in result.stderr
+
+    # Now that we've fetched it, `bst show` will discover the unlisted submodule too
+    result = cli.run(project=project, args=['show', 'target.bst'])
+
+    # Assert a warning or an error depending on what we're checking
+    if fail == 'error':
+        result.assert_main_error(ErrorDomain.PLUGIN, 'git:invalid-submodule')
+    else:
+        result.assert_success()
+        assert "git:invalid-submodule" in result.stderr
+
+
+@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
+@pytest.mark.parametrize("fail", ['warn', 'error'])
+def test_track_invalid_submodule(cli, tmpdir, datafiles, fail):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+
+    # Make the warning an error if we're testing errors
+    if fail == 'error':
+        project_template = {
+            "name": "foo",
+            "fatal-warnings": ['git:invalid-submodule']
+        }
+        _yaml.dump(project_template, os.path.join(project, 'project.conf'))
+
+    # Create the submodule first from the 'subrepofiles' subdir
+    subrepo = create_repo('git', str(tmpdir), 'subrepo')
+    subrepo.create(os.path.join(project, 'subrepofiles'))
+
+    # Create the repo from 'repofiles' subdir
+    repo = create_repo('git', str(tmpdir))
+    ref = repo.create(os.path.join(project, 'repofiles'))
+
+    # Add a submodule pointing to the one we created
+    ref = repo.add_submodule('subdir', 'file://' + subrepo.repo)
+
+    # Add a commit beyond the ref which *removes* the submodule we've added
+    repo.remove_path('subdir')
+
+    # Create the source, this will keep the submodules so initially
+    # the configuration is valid for the ref we're using
+    gitsource = repo.source_config(ref=ref)
+
+    # Write out our test target
+    element = {
+        'kind': 'import',
+        'sources': [
+            gitsource
+        ]
+    }
+    _yaml.dump(element, os.path.join(project, 'target.bst'))
+
+    # Fetch the repo, we will not see the warning because we
+    # are still pointing to a ref which predates the submodules
+    result = cli.run(project=project, args=['fetch', 'target.bst'])
+    result.assert_success()
+    assert "git:invalid-submodule" not in result.stderr
+
+    # In this case, we will get the error directly after tracking,
+    # since the new HEAD does not require any submodules which are
+    # not locally cached, the Source will be CACHED directly after
+    # tracking and the validations will occur as a result.
+    #
+    result = cli.run(project=project, args=['track', 'target.bst'])
+    if fail == 'error':
+        result.assert_main_error(ErrorDomain.STREAM, None)
+        result.assert_task_error(ErrorDomain.PLUGIN, 'git:invalid-submodule')
+    else:
+        result.assert_success()
+        assert "git:invalid-submodule" 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_overwrite_rogue_tag_multiple_remotes(cli, tmpdir, datafiles):
     """When using multiple remotes in cache (i.e. when using aliases), we
     need to make sure we override tags. This is not allowed to fetch
diff --git a/tests/testutils/repo/git.py b/tests/testutils/repo/git.py
index 99b6bb384..5587baacb 100644
--- a/tests/testutils/repo/git.py
+++ b/tests/testutils/repo/git.py
@@ -64,6 +64,12 @@ class Git(Repo):
         subprocess.call(['git', 'commit', '-m', 'Added the submodule'], env=GIT_ENV, cwd=self.repo)
         return self.latest_commit()
 
+    # This can also be used to a file or a submodule
+    def remove_path(self, path):
+        subprocess.call(['git', 'rm', path], env=GIT_ENV, cwd=self.repo)
+        subprocess.call(['git', 'commit', '-m', 'Removing {}'.format(path)], env=GIT_ENV, cwd=self.repo)
+        return self.latest_commit()
+
     def source_config(self, ref=None, checkout_submodules=None):
         config = {
             'kind': 'git',