Merge branch 'tristan/submodule-warnings' into 'master'

Implement submodule warnings

See merge request BuildStream/buildstream!996

5 files changed, 404 insertions, 51 deletions

diff --git a/buildstream/_versions.py b/buildstream/_versions.py
index 42dcb1a31..8472a5b33 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 = 19
+BST_FORMAT_VERSION = 20
 
 
 # The base BuildStream artifact version
diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py
index 7496f1f13..ae8f36bfe 100644
--- a/buildstream/plugins/sources/git.py
+++ b/buildstream/plugins/sources/git.py
@@ -133,7 +133,22 @@ details on common configuration options for sources.
 
 This plugin provides the following :ref:`configurable warnings <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 :class:`core warnings <buildstream.types.CoreWarnings>`:
 
@@ -158,6 +173,8 @@ GIT_MODULES = '.gitmodules'
 
 # Warnings
 WARN_INCONSISTENT_SUBMODULE = "inconsistent-submodule"
+WARN_UNLISTED_SUBMODULE = "unlisted-submodule"
+WARN_INVALID_SUBMODULE = "invalid-submodule"
 
 
 # Because of handling of submodules, we maintain a GitMirror
@@ -305,7 +322,7 @@ class GitMirror(SourceFetcher):
 
         return ref, list(tags)
 
-    def stage(self, directory, track=None):
+    def stage(self, directory):
         fullpath = os.path.join(directory, self.path)
 
         # Using --shared here avoids copying the objects into the checkout, in any
@@ -324,11 +341,7 @@ class GitMirror(SourceFetcher):
 
         self._rebuild_git(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)
 
@@ -344,10 +357,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)
@@ -413,28 +422,6 @@ 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
-        else:
-            _, tag = self.source.check_output([self.source.host_git, 'tag', '--list', track,
-                                               '--contains', self.ref],
-                                              cwd=fullpath,)
-            if tag:
-                return
-
-        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)
-
     def _rebuild_git(self, fullpath):
         if not self.tags:
             return
@@ -563,7 +550,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
@@ -653,8 +639,6 @@ class GitSource(Source):
             # Update self.mirror.ref and node.ref from the self.tracking branch
             ret = self.mirror.latest_commit_with_tags(self.tracking, self.track_tags)
 
-        # Set tracked attribute, parameter for if self.mirror.assert_ref_in_track is needed
-        self.tracked = True
         return ret
 
     def init_workspace(self, directory):
@@ -662,7 +646,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)
 
@@ -678,15 +662,9 @@ 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:
-                if mirror.path in self.submodule_checkout_overrides:
-                    checkout = self.submodule_checkout_overrides[mirror.path]
-                else:
-                    checkout = self.checkout_submodules
-
-                if checkout:
-                    mirror.stage(directory)
+                mirror.stage(directory)
 
     def get_source_fetchers(self):
         yield self.mirror
@@ -694,6 +672,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                     #
     ###########################################################
@@ -718,12 +764,12 @@ 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
+            if self.ignore_submodule(path):
+                continue
+
             # Allow configuration to override the upstream
             # location of the submodules.
             override_url = self.submodule_overrides.get(path)
@@ -747,6 +793,16 @@ class GitSource(Source):
             tags.append((tag, commit_ref, annotated))
         return tags
 
+    # 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 5dc5abb63..bb54110ca 100644
--- a/buildstream/source.py
+++ b/buildstream/source.py
@@ -102,6 +102,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
 --------------------------
@@ -480,9 +485,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                      #
     #############################################################
@@ -659,6 +677,11 @@ class Source(Plugin):
             with context.silence():
                 self.__consistency = self.get_consistency()  # pylint: disable=assignment-from-no-return
 
+                # 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 124f036cf..b9251e36a 100644
--- a/tests/sources/git.py
+++ b/tests/sources/git.py
@@ -457,6 +457,274 @@ def test_ref_not_in_track(cli, tmpdir, datafiles, fail):
 
 @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_track_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, but use
+    # the original ref, let the submodules appear after tracking
+    repo.add_submodule('subdir', 'file://' + subrepo.repo)
+
+    # Create the source, and delete the explicit configuration
+    # of the submodules.
+    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'))
+
+    # 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:unlisted-submodule" not in result.stderr
+
+    # We won't get a warning/error when tracking either, the source
+    # has not become Consistency.CACHED so the opportunity to check
+    # for the warning has not yet arisen.
+    result = cli.run(project=project, args=['track', 'target.bst'])
+    result.assert_success()
+    assert "git:unlisted-submodule" not in result.stderr
+
+    # Fetching the repo at the new ref will finally reveal the warning
+    result = cli.run(project=project, args=['fetch', 'target.bst'])
+    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
+
+
+@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'))
 @pytest.mark.parametrize("ref_format", ['sha1', 'git-describe'])
 @pytest.mark.parametrize("tag,extra_commit", [(False, False), (True, False), (True, True)])
 def test_track_fetch(cli, tmpdir, datafiles, ref_format, tag, extra_commit):
diff --git a/tests/testutils/repo/git.py b/tests/testutils/repo/git.py
index fe3ebd547..b2d480975 100644
--- a/tests/testutils/repo/git.py
+++ b/tests/testutils/repo/git.py
@@ -76,6 +76,12 @@ class Git(Repo):
         self._run_git('commit', '-m', 'Added the submodule')
         return self.latest_commit()
 
+    # This can also be used to a file or a submodule
+    def remove_path(self, path):
+        self._run_git('rm', path)
+        self._run_git('commit', '-m', 'Removing {}'.format(path))
+        return self.latest_commit()
+
     def source_config(self, ref=None, checkout_submodules=None):
         config = {
             'kind': 'git',