core warnings for missing Source.collect_source_info() implementations

src/buildstream/__init__.py

@@ -34,7 +34,6 @@
     from .source import (
         Source,
         SourceError,
-        SourceImplError,
         SourceFetcher,
         SourceInfo,
         SourceInfoMedium,

src/buildstream/_frontend/widget.py

@@ -23,7 +23,6 @@
 
 from .profile import Profile
 from ..types import _Scope
-from ..source import SourceImplError
 from .. import _yaml
 from .. import __version__ as bst_version
 from .. import FileType
@@ -445,18 +444,15 @@ def show_pipeline(self, dependencies, format_):
                 #
                 all_source_infos = []
                 for source in element.sources():
-                    try:
-                        source_infos = source.collect_source_info()
-                    except SourceImplError as e:
-                        source.warn(str(e))
-                        continue
-
-                    serialized_sources = []
-                    for s in source_infos:
-                        serialized = s.serialize()
-                        serialized_sources.append(serialized)
-
-                    all_source_infos += serialized_sources
+                    source_infos = source.collect_source_info()
+
+                    if source_infos is not None:
+                        serialized_sources = []
+                        for s in source_infos:
+                            serialized = s.serialize()
+                            serialized_sources.append(serialized)
+
+                        all_source_infos += serialized_sources
 
                 # Dump the SourceInfo provenance objects in yaml format
                 line = p.fmt_subst(line, "source-info", _yaml.roundtrip_dump_string(all_source_infos))

src/buildstream/source.py

@@ -395,19 +395,6 @@ def __init__(
         super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, reason=reason, temporary=temporary)
 
 
-class SourceImplError(BstError):
-    """This exception is expected to be raised from some unimplemented abstract methods.
-
-    There is no need to raise this exception, however some public abstract methods which
-    are intended to be called by plugins may advertize the raising of this exception
-    in the case of a source plugin which does not implement the said method, in which case
-    it must be handled by the calling plugin.
-    """
-
-    def __init__(self, message, reason=None):
-        super().__init__(message, domain=ErrorDomain.IMPL, reason=reason)
-
-
 @dataclass
 class AliasSubstitution:
     """AliasSubstitution()
@@ -669,7 +656,7 @@ def fetch(self, alias_override: Optional[AliasSubstitution] = None, **kwargs) ->
         """
         raise ImplError("SourceFetcher '{}' does not implement fetch()".format(type(self)))
 
-    def get_source_info(self) -> SourceInfo:
+    def get_source_info(self) -> Optional[SourceInfo]:
         """Get the :class:`.SourceInfo` object describing this source
 
         This method should only be called whenever
@@ -679,14 +666,12 @@ def get_source_info(self) -> SourceInfo:
         SourceInfo objects created by implementors should be created with
         :func:`Source.create_source_info() <buildstream.source.Source.create_source_info>`.
 
-        Returns: the :class:`.SourceInfo` objects describing this source
-
-        Raises:
-           :class:`.SourceImplError`: if this method is unimplemented
+        Returns: the :class:`.SourceInfo` object describing this source, or ``None`` if the
+                 SourceFetcher does not implement this method.
 
         *Since: 2.5*
         """
-        raise SourceImplError("SourceFetcher '{}' does not implement get_source_info()".format(type(self)))
+        return None
 
     #############################################################
     #                       Public Methods                      #
@@ -1068,7 +1053,7 @@ def is_cached(self) -> bool:
         """
         raise ImplError("Source plugin '{}' does not implement is_cached()".format(self.get_kind()))
 
-    def collect_source_info(self) -> Iterable[SourceInfo]:
+    def collect_source_info(self) -> Optional[Iterable[SourceInfo]]:
         """Get the :class:`.SourceInfo` objects describing this source
 
         This method should only be called whenever
@@ -1078,11 +1063,8 @@ def collect_source_info(self) -> Iterable[SourceInfo]:
         SourceInfo objects created by implementors should be created with
         :func:`Source.create_source_info() <buildstream.source.Source.create_source_info>`.
 
-        Returns: the :class:`.SourceInfo` objects describing this source
-
-        Raises:
-           :class:`.SourceImplError`: if the source class does not implement this method and does not implement
-                                      :func:`SourceFether.get_source_info() <buildstream.source.SourceFetcher.get_source_info>`
+        Returns: the :class:`.SourceInfo` objects describing this source, or ``None`` if the
+                 Source does not implement this method.
 
         .. note::
 
@@ -1093,15 +1075,22 @@ def collect_source_info(self) -> Iterable[SourceInfo]:
         """
         source_info = []
         for fetcher in self.get_source_fetchers():
-            source_info.append(fetcher.get_source_info())
+            info = fetcher.get_source_info()
+            if info is not None:
+                source_info.append(info)
 
         # If there are source fetchers, they can either have returned
-        # SourceInfo objects, OR they may have raised SourceImplError, we need
-        # to raise ImplError here in the case there were no source fetchers.
+        # SourceInfo objects, or None.
+        #
+        # We need to issue the warning here and return None in the case that no source info
+        # was reported.
+        #
         if not source_info:
-            raise SourceImplError(
-                "Source plugin '{}' does not implement collect_source_info()".format(self.get_kind())
+            self.warn(
+                "{}: Source.collect_source_info() is not implemented in this plugin".format(self),
+                warning_token=CoreWarnings.UNAVAILABLE_SOURCE_INFO,
             )
+            return None
 
         return source_info
 

src/buildstream/types.py

@@ -117,6 +117,12 @@ class CoreWarnings:
     :ref:`alias <project_source_aliases>`
     """
 
+    UNAVAILABLE_SOURCE_INFO = "unavailable-source-info"
+    """
+    A source was queried for its provenance information but did not implement
+    :func:`Source.collect_source_info() <buildstream.source.Source.collect_source_info>`.
+    """
+
 
 class OverlapAction(FastEnum):
     """OverlapAction()

tests/frontend/show.py

@@ -695,17 +695,28 @@ def test_source_info_extra_data(cli, datafiles):
 
 # Test what happens when encountering a source that doesn't implement collect_source_info()
 #
-@pytest.mark.datafiles(os.path.join(DATA_DIR, "source-info"))
-def test_source_info_unimplemented(cli, datafiles):
-    project = str(datafiles)
+@pytest.mark.datafiles(os.path.join(DATA_DIR, "source-info-unimplemented"))
+@pytest.mark.parametrize(
+    "subdir, expect_fatal",
+    [
+        ("non-fatal", False),
+        ("fatal", True),
+    ],
+    ids=["non-fatal", "fatal"],
+)
+def test_source_info_unimplemented(cli, datafiles, subdir, expect_fatal):
+    project = os.path.join(str(datafiles), subdir)
     result = cli.run(project=project, silent=True, args=["show", "--format", "%{source-info}", "unimplemented.bst"])
-    result.assert_success()
-
-    # Assert empty list but no errors for a source not implementing collect_source_info()
-    #
-    # Note that buildstream internal _yaml doesn't support loading a list as a toplevel element
-    # in the stream, so we just assert the string instead.
-    assert result.output == "[]\n\n"
+    if expect_fatal:
+        result.assert_main_error(ErrorDomain.PLUGIN, CoreWarnings.UNAVAILABLE_SOURCE_INFO)
+    else:
+        # Assert empty list but no errors for a source not implementing collect_source_info()
+        #
+        # Note that buildstream internal _yaml doesn't support loading a list as a toplevel element
+        # in the stream, so we just assert the string instead.
+        result.assert_success()
+        assert result.output == "[]\n\n"
+        assert "WARNING [unavailable-source-info]" in result.stderr
 
 
 # This checks how Source.collect_source_info() works on a workspace,

tests/frontend/source-info-unimplemented/fatal/elements/unimplemented.bst

@@ -0,0 +1,4 @@
+kind: import
+
+sources:
+- kind: fatal_unimplemented

tests/frontend/source-info-unimplemented/fatal/project.conf

@@ -0,0 +1,13 @@
+# Project config for bst show source-info test
+name: test
+min-version: 2.0
+element-path: elements
+
+plugins:
+- origin: local
+  path: plugins
+  sources:
+  - fatal_unimplemented
+
+fatal-warnings:
+- unavailable-source-info

tests/frontend/source-info-unimplemented/non-fatal/elements/unimplemented.bst

@@ -0,0 +1,4 @@
+kind: import
+
+sources:
+- kind: non_fatal_unimplemented

tests/frontend/source-info-unimplemented/non-fatal/plugins/non_fatal_unimplemented.py

@@ -0,0 +1,31 @@
+from buildstream import Source
+
+
+class Unimplemented(Source):
+    BST_MIN_VERSION = "2.0"
+
+    def configure(self, node):
+        pass
+
+    def preflight(self):
+        pass
+
+    def get_unique_key(self):
+        return {}
+
+    def load_ref(self, node):
+        pass
+
+    def get_ref(self):
+        return {}
+
+    def set_ref(self, ref, node):
+        pass
+
+    def is_cached(self):
+        return False
+
+
+# Plugin entry point
+def setup():
+    return Unimplemented

tests/frontend/source-info-unimplemented/non-fatal/project.conf

@@ -0,0 +1,10 @@
+# Project config for bst show source-info test
+name: test
+min-version: 2.0
+element-path: elements
+
+plugins:
+- origin: local
+  path: plugins
+  sources:
+  - non_fatal_unimplemented