diff options
| -rw-r--r-- | python/requirements.txt | 3 | ||||
| -rw-r--r-- | python/tidy/servo_tidy/tidy.py | 62 | ||||
| -rw-r--r-- | python/tidy/servo_tidy_tests/duplicate_keys_buildbot_steps.yml | 7 | ||||
| -rw-r--r-- | python/tidy/servo_tidy_tests/non_list_mapping_buildbot_steps.yml | 2 | ||||
| -rw-r--r-- | python/tidy/servo_tidy_tests/non_string_list_buildbot_steps.yml | 7 | ||||
| -rw-r--r-- | python/tidy/servo_tidy_tests/test_tidy.py | 15 |
6 files changed, 92 insertions, 4 deletions
diff --git a/python/requirements.txt b/python/requirements.txt index 51290d7a554..34794fd5031 100644 --- a/python/requirements.txt +++ b/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 diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py index 682100f6e01..b40bdd67194 100644 --- a/python/tidy/servo_tidy/tidy.py +++ b/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) diff --git a/python/tidy/servo_tidy_tests/duplicate_keys_buildbot_steps.yml b/python/tidy/servo_tidy_tests/duplicate_keys_buildbot_steps.yml new file mode 100644 index 00000000000..ed5d046095f --- /dev/null +++ b/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 diff --git a/python/tidy/servo_tidy_tests/non_list_mapping_buildbot_steps.yml b/python/tidy/servo_tidy_tests/non_list_mapping_buildbot_steps.yml new file mode 100644 index 00000000000..2581aa21d88 --- /dev/null +++ b/python/tidy/servo_tidy_tests/non_list_mapping_buildbot_steps.yml @@ -0,0 +1,2 @@ +--- +non-list-key: "string string" diff --git a/python/tidy/servo_tidy_tests/non_string_list_buildbot_steps.yml b/python/tidy/servo_tidy_tests/non_string_list_buildbot_steps.yml new file mode 100644 index 00000000000..d9255e7cfe5 --- /dev/null +++ b/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 diff --git a/python/tidy/servo_tidy_tests/test_tidy.py b/python/tidy/servo_tidy_tests/test_tidy.py index a6a618d1608..c6fe8bd83fa 100644 --- a/python/tidy/servo_tidy_tests/test_tidy.py +++ b/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" |