Merge branch 'tristan/missing-file-errors' into 'master'

Improve error reporting when files are not found

Closes #947

See merge request BuildStream/buildstream!1216

18 files changed, 121 insertions, 5 deletions

diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py
index 7ccb9a37c..1607c5b5e 100644
--- a/buildstream/_loader/loader.py
+++ b/buildstream/_loader/loader.py
@@ -187,11 +187,12 @@ class Loader():
     #    ticker (callable): A callback to report loaded filenames to the frontend
     #    fetch_subprojects (bool): Whether to fetch subprojects while loading
     #    yaml_cache (YamlCache): A yaml cache
+    #    provenance (Provenance): The location from where the file was referred to, or None
     #
     # Returns:
     #    (LoadElement): A loaded LoadElement
     #
-    def _load_file(self, filename, rewritable, ticker, fetch_subprojects, yaml_cache=None):
+    def _load_file(self, filename, rewritable, ticker, fetch_subprojects, yaml_cache=None, provenance=None):
 
         # Silently ignore already loaded files
         if filename in self._elements:
@@ -208,21 +209,34 @@ class Loader():
                               project=self.project, yaml_cache=yaml_cache)
         except LoadError as e:
             if e.reason == LoadErrorReason.MISSING_FILE:
+
+                if self.project.junction:
+                    message = "Could not find element '{}' in project referred to by junction element '{}'" \
+                              .format(filename, self.project.junction.name)
+                else:
+                    message = "Could not find element '{}' in elements directory '{}'".format(filename, self._basedir)
+
+                if provenance:
+                    message = "{}: {}".format(provenance, message)
+
                 # If we can't find the file, try to suggest plausible
                 # alternatives by stripping the element-path from the given
                 # filename, and verifying that it exists.
-                message = "Could not find element '{}' in elements directory '{}'".format(filename, self._basedir)
                 detail = None
                 elements_dir = os.path.relpath(self._basedir, self.project.directory)
                 element_relpath = os.path.relpath(filename, elements_dir)
                 if filename.startswith(elements_dir) and os.path.exists(os.path.join(self._basedir, element_relpath)):
                     detail = "Did you mean '{}'?".format(element_relpath)
+
                 raise LoadError(LoadErrorReason.MISSING_FILE,
                                 message, detail=detail) from e
+
             elif e.reason == LoadErrorReason.LOADING_DIRECTORY:
                 # If a <directory>.bst file exists in the element path,
                 # let's suggest this as a plausible alternative.
                 message = str(e)
+                if provenance:
+                    message = "{}: {}".format(provenance, message)
                 detail = None
                 if os.path.exists(os.path.join(self._basedir, filename + '.bst')):
                     element_name = filename + '.bst'
@@ -250,13 +264,14 @@ class Loader():
         # Load all dependency files for the new LoadElement
         for dep in dependencies:
             if dep.junction:
-                self._load_file(dep.junction, rewritable, ticker, fetch_subprojects, yaml_cache)
+                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)
             else:
                 loader = self
 
-            dep_element = loader._load_file(dep.name, rewritable, ticker, fetch_subprojects, yaml_cache)
+            dep_element = loader._load_file(dep.name, rewritable, ticker,
+                                            fetch_subprojects, yaml_cache, dep.provenance)
 
             if _yaml.node_get(dep_element.node, str, Symbol.KIND) == 'junction':
                 raise LoadError(LoadErrorReason.INVALID_DATA,
diff --git a/buildstream/_yaml.py b/buildstream/_yaml.py
index fc24a223b..5dde9237e 100644
--- a/buildstream/_yaml.py
+++ b/buildstream/_yaml.py
@@ -69,7 +69,11 @@ class Provenance():
 
     # Convert a Provenance to a string for error reporting
     def __str__(self):
-        return "{} [line {:d} column {:d}]".format(self.filename.shortname, self.line, self.col)
+        filename = self.filename.shortname
+        if self.filename.project and self.filename.project.junction:
+            filename = "{}:{}".format(self.filename.project.junction.name, self.filename.shortname)
+
+        return "{} [line {:d} column {:d}]".format(filename, self.line, self.col)
 
     # Abstract method
     def clone(self):
diff --git a/tests/format/junctions.py b/tests/format/junctions.py
index 0c94bb51a..5c6ebd0bd 100644
--- a/tests/format/junctions.py
+++ b/tests/format/junctions.py
@@ -59,6 +59,36 @@ def test_build_of_same_junction_used_twice(cli, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
+def test_missing_file_in_subproject(cli, datafiles):
+    project = os.path.join(str(datafiles), 'missing-element')
+    result = cli.run(project=project, args=['show', 'target.bst'])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
+
+    # Assert that we have the expected provenance encoded into the error
+    assert "target.bst [line 4 column 2]" in result.stderr
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_missing_file_in_subsubproject(cli, datafiles):
+    project = os.path.join(str(datafiles), 'missing-element')
+    result = cli.run(project=project, args=['show', 'sub-target.bst'])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
+
+    # Assert that we have the expected provenance encoded into the error
+    assert "junction-A.bst:target.bst [line 4 column 2]" in result.stderr
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_missing_junction_in_subproject(cli, datafiles):
+    project = os.path.join(str(datafiles), 'missing-element')
+    result = cli.run(project=project, args=['show', 'sub-target-bad-junction.bst'])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
+
+    # Assert that we have the expected provenance encoded into the error
+    assert "junction-A.bst:bad-junction-target.bst [line 4 column 2]" in result.stderr
+
+
+@pytest.mark.datafiles(DATA_DIR)
 def test_nested_simple(cli, tmpdir, datafiles):
     foo = os.path.join(str(datafiles), 'foo')
     copy_subprojects(foo, datafiles, ['base'])
diff --git a/tests/format/junctions/missing-element/junction-A.bst b/tests/format/junctions/missing-element/junction-A.bst
new file mode 100644
index 000000000..74079f990
--- /dev/null
+++ b/tests/format/junctions/missing-element/junction-A.bst
@@ -0,0 +1,4 @@
+kind: junction
+sources:
+- kind: local
+  path: junctionA
diff --git a/tests/format/junctions/missing-element/junctionA/bad-junction-target.bst b/tests/format/junctions/missing-element/junctionA/bad-junction-target.bst
new file mode 100644
index 000000000..c07c198cd
--- /dev/null
+++ b/tests/format/junctions/missing-element/junctionA/bad-junction-target.bst
@@ -0,0 +1,5 @@
+kind: manual
+
+depends:
+- filename: noelement.bst
+  junction: missing-junction.bst
diff --git a/tests/format/junctions/missing-element/junctionA/junction-B.bst b/tests/format/junctions/missing-element/junctionA/junction-B.bst
new file mode 100644
index 000000000..bc66d7851
--- /dev/null
+++ b/tests/format/junctions/missing-element/junctionA/junction-B.bst
@@ -0,0 +1,4 @@
+kind: junction
+sources:
+- kind: local
+  path: junctionB
diff --git a/tests/format/junctions/missing-element/junctionA/junctionB/project.conf b/tests/format/junctions/missing-element/junctionA/junctionB/project.conf
new file mode 100644
index 000000000..41b8d6c72
--- /dev/null
+++ b/tests/format/junctions/missing-element/junctionA/junctionB/project.conf
@@ -0,0 +1 @@
+name: projectB
diff --git a/tests/format/junctions/missing-element/junctionA/project.conf b/tests/format/junctions/missing-element/junctionA/project.conf
new file mode 100644
index 000000000..5f6ab28a2
--- /dev/null
+++ b/tests/format/junctions/missing-element/junctionA/project.conf
@@ -0,0 +1 @@
+name: projectA
diff --git a/tests/format/junctions/missing-element/junctionA/target.bst b/tests/format/junctions/missing-element/junctionA/target.bst
new file mode 100644
index 000000000..9c3d0bf0e
--- /dev/null
+++ b/tests/format/junctions/missing-element/junctionA/target.bst
@@ -0,0 +1,5 @@
+kind: stack
+
+depends:
+- filename: missing.bst
+  junction: junction-B.bst
diff --git a/tests/format/junctions/missing-element/project.conf b/tests/format/junctions/missing-element/project.conf
new file mode 100644
index 000000000..b32753625
--- /dev/null
+++ b/tests/format/junctions/missing-element/project.conf
@@ -0,0 +1 @@
+name: test
diff --git a/tests/format/junctions/missing-element/sub-target-bad-junction.bst b/tests/format/junctions/missing-element/sub-target-bad-junction.bst
new file mode 100644
index 000000000..f48f6cec9
--- /dev/null
+++ b/tests/format/junctions/missing-element/sub-target-bad-junction.bst
@@ -0,0 +1,5 @@
+kind: stack
+
+depends:
+- filename: bad-junction-target.bst
+  junction: junction-A.bst
diff --git a/tests/format/junctions/missing-element/sub-target.bst b/tests/format/junctions/missing-element/sub-target.bst
new file mode 100644
index 000000000..79e8bc684
--- /dev/null
+++ b/tests/format/junctions/missing-element/sub-target.bst
@@ -0,0 +1,5 @@
+kind: stack
+
+depends:
+- filename: target.bst
+  junction: junction-A.bst
diff --git a/tests/format/junctions/missing-element/target.bst b/tests/format/junctions/missing-element/target.bst
new file mode 100644
index 000000000..69ecef75c
--- /dev/null
+++ b/tests/format/junctions/missing-element/target.bst
@@ -0,0 +1,5 @@
+kind: stack
+
+depends:
+- filename: missing.bst
+  junction: junction-A.bst
diff --git a/tests/format/project.py b/tests/format/project.py
index c746409bb..0f29cac1e 100644
--- a/tests/format/project.py
+++ b/tests/format/project.py
@@ -29,6 +29,26 @@ def test_missing_project_name(cli, datafiles):
 
 
 @pytest.mark.datafiles(os.path.join(DATA_DIR))
+def test_missing_element(cli, datafiles):
+    project = os.path.join(datafiles.dirname, datafiles.basename, "missing-element")
+    result = cli.run(project=project, args=['show', 'manual.bst'])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
+
+    # Assert that we have the expected provenance encoded into the error
+    assert "manual.bst [line 4 column 2]" in result.stderr
+
+
+@pytest.mark.datafiles(os.path.join(DATA_DIR))
+def test_missing_junction(cli, datafiles):
+    project = os.path.join(datafiles.dirname, datafiles.basename, "missing-junction")
+    result = cli.run(project=project, args=['show', 'manual.bst'])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE)
+
+    # Assert that we have the expected provenance encoded into the error
+    assert "manual.bst [line 4 column 2]" in result.stderr
+
+
+@pytest.mark.datafiles(os.path.join(DATA_DIR))
 def test_empty_project_name(cli, datafiles):
     project = os.path.join(datafiles.dirname, datafiles.basename, "emptyname")
     result = cli.run(project=project, args=['workspace', 'list'])
diff --git a/tests/format/project/missing-element/manual.bst b/tests/format/project/missing-element/manual.bst
new file mode 100644
index 000000000..6dd3d7178
--- /dev/null
+++ b/tests/format/project/missing-element/manual.bst
@@ -0,0 +1,4 @@
+kind: manual
+
+depends:
+- missing.bst
diff --git a/tests/format/project/missing-element/project.conf b/tests/format/project/missing-element/project.conf
new file mode 100644
index 000000000..b32753625
--- /dev/null
+++ b/tests/format/project/missing-element/project.conf
@@ -0,0 +1 @@
+name: test
diff --git a/tests/format/project/missing-junction/manual.bst b/tests/format/project/missing-junction/manual.bst
new file mode 100644
index 000000000..0e0ae0860
--- /dev/null
+++ b/tests/format/project/missing-junction/manual.bst
@@ -0,0 +1,5 @@
+kind: manual
+
+depends:
+- filename: element.bst
+  junction: missing.bst
diff --git a/tests/format/project/missing-junction/project.conf b/tests/format/project/missing-junction/project.conf
new file mode 100644
index 000000000..b32753625
--- /dev/null
+++ b/tests/format/project/missing-junction/project.conf
@@ -0,0 +1 @@
+name: test