firka/flutter
3
0
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Provide a default --target-variant for clang_tidy. (flutter/engine#45909)

Browse Source 
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.
This commit is contained in:
Matan Lurey
2023-09-18 15:34:27 -07:00
committed by GitHub
parent c27c8aaa15
commit 8af3ec1fb9
8 changed files with 358 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files

99
engine/src/flutter/tools/clang_tidy/README.md
View File

@@ -1,15 +1,94 @@
# clang_tidy
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.
A wrapper library and program that runs `clang_tidy` on the Flutter engine repo.
```shell
# Assuming you are in the `flutter` root of the engine repo.
dart ./tools/clang_tidy/bin/main.dart
```
$ bin/main.dart --target-variant <engine-variant>
$ bin/main.dart --compile-commands <compile_commands.json-path>
$ bin/main.dart --help
By default, the linter runs over _modified_[^1] files in the _latest_[^2] build
of the engine.
A subset of checks can also be fixed automatically by passing `--fix`:
```shell
dart ./tools/clang_tidy/bin/main.dart --fix
```
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
engine/src/flutter/tools/clang_tidy/lib/clang_tidy.dart
View File

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

163
engine/src/flutter/tools/clang_tidy/lib/src/options.dart
View File

@@ -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,7 +264,7 @@ 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);
}
@@ -257,7 +272,7 @@ class Options {
'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.

3
engine/src/flutter/tools/clang_tidy/pubspec.yaml
View File

@@ -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:

56
engine/src/flutter/tools/clang_tidy/test/clang_tidy_test.dart
View File

@@ -7,6 +7,7 @@ import 'dart:io' as io show Directory, File, Platform, stderr;
import 'package:clang_tidy/clang_tidy.dart';
import 'package:clang_tidy/src/command.dart';
import 'package:clang_tidy/src/options.dart';
import 'package:engine_repo_tools/engine_repo_tools.dart';
import 'package:litetest/litetest.dart';
import 'package:path/path.dart' as path;
import 'package:process/process.dart';
@@ -19,6 +20,7 @@ final class Fixture {
/// Simulates running the tool with the given [args].
factory Fixture.fromCommandLine(List<String> args, {
ProcessManager? processManager,
Engine? engine,
}) {
processManager ??= FakeProcessManager();
final StringBuffer outBuffer = StringBuffer();
@@ -28,6 +30,7 @@ final class Fixture {
outSink: outBuffer,
errSink: errBuffer,
processManager: processManager,
engine: engine,
), errBuffer, outBuffer);
}
@@ -106,21 +109,58 @@ void _withTempFile(String prefix, void Function(String path) func) {
}
Future<int> main(List<String> args) async {
if (args.isEmpty) {
final String? buildCommands =
args.firstOrNull ??
Engine.findWithin().latestOutput()?.compileCommandsJson.path;
if (buildCommands == null || args.length > 1) {
io.stderr.writeln(
'Usage: clang_tidy_test.dart [path/to/compile_commands.json]',
);
return 1;
}
final String buildCommands = args[0];
test('--help gives help', () async {
final Fixture fixture = Fixture.fromCommandLine(<String>['--help']);
final int result = await fixture.tool.run();
test('--help gives help, and uses host_debug by default outside of an engine root', () async {
final io.Directory rootDir = io.Directory.systemTemp.createTempSync('clang_tidy_test');
try {
final Fixture fixture = Fixture.fromCommandLine(
<String>['--help'],
engine: TestEngine.createTemp(rootDir: rootDir)
);
final int result = await fixture.tool.run();
expect(fixture.tool.options.help, isTrue);
expect(result, equals(0));
expect(fixture.errBuffer.toString(), contains('Usage: '));
expect(fixture.tool.options.help, isTrue);
expect(result, equals(0));
final String errors = fixture.errBuffer.toString();
expect(errors, contains('Usage: '));
expect(errors, contains('defaults to "host_debug"'));
} finally {
rootDir.deleteSync(recursive: true);
}
});
test('--help gives help, and uses the latest build by default outside in an engine root', () async {
final io.Directory rootDir = io.Directory.systemTemp.createTempSync('clang_tidy_test');
final io.Directory buildDir = io.Directory(path.join(rootDir.path, 'out', 'host_debug_unopt_arm64'))..createSync(recursive: true);
try {
final Fixture fixture = Fixture.fromCommandLine(
<String>['--help'],
engine: TestEngine.createTemp(rootDir: rootDir, outputs: <TestOutput>[
TestOutput(buildDir),
])
);
final int result = await fixture.tool.run();
expect(fixture.tool.options.help, isTrue);
expect(result, equals(0));
final String errors = fixture.errBuffer.toString();
expect(errors, contains('Usage: '));
expect(errors, contains('defaults to "host_debug_unopt_arm64"'));
} finally {
rootDir.deleteSync(recursive: true);
}
});
test('trimmed clang-tidy output', () {

4
engine/src/flutter/tools/githooks/pubspec.yaml
View File

@@ -18,9 +18,9 @@ environment:
dependencies:
args: any
clang_tidy: any
meta: any
path: any
clang_tidy: any
dev_dependencies:
async_helper: any
@@ -39,6 +39,8 @@ dependency_overrides:
path: ../clang_tidy
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:

139
engine/src/flutter/tools/pkg/engine_repo_tools/lib/engine_repo_tools.dart
View File

@@ -35,6 +35,12 @@ import 'package:path/path.dart' as p;
/// If you have a path to a directory within the `$ENGINE/src` directory, or
/// want to use the current working directory, use [Engine.findWithin].
final class Engine {
const Engine._({
required this.srcDir,
required this.flutterDir,
required this.outDir,
});
/// Creates an [Engine] from a path such as `/Users/.../flutter/engine/src`.
///
/// ```dart
@@ -66,25 +72,44 @@ final class Engine {
// don't want to fail if it doesn't exist.
final io.Directory outDir = io.Directory(p.join(srcPath, 'out'));
return Engine._(srcDir, flutterDir, outDir);
return Engine._(
srcDir: srcDir,
flutterDir: flutterDir,
outDir: outDir,
);
}
/// Creates an [Engine] by looking for a `src/` directory in the given path.
///
/// Similar to [tryFindWithin], but throws a [StateError] if the path is not
/// within a valid engine. This is useful for tools that require an engine
/// and do not have a reasonable fallback or recovery path.
factory Engine.findWithin([String? path]) {
final Engine? engine = tryFindWithin(path);
if (engine == null) {
throw StateError('The path "$path" is not within a valid engine.');
}
return engine;
}
/// Creates an [Engine] by looking for a `src/` directory in the given [path].
///
/// ```dart
/// // Use the current working directory.
/// final Engine engine = Engine.findWithin();
/// print(engine.srcDir.path); // /Users/.../engine/src
///
/// // Use a specific directory.
/// final Engine engine = Engine.findWithin('/Users/.../engine/src/foo/bar/baz');
/// final Engine engine = Engine.findWithin('/Users/.../engine/src/foo/bar');
/// print(engine.srcDir.path); // /Users/.../engine/src
/// ```
///
/// If a path is not provided, the current working directory is used.
/// If a [path] is not provided, the current working directory is used.
///
/// Throws a [StateError] if the path is not within a valid engine.
factory Engine.findWithin([String? path]) {
/// If path does not exist, or is not a directory, an error is thrown.
///
/// Returns `null` if the path is not within a valid engine.
static Engine? tryFindWithin([String? path]) {
path ??= p.current;
// Search parent directories for a `src` directory.
@@ -105,17 +130,9 @@ final class Engine {
maybeSrcDir = maybeSrcDir.parent;
} while (maybeSrcDir.parent.path != maybeSrcDir.path /* at root */);
throw StateError(
'The path "$path" is not within a Flutter engine source directory.'
);
return null;
}
const Engine._(
this.srcDir,
this.flutterDir,
this.outDir,
);
/// The path to the `$ENGINE/src` directory.
final io.Directory srcDir;
@@ -145,12 +162,88 @@ final class Engine {
return null;
}
outputs.sort((Output a, Output b) {
return b.dir.statSync().modified.compareTo(a.dir.statSync().modified);
return b.path.statSync().modified.compareTo(a.path.statSync().modified);
});
return outputs.first;
}
}
/// An implementation of [Engine] that has pre-defined outputs for testing.
final class TestEngine extends Engine {
/// Creates a [TestEngine] with pre-defined paths.
///
/// The [srcDir] and [flutterDir] must exist, but the [outDir] is optional.
///
/// Optionally, provide a list of [outputs] to use, otherwise it is empty.
TestEngine.withPaths({
required super.srcDir,
required super.flutterDir,
required super.outDir,
List<TestOutput> outputs = const <TestOutput>[],
}) : _outputs = outputs, super._() {
if (!srcDir.existsSync()) {
throw ArgumentError.value(srcDir, 'srcDir', 'does not exist');
}
if (!flutterDir.existsSync()) {
throw ArgumentError.value(flutterDir, 'flutterDir', 'does not exist');
}
}
/// Creates a [TestEngine] within a temporary directory.
///
/// The [rootDir] is the temporary directory that will contain the engine.
///
/// Optionally, provide a list of [outputs] to use, otherwise it is empty.
factory TestEngine.createTemp({
required io.Directory rootDir,
List<TestOutput> outputs = const <TestOutput>[],
}) {
final io.Directory srcDir = io.Directory(p.join(rootDir.path, 'src'));
final io.Directory flutterDir = io.Directory(p.join(srcDir.path, 'flutter'));
final io.Directory outDir = io.Directory(p.join(srcDir.path, 'out'));
srcDir.createSync(recursive: true);
flutterDir.createSync(recursive: true);
outDir.createSync(recursive: true);
return TestEngine.withPaths(
srcDir: srcDir,
flutterDir: flutterDir,
outDir: outDir,
outputs: outputs,
);
}
final List<TestOutput> _outputs;
@override
List<Output> outputs() => List<Output>.unmodifiable(_outputs);
@override
Output? latestOutput() {
if (_outputs.isEmpty) {
return null;
}
_outputs.sort((TestOutput a, TestOutput b) {
return b.lastModified.compareTo(a.lastModified);
});
return _outputs.first;
}
}
/// An implementation of [Output] that has a pre-defined path for testing.
final class TestOutput extends Output {
/// Creates a [TestOutput] with a pre-defined path.
///
/// Optionally, provide a [lastModified] date.
TestOutput(super.path, {DateTime? lastModified})
: lastModified = lastModified ?? _defaultLastModified,
super._();
static final DateTime _defaultLastModified = DateTime.now();
/// The last modified date of the output target.
final DateTime lastModified;
}
/// Thrown when an [Engine] could not be created from a path.
sealed class InvalidEngineException implements Exception {
/// Thrown when an [Engine] was created from a path not ending in `src`.
@@ -210,19 +303,15 @@ final class InvalidEngineMissingFlutterDirectoryException implements InvalidEngi
/// Represents a single output target in the `$ENGINE/src/out` directory.
final class Output {
const Output._(this.dir);
const Output._(this.path);
/// The directory containing the output target.
final io.Directory dir;
final io.Directory path;
/// The `compile_commands.json` file for this output target.
/// The `compile_commands.json` file that should exist for this output target.
///
/// Returns `null` if the file does not exist.
io.File? get compileCommandsJson {
final io.File file = io.File(p.join(dir.path, 'compile_commands.json'));
if (!file.existsSync()) {
return null;
}
return file;
/// The file may not exist.
io.File get compileCommandsJson {
return io.File(p.join(path.path, 'compile_commands.json'));
}
}

4
engine/src/flutter/tools/pkg/engine_repo_tools/test/engine_repo_tools_test.dart
View File

@@ -187,7 +187,7 @@ void main() {
io.Directory(p.join(emptyDir.path, 'src', 'out', 'host_debug_unopt_arm64')).createSync(recursive: true);
final Engine engine = Engine.fromSrcPath(p.join(emptyDir.path, 'src'));
final List<String> outputs = engine.outputs().map((Output o) => p.basename(o.dir.path)).toList()..sort();
final List<String> outputs = engine.outputs().map((Output o) => p.basename(o.path.path)).toList()..sort();
expect(outputs, <String>[
'host_debug',
'host_debug_unopt_arm64',
@@ -218,7 +218,7 @@ void main() {
final Engine engine = Engine.fromSrcPath(p.join(emptyDir.path, 'src'));
final Output? latestOutput = engine.latestOutput();
expect(latestOutput, isNotNull);
expect(p.basename(latestOutput!.dir.path), 'host_debug_unopt_arm64');
expect(p.basename(latestOutput!.path.path), 'host_debug_unopt_arm64');
expect(latestOutput.compileCommandsJson, isNotNull);
} finally {
tearDown();