Merge pull request #1523 from apache/tristan/strict-element-names
Error out on invalid element names
diff --git a/NEWS b/NEWS
index 7d6d0a5..18c81c4 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,12 @@
(unreleased)
============
+Format
+------
+ o BREAKING CHANGE: Element names must now bare the `.bst` suffix and not
+ contain invalid characters, as was already documented. Now the warning
+ is replaced by an unconditional error.
+
CLI
---
o BREAKING CHANGE: Removed `--deps plan` from all commands
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py
index df4df84..ea73afd 100644
--- a/src/buildstream/_loader/loader.py
+++ b/src/buildstream/_loader/loader.py
@@ -28,7 +28,7 @@
from .._profile import Topics, PROFILER
from .._includes import Includes
from .._utils import valid_chars_name
-from ..types import CoreWarnings, _KeyStrength
+from ..types import _KeyStrength
from .types import Symbol
from . import loadelement
@@ -125,8 +125,6 @@
LoadErrorReason.INVALID_DATA,
)
- self._warn_invalid_elements(targets)
-
# First pass, recursively load files and populate our table of LoadElements
#
target_elements = []
@@ -267,6 +265,9 @@
# (LoadElement): A partially-loaded LoadElement
#
def _load_file_no_deps(self, filename, provenance_node=None):
+
+ self._assert_element_name(filename, provenance_node)
+
# Load the data and process any conditional statements therein
fullpath = os.path.join(self._basedir, filename)
try:
@@ -530,9 +531,6 @@
dep.set_element(dep_element)
current_element[0].dependencies.append(dep) # pylint: disable=no-member
else:
- # We do not have any more dependencies to load for this
- # element on the queue, report any invalid dep names
- self._warn_invalid_elements(loader_queue[-1][2])
# And pop the element off the queue
loader_queue.pop()
@@ -1000,6 +998,8 @@
loader = self.get_loader(junction_path[-2], provenance_node, load_subprojects=load_subprojects)
return junction_path[-2], junction_path[-1], loader
+ # _warn():
+ #
# Print a warning message, checks warning_token against project configuration
#
# Args:
@@ -1017,47 +1017,34 @@
self.load_context.context.messenger.warn(brief)
- # Print warning messages if any of the specified elements have invalid names.
+ # _assert_element_name():
+ #
+ # Raises an error if any of the specified elements have invalid names.
#
# Valid filenames should end with ".bst" extension.
#
# Args:
- # elements (list): List of element names
+ # filename (str): The element name
+ # provenance_node (Node): The provenance node, or None
#
# Raises:
- # (:class:`.LoadError`): When warning_token is considered fatal by the project configuration
+ # (:class:`.LoadError`): If the element name is invalid
#
- def _warn_invalid_elements(self, elements):
+ def _assert_element_name(self, filename, provenance_node):
+ error_message = None
+ error_reason = None
- # invalid_elements
- #
- # A dict that maps warning types to the matching elements.
- invalid_elements = {
- CoreWarnings.BAD_ELEMENT_SUFFIX: [],
- CoreWarnings.BAD_CHARACTERS_IN_NAME: [],
- }
+ if not filename.endswith(".bst"):
+ error_message = "Element '{}' does not have expected file extension `.bst`".format(filename)
+ error_reason = LoadErrorReason.BAD_ELEMENT_SUFFIX
+ elif not valid_chars_name(filename):
+ error_message = "Element '{}' has invalid characters.".format(filename)
+ error_reason = LoadErrorReason.BAD_CHARACTERS_IN_NAME
- for filename in elements:
- if not filename.endswith(".bst"):
- invalid_elements[CoreWarnings.BAD_ELEMENT_SUFFIX].append(filename)
- if not valid_chars_name(filename):
- invalid_elements[CoreWarnings.BAD_CHARACTERS_IN_NAME].append(filename)
-
- if invalid_elements[CoreWarnings.BAD_ELEMENT_SUFFIX]:
- self._warn(
- "Target elements '{}' do not have expected file extension `.bst` "
- "Improperly named elements will not be discoverable by commands".format(
- invalid_elements[CoreWarnings.BAD_ELEMENT_SUFFIX]
- ),
- warning_token=CoreWarnings.BAD_ELEMENT_SUFFIX,
- )
- if invalid_elements[CoreWarnings.BAD_CHARACTERS_IN_NAME]:
- self._warn(
- "Target elements '{}' have invalid characerts in their name.".format(
- invalid_elements[CoreWarnings.BAD_CHARACTERS_IN_NAME]
- ),
- warning_token=CoreWarnings.BAD_CHARACTERS_IN_NAME,
- )
+ if error_message:
+ if provenance_node is not None:
+ error_message = "{}: {}".format(provenance_node.get_provenance(), error_message)
+ raise LoadError(error_message, error_reason)
# _clean_caches()
#
diff --git a/src/buildstream/exceptions.py b/src/buildstream/exceptions.py
index 4b91189..8d828d0 100644
--- a/src/buildstream/exceptions.py
+++ b/src/buildstream/exceptions.py
@@ -146,3 +146,15 @@
CIRCULAR_REFERENCE = 26
"""A circular element reference was detected"""
+
+ BAD_ELEMENT_SUFFIX = 27
+ """
+ This warning will be produced when an element whose name does not end in .bst
+ is referenced either on the command line or by another element
+ """
+
+ BAD_CHARACTERS_IN_NAME = 28
+ """
+ This warning will be produced when a filename for a target contains invalid
+ characters in its name.
+ """
diff --git a/src/buildstream/types.py b/src/buildstream/types.py
index ac3b180..8c87b4f 100644
--- a/src/buildstream/types.py
+++ b/src/buildstream/types.py
@@ -113,18 +113,6 @@
which is found to be invalid based on the configured track
"""
- BAD_ELEMENT_SUFFIX = "bad-element-suffix"
- """
- This warning will be produced when an element whose name does not end in .bst
- is referenced either on the command line or by another element
- """
-
- BAD_CHARACTERS_IN_NAME = "bad-characters-in-name"
- """
- This warning will be produced when a filename for a target contains invalid
- characters in its name.
- """
-
UNALIASED_URL = "unaliased-url"
"""
A URL used for fetching a sources was specified without specifying any
diff --git a/tests/format/elementnames.py b/tests/format/elementnames.py
new file mode 100644
index 0000000..f059d54
--- /dev/null
+++ b/tests/format/elementnames.py
@@ -0,0 +1,29 @@
+# Pylint doesn't play well with fixtures and dependency injection from pytest
+# pylint: disable=redefined-outer-name
+
+import os
+import pytest
+
+from buildstream.exceptions import ErrorDomain, LoadErrorReason
+from buildstream.testing import cli # pylint: disable=unused-import
+
+DATA_DIR = os.path.dirname(os.path.realpath(__file__))
+
+
+@pytest.mark.parametrize(
+ "target,reason,provenance",
+ [
+ ("farm.pony", LoadErrorReason.BAD_ELEMENT_SUFFIX, None),
+ ('The "quoted" pony.bst', LoadErrorReason.BAD_CHARACTERS_IN_NAME, None),
+ ("bad-suffix-dep.bst", LoadErrorReason.BAD_ELEMENT_SUFFIX, "bad-suffix-dep.bst [line 3 column 2]"),
+ ("bad-chars-dep.bst", LoadErrorReason.BAD_CHARACTERS_IN_NAME, "bad-chars-dep.bst [line 3 column 2]"),
+ ],
+ ids=["toplevel-bad-suffix", "toplevel-bad-chars", "dependency-bad-suffix", "dependency-bad-chars"],
+)
+@pytest.mark.datafiles(DATA_DIR)
+def test_invalid_element_names(cli, datafiles, target, reason, provenance):
+ project = os.path.join(str(datafiles), "elementnames")
+ result = cli.run(project=project, silent=True, args=["show", target])
+ result.assert_main_error(ErrorDomain.LOAD, reason)
+ if provenance:
+ assert provenance in result.stderr
diff --git a/tests/format/elementnames/bad-chars-dep.bst b/tests/format/elementnames/bad-chars-dep.bst
new file mode 100644
index 0000000..d637799
--- /dev/null
+++ b/tests/format/elementnames/bad-chars-dep.bst
@@ -0,0 +1,3 @@
+kind: stack
+depends:
+- <invalid>.bst
diff --git a/tests/format/elementnames/bad-suffix-dep.bst b/tests/format/elementnames/bad-suffix-dep.bst
new file mode 100644
index 0000000..c96604d
--- /dev/null
+++ b/tests/format/elementnames/bad-suffix-dep.bst
@@ -0,0 +1,3 @@
+kind: stack
+depends:
+- rainbow.pony
diff --git a/tests/format/elementnames/project.conf b/tests/format/elementnames/project.conf
new file mode 100644
index 0000000..b0110aa
--- /dev/null
+++ b/tests/format/elementnames/project.conf
@@ -0,0 +1,4 @@
+# Basic project configuration that doesnt override anything
+#
+name: test
+min-version: 2.0
diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py
index 6021ff0..330b9cd 100644
--- a/tests/frontend/buildcheckout.py
+++ b/tests/frontend/buildcheckout.py
@@ -10,7 +10,7 @@
from buildstream.testing import cli # pylint: disable=unused-import
from buildstream.testing import create_repo
-from buildstream.testing._utils.site import IS_WINDOWS, CASD_SEPARATE_USER
+from buildstream.testing._utils.site import CASD_SEPARATE_USER
from buildstream import _yaml
from buildstream.exceptions import ErrorDomain, LoadErrorReason
from buildstream import utils
@@ -170,60 +170,6 @@
@pytest.mark.datafiles(DATA_DIR)
-@pytest.mark.parametrize("strict,hardlinks", [("non-strict", "hardlinks"),])
-def test_build_invalid_suffix(datafiles, cli, strict, hardlinks):
- project = str(datafiles)
-
- result = cli.run(project=project, args=strict_args(["build", "target.foo"], strict))
- result.assert_main_error(ErrorDomain.LOAD, "bad-element-suffix")
-
-
-@pytest.mark.datafiles(DATA_DIR)
-@pytest.mark.parametrize("strict,hardlinks", [("non-strict", "hardlinks"),])
-def test_build_invalid_suffix_dep(datafiles, cli, strict, hardlinks):
- project = str(datafiles)
-
- # target2.bst depends on an element called target.foo
- result = cli.run(project=project, args=strict_args(["build", "target2.bst"], strict))
- result.assert_main_error(ErrorDomain.LOAD, "bad-element-suffix")
-
-
-@pytest.mark.skipif(IS_WINDOWS, reason="Not available on Windows")
-@pytest.mark.datafiles(DATA_DIR)
-def test_build_invalid_filename_chars(datafiles, cli):
- project = str(datafiles)
- element_name = "invalid-chars|<>-in-name.bst"
-
- # The name of this file contains characters that are not allowed by
- # BuildStream, using it should raise a warning.
- element = {
- "kind": "stack",
- }
- _yaml.roundtrip_dump(element, os.path.join(project, "elements", element_name))
-
- result = cli.run(project=project, args=strict_args(["build", element_name], "non-strict"))
- result.assert_main_error(ErrorDomain.LOAD, "bad-characters-in-name")
-
-
-@pytest.mark.skipif(IS_WINDOWS, reason="Not available on Windows")
-@pytest.mark.datafiles(DATA_DIR)
-def test_build_invalid_filename_chars_dep(datafiles, cli):
- project = str(datafiles)
- element_name = "invalid-chars|<>-in-name.bst"
-
- # The name of this file contains characters that are not allowed by
- # BuildStream, and is listed as a dependency of 'invalid-chars-in-dep.bst'.
- # This should also raise a warning.
- element = {
- "kind": "stack",
- }
- _yaml.roundtrip_dump(element, os.path.join(project, "elements", element_name))
-
- result = cli.run(project=project, args=strict_args(["build", "invalid-chars-in-dep.bst"], "non-strict"))
- result.assert_main_error(ErrorDomain.LOAD, "bad-characters-in-name")
-
-
-@pytest.mark.datafiles(DATA_DIR)
@pytest.mark.parametrize("deps", [("run"), ("none"), ("build"), ("all")])
def test_build_checkout_deps(datafiles, cli, deps):
project = str(datafiles)
diff --git a/tests/frontend/project/elements/invalid-chars-in-dep.bst b/tests/frontend/project/elements/invalid-chars-in-dep.bst
deleted file mode 100644
index 6a5ec30..0000000
--- a/tests/frontend/project/elements/invalid-chars-in-dep.bst
+++ /dev/null
@@ -1,8 +0,0 @@
-kind: stack
-description: |
-
- This element itself has a valid name, but depends on elements that have
- invalid names. This should also result in a warning.
-
-depends:
-- invalid-chars|<>-in-name.bst
diff --git a/tests/frontend/project/elements/target2.bst b/tests/frontend/project/elements/target2.bst
deleted file mode 100644
index 259819f..0000000
--- a/tests/frontend/project/elements/target2.bst
+++ /dev/null
@@ -1,7 +0,0 @@
-kind: stack
-description: |
-
- Main stack target for the bst build test
-
-depends:
-- target.foo
diff --git a/tests/frontend/project/project.conf b/tests/frontend/project/project.conf
index c82f85b..5ba3168 100644
--- a/tests/frontend/project/project.conf
+++ b/tests/frontend/project/project.conf
@@ -2,7 +2,3 @@
name: test
min-version: 2.0
element-path: elements
-
-fatal-warnings:
-- bad-element-suffix
-- bad-characters-in-name
