Merge branch 'aevri/junction_load_provenance' into 'master'

loader: provenance in LoadErrors from _get_loader

See merge request BuildStream/buildstream!1237

10 files changed, 116 insertions, 19 deletions

diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py
index 804dc5f7e..2a67a6e2a 100644
--- a/buildstream/_loader/loader.py
+++ b/buildstream/_loader/loader.py
@@ -266,7 +266,7 @@ class Loader():
             if dep.junction:
                 self._load_file(dep.junction, rewritable, ticker, fetch_subprojects, yaml_cache, dep.provenance)
                 loader = self._get_loader(dep.junction, rewritable=rewritable, ticker=ticker,
-                                          fetch_subprojects=fetch_subprojects)
+                                          fetch_subprojects=fetch_subprojects, provenance=dep.provenance)
             else:
                 loader = self
 
@@ -474,7 +474,13 @@ class Loader():
     # Raises: LoadError
     #
     # Returns: A Loader or None if specified junction does not exist
-    def _get_loader(self, filename, *, rewritable=False, ticker=None, level=0, fetch_subprojects=False):
+    def _get_loader(self, filename, *, rewritable=False, ticker=None, level=0,
+                    fetch_subprojects=False, provenance=None):
+
+        provenance_str = ""
+        if provenance is not None:
+            provenance_str = "{}: ".format(provenance)
+
         # return previously determined result
         if filename in self._loaders:
             loader = self._loaders[filename]
@@ -483,8 +489,8 @@ class Loader():
                 # do not allow junctions with the same name in different
                 # subprojects
                 raise LoadError(LoadErrorReason.CONFLICTING_JUNCTION,
-                                "Conflicting junction {} in subprojects, define junction in {}"
-                                .format(filename, self.project.name))
+                                "{}Conflicting junction {} in subprojects, define junction in {}"
+                                .format(provenance_str, filename, self.project.name))
 
             return loader
 
@@ -492,7 +498,8 @@ class Loader():
             # junctions in the parent take precedence over junctions defined
             # in subprojects
             loader = self._parent._get_loader(filename, rewritable=rewritable, ticker=ticker,
-                                              level=level + 1, fetch_subprojects=fetch_subprojects)
+                                              level=level + 1, fetch_subprojects=fetch_subprojects,
+                                              provenance=provenance)
             if loader:
                 self._loaders[filename] = loader
                 return loader
@@ -517,7 +524,8 @@ class Loader():
         meta_element = self._collect_element(self._elements[filename])
         if meta_element.kind != 'junction':
             raise LoadError(LoadErrorReason.INVALID_DATA,
-                            "{}: Expected junction but element kind is {}".format(filename, meta_element.kind))
+                            "{}{}: Expected junction but element kind is {}".format(
+                                provenance_str, filename, meta_element.kind))
 
         element = Element._new_from_meta(meta_element)
         element._preflight()
@@ -535,7 +543,7 @@ class Loader():
                     else:
                         detail = "Try fetching the project with `bst source fetch {}`".format(filename)
                         raise LoadError(LoadErrorReason.SUBPROJECT_FETCH_NEEDED,
-                                        "Subproject fetch needed for junction: {}".format(filename),
+                                        "{}Subproject fetch needed for junction: {}".format(provenance_str, filename),
                                         detail=detail)
 
                 # Handle the case where a subproject has no ref
@@ -543,7 +551,7 @@ class Loader():
                 elif source.get_consistency() == Consistency.INCONSISTENT:
                     detail = "Try tracking the junction element with `bst source track {}`".format(filename)
                     raise LoadError(LoadErrorReason.SUBPROJECT_INCONSISTENT,
-                                    "Subproject has no ref for junction: {}".format(filename),
+                                    "{}Subproject has no ref for junction: {}".format(provenance_str, filename),
                                     detail=detail)
 
         workspace = element._get_workspace()
@@ -571,7 +579,7 @@ class Loader():
         except LoadError as e:
             if e.reason == LoadErrorReason.MISSING_PROJECT_CONF:
                 message = (
-                    "Could not find the project.conf file in the project "
+                    provenance_str + "Could not find the project.conf file in the project "
                     "referred to by junction element '{}'.".format(element.name)
                 )
                 if element.path:
diff --git a/buildstream/_yaml.py b/buildstream/_yaml.py
index 73818c209..26da5e3be 100644
--- a/buildstream/_yaml.py
+++ b/buildstream/_yaml.py
@@ -54,7 +54,7 @@ class ProvenanceFile():
 #
 # Args:
 #   node (dict, list, value): A binding to the originally parsed value
-#   filename (string): The filename the node was loaded from
+#   filename (str): The filename the node was loaded from
 #   toplevel (dict): The toplevel of the loaded file, suitable for later dumps
 #   line (int): The line number where node was parsed
 #   col (int): The column number where node was parsed
diff --git a/tests/format/junctions.py b/tests/format/junctions.py
index 210455cb4..3a215761a 100644
--- a/tests/format/junctions.py
+++ b/tests/format/junctions.py
@@ -81,6 +81,9 @@ def test_junction_missing_project_conf(cli, datafiles):
     result = cli.run(project=project, args=['build', 'app.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION)
 
+    # Assert that we have the expected provenance encoded into the error
+    assert "app.bst [line 6 column 2]" in result.stderr
+
 
 @pytest.mark.datafiles(DATA_DIR)
 def test_workspaced_junction_missing_project_conf(cli, datafiles):
@@ -102,6 +105,9 @@ def test_workspaced_junction_missing_project_conf(cli, datafiles):
     result = cli.run(project=project, args=['build', 'app.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION)
 
+    # Assert that we have the expected provenance encoded into the error
+    assert "app.bst [line 6 column 2]" in result.stderr
+
 
 @pytest.mark.datafiles(DATA_DIR)
 def test_build_of_same_junction_used_twice(cli, datafiles):
@@ -203,6 +209,8 @@ def test_nested_conflict(cli, datafiles):
     result = cli.run(project=project, args=['build', 'target.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.CONFLICTING_JUNCTION)
 
+    assert "bar.bst:target.bst [line 3 column 2]" in result.stderr
+
 
 # Test that we error correctly when the junction element itself is missing
 @pytest.mark.datafiles(DATA_DIR)
@@ -243,6 +251,19 @@ def test_invalid_junction_dep(cli, datafiles):
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)
 
 
+# Test that we error correctly when we junction-depend on a non-junction
+@pytest.mark.datafiles(DATA_DIR)
+def test_invalid_junctiondep_not_a_junction(cli, datafiles):
+    project = os.path.join(str(datafiles), 'invalid')
+    copy_subprojects(project, datafiles, ['base'])
+
+    result = cli.run(project=project, args=['build', 'junctiondep-not-a-junction.bst'])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)
+
+    # Assert that we have the expected provenance encoded into the error
+    assert "junctiondep-not-a-junction.bst [line 3 column 2]" in result.stderr
+
+
 @pytest.mark.datafiles(DATA_DIR)
 def test_options_default(cli, tmpdir, datafiles):
     project = os.path.join(str(datafiles), 'options-default')
@@ -316,6 +337,9 @@ def test_git_show(cli, tmpdir, datafiles):
     result = cli.run(project=project, args=['show', 'target.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_FETCH_NEEDED)
 
+    # Assert that we have the expected provenance encoded into the error
+    assert "target.bst [line 3 column 2]" in result.stderr
+
     # Explicitly fetch subproject
     result = cli.run(project=project, args=['source', 'fetch', 'base.bst'])
     result.assert_success()
@@ -380,6 +404,9 @@ def test_git_missing_project_conf(cli, tmpdir, datafiles):
     result = cli.run(project=project, args=['build', 'app.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION)
 
+    # Assert that we have the expected provenance encoded into the error
+    assert "app.bst [line 6 column 2]" in result.stderr
+
 
 @pytest.mark.datafiles(DATA_DIR)
 def test_cross_junction_names(cli, datafiles):
diff --git a/tests/format/junctions/invalid/junctiondep-not-a-junction.bst b/tests/format/junctions/invalid/junctiondep-not-a-junction.bst
new file mode 100644
index 000000000..c59f1be25
--- /dev/null
+++ b/tests/format/junctions/invalid/junctiondep-not-a-junction.bst
@@ -0,0 +1,4 @@
+kind: stack
+depends:
+- junction: app.bst
+  filename: target.bst
diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py
index dc3c5e4d4..5145b1222 100644
--- a/tests/frontend/buildcheckout.py
+++ b/tests/frontend/buildcheckout.py
@@ -9,7 +9,7 @@ import subprocess
 import pytest
 
 from tests.testutils.site import IS_WINDOWS
-from tests.testutils import generate_junction
+from tests.testutils import generate_junction, yaml_file_get_provenance
 
 from buildstream.plugintestutils import cli  # pylint: disable=unused-import
 from buildstream import _yaml
@@ -457,6 +457,11 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage):
     result = cli.run(project=project, args=['build', 'junction-dep.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT)
 
+    # Assert that we have the expected provenance encoded into the error
+    provenance = yaml_file_get_provenance(
+        element_path, 'junction-dep.bst', key='depends', indices=[0])
+    assert str(provenance) in result.stderr
+
 
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.parametrize("ref_storage", [('inline'), ('project.refs')])
diff --git a/tests/frontend/fetch.py b/tests/frontend/fetch.py
index 1a2c40602..8282e2131 100644
--- a/tests/frontend/fetch.py
+++ b/tests/frontend/fetch.py
@@ -4,7 +4,7 @@
 import os
 import pytest
 
-from tests.testutils import create_repo, generate_junction
+from tests.testutils import create_repo, generate_junction, yaml_file_get_provenance
 
 from buildstream.plugintestutils import cli  # pylint: disable=unused-import
 from buildstream import _yaml
@@ -160,3 +160,8 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage):
     # informing the user to track the junction first
     result = cli.run(project=project, args=['source', 'fetch', 'junction-dep.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT)
+
+    # Assert that we have the expected provenance encoded into the error
+    provenance = yaml_file_get_provenance(
+        element_path, 'junction-dep.bst', key='depends', indices=[0])
+    assert str(provenance) in result.stderr
diff --git a/tests/frontend/show.py b/tests/frontend/show.py
index bb2fc2ee2..7e5ebfb77 100644
--- a/tests/frontend/show.py
+++ b/tests/frontend/show.py
@@ -6,7 +6,7 @@ import sys
 import shutil
 import itertools
 import pytest
-from tests.testutils import generate_junction
+from tests.testutils import generate_junction, yaml_file_get_provenance
 from buildstream.plugintestutils import cli  # pylint: disable=unused-import
 from buildstream import _yaml
 from buildstream._exceptions import ErrorDomain, LoadErrorReason
@@ -277,8 +277,7 @@ def test_unfetched_junction(cli, tmpdir, datafiles, ref_storage, element_name):
 
 @pytest.mark.datafiles(os.path.join(DATA_DIR, 'project'))
 @pytest.mark.parametrize("ref_storage", [('inline'), ('project.refs')])
-@pytest.mark.parametrize("element_name", ['junction-dep.bst', 'junction.bst:import-etc.bst'])
-def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage, element_name):
+def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage):
     project = str(datafiles)
     subproject_path = os.path.join(project, 'files', 'sub-project')
     junction_path = os.path.join(project, 'elements', 'junction.bst')
@@ -305,10 +304,20 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage, element_name
     _yaml.dump(element, element_path)
 
     # Assert the correct error when trying to show the pipeline
-    result = cli.run(project=project, silent=True, args=[
-        'show', element_name])
+    dep_result = cli.run(project=project, silent=True, args=[
+        'show', 'junction-dep.bst'])
+
+    # Assert the correct error when trying to show the pipeline
+    etc_result = cli.run(project=project, silent=True, args=[
+        'show', 'junction.bst:import-etc.bst'])
+
+    # Assert that we have the expected provenance encoded into the error
+    provenance = yaml_file_get_provenance(
+        element_path, 'junction-dep.bst', key='depends', indices=[0])
+    assert str(provenance) in dep_result.stderr
 
-    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT)
+    dep_result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT)
+    etc_result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT)
 
 
 @pytest.mark.datafiles(os.path.join(DATA_DIR, 'project'))
diff --git a/tests/frontend/track.py b/tests/frontend/track.py
index 5a6460d3d..13d1d4646 100644
--- a/tests/frontend/track.py
+++ b/tests/frontend/track.py
@@ -4,7 +4,7 @@
 import stat
 import os
 import pytest
-from tests.testutils import create_repo, generate_junction
+from tests.testutils import create_repo, generate_junction, yaml_file_get_provenance
 
 from buildstream.plugintestutils import cli  # pylint: disable=unused-import
 from buildstream._exceptions import ErrorDomain, LoadErrorReason
@@ -274,6 +274,11 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage):
     result = cli.run(project=project, args=['source', 'track', 'junction-dep.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT)
 
+    # Assert that we have the expected provenance encoded into the error
+    provenance = yaml_file_get_provenance(
+        element_path, 'junction-dep.bst', key='depends', indices=[0])
+    assert str(provenance) in result.stderr
+
 
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.parametrize("ref_storage", [('inline'), ('project.refs')])
@@ -307,6 +312,11 @@ def test_junction_element(cli, tmpdir, datafiles, ref_storage):
     result = cli.run(project=project, args=['show', 'junction-dep.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT)
 
+    # Assert that we have the expected provenance encoded into the error
+    provenance = yaml_file_get_provenance(
+        element_path, 'junction-dep.bst', key='depends', indices=[0])
+    assert str(provenance) in result.stderr
+
     # Now track the junction itself
     result = cli.run(project=project, args=['source', 'track', 'junction.bst'])
     result.assert_success()
diff --git a/tests/testutils/__init__.py b/tests/testutils/__init__.py
index 173cc7c3e..67378c959 100644
--- a/tests/testutils/__init__.py
+++ b/tests/testutils/__init__.py
@@ -29,3 +29,4 @@ from .element_generators import create_element_size, update_element_size
 from .junction import generate_junction
 from .runner_integration import wait_for_cache_granularity
 from .python_repo import setup_pypi_repo
+from .yaml import yaml_file_get_provenance
diff --git a/tests/testutils/yaml.py b/tests/testutils/yaml.py
new file mode 100644
index 000000000..4706a27bb
--- /dev/null
+++ b/tests/testutils/yaml.py
@@ -0,0 +1,28 @@
+from buildstream import _yaml
+
+
+# yaml_file_get_provenance()
+#
+# Load a yaml file and return it's _yaml.Provenance object.
+#
+# This is useful for checking the provenance in BuildStream output is as
+# expected.
+#
+# Args:
+#   path (str): The path to the file to be loaded
+#   shortname (str): How the path should appear in the error
+#   key (str): Optional key to look up in the loaded file
+#   indices (list of indexes): Optional index path, in the case of list values
+#
+# Returns:
+#   The Provenance of the dict, member or list element
+#
+def yaml_file_get_provenance(path, shortname, key=None, indices=None):
+    with open(path) as data:
+        element_yaml = _yaml.load_data(
+            data,
+            _yaml.ProvenanceFile(path, shortname, project=None),
+        )
+    provenance = _yaml.node_get_provenance(element_yaml, key, indices)
+    assert provenance is not None
+    return provenance