Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/lint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ 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" \
"$@"
12 changes: 9 additions & 3 deletions tools/clang_tidy/README.md
Original file line number Diff line number Diff line change
@@ -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 --target-variant <engine-variant> --repo <path-to-repo>
$ bin/main.dart --compile-commands <compile_commands.json-path> --repo <path-to-repo>
$ bin/main.dart --help
```
45 changes: 36 additions & 9 deletions tools/clang_tidy/lib/src/options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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)))));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're computing this here, maybe we don't need the --repo command line argument anymore. Or the other way around, if you have the --repo argument, this is just the parent directory of that, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there any case where --repo needs to handle some other repo directory? I wasn't sure why it was passed in in the first place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern is probably safe here since we're always running the script from source, but it would break if the script were compiled to a snapshot and then the snapshot moved somewhere else before being run.

We can keep this and get rid of the --repo argument if that is simpler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would break if the script were compiled to a snapshot and then the snapshot moved somewhere else before being run.

If you do that and it doesn't work: Give The Script A Break 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed --repo.


/// A class for organizing the options to the Engine linter, and the files
/// that it operates on.
Expand Down Expand Up @@ -49,12 +53,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),
buildCommandsPath: buildCommandsPath,
repoPath: io.Directory(options['repo'] as String),
checksArg: options.wasParsed('checks') ? options['checks'] as String : '',
lintAll: io.Platform.environment['FLUTTER_LINT_ALL'] != null ||
Expand All @@ -70,7 +75,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);
}
Expand All @@ -79,14 +94,17 @@ class Options {
}
return Options._fromArgResults(
argResults,
buildCommandsPath: buildCommands,
errSink: errSink,
);
}

static final ArgParser _argParser = ArgParser()
..addFlag(
'help',
abbr: 'h',
help: 'Print help.',
negatable: false,
)
..addFlag(
'lint-all',
Expand All @@ -112,6 +130,20 @@ class Options {
help: 'Use the given path as the source of compile_commands.json. This '
'file is created by running tools/gn',
)
..addOption(
'target-variant',
aliases: <String>['variant'],
help: 'The engine variant directory containing compile_commands.json '
'created by running tools/gn. Ignored if --compile-commands is also passed.',
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. Ignored if --compile-commands is also passed.',
valueHelp: 'path/to/engine/src',
defaultsTo: path.dirname(_engineRoot),
)
..addOption(
'checks',
help: 'Perform the given checks on the code. Defaults to the empty '
Expand Down Expand Up @@ -156,26 +188,21 @@ 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.';
}

if (!argResults.wasParsed('repo')) {
return 'ERROR: The --repo argument is required.';
}

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.";
}
Expand Down
25 changes: 16 additions & 9 deletions tools/clang_tidy/test/clang_tidy_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import 'package:process_runner/process_runner.dart';
Future<int> main(List<String> args) async {
if (args.length < 2) {
io.stderr.writeln(
'Usage: clang_tidy_test.dart [build commands] [repo root]',
'Usage: clang_tidy_test.dart [path/to/compile_commands.json] [repo root]',
);
return 1;
}
Expand All @@ -37,11 +37,14 @@ Future<int> main(List<String> args) async {
expect(errBuffer.toString(), contains('Usage: '));
});

test('Error when --compile-commands is missing', () async {
test('Error when --repo is missing', () async {
final StringBuffer outBuffer = StringBuffer();
final StringBuffer errBuffer = StringBuffer();
final ClangTidy clangTidy = ClangTidy.fromCommandLine(
<String>[],
<String>[
'--compile-commands',
'/unused',
],
outSink: outBuffer,
errSink: errBuffer,
);
Expand All @@ -51,16 +54,18 @@ Future<int> main(List<String> args) async {
expect(clangTidy.options.help, isFalse);
expect(result, equals(1));
expect(errBuffer.toString(), contains(
'ERROR: The --compile-commands argument is required.',
'ERROR: The --repo argument is required.',
));
});

test('Error when --repo is missing', () async {
test('Error when --compile-commands path does not exist', () async {
final StringBuffer outBuffer = StringBuffer();
final StringBuffer errBuffer = StringBuffer();
final ClangTidy clangTidy = ClangTidy.fromCommandLine(
<String>[
'--compile-commands',
'/does/not/exist',
'--repo',
'/unused',
],
outSink: outBuffer,
Expand All @@ -72,17 +77,19 @@ Future<int> main(List<String> args) async {
expect(clangTidy.options.help, isFalse);
expect(result, equals(1));
expect(errBuffer.toString(), contains(
'ERROR: The --repo argument is required.',
"ERROR: Build commands path /does/not/exist doesn't exist.",
));
});

test('Error when --compile-commands 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(
<String>[
'--compile-commands',
'--src-dir',
'/does/not/exist',
'--target-variant',
'ios_debug_unopt',
'--repo',
'/unused',
],
Expand All @@ -95,7 +102,7 @@ Future<int> main(List<String> args) async {
expect(clangTidy.options.help, isFalse);
expect(result, equals(1));
expect(errBuffer.toString(), contains(
"ERROR: Build commands path /does/not/exist doesn't exist.",
"ERROR: Build commands path /does/not/exist/out/ios_debug_unopt/compile_commands.json doesn't exist.",
));
});

Expand Down