diff --git a/ci/lint.sh b/ci/lint.sh index eeddd3c7b4ce3..6355e637fd1e1 100755 --- a/ci/lint.sh +++ b/ci/lint.sh @@ -41,6 +41,5 @@ cd "$SCRIPT_DIR" "$DART" \ --disable-dart-dev \ "$SRC_DIR/flutter/tools/clang_tidy/bin/main.dart" \ - --compile-commands="$COMPILE_COMMANDS" \ - --repo="$SRC_DIR/flutter" \ + --src-dir="$SRC_DIR" \ "$@" diff --git a/tools/clang_tidy/README.md b/tools/clang_tidy/README.md index 2de22f4b56ce9..460ecbbfb7959 100644 --- a/tools/clang_tidy/README.md +++ b/tools/clang_tidy/README.md @@ -1,9 +1,15 @@ # clang_tidy -This is a Dart program/library that runs clang_tidy over modified files. It -takes two mandatory arguments that point at a compile_commands.json command -and the root of the Flutter engine repo: +This is a Dart program/library that runs clang_tidy over modified files in the Flutter engine repo. + +By default the linter runs on the repo files changed contained in `src/out/host_debug/compile_commands.json` command. +To check files other than in `host_debug` use `--target-variant android_debug_unopt`, +`--target-variant ios_debug_sim_unopt`, etc. + +Alternatively, use `--compile-commands` to specify a path to a `compile_commands.json` file. ``` -$ bin/main.dart --compile-commands --repo +$ bin/main.dart --target-variant +$ bin/main.dart --compile-commands +$ bin/main.dart --help ``` diff --git a/tools/clang_tidy/bin/main.dart b/tools/clang_tidy/bin/main.dart index 405381e4a3d5c..4e3bd27ec9f6e 100644 --- a/tools/clang_tidy/bin/main.dart +++ b/tools/clang_tidy/bin/main.dart @@ -7,8 +7,8 @@ // Runs clang-tidy on files with changes. // // Basic Usage: -// dart bin/main.dart --compile-commands \ -// --repo \ +// dart bin/main.dart --compile-commands +// dart bin/main.dart --target-variant // // User environment variable FLUTTER_LINT_ALL to run on all files. diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index 1c9a517733ee5..77650d226d599 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -45,7 +45,6 @@ class ClangTidy { /// will otherwise go to stderr. ClangTidy({ required io.File buildCommandsPath, - required io.Directory repoPath, String checksArg = '', bool lintAll = false, bool fix = false, @@ -54,7 +53,6 @@ class ClangTidy { }) : options = Options( buildCommandsPath: buildCommandsPath, - repoPath: repoPath, checksArg: checksArg, lintAll: lintAll, fix: fix, @@ -229,16 +227,8 @@ class ClangTidy { if (job.result.exitCode == 0) { continue; } - if (job.exception != null) { - _errSink.writeln( - '\nā— A clang-tidy job failed to run, aborting:\n${job.exception}', - ); - result = 1; - break; - } else { - _errSink.writeln('āŒ Failures for ${job.name}:'); - _errSink.writeln(job.result.stdout); - } + _errSink.writeln('āŒ Failures for ${job.name}:'); + _errSink.writeln(job.result.stdout); result = 1; } return result; diff --git a/tools/clang_tidy/lib/src/command.dart b/tools/clang_tidy/lib/src/command.dart index 7a1d6df8fc234..1bc87d07be766 100644 --- a/tools/clang_tidy/lib/src/command.dart +++ b/tools/clang_tidy/lib/src/command.dart @@ -134,8 +134,10 @@ class Command { filePath, if (checks != null) checks, - if (fix) + if (fix) ...[ '--fix', + '--format-style=file', + ], '--', ]; args.addAll(tidyArgs.split(' ')); diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart index 35862849431a0..264fa37bdb6ff 100644 --- a/tools/clang_tidy/lib/src/options.dart +++ b/tools/clang_tidy/lib/src/options.dart @@ -5,6 +5,10 @@ import 'dart:io' as io show Directory, File, Platform, stderr; import 'package:args/args.dart'; +import 'package:path/path.dart' as path; + +// Path to root of the flutter/engine repository containing this script. +final String _engineRoot = path.dirname(path.dirname(path.dirname(path.dirname(path.fromUri(io.Platform.script))))); /// A class for organizing the options to the Engine linter, and the files /// that it operates on. @@ -12,7 +16,6 @@ class Options { /// Builds an instance of [Options] from the arguments. Options({ required this.buildCommandsPath, - required this.repoPath, this.help = false, this.verbose = false, this.checksArg = '', @@ -30,7 +33,6 @@ class Options { return Options( errorMessage: message, buildCommandsPath: io.File('none'), - repoPath: io.Directory('none'), errSink: errSink, ); } @@ -41,7 +43,6 @@ class Options { return Options( help: true, buildCommandsPath: io.File('none'), - repoPath: io.Directory('none'), errSink: errSink, ); } @@ -49,13 +50,13 @@ class Options { /// Builds an [Options] instance with an [ArgResults] instance. factory Options._fromArgResults( ArgResults options, { + required io.File buildCommandsPath, StringSink? errSink, }) { return Options( help: options['help'] as bool, verbose: options['verbose'] as bool, - buildCommandsPath: io.File(options['compile-commands'] as String), - repoPath: io.Directory(options['repo'] as String), + buildCommandsPath: buildCommandsPath, checksArg: options.wasParsed('checks') ? options['checks'] as String : '', lintAll: io.Platform.environment['FLUTTER_LINT_ALL'] != null || options['lint-all'] as bool, @@ -70,7 +71,17 @@ class Options { StringSink? errSink, }) { final ArgResults argResults = _argParser.parse(arguments); - final String? message = _checkArguments(argResults); + + String? buildCommandsPath = argResults['compile-commands'] as String?; + // path/to/engine/src/out/variant/compile_commands.json + buildCommandsPath ??= path.join( + argResults['src-dir'] as String, + 'out', + argResults['target-variant'] as String, + 'compile_commands.json', + ); + final io.File buildCommands = io.File(buildCommandsPath); + final String? message = _checkArguments(argResults, buildCommands); if (message != null) { return Options._error(message, errSink: errSink); } @@ -79,6 +90,7 @@ class Options { } return Options._fromArgResults( argResults, + buildCommandsPath: buildCommands, errSink: errSink, ); } @@ -86,7 +98,9 @@ class Options { static final ArgParser _argParser = ArgParser() ..addFlag( 'help', + abbr: 'h', help: 'Print help.', + negatable: false, ) ..addFlag( 'lint-all', @@ -103,14 +117,25 @@ class Options { help: 'Print verbose output.', defaultsTo: false, ) - ..addOption( - 'repo', - help: 'Use the given path as the repo path', - ) ..addOption( 'compile-commands', help: 'Use the given path as the source of compile_commands.json. This ' - 'file is created by running tools/gn', + 'file is created by running "tools/gn". Cannot be used with --target-variant ' + 'or --src-dir.', + ) + ..addOption( + 'target-variant', + aliases: ['variant'], + help: 'The engine variant directory containing compile_commands.json ' + 'created by running "tools/gn". Cannot be used with --compile-commands.', + valueHelp: 'host_debug|android_debug_unopt|ios_debug|ios_debug_sim_unopt', + defaultsTo: 'host_debug', + ) + ..addOption( + 'src-dir', + help: 'Path to the engine src directory. Cannot be used with --compile-commands.', + valueHelp: 'path/to/engine/src', + defaultsTo: path.dirname(_engineRoot), ) ..addOption( 'checks', @@ -129,7 +154,7 @@ class Options { final io.File buildCommandsPath; /// The root of the flutter/engine repository. - final io.Directory repoPath; + final io.Directory repoPath = io.Directory(_engineRoot); /// Arguments to plumb through to clang-tidy formatted as a command line /// argument. @@ -156,35 +181,30 @@ class Options { _errSink.writeln(message); } _errSink.writeln( - 'Usage: bin/main.dart [--help] [--lint-all] [--fix] [--verbose] [--diff-branch]', + 'Usage: bin/main.dart [--help] [--lint-all] [--fix] [--verbose] [--diff-branch] [--target-variant variant] [--src-dir path/to/engine/src]', ); _errSink.writeln(_argParser.usage); } /// Command line argument validation. - static String? _checkArguments(ArgResults argResults) { + static String? _checkArguments(ArgResults argResults, io.File buildCommandsPath) { if (argResults.wasParsed('help')) { return null; } - if (!argResults.wasParsed('compile-commands')) { - return 'ERROR: The --compile-commands argument is required.'; + final bool compileCommandsParsed = argResults.wasParsed('compile-commands'); + if (compileCommandsParsed && argResults.wasParsed('target-variant')) { + return 'ERROR: --compile-commands option cannot be used with --target-variant.'; } - if (!argResults.wasParsed('repo')) { - return 'ERROR: The --repo argument is required.'; + if (compileCommandsParsed && argResults.wasParsed('src-dir')) { + return 'ERROR: --compile-commands option cannot be used with --src-dir.'; } - final io.File buildCommandsPath = io.File(argResults['compile-commands'] as String); if (!buildCommandsPath.existsSync()) { return "ERROR: Build commands path ${buildCommandsPath.absolute.path} doesn't exist."; } - final io.Directory repoPath = io.Directory(argResults['repo'] as String); - if (!repoPath.existsSync()) { - return "ERROR: Repo path ${repoPath.absolute.path} doesn't exist."; - } - return null; } } diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index cfde20873660f..76e0e7c775f13 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:io' as io show Directory, File, Platform, stderr; +import 'dart:io' as io show File, Platform, stderr; import 'package:clang_tidy/clang_tidy.dart'; import 'package:clang_tidy/src/command.dart'; @@ -10,14 +10,13 @@ import 'package:litetest/litetest.dart'; import 'package:process_runner/process_runner.dart'; Future main(List args) async { - if (args.length < 2) { + if (args.isEmpty) { io.stderr.writeln( - 'Usage: clang_tidy_test.dart [build commands] [repo root]', + 'Usage: clang_tidy_test.dart [path/to/compile_commands.json]', ); return 1; } final String buildCommands = args[0]; - final String repoRoot = args[1]; test('--help gives help', () async { final StringBuffer outBuffer = StringBuffer(); @@ -37,11 +36,16 @@ Future main(List args) async { expect(errBuffer.toString(), contains('Usage: ')); }); - test('Error when --compile-commands is missing', () async { + test('Error when --compile-commands and --target-variant are used together', () async { final StringBuffer outBuffer = StringBuffer(); final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy.fromCommandLine( - [], + [ + '--compile-commands', + '/unused', + '--target-variant', + 'unused' + ], outSink: outBuffer, errSink: errBuffer, ); @@ -51,17 +55,19 @@ Future main(List args) async { expect(clangTidy.options.help, isFalse); expect(result, equals(1)); expect(errBuffer.toString(), contains( - 'ERROR: The --compile-commands argument is required.', + 'ERROR: --compile-commands option cannot be used with --target-variant.', )); }); - test('Error when --repo is missing', () async { + test('Error when --compile-commands and --src-dir are used together', () async { final StringBuffer outBuffer = StringBuffer(); final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy.fromCommandLine( [ '--compile-commands', '/unused', + '--src-dir', + '/unused', ], outSink: outBuffer, errSink: errBuffer, @@ -72,7 +78,7 @@ Future main(List args) async { expect(clangTidy.options.help, isFalse); expect(result, equals(1)); expect(errBuffer.toString(), contains( - 'ERROR: The --repo argument is required.', + 'ERROR: --compile-commands option cannot be used with --src-dir.', )); }); @@ -83,8 +89,6 @@ Future main(List args) async { [ '--compile-commands', '/does/not/exist', - '--repo', - '/unused', ], outSink: outBuffer, errSink: errBuffer, @@ -99,16 +103,15 @@ Future main(List args) async { )); }); - test('Error when --repo path does not exist', () async { + test('Error when --src-dir path does not exist, uses target variant in path', () async { final StringBuffer outBuffer = StringBuffer(); final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy.fromCommandLine( [ - '--compile-commands', - // This file needs to exist, and be UTF8 line-parsable. - io.Platform.script.path, - '--repo', + '--src-dir', '/does/not/exist', + '--target-variant', + 'ios_debug_unopt', ], outSink: outBuffer, errSink: errBuffer, @@ -119,7 +122,7 @@ Future main(List args) async { expect(clangTidy.options.help, isFalse); expect(result, equals(1)); expect(errBuffer.toString(), contains( - "ERROR: Repo path /does/not/exist doesn't exist.", + "ERROR: Build commands path /does/not/exist/out/ios_debug_unopt/compile_commands.json doesn't exist.", )); }); @@ -128,7 +131,6 @@ Future main(List args) async { final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( buildCommandsPath: io.File(buildCommands), - repoPath: io.Directory(repoRoot), lintAll: true, outSink: outBuffer, errSink: errBuffer, @@ -142,7 +144,6 @@ Future main(List args) async { final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( buildCommandsPath: io.File(buildCommands), - repoPath: io.Directory(repoRoot), outSink: outBuffer, errSink: errBuffer, ); @@ -155,7 +156,6 @@ Future main(List args) async { final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( buildCommandsPath: io.File(buildCommands), - repoPath: io.Directory(repoRoot), lintAll: true, outSink: outBuffer, errSink: errBuffer, @@ -181,7 +181,6 @@ Future main(List args) async { final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( buildCommandsPath: io.File(buildCommands), - repoPath: io.Directory(repoRoot), lintAll: true, outSink: outBuffer, errSink: errBuffer, @@ -218,6 +217,7 @@ Future main(List args) async { '../../buildtools/mac-x64/clang/bin/clang-tidy', filePath, '--fix', + '--format-style=file', '--', '', filePath, diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index 428c10e8307be..e1e36999f7eb0 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -60,7 +60,6 @@ class PrePushCommand extends Command { final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( buildCommandsPath: compileCommands, - repoPath: io.Directory(flutterRoot), outSink: outBuffer, errSink: errBuffer, );