servo: Merge #14166 - Allow tidy to run custom project-specific lints (from Wafflespeanut:tidy); r=frewsxcv
authorRavi Shankar <wafflespeanut@gmail.com>
Sat, 12 Nov 2016 11:06:27 -0600
changeset 340141 a7f336da270e02ae7c32d5d780bcbb53902deecf
parent 340140 a2c4c7bb733cd999af7945c3b4cc210fcc1a89aa
child 340142 66c2a7f27fadbda5c68283beda9d4e8baf27133f
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfrewsxcv
servo: Merge #14166 - Allow tidy to run custom project-specific lints (from Wafflespeanut:tidy); r=frewsxcv <!-- Please describe your changes on the following line: --> Since tidy is a package, it shouldn't contain our WPT lint. This PR changes the file list generator (we already have) into an object, and allows tidy to run custom python lint scripts (specified in the config file) under the context of `LintRunner`. The errors are then chained into our mechanism. r? @frewsxcv (cc @jdm @edunham @aneeshusa and anyone else interested in reviewing it) --- <!-- 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 <!-- Either: --> - [x] There are tests for these changes <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 290a1696dc6b16c421a40dc08abdc072a2fb4cf2
servo/python/servo/lints/wpt_lint.py
servo/python/tidy/servo_tidy/tidy.py
servo/python/tidy/servo_tidy_tests/lints/invalid_error_tuple.py
servo/python/tidy/servo_tidy_tests/lints/no_lint.py
servo/python/tidy/servo_tidy_tests/lints/no_run.py
servo/python/tidy/servo_tidy_tests/lints/not_inherited.py
servo/python/tidy/servo_tidy_tests/lints/not_script
servo/python/tidy/servo_tidy_tests/lints/proper_file.py
servo/python/tidy/servo_tidy_tests/test_tidy.py
servo/servo-tidy.toml
new file mode 100644
--- /dev/null
+++ b/servo/python/servo/lints/wpt_lint.py
@@ -0,0 +1,37 @@
+# Copyright 2013 The Servo Project Developers. See the COPYRIGHT
+# file at the top-level directory of this distribution.
+#
+# Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+# http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+# <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+# option. This file may not be copied, modified, or distributed
+# except according to those terms.
+
+import os
+import site
+
+from servo_tidy.tidy import LintRunner, filter_file
+
+WPT_PATH = os.path.join(".", "tests", "wpt")
+SUITES = ["web-platform-tests", os.path.join("mozilla", "tests")]
+
+
+class Lint(LintRunner):
+    def _get_wpt_files(self, suite):
+        working_dir = os.path.join(WPT_PATH, suite, '')
+        file_iter = self.get_files(working_dir, exclude_dirs=[])
+        print '\nRunning the WPT lint on %s...' % working_dir
+        for f in file_iter:
+            if filter_file(f):
+                yield f[len(working_dir):]
+
+    def run(self):
+        wpt_working_dir = os.path.abspath(os.path.join(WPT_PATH, "web-platform-tests"))
+        for suite in SUITES:
+            files = self._get_wpt_files(suite)
+            site.addsitedir(wpt_working_dir)
+            from tools.lint import lint
+            file_dir = os.path.abspath(os.path.join(WPT_PATH, suite))
+            returncode = lint.lint(file_dir, files, output_json=False)
+            if returncode:
+                yield ("WPT Lint Tool", "", "lint error(s) in Web Platform Tests: exit status %s" % returncode)
--- a/servo/python/tidy/servo_tidy/tidy.py
+++ b/servo/python/tidy/servo_tidy/tidy.py
@@ -4,35 +4,37 @@
 # Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
 # http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
 # <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
 # option. This file may not be copied, modified, or distributed
 # except according to those terms.
 
 import contextlib
 import fnmatch
+import imp
 import itertools
 import json
 import os
 import re
-import site
 import StringIO
 import subprocess
 import sys
-from licenseck import MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
 import colorama
 import toml
 
+from licenseck import MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
+
 CONFIG_FILE_PATH = os.path.join(".", "servo-tidy.toml")
 
 # Default configs
 config = {
     "skip-check-length": False,
     "skip-check-licenses": False,
     "check-ordered-json-keys": [],
+    "lint-scripts": [],
     "ignore": {
         "files": [
             "./.",   # ignore hidden files
         ],
         "directories": [
             "./.",   # ignore hidden directories
         ],
         "packages": [],
@@ -81,44 +83,83 @@ WEBIDL_STANDARDS = [
 def is_iter_empty(iterator):
     try:
         obj = iterator.next()
         return True, itertools.chain((obj,), iterator)
     except StopIteration:
         return False, iterator
 
 
-# A simple wrapper for iterators to show progress (note that it's inefficient for giant iterators)
+# A simple wrapper for iterators to show progress
+# (Note that it's inefficient for giant iterators, since it iterates once to get the upper bound)
 def progress_wrapper(iterator):
     list_of_stuff = list(iterator)
     total_files, progress = len(list_of_stuff), 0
     for idx, thing in enumerate(list_of_stuff):
         progress = int(float(idx + 1) / total_files * 100)
         sys.stdout.write('\r  Progress: %s%% (%d/%d)' % (progress, idx + 1, total_files))
         sys.stdout.flush()
         yield thing
 
 
+class FileList(object):
+    def __init__(self, directory, only_changed_files=False, exclude_dirs=[], progress=True):
+        self.directory = directory
+        self.excluded = exclude_dirs
+        iterator = self._git_changed_files() if only_changed_files else \
+            self._filter_excluded() if exclude_dirs else self._default_walk()
+        # Raise `StopIteration` if the iterator is empty
+        obj = next(iterator)
+        self.generator = itertools.chain((obj,), iterator)
+        if progress:
+            self.generator = progress_wrapper(self.generator)
+
+    def _default_walk(self):
+        for root, _, files in os.walk(self.directory):
+            for f in files:
+                yield os.path.join(root, f)
+
+    def _git_changed_files(self):
+        args = ["git", "log", "-n1", "--merges", "--format=%H"]
+        last_merge = subprocess.check_output(args).strip()
+        args = ["git", "diff", "--name-only", last_merge, self.directory]
+        file_list = subprocess.check_output(args)
+
+        for f in file_list.splitlines():
+            if sys.platform == 'win32':
+                os.path.join(*f.split('/'))
+            if not any(os.path.join('.', os.path.dirname(f)).startswith(path) for path in self.excluded):
+                yield os.path.join('.', f)
+
+    def _filter_excluded(self):
+        for root, dirs, files in os.walk(self.directory, topdown=True):
+            # modify 'dirs' in-place so that we don't do unnecessary traversals in excluded directories
+            dirs[:] = [d for d in dirs if not any(os.path.join(root, d).startswith(name) for name in self.excluded)]
+            for rel_path in files:
+                yield os.path.join(root, rel_path)
+
+    def __iter__(self):
+        return self
+
+    def next(self):
+        return next(self.generator)
+
+
 def filter_file(file_name):
     if any(file_name.startswith(ignored_file) for ignored_file in config["ignore"]["files"]):
         return False
     base_name = os.path.basename(file_name)
     if any(fnmatch.fnmatch(base_name, pattern) for pattern in FILE_PATTERNS_TO_IGNORE):
         return False
     return True
 
 
 def filter_files(start_dir, only_changed_files, progress):
-    file_iter = get_file_list(start_dir, only_changed_files, config["ignore"]["directories"])
-    (has_element, file_iter) = is_iter_empty(file_iter)
-    if not has_element:
-        raise StopIteration
-    if progress:
-        file_iter = progress_wrapper(file_iter)
-
+    file_iter = FileList(start_dir, only_changed_files=only_changed_files,
+                         exclude_dirs=config["ignore"]["directories"], progress=progress)
     for file_name in file_iter:
         base_name = os.path.basename(file_name)
         if not any(fnmatch.fnmatch(base_name, pattern) for pattern in FILE_PATTERNS_TO_CHECK):
             continue
         if not filter_file(file_name):
             continue
         yield file_name
 
@@ -824,41 +865,16 @@ def collect_errors_for_files(files_to_ch
                     # the result will be: `(filename, line, message)`
                     yield (filename,) + error
             lines = contents.splitlines(True)
             for check in line_checking_functions:
                 for error in check(filename, lines):
                     yield (filename,) + error
 
 
-def get_wpt_files(suite, only_changed_files, progress):
-    wpt_dir = os.path.join(".", "tests", "wpt", suite, "")
-    file_iter = get_file_list(os.path.join(wpt_dir), only_changed_files)
-    (has_element, file_iter) = is_iter_empty(file_iter)
-    if not has_element:
-        raise StopIteration
-    print '\nRunning the WPT lint...'
-    if progress:
-        file_iter = progress_wrapper(file_iter)
-    for f in file_iter:
-        if filter_file(f):
-            yield f[len(wpt_dir):]
-
-
-def check_wpt_lint_errors(suite, files):
-    wpt_working_dir = os.path.abspath(os.path.join(".", "tests", "wpt", "web-platform-tests"))
-    if os.path.isdir(wpt_working_dir):
-        site.addsitedir(wpt_working_dir)
-        from tools.lint import lint
-        file_dir = os.path.abspath(os.path.join(".", "tests", "wpt", suite))
-        returncode = lint.lint(file_dir, files, output_json=False)
-        if returncode:
-            yield ("WPT Lint Tool", "", "lint error(s) in Web Platform Tests: exit status {0}".format(returncode))
-
-
 def get_dep_toml_files(only_changed_files=False):
     if not only_changed_files:
         print '\nRunning the dependency licensing lint...'
         for root, directories, filenames in os.walk(".cargo"):
             for filename in filenames:
                 if filename == "Cargo.toml":
                     yield os.path.join(root, filename)
 
@@ -871,59 +887,81 @@ def check_dep_license_errors(filenames, 
             lines = f.readlines()
             for idx, line in enumerate(lines):
                 for license_line in licenses_dep_toml:
                     ok_licensed |= (license_line in line)
             if not ok_licensed:
                 yield (filename, 0, "dependency should contain a valid license.")
 
 
-def get_file_list(directory, only_changed_files=False, exclude_dirs=[]):
-    if only_changed_files:
-        # only check tracked files that have been changed since the last merge
-        args = ["git", "log", "-n1", "--author=bors-servo", "--format=%H"]
-        last_merge = subprocess.check_output(args).strip()
-        args = ["git", "diff", "--name-only", last_merge, directory]
-        file_list = subprocess.check_output(args)
-        for f in file_list.splitlines():
-            f = os.path.join(*f.split("/")) if sys.platform == "win32" else f
-            if not any(os.path.join('.', os.path.dirname(f)).startswith(path) for path in exclude_dirs):
-                yield os.path.join('.', f)
-    elif exclude_dirs:
-        for root, dirs, files in os.walk(directory, topdown=True):
-            # modify 'dirs' in-place so that we don't do unwanted traversals in excluded directories
-            dirs[:] = [d for d in dirs if not any(os.path.join(root, d).startswith(name) for name in exclude_dirs)]
-            for rel_path in files:
-                yield os.path.join(root, rel_path)
-    else:
-        for root, _, files in os.walk(directory):
-            for f in files:
-                yield os.path.join(root, f)
+class LintRunner(object):
+    def __init__(self, lint_path=None, only_changed_files=True, exclude_dirs=[], progress=True):
+        self.only_changed_files = only_changed_files
+        self.exclude_dirs = exclude_dirs
+        self.progress = progress
+        self.path = lint_path
+
+    def check(self):
+        if not os.path.exists(self.path):
+            yield (self.path, 0, "file does not exist")
+            return
+        if not self.path.endswith('.py'):
+            yield (self.path, 0, "lint should be a python script")
+            return
+        dir_name, filename = os.path.split(self.path)
+        sys.path.append(dir_name)
+        module = imp.load_source(filename[:-3], self.path)
+        if hasattr(module, 'Lint'):
+            if issubclass(module.Lint, LintRunner):
+                lint = module.Lint(self.path, self.only_changed_files, self.exclude_dirs, self.progress)
+                for error in lint.run():
+                    if not hasattr(error, '__iter__'):
+                        yield (self.path, 1, "errors should be a tuple of (path, line, reason)")
+                        return
+                    yield error
+            else:
+                yield (self.path, 1, "class 'Lint' should inherit from 'LintRunner'")
+        else:
+            yield (self.path, 1, "script should contain a class named 'Lint'")
+        sys.path.remove(dir_name)
+
+    def get_files(self, path, **kwargs):
+        args = ['only_changed_files', 'exclude_dirs', 'progress']
+        kwargs = {k: kwargs.get(k, getattr(self, k)) for k in args}
+        return FileList(path, **kwargs)
+
+    def run(self):
+        yield (self.path, 0, "class 'Lint' should implement 'run' method")
+
+
+def run_lint_scripts(only_changed_files=False, progress=True):
+    runner = LintRunner(only_changed_files=only_changed_files, progress=progress)
+    for path in config['lint-scripts']:
+        runner.path = path
+        for error in runner.check():
+            yield error
 
 
 def scan(only_changed_files=False, progress=True):
     # check config file for errors
     config_errors = check_config_file(CONFIG_FILE_PATH)
     # check directories contain expected files
     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)
     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)
     # check dependecy licenses
     dep_license_errors = check_dep_license_errors(get_dep_toml_files(only_changed_files), progress)
-    # wpt lint checks
-    wpt_lint_errors = [
-        check_wpt_lint_errors(suite, get_wpt_files(suite, only_changed_files, progress))
-        for suite in ["web-platform-tests", os.path.join("mozilla", "tests")]
-    ]
+    # other lint checks
+    lint_errors = run_lint_scripts(only_changed_files, progress)
     # chain all the iterators
-    errors = itertools.chain(config_errors, directory_errors, file_errors, dep_license_errors, *wpt_lint_errors)
+    errors = itertools.chain(config_errors, directory_errors, file_errors, dep_license_errors, lint_errors)
 
     error = None
     for error in errors:
         colorama.init()
         print "\r\033[94m{}\033[0m:\033[93m{}\033[0m: \033[91m{}\033[0m".format(*error)
 
     print
     if error is None:
new file mode 100644
--- /dev/null
+++ b/servo/python/tidy/servo_tidy_tests/lints/invalid_error_tuple.py
@@ -0,0 +1,5 @@
+from servo_tidy.tidy import LintRunner
+
+class Lint(LintRunner):
+    def run(self):
+        yield None
new file mode 100644
--- /dev/null
+++ b/servo/python/tidy/servo_tidy_tests/lints/no_lint.py
@@ -0,0 +1,5 @@
+from servo_tidy.tidy import LintRunner
+
+class Linter(LintRunner):
+    def run(self):
+        pass
new file mode 100644
--- /dev/null
+++ b/servo/python/tidy/servo_tidy_tests/lints/no_run.py
@@ -0,0 +1,5 @@
+from servo_tidy.tidy import LintRunner
+
+class Lint(LintRunner):
+    def some_method(self):
+        pass
new file mode 100644
--- /dev/null
+++ b/servo/python/tidy/servo_tidy_tests/lints/not_inherited.py
@@ -0,0 +1,2 @@
+class Lint(object):
+    pass
copy from servo/python/tidy/servo_tidy_tests/dir_check/only_webidl/test.webidl
copy to servo/python/tidy/servo_tidy_tests/lints/not_script
new file mode 100644
--- /dev/null
+++ b/servo/python/tidy/servo_tidy_tests/lints/proper_file.py
@@ -0,0 +1,6 @@
+from servo_tidy.tidy import LintRunner
+
+class Lint(LintRunner):
+    def run(self):
+        for _ in [None]:
+            yield ('path', 0, 'foobar')
--- a/servo/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/servo/python/tidy/servo_tidy_tests/test_tidy.py
@@ -183,22 +183,44 @@ class CheckTidiness(unittest.TestCase):
 \t\033[93mfound dependency on version 0.4.9\033[0m
 \t\033[91mbut highest version is 0.5.1\033[0m
 \t\033[93mtry upgrading with\033[0m \033[96m./mach cargo-update -p test:0.4.9\033[0m
 \tThe following packages depend on version 0.4.9:
 \t\ttest2"""
         self.assertEqual(msg, errors.next()[2])
         self.assertNoMoreErrors(errors)
 
+    def test_lint_runner(self):
+        test_path = base_path + 'lints/'
+        runner = tidy.LintRunner(only_changed_files=False, progress=False)
+        runner.path = test_path + 'some-fictional-file'
+        self.assertEqual([(runner.path, 0, "file does not exist")], list(runner.check()))
+        runner.path = test_path + 'not_script'
+        self.assertEqual([(runner.path, 0, "lint should be a python script")],
+                         list(runner.check()))
+        runner.path = test_path + 'not_inherited.py'
+        self.assertEqual([(runner.path, 1, "class 'Lint' should inherit from 'LintRunner'")],
+                         list(runner.check()))
+        runner.path = test_path + 'no_lint.py'
+        self.assertEqual([(runner.path, 1, "script should contain a class named 'Lint'")],
+                         list(runner.check()))
+        runner.path = test_path + 'no_run.py'
+        self.assertEqual([(runner.path, 0, "class 'Lint' should implement 'run' method")],
+                         list(runner.check()))
+        runner.path = test_path + 'invalid_error_tuple.py'
+        self.assertEqual([(runner.path, 1, "errors should be a tuple of (path, line, reason)")],
+                         list(runner.check()))
+        runner.path = test_path + 'proper_file.py'
+        self.assertEqual([('path', 0, "foobar")], list(runner.check()))
+
     def test_file_list(self):
         base_path='./python/tidy/servo_tidy_tests/test_ignored'
-        file_list = tidy.get_file_list(base_path, only_changed_files=False,
-                                       exclude_dirs=[])
+        file_list = tidy.FileList(base_path, only_changed_files=False, exclude_dirs=[])
         lst = list(file_list)
         self.assertEqual([os.path.join(base_path, 'whee', 'test.rs'), os.path.join(base_path, 'whee', 'foo', 'bar.rs')], lst)
-        file_list = tidy.get_file_list(base_path, only_changed_files=False,
-                                       exclude_dirs=[os.path.join(base_path, 'whee', 'foo')])
+        file_list = tidy.FileList(base_path, only_changed_files=False,
+                                  exclude_dirs=[os.path.join(base_path, 'whee', 'foo')])
         lst = list(file_list)
         self.assertEqual([os.path.join(base_path, 'whee', 'test.rs')], lst)
 
 def do_tests():
     suite = unittest.TestLoader().loadTestsFromTestCase(CheckTidiness)
     return 0 if unittest.TextTestRunner(verbosity=2).run(suite).wasSuccessful() else 1
--- a/servo/servo-tidy.toml
+++ b/servo/servo-tidy.toml
@@ -1,15 +1,18 @@
 [configs]
 skip-check-length = false
 skip-check-licenses = false
 check-ordered-json-keys = [
   "./resources/prefs.json",
   "./resources/package-prefs.json",
 ]
+lint-scripts = [
+  "./python/servo/lints/wpt_lint.py",
+]
 
 [ignore]
 # Ignored packages with duplicated versions
 packages = ["bitflags", "lazy_static", "semver"]
 # Files that are ignored for all tidy and lint checks.
 files = [
   # Generated and upstream code combined with our own. Could use cleanup
   "./components/style/gecko_bindings/bindings.rs",