Provide a default --target-variant for clang_tidy.

tools/clang_tidy/README.md

@@ -1,15 +1,94 @@
 # clang_tidy
 
-This is a Dart program/library that runs clang_tidy over modified files in the Flutter engine repo.
+A wrapper library and program that runs `clang_tidy` on 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.
+```shell
+# Assuming you are in the `flutter` root of the engine repo.
+dart ./tools/clang_tidy/bin/main.dart
+```
+
+By default, the linter runs over _modified_[^1] files in the _latest_[^2] build
+of the engine.
 
-Alternatively, use `--compile-commands` to specify a path to a `compile_commands.json` file.
+A subset of checks can also be fixed automatically by passing `--fix`:
 
+```shell
+dart ./tools/clang_tidy/bin/main.dart --fix
 ```
-$ bin/main.dart --target-variant <engine-variant>
-$ bin/main.dart --compile-commands <compile_commands.json-path>
-$ bin/main.dart --help
+
+To configure what lints are enabled, see [`.clang-tidy`](../../.clang-tidy).
+
+> **💡 TIP**: If you're looking for the git pre-commit hook configuration, see
+> [`githooks`](../githooks).
+
+## Advanced Usage
+
+Some common use cases are described below, or use `--help` to see all options.
+
+### Run with checks added or removed
+
+To run adding a check _not_ specified in `.clang-tidy`:
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --checks="<check-name-to-run>"
 ```
+
+It's possible also to use wildcards to add multiple checks:
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --checks="readability-*"
+```
+
+To remove a specific check:
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --checks="-<check-name-to-remove>"
+```
+
+To remove multiple checks:
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --checks="-readability-*"
+```
+
+To remove _all_ checks (usually to add a specific check):
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --checks="-*,<only-check-to-run>"
+```
+
+### Specify a specific build
+
+There are some rules that are only applicable to certain builds, or to check
+a difference in behavior between two builds.
+
+Use `--target-variant` to specify a build:
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --target-variant <engine-variant>
+```
+
+For example, to check the `android_debug_unopt` build:
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --target-variant android_debug_unopt
+```
+
+In rarer cases, for example comparing two different checkouts of the engine,
+use `--src-dir=<path/to/engine/src>`.
+
+### Lint entire repository
+
+When adding a new lint rule, or when checking lint rules that impact files that
+have not changed.
+
+Use `--lint-all` to lint all files in the repo:
+
+```shell
+dart ./tools/clang_tidy/bin/main.dart --lint-all
+```
+
+> **⚠️ WARNING**: This will take a long time to run.
+
+[^1]: Modified files are determined by a `git diff` command compared to `HEAD`.
+[^2]: Latest build is the last updated directory in `src/out/`.

tools/clang_tidy/lib/clang_tidy.dart

@@ -5,6 +5,7 @@
 import 'dart:convert' show LineSplitter, jsonDecode;
 import 'dart:io' as io show File, stderr, stdout;
 
+import 'package:engine_repo_tools/engine_repo_tools.dart';
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 import 'package:process/process.dart';
@@ -92,25 +93,29 @@ class ClangTidy {
     ),
     _outSink = outSink ?? io.stdout,
     _errSink = errSink ?? io.stderr,
-    _processManager = processManager;
+    _processManager = processManager,
+    _engine = null;
 
   /// Builds an instance of [ClangTidy] from a command line.
   ClangTidy.fromCommandLine(
     List<String> args, {
+    Engine? engine,
     StringSink? outSink,
     StringSink? errSink,
     ProcessManager processManager = const LocalProcessManager(),
   }) :
-    options = Options.fromCommandLine(args, errSink: errSink),
+    options = Options.fromCommandLine(args, errSink: errSink, engine: engine),
     _outSink = outSink ?? io.stdout,
     _errSink = errSink ?? io.stderr,
-    _processManager = processManager;
+    _processManager = processManager,
+    _engine = engine;
 
   /// The [Options] that specify how this [ClangTidy] operates.
   final Options options;
   final StringSink _outSink;
   final StringSink _errSink;
   final ProcessManager _processManager;
+  final Engine? _engine;
 
   late final DateTime _startTime;
 
@@ -119,12 +124,12 @@ class ClangTidy {
     _startTime = DateTime.now();
 
     if (options.help) {
-      options.printUsage();
+      options.printUsage(engine: _engine);
       return 0;
     }
 
     if (options.errorMessage != null) {
-      options.printUsage(message: options.errorMessage);
+      options.printUsage(message: options.errorMessage, engine: _engine);
       return 1;
     }
 

tools/clang_tidy/lib/src/options.dart

@@ -5,11 +5,10 @@
 import 'dart:io' as io show Directory, File, Platform, stderr;
 
 import 'package:args/args.dart';
+import 'package:engine_repo_tools/engine_repo_tools.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)))));
-
+final Engine _engineRoot = Engine.findWithin(path.dirname(path.fromUri(io.Platform.script)));
 
 /// Adds warnings as errors for only specific runs.  This is helpful if migrating one platform at a time.
 String? _platformSpecificWarningsAsErrors(ArgResults options) {
@@ -91,8 +90,13 @@ class Options {
   factory Options.fromCommandLine(
     List<String> arguments, {
     StringSink? errSink,
+    Engine? engine,
   }) {
-    final ArgResults argResults = _argParser.parse(arguments);
+    // TODO(matanlurey): Refactor this further, ideally moving all of the engine
+    // resolution logic (i.e. --src-dir, --target-variant, --compile-commands)
+    // into a separate method, and perhaps also adding `engine.output(name)`
+    // to engine_repo_tools instead of path manipulation inlined below.
+    final ArgResults argResults = _argParser(defaultEngine: engine).parse(arguments);
 
     String? buildCommandsPath = argResults['compile-commands'] as String?;
 
@@ -134,74 +138,85 @@ class Options {
     );
   }
 
-  static final ArgParser _argParser = ArgParser()
-    ..addFlag(
-      'help',
-      abbr: 'h',
-      help: 'Print help.',
-      negatable: false,
-    )
-    ..addFlag(
-      'lint-all',
-      help: 'Lint all of the sources, regardless of FLUTTER_NOLINT.',
-    )
-    ..addFlag(
-      'lint-head',
-      help: 'Lint files changed in the tip-of-tree commit.',
-    )
-    ..addFlag(
-      'fix',
-      help: 'Apply suggested fixes.',
-    )
-    ..addFlag(
-      'verbose',
-      help: 'Print verbose output.',
-    )
-    ..addOption(
-      'shard-id',
-      help: 'When used with the shard-commands option this identifies which shard will execute.',
-      valueHelp: 'A number less than 1 + the number of shard-commands arguments.',
-    )
-    ..addOption(
-      'shard-variants',
-      help: 'Comma separated list of other targets, this invocation '
-            'will only execute a subset of the intersection and the difference of the '
-            'compile commands. Use with `shard-id`.'
-    )
-    ..addOption(
-      'compile-commands',
-      help: 'Use the given path as the source of compile_commands.json. This '
-            'file is created by running "tools/gn". Cannot be used with --target-variant '
-            'or --src-dir.',
-    )
-    ..addOption(
-      'target-variant',
-      aliases: <String>['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('mac-host-warnings-as-errors',
+  static ArgParser _argParser({required Engine? defaultEngine}) {
+    defaultEngine ??= _engineRoot;
+    final io.Directory? latestBuild = defaultEngine.latestOutput()?.path;
+    return ArgParser()
+      ..addFlag(
+        'help',
+        abbr: 'h',
+        help: 'Print help.',
+        negatable: false,
+      )
+      ..addFlag(
+        'lint-all',
+        help: 'Lint all of the sources, regardless of FLUTTER_NOLINT.',
+      )
+      ..addFlag(
+        'lint-head',
+        help: 'Lint files changed in the tip-of-tree commit.',
+      )
+      ..addFlag(
+        'fix',
+        help: 'Apply suggested fixes.',
+      )
+      ..addFlag(
+        'verbose',
+        help: 'Print verbose output.',
+      )
+      ..addOption(
+        'shard-id',
+        help: 'When used with the shard-commands option this identifies which shard will execute.',
+        valueHelp: 'A number less than 1 + the number of shard-commands arguments.',
+      )
+      ..addOption(
+        'shard-variants',
+        help: 'Comma separated list of other targets, this invocation '
+              'will only execute a subset of the intersection and the difference of the '
+              'compile commands. Use with `shard-id`.'
+      )
+      ..addOption(
+        'compile-commands',
+        help: 'Use the given path as the source of compile_commands.json. This '
+              'file is created by running "tools/gn". Cannot be used with --target-variant '
+              'or --src-dir.',
+      )
+      ..addOption(
+        'target-variant',
+        aliases: <String>['variant'],
+        help: 'The engine variant directory name containing compile_commands.json '
+              'created by running "tools/gn".\n\nIf not provided, the default is '
+              'the latest build in the engine defined by --src-dir (or the '
+              'default path, see --src-dir for details).\n\n'
+              'Cannot be used with --compile-commands.',
+        valueHelp: 'host_debug|android_debug_unopt|ios_debug|ios_debug_sim_unopt',
+        defaultsTo: latestBuild == null ? 'host_debug' : path.basename(latestBuild.path),
+      )
+      ..addOption('mac-host-warnings-as-errors',
+          help:
+              'checks that will be treated as errors when running debug_host on mac.')
+      ..addOption(
+        'src-dir',
         help:
-            'checks that will be treated as errors when running debug_host on mac.')
-    ..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',
-      help: 'Perform the given checks on the code. Defaults to the empty '
-            'string, indicating all checks should be performed.',
-      defaultsTo: '',
-    )
-    ..addFlag(
-      'enable-check-profile',
-      help: 'Enable per-check timing profiles and print a report to stderr.',
-      negatable: false,
-    );
+              'Path to the engine src directory.\n\n'
+              'If not provided, the default is the engine root directory that '
+              'contains the `clang_tidy` tool.\n\n'
+              'Cannot be used with --compile-commands.',
+        valueHelp: 'path/to/engine/src',
+        defaultsTo: _engineRoot.srcDir.path,
+      )
+      ..addOption(
+        'checks',
+        help: 'Perform the given checks on the code. Defaults to the empty '
+              'string, indicating all checks should be performed.',
+        defaultsTo: '',
+      )
+      ..addFlag(
+        'enable-check-profile',
+        help: 'Enable per-check timing profiles and print a report to stderr.',
+        negatable: false,
+      );
+  }
 
   /// Whether to print a help message and exit.
   final bool help;
@@ -219,7 +234,7 @@ class Options {
   final int? shardId;
 
   /// The root of the flutter/engine repository.
-  final io.Directory repoPath = io.Directory(_engineRoot);
+  final io.Directory repoPath = _engineRoot.flutterDir;
 
   /// Argument sent as `warnings-as-errors` to clang-tidy.
   final String? warningsAsErrors;
@@ -249,15 +264,15 @@ class Options {
   final StringSink _errSink;
 
   /// Print command usage with an additional message.
-  void printUsage({String? message}) {
+  void printUsage({String? message, required Engine? engine}) {
     if (message != null) {
       _errSink.writeln(message);
     }
     _errSink.writeln(
       'Usage: bin/main.dart [--help] [--lint-all] [--lint-head] [--fix] [--verbose] '
       '[--diff-branch] [--target-variant variant] [--src-dir path/to/engine/src]',
     );
-    _errSink.writeln(_argParser.usage);
+    _errSink.writeln(_argParser(defaultEngine: engine).usage);
   }
 
   /// Command line argument validation.

tools/clang_tidy/pubspec.yaml

@@ -18,6 +18,7 @@ environment:
 
 dependencies:
   args: any
+  engine_repo_tools: any
   meta: any
   path: any
   process: any
@@ -38,6 +39,8 @@ dependency_overrides:
     path: ../../../third_party/dart/pkg/async_helper
   collection:
     path: ../../../third_party/dart/third_party/pkg/collection
+  engine_repo_tools:
+    path: ../pkg/engine_repo_tools
   expect:
     path: ../../../third_party/dart/pkg/expect
   file: