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 6 commits
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
8 changes: 7 additions & 1 deletion ci/clang_tidy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,12 @@ else
fix_flag="--fix --lint-all"
fi

COMPILE_COMMANDS="$SRC_DIR/out/host_debug/compile_commands.json"
# Determine wether to use x64 or arm64.
if command -v arch &> /dev/null && [[ $(arch) == "arm64" ]]; then
CLANG_TIDY_PATH="buildtools/mac-arm64/clang/bin/clang-tidy"
fi

COMPILE_COMMANDS="$SRC_DIR/out/$BUILD_DIR/compile_commands.json"
Copy link
Member

Choose a reason for hiding this comment

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

How is $BUILD_DIR defined?

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 doesn't. This effectively makes gn get called every time which isn't an error. In fact it might be desired. It's quick to execute (1704ms) and means you'll get coverage from newly added files. I'm going to delete the check.

Copy link
Member

Choose a reason for hiding this comment

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

gn run time can vary a lot. Some runs on my M1 take >10s. Here's a run on CI taking >4s: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/194351/overview.

Sorry to nitpick, but would it be okay to just fix the arm vs. intel issue in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, no problem. I brought back the check. What is it doing in all that 10s? In CI we should have just run gn... Oh I guess it won't match though because of the --force-mac-arm64. How I loathe that flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually for a moment considered making that check smarter. It should be doing a timestamp check against the repository. That will take a fair bit of work though so I left it for a future exercise. The indiscriminate gn doesn't have that bug.

Copy link
Member

Choose a reason for hiding this comment

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

The clang_tidy script depends on engine_repo_tools, which already has a way of finding the most recently generated out/ directory: https://github.com/flutter/engine/blob/main/tools/pkg/engine_repo_tools/lib/engine_repo_tools.dart#L159, so that could be a good way to avoid adding similar logic in the shell script.

if [ ! -f "$COMPILE_COMMANDS" ]; then
(cd "$SRC_DIR"; ./flutter/tools/gn)
fi
Expand All @@ -58,6 +63,7 @@ cd "$SCRIPT_DIR"
--disable-dart-dev \
"$SRC_DIR/flutter/tools/clang_tidy/bin/main.dart" \
--src-dir="$SRC_DIR" \
${CLANG_TIDY_PATH:+--clang-tidy="$SRC_DIR/$CLANG_TIDY_PATH"} \
$fix_flag \
"$@" && true # errors ignored
clang_tidy_return=$?
Expand Down
1 change: 1 addition & 0 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ std::unique_ptr<Shell> Shell::Create(
auto resource_cache_limit_calculator =
std::make_shared<ResourceCacheLimitCalculator>(
settings.resource_cache_max_bytes_threshold);

return CreateWithSnapshot(platform_data, //
task_runners, //
/*parent_thread_merger=*/nullptr, //
Expand Down
3 changes: 2 additions & 1 deletion tools/clang_tidy/lib/src/command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,9 @@ class Command {
'--',
];
args.addAll(tidyArgs.split(' '));
final String clangTidyPath = options.clangTidyPath?.path ?? tidyPath;
return WorkerJob(
<String>[tidyPath, ...args],
<String>[clangTidyPath, ...args],
workingDirectory: directory,
name: 'clang-tidy on $filePath',
printOutput: options.verbose,
Expand Down
13 changes: 13 additions & 0 deletions tools/clang_tidy/lib/src/options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class Options {
this.shardCommandsPaths = const <io.File>[],
this.enableCheckProfile = false,
StringSink? errSink,
this.clangTidyPath,
}) : checks = checksArg.isNotEmpty ? '--checks=$checksArg' : null,
_errSink = errSink ?? io.stderr;

Expand Down Expand Up @@ -69,6 +70,7 @@ class Options {
StringSink? errSink,
required List<io.File> shardCommandsPaths,
int? shardId,
io.File? clangTidyPath,
}) {
final LintTarget lintTarget;
if (options.wasParsed('lint-all') || io.Platform.environment['FLUTTER_LINT_ALL'] != null) {
Expand All @@ -92,6 +94,7 @@ class Options {
shardCommandsPaths: shardCommandsPaths,
shardId: shardId,
enableCheckProfile: options['enable-check-profile'] as bool,
clangTidyPath: clangTidyPath,
);
}

Expand Down Expand Up @@ -138,12 +141,16 @@ class Options {
if (shardId != null && (shardId > shardCommands.length || shardId < 0)) {
return Options._error('Invalid shard-id value: $shardId.', errSink: errSink);
}
final io.File? clangTidyPath = ((String? path) => path == null
? null
: io.File(path))(argResults['clang-tidy'] as String?);
return Options._fromArgResults(
argResults,
buildCommandsPath: buildCommands,
errSink: errSink,
shardCommandsPaths: shardCommands,
shardId: shardId,
clangTidyPath: clangTidyPath,
);
}

Expand Down Expand Up @@ -227,6 +234,10 @@ class Options {
'string, indicating all checks should be performed.',
defaultsTo: '',
)
..addOption('clang-tidy',
help:
'Path to the clang-tidy executable. Defaults to deriving the path\n'
'from compile_commands.json.')
..addFlag(
'enable-check-profile',
help: 'Enable per-check timing profiles and print a report to stderr.',
Expand Down Expand Up @@ -276,6 +287,8 @@ class Options {

final StringSink _errSink;

final io.File? clangTidyPath;

/// Print command usage with an additional message.
void printUsage({String? message, required Engine? engine}) {
if (message != null) {
Expand Down
17 changes: 17 additions & 0 deletions tools/clang_tidy/test/clang_tidy_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,23 @@ Future<int> main(List<String> args) async {
});
});

test('clang-tidy specified', () async {
_withTempFile('shard-id-valid', (String path) {
final Options options = Options.fromCommandLine( <String>[
'--clang-tidy=foo/bar',
],);
expect(options.clangTidyPath, isNotNull);
expect(options.clangTidyPath!.path, equals('foo/bar'));
});
});

test('clang-tidy unspecified', () async {
_withTempFile('shard-id-valid', (String path) {
final Options options = Options.fromCommandLine( <String>[],);
expect(options.clangTidyPath, isNull);
});
});

test('shard-id invalid', () async {
_withTempFile('shard-id-valid', (String path) {
final StringBuffer errBuffer = StringBuffer();
Expand Down