Skip to content

Commit

Permalink
Provide a default --target-variant for clang_tidy. (flutter#45909)
Browse files Browse the repository at this point in the history 
Using `engine_repo_tools`, provide a default `--target-variant` for the `clang_tidy` tool.

Mostly test code added below, the major change is that `--target-variant` now has a default other than `host_debug` if we're running inside of an engine root (which we ~always are). I didn't make any other changes (i.e. to the pre-commit hook) to keep this change smallish.

Also rewrote the `README.md` to represent the current state as best I could.
  • Loading branch information
@matanlurey
matanlurey authored Sep 18, 2023
1 parent 21372b1 commit 6036024
Show file tree
Hide file tree
Showing 8 changed files with 356 additions and 123 deletions.
95 changes: 87 additions & 8 deletions tools/clang_tidy/README.md
View file
Original file line number Diff line number Diff line change
@@ -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/`.
15 changes: 10 additions & 5 deletions tools/clang_tidy/lib/clang_tidy.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;

Expand All @@ -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;
}

Expand Down
163 changes: 89 additions & 74 deletions tools/clang_tidy/lib/src/options.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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?;

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions tools/clang_tidy/pubspec.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ environment:

dependencies:
args: any
engine_repo_tools: any
meta: any
path: any
process: any
Expand All @@ -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:
Expand Down
Loading

0 comments on commit 6036024

Please sign in to comment.