From eeeeae3bff6a5bd86e9e28188087f058b1eb0a78 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Mon, 17 Sep 2018 16:09:02 -0500 Subject: [PATCH] refactor: use one script to launch all linters (#14622) * refactor: use one script to launch py, js, cc linters * lint changed files as precommit hook * fix py linter in GN build * add overlooked files to linter search path --- .eslintignore | 4 - .vsts/lint.yml | 4 +- package.json | 10 +-- script/cpplint.js | 120 ------------------------------ script/dbus_mock.py | 1 - script/lint.js | 174 ++++++++++++++++++++++++++++++++++++++++++++ script/pylint.py | 23 ------ 7 files changed, 182 insertions(+), 154 deletions(-) delete mode 100644 .eslintignore delete mode 100755 script/cpplint.js create mode 100755 script/lint.js delete mode 100755 script/pylint.py diff --git a/.eslintignore b/.eslintignore deleted file mode 100644 index 9b0763ffc473..000000000000 --- a/.eslintignore +++ /dev/null @@ -1,4 +0,0 @@ -vendor -node_modules -spec/node_modules -spec/static diff --git a/.vsts/lint.yml b/.vsts/lint.yml index 10c854bcac87..0982b8dd09b1 100644 --- a/.vsts/lint.yml +++ b/.vsts/lint.yml @@ -8,10 +8,12 @@ pool: steps: - bash: | - git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git "${AGENT_BUILDDIRECTORY}/depot_tools" + git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git "${AGENT_BUILDDIRECTORY}/third_party/depot_tools" echo "##vso[task.setvariable variable=PATH]$PATH:${AGENT_BUILDDIRECTORY}/depot_tools" name: Setup_depot_tools - bash: | + export PATH="$PATH:${AGENT_BUILDDIRECTORY}/third_party/depot_tools" + echo "##vso[task.setvariable variable=PATH]$PATH" npm install npm run lint diff --git a/package.json b/package.json index 4a166fe9859f..7958d82f4d3b 100644 --- a/package.json +++ b/package.json @@ -43,18 +43,18 @@ "bump-version": "./script/bump-version.py", "check-tls": "python ./script/tls.py", "clang-format": "find atom/ brightray/ chromium_src/ -iname *.h -o -iname *.cc -o -iname *.mm | xargs clang-format -i", - "lint": "npm run lint:js && npm run lint:cpp && npm run lint:clang-format && npm run lint:py && npm run lint:docs", - "lint:js": "eslint . --cache", + "lint": "./script/lint.js && npm run lint:clang-format && npm run lint:docs", + "lint:js": "./script/lint.js --js", "lint:clang-format": "python script/run-clang-format.py -r -c atom/ chromium_src/ brightray/ || (echo \"\\nCode not formatted correctly.\" && exit 1)", - "lint:cpp": "node ./script/cpplint.js", - "lint:py": "python ./script/pylint.py", + "lint:cpp": "./script/lint.js --cc", + "lint:py": "./script/lint.js --py", "lint:docs": "remark docs -qf && npm run lint:js-in-markdown && npm run create-typescript-definitions && npm run lint:docs-relative-links", "lint:docs-relative-links": "python ./script/check-relative-doc-links.py", "lint:js-in-markdown": "standard-markdown docs", "create-api-json": "electron-docs-linter docs --outfile=electron-api.json", "create-typescript-definitions": "npm run create-api-json && electron-typescript-definitions --in=electron-api.json --out=electron.d.ts", "preinstall": "node -e 'process.exit(0)'", - "precommit": "python script/run-clang-format.py -r -c atom/ chromium_src/ brightray/ && node ./script/cpplint.js -c && npm run lint:js && remark docs -qf || (echo \"Code not formatted correctly.\" && exit 1)", + "precommit": "python script/run-clang-format.py -r -c atom/ chromium_src/ brightray/ && ./script/lint.js -c && remark docs -qf || (echo \"Code not formatted correctly.\" && exit 1)", "prepack": "check-for-leaks", "prepush": "check-for-leaks", "repl": "node ./script/start.js --interactive", diff --git a/script/cpplint.js b/script/cpplint.js deleted file mode 100755 index 2baef34406e4..000000000000 --- a/script/cpplint.js +++ /dev/null @@ -1,120 +0,0 @@ -#!/usr/bin/env node - -const { GitProcess } = require('dugite') -const childProcess = require('child_process') -const klaw = require('klaw') -const minimist = require('minimist') -const path = require('path') - -const SOURCE_ROOT = path.normalize(path.dirname(__dirname)) - -function callCpplint (filenames, args) { - const linter = 'cpplint.py' - if (args.verbose) console.log([linter, ...filenames].join(' ')) - try { - childProcess.execFile(linter, filenames, { cwd: SOURCE_ROOT }, error => { - if (error) { - for (const line of error.message.split(/[\r\n]+/)) { - if (line.length && !line.startsWith('Done processing ')) { - console.warn(line) - } - } - if (error.message.includes('Command failed')) { - process.exit(1) - } - } - }) - } catch (e) { - process.exit(1) - } -} - -function parseCommandLine () { - let help - const opts = minimist(process.argv.slice(2), { - boolean: ['help', 'onlyChanged', 'verbose'], - default: { help: false, onlyChanged: false, verbose: false }, - alias: { c: 'onlyChanged', 'only-changed': 'onlyChanged', h: 'help', v: 'verbose' }, - unknown: arg => { help = true } - }) - if (help || opts.help) { - console.log('Usage: cpplint.js [-h|--help] [-c|--only-changed] [-v|--verbose]') - process.exit(0) - } - return opts -} - -async function findChangedFiles (top) { - const result = await GitProcess.exec(['diff', 'HEAD', '--name-only'], top) - if (result.exitCode !== 0) { - console.log('Failed to find changed files', GitProcess.parseError(result.stderr)) - process.exit(1) - } - const relativePaths = result.stdout.split(/\r\n|\r|\n/g) - const absolutePaths = relativePaths.map(x => path.join(top, x)) - return new Set(absolutePaths) -} - -async function findFiles (top, test) { - return new Promise((resolve, reject) => { - const matches = [] - klaw(top) - .on('end', () => resolve(matches)) - .on('data', item => { - if (test(item.path)) matches.push(item.path) - }) - }) -} - -const blacklist = new Set([ - ['atom', 'browser', 'mac', 'atom_application.h'], - ['atom', 'browser', 'mac', 'atom_application_delegate.h'], - ['atom', 'browser', 'resources', 'win', 'resource.h'], - ['atom', 'browser', 'ui', 'cocoa', 'atom_menu_controller.h'], - ['atom', 'browser', 'ui', 'cocoa', 'atom_ns_window.h'], - ['atom', 'browser', 'ui', 'cocoa', 'atom_ns_window_delegate.h'], - ['atom', 'browser', 'ui', 'cocoa', 'atom_preview_item.h'], - ['atom', 'browser', 'ui', 'cocoa', 'atom_touch_bar.h'], - ['atom', 'browser', 'ui', 'cocoa', 'touch_bar_forward_declarations.h'], - ['atom', 'browser', 'ui', 'cocoa', 'NSColor+Hex.h'], - ['atom', 'browser', 'ui', 'cocoa', 'NSString+ANSI.h'], - ['atom', 'common', 'api', 'api_messages.h'], - ['atom', 'common', 'common_message_generator.cc'], - ['atom', 'common', 'common_message_generator.h'], - ['atom', 'common', 'node_includes.h'], - ['atom', 'node', 'osfhandle.cc'], - ['brightray', 'browser', 'mac', 'bry_inspectable_web_contents_view.h'], - ['brightray', 'browser', 'mac', 'event_dispatching_window.h'], - ['brightray', 'browser', 'mac', 'notification_center_delegate.h'], - ['brightray', 'browser', 'win', 'notification_presenter_win7.h'], - ['brightray', 'browser', 'win', 'win32_desktop_notifications', 'common.h'], - ['brightray', 'browser', 'win', 'win32_desktop_notifications', - 'desktop_notification_controller.cc'], - ['brightray', 'browser', 'win', 'win32_desktop_notifications', - 'desktop_notification_controller.h'], - ['brightray', 'browser', 'win', 'win32_desktop_notifications', 'toast.h'], - ['brightray', 'browser', 'win', 'win32_notification.h'] -].map(tokens => path.join(SOURCE_ROOT, ...tokens))) - -async function main () { - const args = parseCommandLine() - - let filenames = [] - const isCCFile = filename => filename.endsWith('.cc') || filename.endsWith('.h') - const CC_ROOTS = ['atom', 'brightray'].map(x => path.join(SOURCE_ROOT, x)) - for (const root of CC_ROOTS) { - const files = await findFiles(root, isCCFile) - filenames.push(...files) - } - - filenames = filenames.filter(x => !blacklist.has(x)) - - if (args.onlyChanged) { - const whitelist = await findChangedFiles(SOURCE_ROOT) - filenames = filenames.filter(x => whitelist.has(x)) - } - - if (filenames.length) callCpplint(filenames, args) -} - -if (process.mainModule === module) main() diff --git a/script/dbus_mock.py b/script/dbus_mock.py index 77540e402713..5489b903f18d 100644 --- a/script/dbus_mock.py +++ b/script/dbus_mock.py @@ -25,7 +25,6 @@ def start(): if __name__ == '__main__': start() try: - print(sys.argv) subprocess.check_call(sys.argv[1:]) finally: stop() diff --git a/script/lint.js b/script/lint.js new file mode 100755 index 000000000000..bf5446737133 --- /dev/null +++ b/script/lint.js @@ -0,0 +1,174 @@ +#!/usr/bin/env node + +const { GitProcess } = require('dugite') +const childProcess = require('child_process') +const klaw = require('klaw') +const minimist = require('minimist') +const path = require('path') + +const SOURCE_ROOT = path.normalize(path.dirname(__dirname)) + +const BLACKLIST = new Set([ + ['atom', 'browser', 'mac', 'atom_application.h'], + ['atom', 'browser', 'mac', 'atom_application_delegate.h'], + ['atom', 'browser', 'resources', 'win', 'resource.h'], + ['atom', 'browser', 'ui', 'cocoa', 'atom_menu_controller.h'], + ['atom', 'browser', 'ui', 'cocoa', 'atom_ns_window.h'], + ['atom', 'browser', 'ui', 'cocoa', 'atom_ns_window_delegate.h'], + ['atom', 'browser', 'ui', 'cocoa', 'atom_preview_item.h'], + ['atom', 'browser', 'ui', 'cocoa', 'atom_touch_bar.h'], + ['atom', 'browser', 'ui', 'cocoa', 'touch_bar_forward_declarations.h'], + ['atom', 'browser', 'ui', 'cocoa', 'NSColor+Hex.h'], + ['atom', 'browser', 'ui', 'cocoa', 'NSString+ANSI.h'], + ['atom', 'common', 'api', 'api_messages.h'], + ['atom', 'common', 'common_message_generator.cc'], + ['atom', 'common', 'common_message_generator.h'], + ['atom', 'common', 'node_includes.h'], + ['atom', 'node', 'osfhandle.cc'], + ['brightray', 'browser', 'mac', 'bry_inspectable_web_contents_view.h'], + ['brightray', 'browser', 'mac', 'event_dispatching_window.h'], + ['brightray', 'browser', 'mac', 'notification_center_delegate.h'], + ['brightray', 'browser', 'win', 'notification_presenter_win7.h'], + ['brightray', 'browser', 'win', 'win32_desktop_notifications', 'common.h'], + ['brightray', 'browser', 'win', 'win32_desktop_notifications', + 'desktop_notification_controller.cc'], + ['brightray', 'browser', 'win', 'win32_desktop_notifications', + 'desktop_notification_controller.h'], + ['brightray', 'browser', 'win', 'win32_desktop_notifications', 'toast.h'], + ['brightray', 'browser', 'win', 'win32_notification.h'], + ['spec', 'static', 'jquery-2.0.3.min.js'] +].map(tokens => path.join(SOURCE_ROOT, ...tokens))) + +function spawnAndCheckExitCode (cmd, args, opts) { + opts = Object.assign({ stdio: 'inherit' }, opts) + const status = childProcess.spawnSync(cmd, args, opts).status + if (status) process.exit(status) +} + +const LINTERS = [ { + key: 'c++', + roots: ['atom', 'brightray'], + test: filename => filename.endsWith('.cc') || filename.endsWith('.h'), + run: (opts, filenames) => { + const result = childProcess.spawnSync('cpplint.py', filenames, { encoding: 'utf8' }) + // cpplint.py writes EVERYTHING to stderr, including status messages + if (result.stderr) { + for (const line of result.stderr.split(/[\r\n]+/)) { + if (line.length && !line.startsWith('Done processing ') && line !== 'Total errors found: 0') { + console.warn(line) + } + } + } + if (result.status) process.exit(result.status) + } +}, { + key: 'python', + roots: ['script'], + test: filename => filename.endsWith('.py'), + run: (opts, filenames) => { + const rcfile = path.normalize(path.join(SOURCE_ROOT, '..', 'third_party', 'depot_tools', 'pylintrc')) + const args = ['--rcfile=' + rcfile, ...filenames] + const env = Object.assign({ PYTHONPATH: path.join(SOURCE_ROOT, 'script') }, process.env) + spawnAndCheckExitCode('pylint.py', args, { env }) + } +}, { + key: 'javascript', + roots: ['lib', 'spec', 'script', 'default_app'], + test: filename => filename.endsWith('.js'), + run: (opts, filenames) => { + const cmd = path.join(SOURCE_ROOT, 'node_modules', '.bin', 'eslint') + const args = [ '--cache', ...filenames ] + if (opts.fix) args.unshift('--fix') + spawnAndCheckExitCode(cmd, args, { cwd: SOURCE_ROOT }) + } +}] + +function parseCommandLine () { + let help + const opts = minimist(process.argv.slice(2), { + boolean: [ 'c++', 'javascript', 'python', 'help', 'changed', 'fix', 'verbose' ], + alias: { 'c++': ['cc', 'cpp', 'cxx'], javascript: ['js', 'es'], python: 'py', changed: 'c', help: 'h', verbose: 'v' }, + unknown: arg => { help = true } + }) + if (help || opts.help) { + console.log('Usage: script/lint.js [--cc] [--js] [--py] [-c|--changed] [-h|--help] [-v|--verbose] [--fix]') + process.exit(0) + } + return opts +} + +async function findChangedFiles (top) { + const result = await GitProcess.exec(['diff', 'HEAD', '--name-only'], top) + if (result.exitCode !== 0) { + console.log('Failed to find changed files', GitProcess.parseError(result.stderr)) + process.exit(1) + } + const relativePaths = result.stdout.split(/\r\n|\r|\n/g) + const absolutePaths = relativePaths.map(x => path.join(top, x)) + return new Set(absolutePaths) +} + +async function findMatchingFiles (top, test) { + return new Promise((resolve, reject) => { + const matches = [] + klaw(top) + .on('end', () => resolve(matches)) + .on('data', item => { + if (test(item.path)) { + matches.push(item.path) + } + }) + }) +} + +async function findFiles (args, linter) { + let filenames = [] + let whitelist = null + + // build the whitelist + if (args.changed) { + whitelist = await findChangedFiles(SOURCE_ROOT) + if (!whitelist.size) { + return filenames + } + } + + // accumulate the raw list of files + for (const root of linter.roots) { + const files = await findMatchingFiles(path.join(SOURCE_ROOT, root), linter.test) + filenames.push(...files) + } + + // remove blacklisted files + filenames = filenames.filter(x => !BLACKLIST.has(x)) + + // if a whitelist exists, remove anything not in it + if (whitelist) { + filenames = filenames.filter(x => whitelist.has(x)) + } + + return filenames +} + +async function main () { + const opts = parseCommandLine() + + // no mode specified? run 'em all + if (!opts['c++'] && !opts.javascript && !opts.python) { + opts['c++'] = opts.javascript = opts.python = true + } + + const linters = LINTERS.filter(x => opts[x.key]) + + for (const linter of linters) { + const filenames = await findFiles(opts, linter) + if (filenames.length) { + if (opts.verbose) { console.log(`linting ${filenames.length} ${linter.key} ${filenames.length === 1 ? 'file' : 'files'}`) } + linter.run(opts, filenames) + } + } +} + +if (process.mainModule === module) { + main() +} diff --git a/script/pylint.py b/script/pylint.py deleted file mode 100755 index a4cb7fdfcb06..000000000000 --- a/script/pylint.py +++ /dev/null @@ -1,23 +0,0 @@ -#!/usr/bin/env python - -import glob -import os -import subprocess -import sys - -SOURCE_ROOT = os.path.abspath(os.path.dirname(os.path.dirname(__file__))) - - -def main(): - os.chdir(SOURCE_ROOT) - - pylint = os.path.join(SOURCE_ROOT, 'vendor', 'depot_tools', 'pylint.py') - settings = ['--rcfile=vendor/depot_tools/pylintrc'] - pys = glob.glob('script/*.py') - env = os.environ.copy() - env['PYTHONPATH'] = 'script' - subprocess.check_call([sys.executable, pylint] + settings + pys, env=env) - - -if __name__ == '__main__': - sys.exit(main())