From 280bbfc763cf1154e7fef04eda1565122254bcdc Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Fri, 31 Jul 2020 03:23:30 +0000 Subject: [PATCH] This makes the lint script use multiprocessing to speed it up. (#19987) I got tired of waiting for it to run, so I added some of the "worker" queue code that I wrote for the assets-for-api-docs generator. I also tried out putting all the files in one call to clang-tidy with the -p argument, but that was still a lot slower because it runs them all on one core. This runs separate jobs for each file, simultaneously, and then reports the results at the end (associated with each file, of course). --- ci/bin/lint.dart | 256 +++++++++++++++++++++++++++++++++++++++++++++++ ci/lint.dart | 168 ------------------------------- ci/lint.sh | 46 ++++++++- ci/pubspec.yaml | 9 ++ 4 files changed, 307 insertions(+), 172 deletions(-) create mode 100644 ci/bin/lint.dart delete mode 100644 ci/lint.dart create mode 100644 ci/pubspec.yaml diff --git a/ci/bin/lint.dart b/ci/bin/lint.dart new file mode 100644 index 00000000000..658388f4e97 --- /dev/null +++ b/ci/bin/lint.dart @@ -0,0 +1,256 @@ +/// Runs clang-tidy on files with changes. +/// +/// usage: +/// dart lint.dart [clang-tidy checks] +/// +/// User environment variable FLUTTER_LINT_ALL to run on all files. + +import 'dart:async' show Completer; +import 'dart:convert' show jsonDecode, utf8, LineSplitter; +import 'dart:io' show File, exit, Directory, FileSystemEntity, Platform, stderr; + +import 'package:args/args.dart'; +import 'package:path/path.dart' as path; +import 'package:process_runner/process_runner.dart'; + +String _linterOutputHeader = ''' +┌──────────────────────────┐ +│ Engine Clang Tidy Linter │ +└──────────────────────────┘ +The following errors have been reported by the Engine Clang Tidy Linter. For +more information on addressing these issues please see: +https://github.com/flutter/flutter/wiki/Engine-Clang-Tidy-Linter +'''; + +class Command { + Directory directory = Directory(''); + String command = ''; + File file = File(''); +} + +Command parseCommand(Map map) { + final Directory dir = Directory(map['directory'] as String).absolute; + return Command() + ..directory = dir + ..command = map['command'] as String + ..file = File(path.normalize(path.join(dir.path, map['file'] as String))); +} + +String calcTidyArgs(Command command) { + String result = command.command; + result = result.replaceAll(RegExp(r'\S*clang/bin/clang'), ''); + result = result.replaceAll(RegExp(r'-MF \S*'), ''); + return result; +} + +String calcTidyPath(Command command) { + final RegExp regex = RegExp(r'\S*clang/bin/clang'); + return regex + .stringMatch(command.command) + ?.replaceAll('clang/bin/clang', 'clang/bin/clang-tidy') ?? + ''; +} + +bool isNonEmptyString(String str) => str.isNotEmpty; + +bool containsAny(File file, Iterable queries) { + return queries.where((File query) => path.equals(query.path, file.path)).isNotEmpty; +} + +/// Returns a list of all non-deleted files which differ from the nearest +/// merge-base with `master`. If it can't find a fork point, uses the default +/// merge-base. +Future> getListOfChangedFiles(Directory repoPath) async { + final ProcessRunner processRunner = ProcessRunner(defaultWorkingDirectory: repoPath); + final ProcessRunnerResult fetchResult = await processRunner.runProcess( + ['git', 'fetch', 'upstream', 'master'], + failOk: true, + ); + if (fetchResult.exitCode != 0) { + await processRunner.runProcess(['git', 'fetch', 'origin', 'master']); + } + final Set result = {}; + ProcessRunnerResult mergeBaseResult = await processRunner.runProcess( + ['git', 'merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'], + failOk: true); + if (mergeBaseResult.exitCode != 0) { + if (verbose) { + stderr.writeln("Didn't find a fork point, falling back to default merge base."); + } + mergeBaseResult = await processRunner + .runProcess(['git', 'merge-base', 'FETCH_HEAD', 'HEAD'], failOk: false); + } + final String mergeBase = mergeBaseResult.stdout.trim(); + final ProcessRunnerResult masterResult = await processRunner + .runProcess(['git', 'diff', '--name-only', '--diff-filter=ACMRT', mergeBase]); + result.addAll(masterResult.stdout.split('\n').where(isNonEmptyString)); + return result.map((String filePath) => File(path.join(repoPath.path, filePath))).toList(); +} + +Future> dirContents(Directory dir) { + final List files = []; + final Completer> completer = Completer>(); + final Stream lister = dir.list(recursive: true); + lister.listen((FileSystemEntity file) => file is File ? files.add(file) : null, + onError: (Object e) => completer.completeError(e), onDone: () => completer.complete(files)); + return completer.future; +} + +File buildFileAsRepoFile(String buildFile, Directory repoPath) { + // Removes the "../../flutter" from the build files to make it relative to the flutter + // dir. + final String relativeBuildFile = path.joinAll(path.split(buildFile).sublist(3)); + final File result = File(path.join(repoPath.absolute.path, relativeBuildFile)); + print('Build file: $buildFile => ${result.path}'); + return result; +} + +Future shouldIgnoreFile(File file) async { + if (path.split(file.path).contains('third_party')) { + return 'third_party'; + } else { + final RegExp exp = RegExp(r'//\s*FLUTTER_NOLINT'); + await for (String line + in file.openRead().transform(utf8.decoder).transform(const LineSplitter())) { + if (exp.hasMatch(line)) { + return 'FLUTTER_NOLINT'; + } else if (line.isNotEmpty && line[0] != '\n' && line[0] != '/') { + // Quick out once we find a line that isn't empty or a comment. The + // FLUTTER_NOLINT must show up before the first real code. + return ''; + } + } + return ''; + } +} + +void _usage(ArgParser parser, {int exitCode = 1}) { + stderr.writeln('lint.dart [--help] [--lint-all] [--verbose] [--diff-branch]'); + stderr.writeln(parser.usage); + exit(exitCode); +} + +bool verbose = false; + +void main(List arguments) async { + final ArgParser parser = ArgParser(); + parser.addFlag('help', help: 'Print help.'); + parser.addFlag('lint-all', + help: 'lint all of the sources, regardless of FLUTTER_NOLINT.', defaultsTo: false); + parser.addFlag('verbose', help: 'Print verbose output.', defaultsTo: verbose); + parser.addOption('repo', help: 'Use the given path as the repo path'); + parser.addOption('compile-commands', + help: 'Use the given path as the source of compile_commands.json. This ' + 'file is created by running tools/gn'); + parser.addOption('checks', + help: 'Perform the given checks on the code. Defaults to the empty ' + 'string, indicating all checks should be performed.', + defaultsTo: ''); + final ArgResults options = parser.parse(arguments); + + verbose = options['verbose'] as bool; + + if (options['help'] as bool) { + _usage(parser, exitCode: 0); + } + + if (!options.wasParsed('compile-commands')) { + stderr.writeln('ERROR: The --compile-commands argument is requried.'); + _usage(parser); + } + + if (!options.wasParsed('repo')) { + stderr.writeln('ERROR: The --repo argument is requried.'); + _usage(parser); + } + + final File buildCommandsPath = File(options['compile-commands'] as String); + if (!buildCommandsPath.existsSync()) { + stderr.writeln("ERROR: Build commands path ${buildCommandsPath.absolute.path} doesn't exist."); + _usage(parser); + } + + final Directory repoPath = Directory(options['repo'] as String); + if (!repoPath.existsSync()) { + stderr.writeln("ERROR: Repo path ${repoPath.absolute.path} doesn't exist."); + _usage(parser); + } + + print(_linterOutputHeader); + + final String checksArg = options.wasParsed('checks') ? options['checks'] as String : ''; + final String checks = checksArg.isNotEmpty ? '--checks=$checksArg' : '--config='; + final bool lintAll = + Platform.environment['FLUTTER_LINT_ALL'] != null || options['lint-all'] as bool; + final List changedFiles = + lintAll ? await dirContents(repoPath) : await getListOfChangedFiles(repoPath); + + if (verbose) { + print('Checking lint in repo at $repoPath.'); + if (checksArg.isNotEmpty) { + print('Checking for specific checks: $checks.'); + } + if (lintAll) { + print('Checking all ${changedFiles.length} files the repo dir.'); + } else { + print('Dectected ${changedFiles.length} files that have changed'); + } + } + + final List buildCommandMaps = + jsonDecode(await buildCommandsPath.readAsString()) as List; + final List buildCommands = buildCommandMaps + .map((dynamic x) => parseCommand(x as Map)) + .toList(); + final Command firstCommand = buildCommands[0]; + final String tidyPath = calcTidyPath(firstCommand); + assert(tidyPath.isNotEmpty); + final List changedFileBuildCommands = + buildCommands.where((Command x) => containsAny(x.file, changedFiles)).toList(); + + if (changedFileBuildCommands.isEmpty) { + print('No changed files that have build commands associated with them ' + 'were found.'); + exit(0); + } + + if (verbose) { + print('Found ${changedFileBuildCommands.length} files that have build ' + 'commands associated with them and can be lint checked.'); + } + + int exitCode = 0; + final List jobs = []; + for (Command command in changedFileBuildCommands) { + final String relativePath = path.relative(command.file.path, from: repoPath.parent.path); + final String ignoreReason = await shouldIgnoreFile(command.file); + if (ignoreReason.isEmpty) { + final String tidyArgs = calcTidyArgs(command); + final List args = [command.file.path, checks, '--']; + args.addAll(tidyArgs?.split(' ') ?? []); + print('🔶 linting $relativePath'); + jobs.add(WorkerJob( + [tidyPath, ...args], + workingDirectory: command.directory, + name: 'clang-tidy on ${command.file.path}', + )); + } else { + print('🔷 ignoring $relativePath ($ignoreReason)'); + } + } + final ProcessPool pool = ProcessPool(); + + await for (final WorkerJob job in pool.startWorkers(jobs)) { + if (job.result.stdout.isEmpty) { + continue; + } + print('❌ Failures for ${job.name}:'); + print(job.result.stdout); + exitCode = 1; + } + print('\n'); + if (exitCode == 0) { + print('No lint problems found.'); + } + exit(exitCode); +} diff --git a/ci/lint.dart b/ci/lint.dart deleted file mode 100644 index 933da7d8a3c..00000000000 --- a/ci/lint.dart +++ /dev/null @@ -1,168 +0,0 @@ -/// Runs clang-tidy on files with changes. -/// -/// usage: -/// dart lint.dart [clang-tidy checks] -/// -/// User environment variable FLUTTER_LINT_ALL to run on all files. - -import 'dart:io' - show - File, - Process, - ProcessResult, - exit, - Directory, - FileSystemEntity, - Platform; -import 'dart:convert' show jsonDecode, utf8, LineSplitter; -import 'dart:async' show Completer; - -String _linterOutputHeader = '''┌──────────────────────────┐ -│ Engine Clang Tidy Linter │ -└──────────────────────────┘ -The following errors have been reported by the Engine Clang Tidy Linter. For -more information on addressing these issues please see: -https://github.com/flutter/flutter/wiki/Engine-Clang-Tidy-Linter -'''; - -class Command { - String directory; - String command; - String file; -} - -Command parseCommand(Map map) { - return Command() - ..directory = map['directory'] - ..command = map['command'] - ..file = map['file']; -} - -String calcTidyArgs(Command command) { - String result = command.command; - result = result.replaceAll(RegExp(r'\S*clang/bin/clang'), ''); - result = result.replaceAll(RegExp(r'-MF \S*'), ''); - return result; -} - -String calcTidyPath(Command command) { - final RegExp regex = RegExp(r'\S*clang/bin/clang'); - return regex - .stringMatch(command.command) - .replaceAll('clang/bin/clang', 'clang/bin/clang-tidy'); -} - -bool isNonEmptyString(String str) => str.length > 0; - -bool containsAny(String str, List queries) { - for (String query in queries) { - if (str.contains(query)) { - return true; - } - } - return false; -} - -/// Returns a list of all files with current changes or differ from `master`. -List getListOfChangedFiles(String repoPath) { - final Set result = Set(); - final ProcessResult diffResult = Process.runSync( - 'git', ['diff', '--name-only'], - workingDirectory: repoPath); - final ProcessResult diffCachedResult = Process.runSync( - 'git', ['diff', '--cached', '--name-only'], - workingDirectory: repoPath); - - final ProcessResult fetchResult = - Process.runSync('git', ['fetch', 'upstream', 'master']); - if (fetchResult.exitCode != 0) { - Process.runSync('git', ['fetch', 'origin', 'master']); - } - final ProcessResult mergeBaseResult = Process.runSync( - 'git', ['merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'], - workingDirectory: repoPath); - final String mergeBase = mergeBaseResult.stdout.trim(); - final ProcessResult masterResult = Process.runSync( - 'git', ['diff', '--name-only', mergeBase], - workingDirectory: repoPath); - result.addAll(diffResult.stdout.split('\n').where(isNonEmptyString)); - result.addAll(diffCachedResult.stdout.split('\n').where(isNonEmptyString)); - result.addAll(masterResult.stdout.split('\n').where(isNonEmptyString)); - return result.toList(); -} - -Future> dirContents(String repoPath) { - Directory dir = Directory(repoPath); - var files = []; - var completer = new Completer>(); - var lister = dir.list(recursive: true); - lister.listen((FileSystemEntity file) => files.add(file.path), - // should also register onError - onDone: () => completer.complete(files)); - return completer.future; -} - -Future shouldIgnoreFile(String path) async { - if (path.contains('/third_party/')) { - return true; - } else { - final RegExp exp = RegExp(r'//.*FLUTTER_NOLINT'); - await for (String line in File(path.substring(6)) - .openRead() - .transform(utf8.decoder) - .transform(const LineSplitter())) { - if (exp.hasMatch(line)) { - return true; - } else if (line.length > 0 && line[0] != '\n' && line[0] != '/') { - // Quick out once we find a line that isn't empty or a comment. The - // FLUTTER_NOLINT must show up before the first real code. - return false; - } - } - return false; - } -} - -void main(List arguments) async { - final String buildCommandsPath = arguments[0]; - final String repoPath = arguments[1]; - final String checks = - arguments.length >= 3 ? '--checks=${arguments[2]}' : '--config='; - final List changedFiles = - Platform.environment['FLUTTER_LINT_ALL'] != null - ? await dirContents(repoPath) - : getListOfChangedFiles(repoPath); - /// TODO(gaaclarke): Convert FLUTTER_LINT_ALL to a command-line flag and add - /// `--verbose` flag. - - final List buildCommandMaps = - jsonDecode(await new File(buildCommandsPath).readAsString()); - final List buildCommands = - buildCommandMaps.map((x) => parseCommand(x)).toList(); - final Command firstCommand = buildCommands[0]; - final String tidyPath = calcTidyPath(firstCommand); - final List changedFileBuildCommands = - buildCommands.where((x) => containsAny(x.file, changedFiles)).toList(); - - print(_linterOutputHeader); - int exitCode = 0; - //TODO(aaclarke): Coalesce this into one call using the `-p` arguement. - for (Command command in changedFileBuildCommands) { - if (!(await shouldIgnoreFile(command.file))) { - final String tidyArgs = calcTidyArgs(command); - final List args = [command.file, checks, '--']; - args.addAll(tidyArgs.split(' ')); - print('🔶 linting ${command.file}'); - final Process process = await Process.start(tidyPath, args, - workingDirectory: command.directory, runInShell: false); - process.stdout.transform(utf8.decoder).listen((data) { - print(data); - exitCode = 1; - }); - await process.exitCode; - } else { - print('🔷 ignoring ${command.file}'); - } - } - exit(exitCode); -} diff --git a/ci/lint.sh b/ci/lint.sh index 023d98292ae..c3292d8e369 100755 --- a/ci/lint.sh +++ b/ci/lint.sh @@ -2,9 +2,47 @@ set -e -COMPILE_COMMANDS="out/compile_commands.json" -if [ ! -f $COMPILE_COMMANDS ]; then - ./flutter/tools/gn +# Needed because if it is set, cd may print the path it changed to. +unset CDPATH + +# On Mac OS, readlink -f doesn't work, so follow_links traverses the path one +# link at a time, and then cds into the link destination and find out where it +# ends up. +# +# The returned filesystem path must be a format usable by Dart's URI parser, +# since the Dart command line tool treats its argument as a file URI, not a +# filename. For instance, multiple consecutive slashes should be reduced to a +# single slash, since double-slashes indicate a URI "authority", and these are +# supposed to be filenames. There is an edge case where this will return +# multiple slashes: when the input resolves to the root directory. However, if +# that were the case, we wouldn't be running this shell, so we don't do anything +# about it. +# +# The function is enclosed in a subshell to avoid changing the working directory +# of the caller. +function follow_links() ( + cd -P "$(dirname -- "$1")" + file="$PWD/$(basename -- "$1")" + while [[ -h "$file" ]]; do + cd -P "$(dirname -- "$file")" + file="$(readlink -- "$file")" + cd -P "$(dirname -- "$file")" + file="$PWD/$(basename -- "$file")" + done + echo "$file" +) +PROG_NAME="$(follow_links "${BASH_SOURCE[0]}")" +CI_DIR="$(cd "${PROG_NAME%/*}" ; pwd -P)" +SRC_DIR="$(cd "$CI_DIR/../.."; pwd -P)" + +COMPILE_COMMANDS="$SRC_DIR/out/compile_commands.json" +if [ ! -f "$COMPILE_COMMANDS" ]; then + (cd $SRC_DIR; ./flutter/tools/gn) fi -dart flutter/ci/lint.dart $COMPILE_COMMANDS flutter/ +cd "$CI_DIR" +pub get && dart \ + bin/lint.dart \ + --compile-commands="$COMPILE_COMMANDS" \ + --repo="$SRC_DIR/flutter" \ + "$@" diff --git a/ci/pubspec.yaml b/ci/pubspec.yaml new file mode 100644 index 00000000000..d345cab6e78 --- /dev/null +++ b/ci/pubspec.yaml @@ -0,0 +1,9 @@ +name: ci_scripts + +dependencies: + args: ^1.6.0 + path: ^1.7.0 + process_runner: ^2.0.3 + +environment: + sdk: '>=2.8.0 <3.0.0'