From 907f5fcd38149dc16664290c2a3d6971f2115be5 Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Mon, 2 Jun 2025 17:19:41 +0900 Subject: [PATCH 1/3] types.py: Add CoreWarnings.UNAVAILABLE_SOURCE_INFO --- src/buildstream/types.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/buildstream/types.py b/src/buildstream/types.py index 778941768..2ee92afa7 100644 --- a/src/buildstream/types.py +++ b/src/buildstream/types.py @@ -117,6 +117,12 @@ class CoreWarnings: :ref:`alias ` """ + UNAVAILABLE_SOURCE_INFO = "unavailable-source-info" + """ + A source was queried for its provenance information but did not implement + :func:`Source.collect_source_info() `. + """ + class OverlapAction(FastEnum): """OverlapAction() From 3be4aa9e96d642a06dec1b1785cb70b5686838f6 Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Mon, 2 Jun 2025 17:37:03 +0900 Subject: [PATCH 2/3] source.py: Issue a configurable warning when collect_source_info() is unimplemented This allows the project to have a policy of only allowing usage of sources which implement Source.collect_source_info() (or SourceFetcher.get_source_info()). --- src/buildstream/__init__.py | 1 - src/buildstream/_frontend/widget.py | 22 ++++++------- src/buildstream/source.py | 49 +++++++++++------------------ 3 files changed, 28 insertions(+), 44 deletions(-) diff --git a/src/buildstream/__init__.py b/src/buildstream/__init__.py index 01d1d6535..54d509494 100644 --- a/src/buildstream/__init__.py +++ b/src/buildstream/__init__.py @@ -34,7 +34,6 @@ from .source import ( Source, SourceError, - SourceImplError, SourceFetcher, SourceInfo, SourceInfoMedium, diff --git a/src/buildstream/_frontend/widget.py b/src/buildstream/_frontend/widget.py index 6c15f7263..09b824ff3 100644 --- a/src/buildstream/_frontend/widget.py +++ b/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)) diff --git a/src/buildstream/source.py b/src/buildstream/source.py index 21d060a86..8ba64c600 100644 --- a/src/buildstream/source.py +++ b/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() `. - 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() `. - 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() ` + 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 From cac1ea80c9d5cd7b10d8abf3992153585d795938 Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Mon, 2 Jun 2025 18:10:25 +0900 Subject: [PATCH 3/3] tests/frontend/show.py: Test unimplemented collect_source_info() This elaborates on the unimplemented test case to check behavior of fatal vs non-fatal warnings. --- tests/frontend/show.py | 31 +++++++++++++------ .../fatal/elements/unimplemented.bst | 4 +++ .../fatal/plugins/fatal_unimplemented.py} | 0 .../fatal/project.conf | 13 ++++++++ .../non-fatal/elements/unimplemented.bst | 4 +++ .../plugins/non_fatal_unimplemented.py | 31 +++++++++++++++++++ .../non-fatal/project.conf | 10 ++++++ .../source-info/elements/unimplemented.bst | 4 --- 8 files changed, 83 insertions(+), 14 deletions(-) create mode 100644 tests/frontend/source-info-unimplemented/fatal/elements/unimplemented.bst rename tests/frontend/{source-info/plugins/unimplemented.py => source-info-unimplemented/fatal/plugins/fatal_unimplemented.py} (100%) create mode 100644 tests/frontend/source-info-unimplemented/fatal/project.conf create mode 100644 tests/frontend/source-info-unimplemented/non-fatal/elements/unimplemented.bst create mode 100644 tests/frontend/source-info-unimplemented/non-fatal/plugins/non_fatal_unimplemented.py create mode 100644 tests/frontend/source-info-unimplemented/non-fatal/project.conf delete mode 100644 tests/frontend/source-info/elements/unimplemented.bst diff --git a/tests/frontend/show.py b/tests/frontend/show.py index f30b1cb88..833a7addf 100644 --- a/tests/frontend/show.py +++ b/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, diff --git a/tests/frontend/source-info-unimplemented/fatal/elements/unimplemented.bst b/tests/frontend/source-info-unimplemented/fatal/elements/unimplemented.bst new file mode 100644 index 000000000..6691ca2a6 --- /dev/null +++ b/tests/frontend/source-info-unimplemented/fatal/elements/unimplemented.bst @@ -0,0 +1,4 @@ +kind: import + +sources: +- kind: fatal_unimplemented diff --git a/tests/frontend/source-info/plugins/unimplemented.py b/tests/frontend/source-info-unimplemented/fatal/plugins/fatal_unimplemented.py similarity index 100% rename from tests/frontend/source-info/plugins/unimplemented.py rename to tests/frontend/source-info-unimplemented/fatal/plugins/fatal_unimplemented.py diff --git a/tests/frontend/source-info-unimplemented/fatal/project.conf b/tests/frontend/source-info-unimplemented/fatal/project.conf new file mode 100644 index 000000000..84893879d --- /dev/null +++ b/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 diff --git a/tests/frontend/source-info-unimplemented/non-fatal/elements/unimplemented.bst b/tests/frontend/source-info-unimplemented/non-fatal/elements/unimplemented.bst new file mode 100644 index 000000000..4e2686bd4 --- /dev/null +++ b/tests/frontend/source-info-unimplemented/non-fatal/elements/unimplemented.bst @@ -0,0 +1,4 @@ +kind: import + +sources: +- kind: non_fatal_unimplemented diff --git a/tests/frontend/source-info-unimplemented/non-fatal/plugins/non_fatal_unimplemented.py b/tests/frontend/source-info-unimplemented/non-fatal/plugins/non_fatal_unimplemented.py new file mode 100644 index 000000000..828d7889b --- /dev/null +++ b/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 diff --git a/tests/frontend/source-info-unimplemented/non-fatal/project.conf b/tests/frontend/source-info-unimplemented/non-fatal/project.conf new file mode 100644 index 000000000..689ebc023 --- /dev/null +++ b/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 diff --git a/tests/frontend/source-info/elements/unimplemented.bst b/tests/frontend/source-info/elements/unimplemented.bst deleted file mode 100644 index d8527e9f5..000000000 --- a/tests/frontend/source-info/elements/unimplemented.bst +++ /dev/null @@ -1,4 +0,0 @@ -kind: import - -sources: -- kind: unimplemented