servo: Merge #14051 - Adding linting checks for buildbot_steps.yml (from birryree:tidy-check-buildbot-steps); r=aneeshusa

This pull request adds some tidy checks around YAML files, and specifically `buildbot_steps.yml`.

Tidy checks added:

* YAML files are checked for well-formedness/parse-ability
* Whether a YAML file has duplicate keys
* Whether a `buildbot_steps.yml` file contains only mappings to list-of-strings.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [x] These changes fix #13838 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

…ing checking for correct mappings and duplicate YAML keys. Added unit tests to test_tidy.py.

Source-Repo: https://github.com/servo/servo
Source-Revision: 21ad1c210997daba82ec49e1572c7b0634b6f337

servo/python/requirements.txt

@@ -12,6 +12,9 @@ flake8 == 2.4.1
 pep8 == 1.5.7
 pyflakes == 0.8.1
 
+# For buildbot checking
+PyYAML == 3.12
+
 # For test-webidl
 ply == 3.8
 

servo/python/tidy/servo_tidy/tidy.py

@@ -19,6 +19,7 @@ import subprocess
 import sys
 import colorama
 import toml
+import yaml
 
 from licenseck import MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
 
@@ -47,7 +48,8 @@ COMMENTS = ["// ", "# ", " *", "/* "]
 # File patterns to include in the non-WPT tidy check.
 FILE_PATTERNS_TO_CHECK = ["*.rs", "*.rc", "*.cpp", "*.c",
                           "*.h", "Cargo.lock", "*.py", "*.sh",
-                          "*.toml", "*.webidl", "*.json", "*.html"]
+                          "*.toml", "*.webidl", "*.json", "*.html",
+                          "*.yml"]
 
 # File patterns that are ignored for all tidy and lint checks.
 FILE_PATTERNS_TO_IGNORE = ["*.#*", "*.pyc", "fake-ld.sh"]
@@ -178,7 +180,7 @@ def is_apache_licensed(header):
 
 
 def check_license(file_name, lines):
-    if any(file_name.endswith(ext) for ext in (".toml", ".lock", ".json", ".html")) or \
+    if any(file_name.endswith(ext) for ext in (".yml", ".toml", ".lock", ".json", ".html")) or \
        config["skip-check-licenses"]:
         raise StopIteration
 
@@ -216,7 +218,7 @@ def check_modeline(file_name, lines):
 
 
 def check_length(file_name, idx, line):
-    if any(file_name.endswith(ext) for ext in (".lock", ".json", ".html", ".toml")) or \
+    if any(file_name.endswith(ext) for ext in (".yml", ".lock", ".json", ".html", ".toml")) or \
        config["skip-check-length"]:
         raise StopIteration
 
@@ -675,6 +677,58 @@ def check_webidl_spec(file_name, contents):
     yield (0, "No specification link found.")
 
 
+def duplicate_key_yaml_constructor(loader, node, deep=False):
+    mapping = {}
+    for key_node, value_node in node.value:
+        key = loader.construct_object(key_node, deep=deep)
+        if key in mapping:
+            raise KeyError(key)
+        value = loader.construct_object(value_node, deep=deep)
+        mapping[key] = value
+    return loader.construct_mapping(node, deep)
+
+
+def lint_buildbot_steps_yaml(mapping):
+    # Check for well-formedness of contents
+    # A well-formed buildbot_steps.yml should be a map to list of strings
+    for k in mapping.keys():
+        if not isinstance(mapping[k], list):
+            raise ValueError("Key '{}' maps to type '{}', but list expected".format(k, type(mapping[k]).__name__))
+
+        # check if value is a list of strings
+        for item in itertools.ifilter(lambda i: not isinstance(i, str), mapping[k]):
+            raise ValueError("List mapped to '{}' contains non-string element".format(k))
+
+
+class SafeYamlLoader(yaml.SafeLoader):
+    """Subclass of yaml.SafeLoader to avoid mutating the global SafeLoader."""
+    pass
+
+
+def check_yaml(file_name, contents):
+    if not file_name.endswith("buildbot_steps.yml"):
+        raise StopIteration
+
+    # YAML specification doesn't explicitly disallow
+    # duplicate keys, but they shouldn't be allowed in
+    # buildbot_steps.yml as it could lead to confusion
+    SafeYamlLoader.add_constructor(
+        yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG,
+        duplicate_key_yaml_constructor
+    )
+
+    try:
+        contents = yaml.load(contents, Loader=SafeYamlLoader)
+        lint_buildbot_steps_yaml(contents)
+    except yaml.YAMLError as e:
+        line = e.problem_mark.line + 1 if hasattr(e, 'problem_mark') else None
+        yield (line, e)
+    except KeyError as e:
+        yield (None, "Duplicated Key ({})".format(e.message))
+    except ValueError as e:
+        yield (None, e.message)
+
+
 def check_for_possible_duplicate_json_keys(key_value_pairs):
     keys = [x[0] for x in key_value_pairs]
     seen_keys = set()
@@ -964,7 +1018,7 @@ def scan(only_changed_files=False, progress=True):
     directory_errors = check_directory_files(config['check_ext'])
     # standard checks
     files_to_check = filter_files('.', only_changed_files, progress)
-    checking_functions = (check_flake8, check_lock, check_webidl_spec, check_json)
+    checking_functions = (check_flake8, check_lock, check_webidl_spec, check_json, check_yaml)
     line_checking_functions = (check_license, check_by_line, check_toml, check_shell,
                                check_rust, check_spec, check_modeline)
     file_errors = collect_errors_for_files(files_to_check, checking_functions, line_checking_functions)

servo/python/tidy/servo_tidy_tests/duplicate_keys_buildbot_steps.yml

@@ -0,0 +1,7 @@
+---
+duplicate_yaml_key:
+    - value1
+other_key:
+    - value2
+duplicate_yaml_key:
+    - value3

servo/python/tidy/servo_tidy_tests/non_list_mapping_buildbot_steps.yml

@@ -0,0 +1,2 @@
+---
+non-list-key: "string string"

servo/python/tidy/servo_tidy_tests/non_string_list_buildbot_steps.yml

@@ -0,0 +1,7 @@
+---
+# This is a buildbot_steps.yml file that should break linting becasue it is not a
+# mapping to a list of strings
+mapping_key:
+    - - list_of_list
+      - sublist_item1
+      - sublist_item2

servo/python/tidy/servo_tidy_tests/test_tidy.py

@@ -177,6 +177,21 @@ class CheckTidiness(unittest.TestCase):
         self.assertEqual('Unordered key (found b before a)', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
+    def test_yaml_with_duplicate_key(self):
+        errors = tidy.collect_errors_for_files(iterFile('duplicate_keys_buildbot_steps.yml'), [tidy.check_yaml], [], print_text=False)
+        self.assertEqual('Duplicated Key (duplicate_yaml_key)', errors.next()[2])
+        self.assertNoMoreErrors(errors)
+
+    def test_non_list_mapped_buildbot_steps(self):
+        errors = tidy.collect_errors_for_files(iterFile('non_list_mapping_buildbot_steps.yml'), [tidy.check_yaml], [], print_text=False)
+        self.assertEqual("Key 'non-list-key' maps to type 'str', but list expected", errors.next()[2])
+        self.assertNoMoreErrors(errors)
+
+    def test_non_string_list_mapping_buildbot_steps(self):
+        errors = tidy.collect_errors_for_files(iterFile('non_string_list_buildbot_steps.yml'), [tidy.check_yaml], [], print_text=False)
+        self.assertEqual("List mapped to 'mapping_key' contains non-string element", errors.next()[2])
+        self.assertNoMoreErrors(errors)
+
     def test_lock(self):
         errors = tidy.collect_errors_for_files(iterFile('duplicated_package.lock'), [tidy.check_lock], [], print_text=False)
         msg = """duplicate versions for package "test"