Merge branch 'tristan/get-loader-provenance' into 'master'

Make provenance mandatory in `Loader.get_loader()`

See merge request BuildStream/buildstream!1960

5 files changed, 25 insertions, 34 deletions

diff --git a/src/buildstream/_includes.py b/src/buildstream/_includes.py
index 0c77e5fa1..1556bc613 100644
--- a/src/buildstream/_includes.py
+++ b/src/buildstream/_includes.py
@@ -78,39 +78,24 @@ class Includes:
 
         if includes_node:
             if type(includes_node) is ScalarNode:  # pylint: disable=unidiomatic-typecheck
-                includes = [includes_node.as_str()]
+                includes = [includes_node]
             else:
-                includes = includes_node.as_str_list()
+                includes = includes_node
 
             del node["(@)"]
 
             for include in reversed(includes):
-                if only_local and ":" in include:
+                if only_local and ":" in include.as_str():
                     continue
-                try:
-                    include_node, file_path, sub_loader = self._include_file(include, current_loader)
-                except LoadError as e:
-                    include_provenance = includes_node.get_provenance()
-                    if e.reason == LoadErrorReason.MISSING_FILE:
-                        message = "{}: Include block references a file that could not be found: '{}'.".format(
-                            include_provenance, include
-                        )
-                        raise LoadError(message, LoadErrorReason.MISSING_FILE) from e
-                    if e.reason == LoadErrorReason.LOADING_DIRECTORY:
-                        message = "{}: Include block references a directory instead of a file: '{}'.".format(
-                            include_provenance, include
-                        )
-                        raise LoadError(message, LoadErrorReason.LOADING_DIRECTORY) from e
-
-                    # Otherwise, we don't know the reason, so just raise
-                    raise
 
+                include_node, file_path, sub_loader = self._include_file(include, current_loader)
                 if file_path in included:
                     include_provenance = includes_node.get_provenance()
                     raise LoadError(
                         "{}: trying to recursively include {}".format(include_provenance, file_path),
                         LoadErrorReason.RECURSIVE_INCLUDE,
                     )
+
                 # Because the included node will be modified, we need
                 # to copy it so that we do not modify the toplevel
                 # node of the provenance.
@@ -144,14 +129,16 @@ class Includes:
     # Load include YAML file from with a loader.
     #
     # Args:
-    #    include (str): file path relative to loader's project directory.
-    #                   Can be prefixed with junctio name.
+    #    include (ScalarNode): file path relative to loader's project directory.
+    #                          Can be prefixed with junctio name.
     #    loader (Loader): Loader for the current project.
     def _include_file(self, include, loader):
+        provenance = include.get_provenance()
+        include = include.as_str()
         shortname = include
         if ":" in include:
             junction, include = include.rsplit(":", 1)
-            current_loader = loader.get_loader(junction)
+            current_loader = loader.get_loader(junction, provenance)
             current_loader.project.ensure_fully_loaded()
         else:
             current_loader = loader
@@ -160,7 +147,13 @@ class Includes:
         file_path = os.path.join(directory, include)
         key = (current_loader, file_path)
         if key not in self._loaded:
-            self._loaded[key] = _yaml.load(file_path, shortname=shortname, project=project, copy_tree=self._copy_tree)
+            try:
+                self._loaded[key] = _yaml.load(
+                    file_path, shortname=shortname, project=project, copy_tree=self._copy_tree
+                )
+            except LoadError as e:
+                raise LoadError("{}: {}".format(provenance, e), e.reason, detail=e.detail) from e
+
         return self._loaded[key], file_path, current_loader
 
     # _process_value()
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py
index 17f0d906f..5a5d24dc4 100644
--- a/src/buildstream/_loader/loader.py
+++ b/src/buildstream/_loader/loader.py
@@ -164,15 +164,15 @@ class Loader:
     #
     # Args:
     #   name (str): Name of junction, may have multiple `:` in the name
+    #   provenance (ProvenanceInformation): The provenance
     #   rewritable (bool): Whether the loaded files should be rewritable
     #                      this is a bit more expensive due to deep copies
     #   ticker (callable): An optional function for tracking load progress
-    #   provenance (ProvenanceInformation): The provenance
     #
     # Returns:
     #   (Loader): loader for sub-project
     #
-    def get_loader(self, name, *, rewritable=False, ticker=None, level=0, provenance=None):
+    def get_loader(self, name, provenance, *, rewritable=False, ticker=None, level=0):
         junction_path = name.split(":")
         loader = self
 
@@ -421,9 +421,7 @@ class Loader:
                 current_element[2].append(dep.name)
 
                 if dep.junction:
-                    loader = self.get_loader(
-                        dep.junction, rewritable=rewritable, ticker=ticker, provenance=dep.provenance
-                    )
+                    loader = self.get_loader(dep.junction, dep.provenance, rewritable=rewritable, ticker=ticker)
                     dep_element = loader._load_file(dep.name, rewritable, ticker, dep.provenance)
                 else:
                     dep_element = self._elements.get(dep.name)
@@ -780,7 +778,7 @@ class Loader:
         if len(junction_path) == 1:
             return None, junction_path[-1], self
         else:
-            loader = self.get_loader(junction_path[-2], rewritable=rewritable, ticker=ticker, provenance=provenance)
+            loader = self.get_loader(junction_path[-2], provenance, rewritable=rewritable, ticker=ticker)
             return junction_path[-2], junction_path[-1], loader
 
     # Print a warning message, checks warning_token against project configuration
diff --git a/src/buildstream/_pluginfactory/pluginoriginjunction.py b/src/buildstream/_pluginfactory/pluginoriginjunction.py
index 8c1d560fb..4e0a53cfb 100644
--- a/src/buildstream/_pluginfactory/pluginoriginjunction.py
+++ b/src/buildstream/_pluginfactory/pluginoriginjunction.py
@@ -35,7 +35,7 @@ class PluginOriginJunction(PluginOrigin):
         # Get access to the project indicated by the junction,
         # possibly loading it as a side effect.
         #
-        loader = self.project.loader.get_loader(self._junction, provenance=self.provenance)
+        loader = self.project.loader.get_loader(self._junction, self.provenance)
         project = loader.project
         project.ensure_fully_loaded()
 
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index 373311a47..1e59b2a1c 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -283,7 +283,7 @@ cpdef MappingNode load(str filename, str shortname=None, bint copy_tree=False, o
         raise LoadError("Could not find file at {}".format(filename),
                         LoadErrorReason.MISSING_FILE) from e
     except IsADirectoryError as e:
-        raise LoadError("{} is a directory. bst command expects a .bst file.".format(filename),
+        raise LoadError("{} is a directory".format(filename),
                         LoadErrorReason.LOADING_DIRECTORY) from e
     except LoadError as e:
         raise LoadError("{}: {}".format(displayname, e), e.reason) from e
diff --git a/tests/format/include.py b/tests/format/include.py
index 5c273e1a0..d57dd8c19 100644
--- a/tests/format/include.py
+++ b/tests/format/include.py
@@ -44,7 +44,7 @@ def test_include_missing_file(cli, tmpdir):
     result = cli.run(project=str(tmpdir), args=["show", str(element.basename)])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
     # Make sure the root cause provenance is in the output.
-    assert "line 4 column 2" in result.stderr
+    assert "include_missing_file.bst [line 4 column 4]" in result.stderr
 
 
 def test_include_dir(cli, tmpdir):
@@ -68,7 +68,7 @@ def test_include_dir(cli, tmpdir):
     result = cli.run(project=str(tmpdir), args=["show", str(element.basename)])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.LOADING_DIRECTORY)
     # Make sure the root cause provenance is in the output.
-    assert "line 4 column 2" in result.stderr
+    assert "include_dir.bst [line 4 column 4]" in result.stderr
 
 
 @pytest.mark.datafiles(DATA_DIR)