Merge branch 'chandan/element-filename-requirements' into 'master'

Add warnings about invalid characters in filename

See merge request BuildStream/buildstream!1028

7 files changed, 127 insertions, 18 deletions

diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py
index 0de0e2b9c..16e880388 100644
--- a/buildstream/_loader/loader.py
+++ b/buildstream/_loader/loader.py
@@ -99,7 +99,6 @@ class Loader():
     # Returns: The toplevel LoadElement
     def load(self, targets, rewritable=False, ticker=None, fetch_subprojects=False):
 
-        invalid_elements = []
         for filename in targets:
             if os.path.isabs(filename):
                 # XXX Should this just be an assertion ?
@@ -109,14 +108,8 @@ class Loader():
                                 "path to the base project directory: {}"
                                 .format(filename, self._basedir))
 
-            if not filename.endswith(".bst"):
-                invalid_elements.append(filename)
+        self._warn_invalid_elements(targets)
 
-        if invalid_elements:
-            self._warn("Target elements '{}' do not have expected file extension `.bst` "
-                       "Improperly named elements will not be discoverable by commands"
-                       .format(invalid_elements),
-                       warning_token=CoreWarnings.BAD_ELEMENT_SUFFIX)
         # First pass, recursively load files and populate our table of LoadElements
         #
         deps = []
@@ -280,12 +273,7 @@ class Loader():
         self._elements[filename] = element
 
         # Load all dependency files for the new LoadElement
-        invalid_elements = []
         for dep in element.deps:
-            if not dep.name.endswith(".bst"):
-                invalid_elements.append(dep.name)
-                continue
-
             if dep.junction:
                 self._load_file(dep.junction, rewritable, ticker, fetch_subprojects, yaml_cache)
                 loader = self._get_loader(dep.junction, rewritable=rewritable, ticker=ticker,
@@ -300,11 +288,9 @@ class Loader():
                                 "{}: Cannot depend on junction"
                                 .format(dep.provenance))
 
-        if invalid_elements:
-            self._warn("The following dependencies do not have expected file extension `.bst`: {} "
-                       "Improperly named elements will not be discoverable by commands"
-                       .format(invalid_elements),
-                       warning_token=CoreWarnings.BAD_ELEMENT_SUFFIX)
+        deps_names = [dep.name for dep in element.deps]
+        self._warn_invalid_elements(deps_names)
+
         return element
 
     # _check_circular_deps():
@@ -679,3 +665,69 @@ class Loader():
 
         message = Message(None, MessageType.WARN, brief)
         self._context.message(message)
+
+    # Print warning messages if any of the specified elements have invalid names.
+    #
+    # Valid filenames should end with ".bst" extension.
+    #
+    # Args:
+    #    elements (list): List of element names
+    #
+    # Raises:
+    #     (:class:`.LoadError`): When warning_token is considered fatal by the project configuration
+    #
+    def _warn_invalid_elements(self, elements):
+
+        # invalid_elements
+        #
+        # A dict that maps warning types to the matching elements.
+        invalid_elements = {
+            CoreWarnings.BAD_ELEMENT_SUFFIX: [],
+            CoreWarnings.BAD_CHARACTERS_IN_NAME: [],
+        }
+
+        for filename in elements:
+            if not filename.endswith(".bst"):
+                invalid_elements[CoreWarnings.BAD_ELEMENT_SUFFIX].append(filename)
+            if not self._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)
+
+    # Check if given filename containers valid characters.
+    #
+    # Args:
+    #    name (str): Name of the file
+    #
+    # Returns:
+    #    (bool): True if all characters are valid, False otherwise.
+    #
+    def _valid_chars_name(self, name):
+        for char in name:
+            char_val = ord(char)
+
+            # 0-31 are control chars, 127 is DEL, and >127 means non-ASCII
+            if char_val <= 31 or char_val >= 127:
+                return False
+
+            # Disallow characters that are invalid on Windows. The list can be
+            # found at https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file
+            #
+            # Note that although : (colon) is not allowed, we do not raise
+            # warnings because of that, since we use it as a separator for
+            # junctioned elements.
+            #
+            # We also do not raise warnings on slashes since they are used as
+            # path separators.
+            if char in r'<>"|?*':
+                return False
+
+        return True
diff --git a/buildstream/types.py b/buildstream/types.py
index 5e6265b4e..23d78b08c 100644
--- a/buildstream/types.py
+++ b/buildstream/types.py
@@ -105,6 +105,12 @@ class CoreWarnings():
     is referenced either on the command line or by another element
     """
 
+    BAD_CHARACTERS_IN_NAME = "bad-characters-in-name"
+    """
+    This warning will be produces when filename for a target contains invalid
+    characters in its name.
+    """
+
 
 # _KeyStrength():
 #
diff --git a/doc/source/format_declaring.rst b/doc/source/format_declaring.rst
index 57ea4488a..714e1fa33 100644
--- a/doc/source/format_declaring.rst
+++ b/doc/source/format_declaring.rst
@@ -526,3 +526,27 @@ read-only variables are also dynamically declared by BuildStream:
   build, support for this is conditional on the element type
   and the build system used (any element using 'make' can
   implement this).
+
+
+Naming elements
+---------------
+When naming the element files, use the following rules:
+
+* The name of the file must have ``.bst`` extension.
+
+* All characters in the name must be printable 7-bit ASCII characters.
+
+* Following characters are reserved and must not be part of the name:
+
+  - ``<`` (less than)
+  - ``>`` (greater than)
+  - ``:`` (colon)
+  - ``"`` (double quote)
+  - ``/`` (forward slash)
+  - ``\`` (backslash)
+  - ``|`` (vertical bar)
+  - ``?`` (question mark)
+  - ``*`` (asterisk)
+
+BuildStream will attempt to raise warnings when any of these rules are violated
+but that may not always be possible.
diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py
index 03bba0b26..dc7ce6847 100644
--- a/tests/frontend/buildcheckout.py
+++ b/tests/frontend/buildcheckout.py
@@ -86,6 +86,20 @@ def test_build_invalid_suffix_dep(datafiles, cli, strict, hardlinks):
 
 
 @pytest.mark.datafiles(DATA_DIR)
+def test_build_invalid_filename_chars(datafiles, cli):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    result = cli.run(project=project, args=strict_args(['build', 'invalid-chars|<>-in-name.bst'], 'non-strict'))
+    result.assert_main_error(ErrorDomain.LOAD, "bad-characters-in-name")
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_build_invalid_filename_chars_dep(datafiles, cli):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    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")])
 def test_build_checkout_deps(datafiles, cli, deps):
     project = os.path.join(datafiles.dirname, datafiles.basename)
diff --git a/tests/frontend/project/elements/invalid-chars-in-dep.bst b/tests/frontend/project/elements/invalid-chars-in-dep.bst
new file mode 100644
index 000000000..6a5ec30c8
--- /dev/null
+++ b/tests/frontend/project/elements/invalid-chars-in-dep.bst
@@ -0,0 +1,8 @@
+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/invalid-chars|<>-in-name.bst b/tests/frontend/project/elements/invalid-chars|<>-in-name.bst
new file mode 100644
index 000000000..bc6a13110
--- /dev/null
+++ b/tests/frontend/project/elements/invalid-chars|<>-in-name.bst
@@ -0,0 +1,4 @@
+kind: stack
+description: |
+  The name of this files contains characters that are not allowed by
+  BuildStream, using it should raise a warning.
diff --git a/tests/frontend/project/project.conf b/tests/frontend/project/project.conf
index a7e4a023c..ed18221e4 100644
--- a/tests/frontend/project/project.conf
+++ b/tests/frontend/project/project.conf
@@ -5,3 +5,4 @@ element-path: elements
 
 fatal-warnings:
 - bad-element-suffix
+- bad-characters-in-name