Merge branch 'aevri/include-error' into 'master'

More user-friendly reporting on include errors

See merge request BuildStream/buildstream!891

2 files changed, 68 insertions, 11 deletions

diff --git a/buildstream/_includes.py b/buildstream/_includes.py
index df14c9f2d..1312d1982 100644
--- a/buildstream/_includes.py
+++ b/buildstream/_includes.py
@@ -40,19 +40,34 @@ class Includes:
             includes = [_yaml.node_get(node, str, '(@)')]
         else:
             includes = _yaml.node_get(node, list, '(@)', default_value=None)
+
+        include_provenance = None
         if '(@)' in node:
+            include_provenance = _yaml.node_get_provenance(node, key='(@)')
             del node['(@)']
 
         if includes:
             for include in reversed(includes):
                 if only_local and ':' in include:
                     continue
-                include_node, file_path, sub_loader = self._include_file(include,
-                                                                         current_loader)
+                try:
+                    include_node, file_path, sub_loader = self._include_file(include,
+                                                                             current_loader)
+                except LoadError as e:
+                    if e.reason == LoadErrorReason.MISSING_FILE:
+                        message = "{}: Include block references a file that could not be found: '{}'.".format(
+                            include_provenance, include)
+                        raise LoadError(LoadErrorReason.MISSING_FILE, message) from e
+                    elif e.reason == LoadErrorReason.LOADING_DIRECTORY:
+                        message = "{}: Include block references a directory instead of a file: '{}'.".format(
+                            include_provenance, include)
+                        raise LoadError(LoadErrorReason.LOADING_DIRECTORY, message) from e
+                    else:
+                        raise
+
                 if file_path in included:
-                    provenance = _yaml.node_get_provenance(node)
                     raise LoadError(LoadErrorReason.RECURSIVE_INCLUDE,
-                                    "{}: trying to recursively include {}". format(provenance,
+                                    "{}: trying to recursively include {}". format(include_provenance,
                                                                                    file_path))
                 # Because the included node will be modified, we need
                 # to copy it so that we do not modify the toplevel
@@ -101,7 +116,7 @@ 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(os.path.join(directory, include),
+            self._loaded[key] = _yaml.load(file_path,
                                            shortname=shortname,
                                            project=project,
                                            copy_tree=self._copy_tree)
diff --git a/tests/format/include.py b/tests/format/include.py
index 1db37083e..83e19ad28 100644
--- a/tests/format/include.py
+++ b/tests/format/include.py
@@ -1,4 +1,5 @@
 import os
+import textwrap
 import pytest
 from buildstream import _yaml
 from buildstream._exceptions import ErrorDomain, LoadErrorReason
@@ -27,6 +28,46 @@ def test_include_project_file(cli, datafiles):
     assert loaded['included'] == 'True'
 
 
+def test_include_missing_file(cli, tmpdir):
+    tmpdir.join('project.conf').write('{"name": "test"}')
+    element = tmpdir.join('include_missing_file.bst')
+
+    # Normally we would use dicts and _yaml.dump to write such things, but here
+    # we want to be sure of a stable line and column number.
+    element.write(textwrap.dedent("""
+        kind: manual
+
+        "(@)":
+          - nosuch.yaml
+    """).strip())
+
+    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
+
+
+def test_include_dir(cli, tmpdir):
+    tmpdir.join('project.conf').write('{"name": "test"}')
+    tmpdir.mkdir('subdir')
+    element = tmpdir.join('include_dir.bst')
+
+    # Normally we would use dicts and _yaml.dump to write such things, but here
+    # we want to be sure of a stable line and column number.
+    element.write(textwrap.dedent("""
+        kind: manual
+
+        "(@)":
+          - subdir/
+    """).strip())
+
+    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
+
+
 @pytest.mark.datafiles(DATA_DIR)
 def test_include_junction_file(cli, tmpdir, datafiles):
     project = os.path.join(str(datafiles), 'junction')
@@ -47,7 +88,7 @@ def test_include_junction_file(cli, tmpdir, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_include_junction_options(cli, tmpdir, datafiles):
+def test_include_junction_options(cli, datafiles):
     project = os.path.join(str(datafiles), 'options')
 
     result = cli.run(project=project, args=[
@@ -128,7 +169,7 @@ def test_junction_element_not_partial_project_file(cli, tmpdir, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_include_element_overrides(cli, tmpdir, datafiles):
+def test_include_element_overrides(cli, datafiles):
     project = os.path.join(str(datafiles), 'overrides')
 
     result = cli.run(project=project, args=[
@@ -143,7 +184,7 @@ def test_include_element_overrides(cli, tmpdir, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_include_element_overrides_composition(cli, tmpdir, datafiles):
+def test_include_element_overrides_composition(cli, datafiles):
     project = os.path.join(str(datafiles), 'overrides')
 
     result = cli.run(project=project, args=[
@@ -158,7 +199,7 @@ def test_include_element_overrides_composition(cli, tmpdir, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_include_element_overrides_sub_include(cli, tmpdir, datafiles):
+def test_include_element_overrides_sub_include(cli, datafiles):
     project = os.path.join(str(datafiles), 'sub-include')
 
     result = cli.run(project=project, args=[
@@ -192,7 +233,7 @@ def test_junction_do_not_use_included_overrides(cli, tmpdir, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_conditional_in_fragment(cli, tmpdir, datafiles):
+def test_conditional_in_fragment(cli, datafiles):
     project = os.path.join(str(datafiles), 'conditional')
 
     result = cli.run(project=project, args=[
@@ -222,7 +263,7 @@ def test_inner(cli, datafiles):
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_recusive_include(cli, tmpdir, datafiles):
+def test_recursive_include(cli, datafiles):
     project = os.path.join(str(datafiles), 'recursive')
 
     result = cli.run(project=project, args=[
@@ -231,6 +272,7 @@ def test_recusive_include(cli, tmpdir, datafiles):
         '--format', '%{vars}',
         'element.bst'])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_INCLUDE)
+    assert 'line 2 column 2' in result.stderr
 
 
 @pytest.mark.datafiles(DATA_DIR)