From e04895a0986b798a2491331313a1b5275e2f9e82 Mon Sep 17 00:00:00 2001 From: Andrew Halberstadt Date: Thu, 29 Mar 2018 14:50:17 -0400 Subject: [PATCH] Bug 1373368 - [mozlint] Raise if a non-existant path is specified in a lint config, r=standard8 Since I left the next two patches to bitrot, I realized that a path I had added didn't exist anymore. We should definitely error out if non-existant paths are specified, otherwise the lists will become outdated and it will be possible to accidentally disable linting on some files. I discovered a few instances of this already in our existing definitions. MozReview-Commit-ID: 8jsTKLI0nFE --HG-- extra : rebase_source : acceb0b129fc472fb456ff527e4c8c52228edd59 --- python/mozlint/mozlint/errors.py | 4 +- python/mozlint/mozlint/parser.py | 44 ++++++++++++++----- python/mozlint/mozlint/roller.py | 2 +- python/mozlint/test/linters/explicit_path.yml | 2 +- .../test/linters/non_existing_exclude.yml | 7 +++ .../test/linters/non_existing_include.yml | 7 +++ .../linters/non_existing_support_files.yml | 7 +++ python/mozlint/test/test_filterpaths.py | 4 +- python/mozlint/test/test_parser.py | 7 ++- tools/lint/mingw-capitalization.yml | 1 - tools/lint/py2.yml | 2 - tools/lint/py3.yml | 1 - tools/lint/test/conftest.py | 2 +- 13 files changed, 67 insertions(+), 23 deletions(-) create mode 100644 python/mozlint/test/linters/non_existing_exclude.yml create mode 100644 python/mozlint/test/linters/non_existing_include.yml create mode 100644 python/mozlint/test/linters/non_existing_support_files.yml diff --git a/python/mozlint/mozlint/errors.py b/python/mozlint/mozlint/errors.py index 6b3a599bb9d3..4edf1a38eac4 100644 --- a/python/mozlint/mozlint/errors.py +++ b/python/mozlint/mozlint/errors.py @@ -4,8 +4,6 @@ from __future__ import absolute_import -import os - class LintException(Exception): pass @@ -18,7 +16,7 @@ class LinterNotFound(LintException): class LinterParseError(LintException): def __init__(self, path, message): - LintException.__init__(self, "{}: {}".format(os.path.basename(path), message)) + LintException.__init__(self, "{}: {}".format(path, message)) class LintersNotConfigured(LintException): diff --git a/python/mozlint/mozlint/parser.py b/python/mozlint/mozlint/parser.py index 2a4f090e84ed..bd81027abc10 100644 --- a/python/mozlint/mozlint/parser.py +++ b/python/mozlint/mozlint/parser.py @@ -21,33 +21,57 @@ class Parser(object): 'payload', ) + def __init__(self, root): + self.root = root + def __call__(self, path): return self.parse(path) def _validate(self, linter): + relpath = os.path.relpath(linter['path'], self.root) + missing_attrs = [] for attr in self.required_attributes: if attr not in linter: missing_attrs.append(attr) if missing_attrs: - raise LinterParseError(linter['path'], "Missing required attribute(s): " - "{}".format(','.join(missing_attrs))) + raise LinterParseError(relpath, "Missing required attribute(s): " + "{}".format(','.join(missing_attrs))) if linter['type'] not in supported_types: - raise LinterParseError(linter['path'], "Invalid type '{}'".format(linter['type'])) + raise LinterParseError(relpath, "Invalid type '{}'".format(linter['type'])) for attr in ('include', 'exclude'): - if attr in linter and (not isinstance(linter[attr], list) or - not all(isinstance(a, basestring) for a in linter[attr])): - raise LinterParseError(linter['path'], "The {} directive must be a " - "list of strings!".format(attr)) + if attr not in linter: + continue + + if not isinstance(linter[attr], list) or \ + not all(isinstance(a, basestring) for a in linter[attr]): + raise LinterParseError(relpath, "The {} directive must be a " + "list of strings!".format(attr)) + invalid_paths = set() + for path in linter[attr]: + if '*' in path: + continue + + abspath = path + if not os.path.isabs(abspath): + abspath = os.path.join(self.root, path) + + if not os.path.exists(abspath): + invalid_paths.add(' ' + path) + + if invalid_paths: + raise LinterParseError(relpath, "The {} directive contains the following " + "paths that don't exist:\n{}".format( + attr, '\n'.join(sorted(invalid_paths)))) if 'setup' in linter: if linter['setup'].count(':') != 1: - raise LinterParseError(linter['path'], "The setup attribute '{!r}' must have the " - "form 'module:object'".format( - linter['setup'])) + raise LinterParseError(relpath, "The setup attribute '{!r}' must have the " + "form 'module:object'".format( + linter['setup'])) if 'extensions' in linter: linter['extensions'] = [e.strip('.') for e in linter['extensions']] diff --git a/python/mozlint/mozlint/roller.py b/python/mozlint/mozlint/roller.py index 402cae260743..3b6c382ecf66 100644 --- a/python/mozlint/mozlint/roller.py +++ b/python/mozlint/mozlint/roller.py @@ -90,7 +90,7 @@ class LintRoller(object): MAX_PATHS_PER_JOB = 50 # set a max size to prevent command lines that are too long on Windows def __init__(self, root, **lintargs): - self.parse = Parser() + self.parse = Parser(root) try: self.vcs = get_repository_object(root) except InvalidRepoPath: diff --git a/python/mozlint/test/linters/explicit_path.yml b/python/mozlint/test/linters/explicit_path.yml index 80630518993a..1e7e8f4bf1d4 100644 --- a/python/mozlint/test/linters/explicit_path.yml +++ b/python/mozlint/test/linters/explicit_path.yml @@ -3,6 +3,6 @@ ExplicitPathLinter: description: Only lint a specific file name rule: no-foobar include: - - no_foobar.js + - files/no_foobar.js type: string payload: foobar diff --git a/python/mozlint/test/linters/non_existing_exclude.yml b/python/mozlint/test/linters/non_existing_exclude.yml new file mode 100644 index 000000000000..819012302753 --- /dev/null +++ b/python/mozlint/test/linters/non_existing_exclude.yml @@ -0,0 +1,7 @@ +--- +BadExcludeLinter: + description: Has an invalid exclude directive. + exclude: + - files/does_not_exist + type: string + payload: foobar diff --git a/python/mozlint/test/linters/non_existing_include.yml b/python/mozlint/test/linters/non_existing_include.yml new file mode 100644 index 000000000000..5443d751edbe --- /dev/null +++ b/python/mozlint/test/linters/non_existing_include.yml @@ -0,0 +1,7 @@ +--- +BadIncludeLinter: + description: Has an invalid include directive. + include: + - files/does_not_exist + type: string + payload: foobar diff --git a/python/mozlint/test/linters/non_existing_support_files.yml b/python/mozlint/test/linters/non_existing_support_files.yml new file mode 100644 index 000000000000..e636fadf93be --- /dev/null +++ b/python/mozlint/test/linters/non_existing_support_files.yml @@ -0,0 +1,7 @@ +--- +BadSupportFilesLinter: + description: Has an invalid support-files directive. + support-files: + - files/does_not_exist + type: string + payload: foobar diff --git a/python/mozlint/test/test_filterpaths.py b/python/mozlint/test/test_filterpaths.py index 839186f9df22..c651586dd8a6 100644 --- a/python/mozlint/test/test_filterpaths.py +++ b/python/mozlint/test/test_filterpaths.py @@ -5,8 +5,8 @@ from __future__ import absolute_import import os -import sys +import mozunit import pytest from mozlint import pathutils @@ -90,4 +90,4 @@ def test_include_exclude(filterpaths): if __name__ == '__main__': - sys.exit(pytest.main(['--verbose', __file__])) + mozunit.main() diff --git a/python/mozlint/test/test_parser.py b/python/mozlint/test/test_parser.py index dae0673d24e9..bde5fcf08e42 100644 --- a/python/mozlint/test/test_parser.py +++ b/python/mozlint/test/test_parser.py @@ -15,10 +15,12 @@ from mozlint.errors import ( LinterParseError, ) +here = os.path.abspath(os.path.dirname(__file__)) + @pytest.fixture(scope='module') def parse(lintdir): - parser = Parser() + parser = Parser(here) def _parse(name): path = os.path.join(lintdir, name) @@ -48,6 +50,9 @@ def test_parse_valid_linter(parse): 'invalid_exclude.yml', 'missing_attrs.yml', 'missing_definition.yml', + 'non_existing_include.yml', + 'non_existing_exclude.yml', + 'non_existing_support_files.yml', ]) def test_parse_invalid_linter(parse, linter): with pytest.raises(LinterParseError): diff --git a/tools/lint/mingw-capitalization.yml b/tools/lint/mingw-capitalization.yml index 42fd279d0e0c..b6cb4fd69a72 100644 --- a/tools/lint/mingw-capitalization.yml +++ b/tools/lint/mingw-capitalization.yml @@ -11,7 +11,6 @@ mingw-capitalization: - gfx/angle/checkout/src/common/tls.cpp - gfx/cairo/cairo/src/cairo-atomic-private.h - gfx/harfbuzz/src/hb-directwrite.cc - - gfx/skia/skia/src/views/win/SkOSWindow_win.cpp - gfx/skia/skia/src/xps/SkXPSDevice.cpp - gfx/skia/skia/src/xps/SkXPSDevice.h - gfx/skia/skia/src/xps/SkXPSDocument.h diff --git a/tools/lint/py2.yml b/tools/lint/py2.yml index c6e5164df420..d3b1320e6ac5 100644 --- a/tools/lint/py2.yml +++ b/tools/lint/py2.yml @@ -26,13 +26,11 @@ py2: - netwerk - nsprpub - other-licenses - - probes/trace-gen.py - python/devtools - python/mach - python/mozbuild - python/mozversioncontrol - security - - services/common/tests/mach_commands.py - servo - taskcluster/docker/funsize-update-generator - testing/awsy diff --git a/tools/lint/py3.yml b/tools/lint/py3.yml index 7e1f72e0bd0d..83c59d3c70de 100644 --- a/tools/lint/py3.yml +++ b/tools/lint/py3.yml @@ -26,7 +26,6 @@ py3: - nsprpub - security/manager/ssl - security/nss - - services/common/tests/mach_commands.py - servo - testing/awsy - testing/firefox-ui/harness/firefox_ui_harness/runners/update.py diff --git a/tools/lint/test/conftest.py b/tools/lint/test/conftest.py index f31e1a6d5863..5851532cebd2 100644 --- a/tools/lint/test/conftest.py +++ b/tools/lint/test/conftest.py @@ -55,7 +55,7 @@ def config(request): name = request.module.LINTER config_path = os.path.join(lintdir, '{}.yml'.format(name)) - parser = Parser() + parser = Parser(build.topsrcdir) # TODO Don't assume one linter per yaml file return parser.parse(config_path)[0]