From d0dfeaa05c048970c4f53e362d2a317bfa617dd5 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 21 Aug 2023 17:19:17 -0700 Subject: [PATCH 01/12] Augment the clang-tidy pre-push to use a different config. --- .clang-tidy | 8 ++- .clang-tidy-for-githooks | 41 +++++++++++++ tools/clang_tidy/lib/clang_tidy.dart | 3 + tools/clang_tidy/lib/src/command.dart | 2 + tools/clang_tidy/lib/src/options.dart | 15 +++++ tools/githooks/lib/src/pre_push_command.dart | 60 ++++++++++++-------- tools/githooks/pre-push | 2 +- 7 files changed, 103 insertions(+), 28 deletions(-) create mode 100644 .clang-tidy-for-githooks diff --git a/.clang-tidy b/.clang-tidy index 716f63fbb6f1e..9fd40361b53db 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,6 +1,8 @@ -# Prefix check with "-" to ignore. -# Note: Some of the checks here are used as errors selectively, see -# //ci/lint.sh +# Keep in sync with .clang-tidy-for-githooks. +# +# These checks are run by the CI presubmit script, but are not run by the +# githooks presubmit script. The githooks presubmit script runs a subset of +# these checks (often due to performance reasons, i.e. it taking forever). Checks: "bugprone-use-after-move,\ bugprone-unchecked-optional-access,\ clang-analyzer-*,\ diff --git a/.clang-tidy-for-githooks b/.clang-tidy-for-githooks new file mode 100644 index 0000000000000..fe3129fdbae2b --- /dev/null +++ b/.clang-tidy-for-githooks @@ -0,0 +1,41 @@ +# Keep in sync with .clang-tidy. +# +# These checks are run by the git hooks presubmit script, but are not run by the +# CI presubmit script. The CI presubmit script runs a superset of these checks +# (it's OK if CI takes 10s of minutes, but not git push). +Checks: "bugprone-use-after-move,\ +bugprone-unchecked-optional-access,\ +clang-analyzer-*,\ +clang-diagnostic-*,\ +darwin-*,\ +google-*,\ +modernize-use-default-member-init,\ +objc-*,\ +-objc-nsinvocation-argument-lifetime,\ +readability-identifier-naming,\ +-google-build-using-namespace,\ +-google-default-arguments,\ +-google-objc-global-variable-declaration,\ +-google-objc-avoid-throwing-exception,\ +-google-readability-casting,\ +-clang-analyzer-nullability.NullPassedToNonnull,\ +-clang-analyzer-nullability.NullablePassedToNonnull,\ +-clang-analyzer-nullability.NullReturnedFromNonnull,\ +-clang-analyzer-nullability.NullableReturnedFromNonnull,\ +performance-move-const-arg" + +CheckOptions: + - key: modernize-use-default-member-init.UseAssignment + value: true + - key: readability-identifier-naming.EnumConstantCase + value: 'CamelCase' + - key: readability-identifier-naming.EnumConstantPrefix + value: 'k' + - key: readability-identifier-naming.GlobalConstantCase + value: 'CamelCase' + - key: readability-identifier-naming.GlobalConstantPrefix + value: 'k' + - key: readability-identifier-naming.PrivateMemberCase + value: 'lower_case' + - key: readability-identifier-naming.PrivateMemberSuffix + value: '_' diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index c0bffa0afc126..2d85d7390e8a1 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -47,6 +47,7 @@ class ClangTidy { /// an instance of [ClangTidy]. /// /// `buildCommandsPath` is the path to the build_commands.json file. + /// `configPath` is a path to a `.clang-tidy` config file. /// `repoPath` is the path to the Engine repo. /// `checksArg` are specific checks for clang-tidy to do. /// `lintAll` when true indicates that all files should be linted. @@ -56,6 +57,7 @@ class ClangTidy { /// will otherwise go to stderr. ClangTidy({ required io.File buildCommandsPath, + io.File? configPath, String checksArg = '', bool lintAll = false, bool lintHead = false, @@ -65,6 +67,7 @@ class ClangTidy { }) : options = Options( buildCommandsPath: buildCommandsPath, + configPath: configPath, checksArg: checksArg, lintAll: lintAll, lintHead: lintHead, diff --git a/tools/clang_tidy/lib/src/command.dart b/tools/clang_tidy/lib/src/command.dart index 41dbea5914592..9a8636f1fff47 100644 --- a/tools/clang_tidy/lib/src/command.dart +++ b/tools/clang_tidy/lib/src/command.dart @@ -135,6 +135,8 @@ class Command { final List args = [ filePath, '--warnings-as-errors=${options.warningsAsErrors ?? '*'}', + if (options.configPath != null) + '--config-file=${options.configPath}', if (options.checks != null) options.checks!, if (options.fix) ...[ diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart index 401d34b8e81bc..0f1485c5903cf 100644 --- a/tools/clang_tidy/lib/src/options.dart +++ b/tools/clang_tidy/lib/src/options.dart @@ -28,6 +28,7 @@ class Options { required this.buildCommandsPath, this.help = false, this.verbose = false, + this.configPath, this.checksArg = '', this.lintAll = false, this.lintHead = false, @@ -70,10 +71,21 @@ class Options { required List shardCommandsPaths, int? shardId, }) { + io.File? configPath; + if (options.wasParsed('config')) { + configPath = io.File(options['config'] as String); + if (!configPath.existsSync()) { + return Options._error( + 'ERROR: Config file ${configPath.absolute.path} does not exist.', + errSink: errSink, + ); + } + } return Options( help: options['help'] as bool, verbose: options['verbose'] as bool, buildCommandsPath: buildCommandsPath, + configPath: configPath, checksArg: options.wasParsed('checks') ? options['checks'] as String : '', lintAll: io.Platform.environment['FLUTTER_LINT_ALL'] != null || options['lint-all'] as bool, @@ -212,6 +224,9 @@ class Options { /// The location of the compile_commands.json file. final io.File buildCommandsPath; + /// A location of a `.clang-tidy` configuration file. + final io.File? configPath; + /// The location of shard compile_commands.json files. final List shardCommandsPaths; diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index 6b456cbe53ebe..d8457c16aceaa 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -24,8 +24,9 @@ class PrePushCommand extends Command { final String flutterRoot = globalResults!['flutter']! as String; if (!enableClangTidy) { - print('The clang-tidy check is disabled. To enable set the environment ' - 'variable PRE_PUSH_CLANG_TIDY to any value.'); + print( + 'The clang-tidy check was explicitly disabled. To enable clear ' + 'the environment variable PRE_PUSH_CLANG_TIDY or set it to true.'); } final List checkResults = [ @@ -38,36 +39,31 @@ class PrePushCommand extends Command { return !checkResults.contains(false); } + static const List _checkForHostTargets = [ + 'host_debug_unopt_arm64', + 'host_debug_arm64', + 'host_debug_unopt', + 'host_debug', + ]; + Future _runClangTidy(String flutterRoot, bool verbose) async { io.stdout.writeln('Starting clang-tidy checks.'); final Stopwatch sw = Stopwatch()..start(); - // First ensure that out/host_debug/compile_commands.json exists by running - // //flutter/tools/gn. - io.File compileCommands = io.File(path.join( - flutterRoot, - '..', - 'out', - 'host_debug', - 'compile_commands.json', - )); - if (!compileCommands.existsSync()) { - compileCommands = io.File(path.join( - flutterRoot, - '..', - 'out', - 'host_debug_unopt', - 'compile_commands.json', - )); - if (!compileCommands.existsSync()) { - io.stderr.writeln('clang-tidy requires a fully built host_debug or ' - 'host_debug_unopt build directory'); - return false; - } + // First ensure that out/host_{{flags}}/compile_commands.json exists by running + // //flutter/tools/gn. See _checkForHostTargets above for supported targets. + final io.File? compileCommands = _findCompileCommands(flutterRoot); + if (compileCommands == null) { + io.stderr.writeln( + 'clang-tidy requires a fully built host directory, such as: ' + '${_checkForHostTargets.join(', ')}.' + ); + return false; } final StringBuffer outBuffer = StringBuffer(); final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( buildCommandsPath: compileCommands, + configPath: io.File(path.join(flutterRoot, '.clang-tidy-for-githooks')), outSink: outBuffer, errSink: errBuffer, ); @@ -81,6 +77,22 @@ class PrePushCommand extends Command { return true; } + io.File? _findCompileCommands(String flutterRoot) { + for (final String dir in _checkForHostTargets) { + final io.File file = io.File(path.join( + flutterRoot, + '..', + 'out', + dir, + 'compile_commands.json', + )); + if (file.existsSync()) { + return file; + } + } + return null; + } + Future _runFormatter(String flutterRoot, bool verbose) async { io.stdout.writeln('Starting formatting checks.'); final Stopwatch sw = Stopwatch()..start(); diff --git a/tools/githooks/pre-push b/tools/githooks/pre-push index 0f629a08c8704..2505421b5fc25 100755 --- a/tools/githooks/pre-push +++ b/tools/githooks/pre-push @@ -23,7 +23,7 @@ def Main(argv): FLUTTER_DIR, ] - if ENABLE_CLANG_TIDY is not None: + if ENABLE_CLANG_TIDY is not False: githook_args += [ '--enable-clang-tidy', ] From f38c2642cb3b7c280967feb5bb00f0e3b5d89809 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 21 Aug 2023 17:29:24 -0700 Subject: [PATCH 02/12] Fix and add test. --- tools/clang_tidy/lib/src/options.dart | 9 +++-- tools/clang_tidy/test/clang_tidy_test.dart | 38 ++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart index 0f1485c5903cf..4b1c1c2d07800 100644 --- a/tools/clang_tidy/lib/src/options.dart +++ b/tools/clang_tidy/lib/src/options.dart @@ -72,8 +72,8 @@ class Options { int? shardId, }) { io.File? configPath; - if (options.wasParsed('config')) { - configPath = io.File(options['config'] as String); + if (options.wasParsed('config-file')) { + configPath = io.File(options['config-file'] as String); if (!configPath.existsSync()) { return Options._error( 'ERROR: Config file ${configPath.absolute.path} does not exist.', @@ -209,6 +209,11 @@ class Options { 'string, indicating all checks should be performed.', defaultsTo: '', ) + ..addOption( + 'config-file', + help: 'Path to a .clang-tidy configuration file.', + valueHelp: 'path/to/.clang-tidy', + ) ..addFlag( 'enable-check-profile', help: 'Enable per-check timing profiles and print a report to stderr.', diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 9b2d6d0e160e7..196166e418262 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -148,6 +148,44 @@ Future main(List args) async { print(outBuffer); }); + test('Accepts --config-file', () async { + // If buildCommands is in "$ENGINE/src/out/host_debug", then the config + // file should be in "$ENGINE/src/flutter/.clang-tidy-for-githooks". + late final String flutterRoot; + + // Find the 'src' directory and append 'flutter/.clang-tidy-for-githooks'. + { + final List buildCommandParts = path.split(path.absolute(buildCommands)); + for (int i = 0; i < buildCommandParts.length; ++i) { + if (buildCommandParts[i] == 'src') { + flutterRoot = path.joinAll([ + ...buildCommandParts.sublist(0, i + 1), + 'flutter', + ]); + break; + } + } + } + + final StringBuffer outBuffer = StringBuffer(); + final StringBuffer errBuffer = StringBuffer(); + final ClangTidy clangTidy = ClangTidy.fromCommandLine( + [ + '--compile-commands', + buildCommands, + '--config-file=${path.join(flutterRoot, '.clang-tidy-for-githooks')}', + ], + outSink: outBuffer, + errSink: errBuffer, + ); + + final int result = await clangTidy.run(); + + expect(result, equals(0)); + expect(clangTidy.options.configPath?.path, endsWith('.clang-tidy-for-githooks')); + print(outBuffer); + }); + test('shard-id valid', () async { _withTempFile('shard-id-valid', (String path) { final Options options = Options.fromCommandLine( [ From 7c7d3852a99ffeceba8fa72b46dd0b3e2c388e92 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 21 Aug 2023 17:29:47 -0700 Subject: [PATCH 03/12] Whitespace. --- tools/githooks/lib/src/pre_push_command.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index d8457c16aceaa..8ae5e27a10173 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -55,7 +55,7 @@ class PrePushCommand extends Command { if (compileCommands == null) { io.stderr.writeln( 'clang-tidy requires a fully built host directory, such as: ' - '${_checkForHostTargets.join(', ')}.' + '${_checkForHostTargets.join(', ')}.' ); return false; } From a278a9ca1cdfccb382872484a707b8864342c6b3 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 21 Aug 2023 17:47:36 -0700 Subject: [PATCH 04/12] Ignore licenses. --- ci/licenses_golden/excluded_files | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 91b8b6d2c283a..263ca3e6f301d 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -14,6 +14,7 @@ ../../../flutter/.ci.yaml ../../../flutter/.clang-format ../../../flutter/.clang-tidy +../../../flutter/.clang-tidy-for-githooks ../../../flutter/.git ../../../flutter/.gitattributes ../../../flutter/.github From dce3cede28bfaa904410245b01b5e921037431d6 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 21 Aug 2023 18:29:31 -0700 Subject: [PATCH 05/12] Also update this I suppose. --- tools/licenses/lib/paths.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/licenses/lib/paths.dart b/tools/licenses/lib/paths.dart index 93ab31f5ffb8a..00b5ba4f6953e 100644 --- a/tools/licenses/lib/paths.dart +++ b/tools/licenses/lib/paths.dart @@ -20,6 +20,7 @@ final Set skippedPaths = { r'build', // only used by build r'build_overrides', // only used by build r'buildtools', // only used by build + r'flutter/.clang-tidy-for-githooks', // nearly identical to .clang-tidy r'flutter/build', r'flutter/ci', r'flutter/docs', From 46d184e5905b51940b440ef616a54a5e899542b2 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 22 Aug 2023 09:58:04 -0700 Subject: [PATCH 06/12] Use some suggestions from Zach on clang_tidy resolution. --- tools/clang_tidy/lib/src/options.dart | 19 +++---- tools/githooks/lib/src/pre_push_command.dart | 53 +++++++++++++++----- tools/githooks/pubspec.yaml | 2 +- 3 files changed, 49 insertions(+), 25 deletions(-) diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart index 4b1c1c2d07800..c99944182b3d7 100644 --- a/tools/clang_tidy/lib/src/options.dart +++ b/tools/clang_tidy/lib/src/options.dart @@ -71,21 +71,11 @@ class Options { required List shardCommandsPaths, int? shardId, }) { - io.File? configPath; - if (options.wasParsed('config-file')) { - configPath = io.File(options['config-file'] as String); - if (!configPath.existsSync()) { - return Options._error( - 'ERROR: Config file ${configPath.absolute.path} does not exist.', - errSink: errSink, - ); - } - } return Options( help: options['help'] as bool, verbose: options['verbose'] as bool, buildCommandsPath: buildCommandsPath, - configPath: configPath, + configPath: options.wasParsed('config-file') ? io.File(options['config-file'] as String) : null, checksArg: options.wasParsed('checks') ? options['checks'] as String : '', lintAll: io.Platform.environment['FLUTTER_LINT_ALL'] != null || options['lint-all'] as bool, @@ -291,6 +281,13 @@ class Options { return 'ERROR: --compile-commands option cannot be used with --target-variant.'; } + if (argResults.wasParsed('config-file')) { + final io.File configPath = io.File(argResults['config-file'] as String); + if (!configPath.existsSync()) { + return 'ERROR: Config file ${configPath.path} does not exist.'; + } + } + if (compileCommandsParsed && argResults.wasParsed('src-dir')) { return 'ERROR: --compile-commands option cannot be used with --src-dir.'; } diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index 8ae5e27a10173..cbe9058044786 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -49,9 +49,10 @@ class PrePushCommand extends Command { Future _runClangTidy(String flutterRoot, bool verbose) async { io.stdout.writeln('Starting clang-tidy checks.'); final Stopwatch sw = Stopwatch()..start(); + // First ensure that out/host_{{flags}}/compile_commands.json exists by running // //flutter/tools/gn. See _checkForHostTargets above for supported targets. - final io.File? compileCommands = _findCompileCommands(flutterRoot); + final io.File? compileCommands = _findMostRelevantCompileCommands(flutterRoot, verbose: verbose); if (compileCommands == null) { io.stderr.writeln( 'clang-tidy requires a fully built host directory, such as: ' @@ -59,6 +60,11 @@ class PrePushCommand extends Command { ); return false; } + + // Because we are using a heuristic to pick a host build directory, we + // should print some debug information explaining which directory we picked. + io.stdout.writeln('Using compile_commands.json from ${compileCommands.parent.path}'); + final StringBuffer outBuffer = StringBuffer(); final StringBuffer errBuffer = StringBuffer(); final ClangTidy clangTidy = ClangTidy( @@ -77,20 +83,41 @@ class PrePushCommand extends Command { return true; } - io.File? _findCompileCommands(String flutterRoot) { - for (final String dir in _checkForHostTargets) { - final io.File file = io.File(path.join( - flutterRoot, - '..', - 'out', - dir, - 'compile_commands.json', - )); - if (file.existsSync()) { - return file; + /// Returns the most recent `compile_commands.json` for the given root. + /// + /// For example, if the following builds exist with the following timestamps: + /// + /// ```txt + /// + /// out/host_debug_unopt_arm64/compile_commands.json 1/1/2023 + /// out/host_debug_arm64/compile_commands.json 1/2/2023 + /// out/host_debug_unopt/compile_commands.json 1/3/2023 + /// out/host_debug/compile_commands.json 1/4/2023 + /// ``` + /// + /// ... then the returned file will be `out/host_debug/compile_commands.json`. + io.File? _findMostRelevantCompileCommands(String flutterRoot, {required bool verbose}) { + // Create a list of all the compile_commands.json files that exist, + // including their last modified time. + final List<(io.File, DateTime)> compileCommandsFiles = _checkForHostTargets + .map((String target) => io.File(path.join(flutterRoot, 'out', target, 'compile_commands.json'))) + .where((io.File file) => file.existsSync()) + .map((io.File file) => (file, file.lastModifiedSync())) + .toList(); + + // Sort the list by last modified time, most recent first. + compileCommandsFiles.sort(((io.File, DateTime) a, (io.File, DateTime) b) => b.$2.compareTo(a.$2)); + + // If there are more than one entry, and we're in verbose mode, explain. + if (verbose && compileCommandsFiles.length > 1) { + io.stdout.writeln('Found multiple compile_commands.json files. Using the most recent one.'); + for (final (io.File file, DateTime lastModified) in compileCommandsFiles) { + io.stdout.writeln(' ${file.path} (last modified: $lastModified)'); } } - return null; + + // Return the first file in the list, or null if the list is empty. + return compileCommandsFiles.firstOrNull?.$1; } Future _runFormatter(String flutterRoot, bool verbose) async { diff --git a/tools/githooks/pubspec.yaml b/tools/githooks/pubspec.yaml index 6b5bb5c2340a7..2ea2b17c7520b 100644 --- a/tools/githooks/pubspec.yaml +++ b/tools/githooks/pubspec.yaml @@ -5,7 +5,7 @@ name: githooks publish_to: none environment: - sdk: '>=3.0.0-0 <4.0.0' + sdk: '>=3.0.0 <4.0.0' # Do not add any dependencies that require more than what is provided in # //third_party.pkg, //third_party/dart/pkg, or From 2ffa7b352669110a02e8c88117eaae7008a02e51 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 22 Aug 2023 14:00:54 -0700 Subject: [PATCH 07/12] Address comments, accumulate different kinds of tech debt. --- .clang-tidy | 49 ++++++------ .clang-tidy-for-githooks | 41 ---------- ci/licenses_golden/excluded_files | 1 - tools/clang_tidy/lib/clang_tidy.dart | 14 +++- tools/clang_tidy/lib/src/command.dart | 4 +- tools/clang_tidy/lib/src/hooks_exclude.dart | 40 ++++++++++ tools/clang_tidy/lib/src/options.dart | 10 +++ tools/clang_tidy/test/clang_tidy_test.dart | 38 ++++++++- tools/githooks/lib/src/pre_push_command.dart | 15 ++-- tools/githooks/test/githooks_test.dart | 81 ++++++++++++++++++++ tools/licenses/lib/paths.dart | 1 - 11 files changed, 217 insertions(+), 77 deletions(-) delete mode 100644 .clang-tidy-for-githooks create mode 100644 tools/clang_tidy/lib/src/hooks_exclude.dart diff --git a/.clang-tidy b/.clang-tidy index 9fd40361b53db..0bc769e37ee76 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,29 +1,30 @@ -# Keep in sync with .clang-tidy-for-githooks. -# # These checks are run by the CI presubmit script, but are not run by the # githooks presubmit script. The githooks presubmit script runs a subset of -# these checks (often due to performance reasons, i.e. it taking forever). -Checks: "bugprone-use-after-move,\ -bugprone-unchecked-optional-access,\ -clang-analyzer-*,\ -clang-diagnostic-*,\ -darwin-*,\ -google-*,\ -modernize-use-default-member-init,\ -objc-*,\ --objc-nsinvocation-argument-lifetime,\ -readability-identifier-naming,\ --google-build-using-namespace,\ --google-default-arguments,\ --google-objc-global-variable-declaration,\ --google-objc-avoid-throwing-exception,\ --google-readability-casting,\ --clang-analyzer-nullability.NullPassedToNonnull,\ --clang-analyzer-nullability.NullablePassedToNonnull,\ --clang-analyzer-nullability.NullReturnedFromNonnull,\ --clang-analyzer-nullability.NullableReturnedFromNonnull,\ -performance-move-const-arg,\ -performance-unnecessary-value-param" +# these checks. +# +# See "./tools/clang_tidy/lib/src/hooks_exclude.dart". +Checks: >- + bugprone-use-after-move, + bugprone-unchecked-optional-access, + clang-analyzer-*, + clang-diagnostic-*, + darwin-*, + google-*, + modernize-use-default-member-init, + objc-*, + -objc-nsinvocation-argument-lifetime, + readability-identifier-naming, + -google-build-using-namespace, + -google-default-arguments, + -google-objc-global-variable-declaration, + -google-objc-avoid-throwing-exception, + -google-readability-casting, + -clang-analyzer-nullability.NullPassedToNonnull, + -clang-analyzer-nullability.NullablePassedToNonnull, + -clang-analyzer-nullability.NullReturnedFromNonnull, + -clang-analyzer-nullability.NullableReturnedFromNonnull, + performance-move-const-arg, + performance-unnecessary-value-param CheckOptions: - key: modernize-use-default-member-init.UseAssignment diff --git a/.clang-tidy-for-githooks b/.clang-tidy-for-githooks deleted file mode 100644 index fe3129fdbae2b..0000000000000 --- a/.clang-tidy-for-githooks +++ /dev/null @@ -1,41 +0,0 @@ -# Keep in sync with .clang-tidy. -# -# These checks are run by the git hooks presubmit script, but are not run by the -# CI presubmit script. The CI presubmit script runs a superset of these checks -# (it's OK if CI takes 10s of minutes, but not git push). -Checks: "bugprone-use-after-move,\ -bugprone-unchecked-optional-access,\ -clang-analyzer-*,\ -clang-diagnostic-*,\ -darwin-*,\ -google-*,\ -modernize-use-default-member-init,\ -objc-*,\ --objc-nsinvocation-argument-lifetime,\ -readability-identifier-naming,\ --google-build-using-namespace,\ --google-default-arguments,\ --google-objc-global-variable-declaration,\ --google-objc-avoid-throwing-exception,\ --google-readability-casting,\ --clang-analyzer-nullability.NullPassedToNonnull,\ --clang-analyzer-nullability.NullablePassedToNonnull,\ --clang-analyzer-nullability.NullReturnedFromNonnull,\ --clang-analyzer-nullability.NullableReturnedFromNonnull,\ -performance-move-const-arg" - -CheckOptions: - - key: modernize-use-default-member-init.UseAssignment - value: true - - key: readability-identifier-naming.EnumConstantCase - value: 'CamelCase' - - key: readability-identifier-naming.EnumConstantPrefix - value: 'k' - - key: readability-identifier-naming.GlobalConstantCase - value: 'CamelCase' - - key: readability-identifier-naming.GlobalConstantPrefix - value: 'k' - - key: readability-identifier-naming.PrivateMemberCase - value: 'lower_case' - - key: readability-identifier-naming.PrivateMemberSuffix - value: '_' diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 263ca3e6f301d..91b8b6d2c283a 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -14,7 +14,6 @@ ../../../flutter/.ci.yaml ../../../flutter/.clang-format ../../../flutter/.clang-tidy -../../../flutter/.clang-tidy-for-githooks ../../../flutter/.git ../../../flutter/.gitattributes ../../../flutter/.github diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index 2d85d7390e8a1..fb9e96c55690b 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -11,6 +11,7 @@ import 'package:process_runner/process_runner.dart'; import 'src/command.dart'; import 'src/git_repo.dart'; +import 'src/hooks_exclude.dart'; import 'src/options.dart'; const String _linterOutputHeader = ''' @@ -48,6 +49,7 @@ class ClangTidy { /// /// `buildCommandsPath` is the path to the build_commands.json file. /// `configPath` is a path to a `.clang-tidy` config file. + /// `excludeSlowChecks` when true indicates that slow checks should be skipped. /// `repoPath` is the path to the Engine repo. /// `checksArg` are specific checks for clang-tidy to do. /// `lintAll` when true indicates that all files should be linted. @@ -58,6 +60,7 @@ class ClangTidy { ClangTidy({ required io.File buildCommandsPath, io.File? configPath, + bool excludeSlowChecks = false, String checksArg = '', bool lintAll = false, bool lintHead = false, @@ -68,6 +71,7 @@ class ClangTidy { options = Options( buildCommandsPath: buildCommandsPath, configPath: configPath, + excludeSlowChecks: excludeSlowChecks, checksArg: checksArg, lintAll: lintAll, lintHead: lintHead, @@ -295,6 +299,10 @@ class ClangTidy { List commands, Options options, ) async { + io.File? configPath = options.configPath; + if (configPath != null && options.excludeSlowChecks) { + configPath = _createClangTidyConfigExcludingSlowLints(configPath); + } bool sawMalformed = false; final List jobs = []; for (final Command command in commands) { @@ -314,7 +322,7 @@ class ClangTidy { sawMalformed = true; case LintAction.lint: _outSink.writeln('🔶 linting $relativePath'); - jobs.add(command.createLintJob(options)); + jobs.add(command.createLintJob(options, withPath: configPath)); case LintAction.skipThirdParty: _outSink.writeln('🔷 ignoring $relativePath (third_party)'); case LintAction.skipMissing: @@ -324,6 +332,10 @@ class ClangTidy { return _ComputeJobsResult(jobs, sawMalformed); } + static io.File _createClangTidyConfigExcludingSlowLints(io.File configPath) { + return rewriteClangTidyConfig(configPath); + } + static Iterable _trimGenerator(String output) sync* { const LineSplitter splitter = LineSplitter(); final List lines = splitter.convert(output); diff --git a/tools/clang_tidy/lib/src/command.dart b/tools/clang_tidy/lib/src/command.dart index 9a8636f1fff47..b19c19375b3b4 100644 --- a/tools/clang_tidy/lib/src/command.dart +++ b/tools/clang_tidy/lib/src/command.dart @@ -131,12 +131,12 @@ class Command { } /// The job for the process runner for the lint needed for this command. - WorkerJob createLintJob(Options options) { + WorkerJob createLintJob(Options options, {io.File? withPath}) { final List args = [ filePath, '--warnings-as-errors=${options.warningsAsErrors ?? '*'}', if (options.configPath != null) - '--config-file=${options.configPath}', + '--config-file=${withPath != null ? withPath.path : options.configPath}', if (options.checks != null) options.checks!, if (options.fix) ...[ diff --git a/tools/clang_tidy/lib/src/hooks_exclude.dart b/tools/clang_tidy/lib/src/hooks_exclude.dart new file mode 100644 index 0000000000000..dfd46a6ff1303 --- /dev/null +++ b/tools/clang_tidy/lib/src/hooks_exclude.dart @@ -0,0 +1,40 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io show Directory, File; + +import 'package:path/path.dart' as path; + +// TODO(matanl): Replace this file by embedding in //.clang-tidy directly. +// +// By bringing in package:yaml, we can instead embed this in a key in the +// .clang-tidy file, read the checks in, and create a new .clang-tidy file (i.e. +// in /tmp/.../.clang-tidy) with the checks we want to run. +// +// However that requires a bit more work, so for now we just have this file. +const List _kExcludeChecks = [ + 'performance-unnecessary-value-param', +]; + +/// Given a `.clang-tidy` file, rewrites it to exclude non-performant checks. +/// +/// Returns a path to the rewritten file. +io.File rewriteClangTidyConfig(io.File input) { + // Because the file is YAML, and we aren't using a YAML package to parse it, + // instead we'll carefully remove the name of the check, optionally followed + // by a comma and a newline. + String contents = input.readAsStringSync(); + + for (final String check in _kExcludeChecks) { + // \s+{{CHECK}},?\n, with {{CHECK}} escaped for regex. + final RegExp checkRegex = RegExp(r'\s+' + check + r',?\n'); + contents = contents.replaceAll(checkRegex, ''); + } + + final io.Directory tmpDir = io.Directory.systemTemp.createTempSync('clang_tidy'); + final io.File output = io.File(path.join(tmpDir.path, '.clang-tidy-for-githooks')); + output.writeAsStringSync(contents); + + return output; +} diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart index c99944182b3d7..02f3c2e6888e8 100644 --- a/tools/clang_tidy/lib/src/options.dart +++ b/tools/clang_tidy/lib/src/options.dart @@ -29,6 +29,7 @@ class Options { this.help = false, this.verbose = false, this.configPath, + this.excludeSlowChecks = false, this.checksArg = '', this.lintAll = false, this.lintHead = false, @@ -76,6 +77,7 @@ class Options { verbose: options['verbose'] as bool, buildCommandsPath: buildCommandsPath, configPath: options.wasParsed('config-file') ? io.File(options['config-file'] as String) : null, + excludeSlowChecks: options['exclude-slow-checks'] as bool, checksArg: options.wasParsed('checks') ? options['checks'] as String : '', lintAll: io.Platform.environment['FLUTTER_LINT_ALL'] != null || options['lint-all'] as bool, @@ -204,6 +206,11 @@ class Options { help: 'Path to a .clang-tidy configuration file.', valueHelp: 'path/to/.clang-tidy', ) + ..addFlag( + 'exclude-slow-checks', + help: 'Exclude checks that are too slow for git hooks.', + negatable: false, + ) ..addFlag( 'enable-check-profile', help: 'Enable per-check timing profiles and print a report to stderr.', @@ -222,6 +229,9 @@ class Options { /// A location of a `.clang-tidy` configuration file. final io.File? configPath; + /// Whether to exclude lints that are too slow for git hooks. + final bool excludeSlowChecks; + /// The location of shard compile_commands.json files. final List shardCommandsPaths; diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 196166e418262..9a24cccd93dc5 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -6,6 +6,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/hooks_exclude.dart'; import 'package:clang_tidy/src/options.dart'; import 'package:litetest/litetest.dart'; import 'package:path/path.dart' as path; @@ -173,7 +174,7 @@ Future main(List args) async { [ '--compile-commands', buildCommands, - '--config-file=${path.join(flutterRoot, '.clang-tidy-for-githooks')}', + '--config-file=${path.join(flutterRoot, '.clang-tidy')}', ], outSink: outBuffer, errSink: errBuffer, @@ -182,7 +183,7 @@ Future main(List args) async { final int result = await clangTidy.run(); expect(result, equals(0)); - expect(clangTidy.options.configPath?.path, endsWith('.clang-tidy-for-githooks')); + expect(clangTidy.options.configPath?.path, endsWith('.clang-tidy')); print(outBuffer); }); @@ -516,5 +517,38 @@ Future main(List args) async { expect(lintAction, equals(LintAction.lint)); }); + test('rewriteClangTidyConfig should remove "performance-unnecessary-value-param"', () { + final io.Directory tmpDir = io.Directory.systemTemp.createTempSync('clang_tidy_test'); + + final io.File input = io.File(path.join(tmpDir.path, '.clang-tidy')); + input.writeAsStringSync(''' +Checks: >- + bugprone-use-after-move, + bugprone-unchecked-optional-access, + clang-analyzer-*, + clang-diagnostic-*, + darwin-*, + google-*, + modernize-use-default-member-init, + objc-*, + -objc-nsinvocation-argument-lifetime, + readability-identifier-naming, + -google-build-using-namespace, + -google-default-arguments, + -google-objc-global-variable-declaration, + -google-objc-avoid-throwing-exception, + -google-readability-casting, + -clang-analyzer-nullability.NullPassedToNonnull, + -clang-analyzer-nullability.NullablePassedToNonnull, + -clang-analyzer-nullability.NullReturnedFromNonnull, + -clang-analyzer-nullability.NullableReturnedFromNonnull, + performance-move-const-arg, + performance-unnecessary-value-param + '''); + + final io.File output = rewriteClangTidyConfig(input); + expect(output.readAsStringSync().contains('performance-unnecessary-value-param'), isFalse); + }); + return 0; } diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index cbe9058044786..04924a30fc32d 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -6,6 +6,7 @@ import 'dart:io' as io; import 'package:args/command_runner.dart'; import 'package:clang_tidy/clang_tidy.dart'; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; /// The command that implements the pre-push githook @@ -39,7 +40,9 @@ class PrePushCommand extends Command { return !checkResults.contains(false); } - static const List _checkForHostTargets = [ + /// Different `host_xxx` targets are built depending on the host platform. + @visibleForTesting + static const List supportedHostTargets = [ 'host_debug_unopt_arm64', 'host_debug_arm64', 'host_debug_unopt', @@ -52,11 +55,11 @@ class PrePushCommand extends Command { // First ensure that out/host_{{flags}}/compile_commands.json exists by running // //flutter/tools/gn. See _checkForHostTargets above for supported targets. - final io.File? compileCommands = _findMostRelevantCompileCommands(flutterRoot, verbose: verbose); + final io.File? compileCommands = findMostRelevantCompileCommands(flutterRoot, verbose: verbose); if (compileCommands == null) { io.stderr.writeln( 'clang-tidy requires a fully built host directory, such as: ' - '${_checkForHostTargets.join(', ')}.' + '${supportedHostTargets.join(', ')}.' ); return false; } @@ -70,6 +73,7 @@ class PrePushCommand extends Command { final ClangTidy clangTidy = ClangTidy( buildCommandsPath: compileCommands, configPath: io.File(path.join(flutterRoot, '.clang-tidy-for-githooks')), + excludeSlowChecks: true, outSink: outBuffer, errSink: errBuffer, ); @@ -96,10 +100,11 @@ class PrePushCommand extends Command { /// ``` /// /// ... then the returned file will be `out/host_debug/compile_commands.json`. - io.File? _findMostRelevantCompileCommands(String flutterRoot, {required bool verbose}) { + @visibleForTesting + static io.File? findMostRelevantCompileCommands(String flutterRoot, {required bool verbose}) { // Create a list of all the compile_commands.json files that exist, // including their last modified time. - final List<(io.File, DateTime)> compileCommandsFiles = _checkForHostTargets + final List<(io.File, DateTime)> compileCommandsFiles = supportedHostTargets .map((String target) => io.File(path.join(flutterRoot, 'out', target, 'compile_commands.json'))) .where((io.File file) => file.existsSync()) .map((io.File file) => (file, file.lastModifiedSync())) diff --git a/tools/githooks/test/githooks_test.dart b/tools/githooks/test/githooks_test.dart index 2b347d31dd20f..5e1014690da6b 100644 --- a/tools/githooks/test/githooks_test.dart +++ b/tools/githooks/test/githooks_test.dart @@ -5,7 +5,9 @@ import 'dart:io' as io; import 'package:githooks/githooks.dart'; +import 'package:githooks/src/pre_push_command.dart'; import 'package:litetest/litetest.dart'; +import 'package:path/path.dart' as path; void main() { test('Fails gracefully without a command', () async { @@ -66,4 +68,83 @@ void main() { } expect(result, equals(1)); }); + + group('findMostRelevantCompileCommands', () { + late io.Directory fakeFlutterRoot; + + // We can't use standard setUp because this package uses 'litetest'. + void setUp() { + fakeFlutterRoot = io.Directory.systemTemp.createTempSync('flutter_tools_githooks_test'); + } + + void createHostFor(String target, {DateTime? lastModified}) { + final io.Directory host = io.Directory(path.join(fakeFlutterRoot.path, 'out', target)); + host.createSync(recursive: true); + + final io.File compileCommands = io.File(path.join(host.path, 'compile_commands.json')); + compileCommands.createSync(); + if (lastModified != null) { + compileCommands.setLastModifiedSync(lastModified); + } + } + + test('returns null if there are no built outputs', () { + setUp(); + + expect( + PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false), + isNull, + ); + }); + + test('returns the most recently modified compile_commands.json', () { + setUp(); + + // Assume host_debug_unopt was created on 8/5, and then *_arm64 on 8/6. + createHostFor('host_debug_unopt', lastModified: DateTime(2023, 8, 5)); + createHostFor('host_debug_unopt_arm64', lastModified: DateTime(2023, 8, 6)); + + expect( + PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false)!.path, + equals(path.join(fakeFlutterRoot.path, 'out', 'host_debug_unopt_arm64', 'compile_commands.json')), + ); + }); + + test('in verbose mode, if there are multiple outputs, prints all of them', () { + final StringBuffer outBuffer = StringBuffer(); + io.IOOverrides.runZoned(() { + setUp(); + + createHostFor('host_debug_unopt', lastModified: DateTime(2023, 8, 5)); + createHostFor('host_debug_unopt_arm64', lastModified: DateTime(2023, 8, 6)); + PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: true); + }, stdout: () => _BufferedStdOut(outBuffer)); + + final String outString = outBuffer.toString(); + expect(outString, contains('out/host_debug_unopt/compile_commands.json')); + expect(outString, contains('out/host_debug_unopt_arm64/compile_commands.json')); + }); + }); +} + +final class _BufferedStdOut implements io.Stdout { + _BufferedStdOut(this.buffer); + + final StringBuffer buffer; + + // We don't need to implement any other methods. + @override + dynamic noSuchMethod(Invocation invocation) { + return super.noSuchMethod(invocation); + } + + @override + void write(Object? obj) { + buffer.write(obj); + } + + @override + void writeln([Object? obj = '']) { + buffer.writeln(obj); + } } diff --git a/tools/licenses/lib/paths.dart b/tools/licenses/lib/paths.dart index 00b5ba4f6953e..93ab31f5ffb8a 100644 --- a/tools/licenses/lib/paths.dart +++ b/tools/licenses/lib/paths.dart @@ -20,7 +20,6 @@ final Set skippedPaths = { r'build', // only used by build r'build_overrides', // only used by build r'buildtools', // only used by build - r'flutter/.clang-tidy-for-githooks', // nearly identical to .clang-tidy r'flutter/build', r'flutter/ci', r'flutter/docs', From 30c4ee836fb30842030ad783785de0c35bd84c86 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 22 Aug 2023 14:02:20 -0700 Subject: [PATCH 08/12] Format. --- tools/clang_tidy/lib/src/hooks_exclude.dart | 4 ++-- tools/githooks/lib/src/pre_push_command.dart | 8 ++++---- tools/githooks/test/githooks_test.dart | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/clang_tidy/lib/src/hooks_exclude.dart b/tools/clang_tidy/lib/src/hooks_exclude.dart index dfd46a6ff1303..b0c9f8d28c89a 100644 --- a/tools/clang_tidy/lib/src/hooks_exclude.dart +++ b/tools/clang_tidy/lib/src/hooks_exclude.dart @@ -18,7 +18,7 @@ const List _kExcludeChecks = [ ]; /// Given a `.clang-tidy` file, rewrites it to exclude non-performant checks. -/// +/// /// Returns a path to the rewritten file. io.File rewriteClangTidyConfig(io.File input) { // Because the file is YAML, and we aren't using a YAML package to parse it, @@ -36,5 +36,5 @@ io.File rewriteClangTidyConfig(io.File input) { final io.File output = io.File(path.join(tmpDir.path, '.clang-tidy-for-githooks')); output.writeAsStringSync(contents); - return output; + return output; } diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index 04924a30fc32d..2369c482c5d47 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -88,9 +88,9 @@ class PrePushCommand extends Command { } /// Returns the most recent `compile_commands.json` for the given root. - /// + /// /// For example, if the following builds exist with the following timestamps: - /// + /// /// ```txt /// /// out/host_debug_unopt_arm64/compile_commands.json 1/1/2023 @@ -98,7 +98,7 @@ class PrePushCommand extends Command { /// out/host_debug_unopt/compile_commands.json 1/3/2023 /// out/host_debug/compile_commands.json 1/4/2023 /// ``` - /// + /// /// ... then the returned file will be `out/host_debug/compile_commands.json`. @visibleForTesting static io.File? findMostRelevantCompileCommands(String flutterRoot, {required bool verbose}) { @@ -109,7 +109,7 @@ class PrePushCommand extends Command { .where((io.File file) => file.existsSync()) .map((io.File file) => (file, file.lastModifiedSync())) .toList(); - + // Sort the list by last modified time, most recent first. compileCommandsFiles.sort(((io.File, DateTime) a, (io.File, DateTime) b) => b.$2.compareTo(a.$2)); diff --git a/tools/githooks/test/githooks_test.dart b/tools/githooks/test/githooks_test.dart index 5e1014690da6b..9087db0816e0a 100644 --- a/tools/githooks/test/githooks_test.dart +++ b/tools/githooks/test/githooks_test.dart @@ -92,7 +92,7 @@ void main() { setUp(); expect( - PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false), + PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false), isNull, ); }); @@ -105,7 +105,7 @@ void main() { createHostFor('host_debug_unopt_arm64', lastModified: DateTime(2023, 8, 6)); expect( - PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false)!.path, + PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false)!.path, equals(path.join(fakeFlutterRoot.path, 'out', 'host_debug_unopt_arm64', 'compile_commands.json')), ); }); From 5c3c5afbd6a08bf27ef8d79a08d6093acc9192f5 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 22 Aug 2023 14:02:45 -0700 Subject: [PATCH 09/12] Remove whitespace. --- tools/githooks/lib/src/pre_push_command.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index 2369c482c5d47..1f4559086d425 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -52,7 +52,7 @@ class PrePushCommand extends Command { Future _runClangTidy(String flutterRoot, bool verbose) async { io.stdout.writeln('Starting clang-tidy checks.'); final Stopwatch sw = Stopwatch()..start(); - + // First ensure that out/host_{{flags}}/compile_commands.json exists by running // //flutter/tools/gn. See _checkForHostTargets above for supported targets. final io.File? compileCommands = findMostRelevantCompileCommands(flutterRoot, verbose: verbose); From 70e2a25e68c6d2157b9ac89df495fc30b5703a0f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 22 Aug 2023 14:09:38 -0700 Subject: [PATCH 10/12] Fix resolution. --- tools/githooks/lib/src/pre_push_command.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/githooks/lib/src/pre_push_command.dart b/tools/githooks/lib/src/pre_push_command.dart index 1f4559086d425..166c91bb77b8b 100644 --- a/tools/githooks/lib/src/pre_push_command.dart +++ b/tools/githooks/lib/src/pre_push_command.dart @@ -102,10 +102,12 @@ class PrePushCommand extends Command { /// ... then the returned file will be `out/host_debug/compile_commands.json`. @visibleForTesting static io.File? findMostRelevantCompileCommands(String flutterRoot, {required bool verbose}) { + final String engineRoot = path.normalize(path.join(flutterRoot, '../')); + // Create a list of all the compile_commands.json files that exist, // including their last modified time. final List<(io.File, DateTime)> compileCommandsFiles = supportedHostTargets - .map((String target) => io.File(path.join(flutterRoot, 'out', target, 'compile_commands.json'))) + .map((String target) => io.File(path.join(engineRoot, 'out', target, 'compile_commands.json'))) .where((io.File file) => file.existsSync()) .map((io.File file) => (file, file.lastModifiedSync())) .toList(); From 2becd7208bb676689438ce327ba11c8172d3336f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 22 Aug 2023 14:31:15 -0700 Subject: [PATCH 11/12] Delete temp directory. --- tools/clang_tidy/lib/clang_tidy.dart | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index fb9e96c55690b..5dac58af7629f 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -3,7 +3,7 @@ // found in the LICENSE file. import 'dart:convert' show LineSplitter, jsonDecode; -import 'dart:io' as io show File, stderr, stdout; +import 'dart:io' as io show Directory, File, stderr, stdout; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; @@ -161,10 +161,20 @@ class ClangTidy { ); } + io.File? configPath = options.configPath; + io.Directory? rewriteDir; + if (configPath != null && options.excludeSlowChecks) { + configPath = _createClangTidyConfigExcludingSlowLints(configPath); + rewriteDir = io.Directory(path.dirname(configPath.path)); + } final _ComputeJobsResult computeJobsResult = await _computeJobs( changedFileBuildCommands, options, + configPath, ); + if (rewriteDir != null) { + rewriteDir.deleteSync(recursive: true); + } final int computeResult = computeJobsResult.sawMalformed ? 1 : 0; final List jobs = computeJobsResult.jobs; @@ -298,11 +308,8 @@ class ClangTidy { Future<_ComputeJobsResult> _computeJobs( List commands, Options options, + io.File? configPath, ) async { - io.File? configPath = options.configPath; - if (configPath != null && options.excludeSlowChecks) { - configPath = _createClangTidyConfigExcludingSlowLints(configPath); - } bool sawMalformed = false; final List jobs = []; for (final Command command in commands) { From bd3e24ca1dfd710024b35e6c8559dd25b8147c6c Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 22 Aug 2023 17:26:24 -0700 Subject: [PATCH 12/12] Fix test. --- tools/githooks/test/githooks_test.dart | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/githooks/test/githooks_test.dart b/tools/githooks/test/githooks_test.dart index 9087db0816e0a..87d75219720d1 100644 --- a/tools/githooks/test/githooks_test.dart +++ b/tools/githooks/test/githooks_test.dart @@ -70,15 +70,18 @@ void main() { }); group('findMostRelevantCompileCommands', () { + late io.Directory fakeEngineRoot; late io.Directory fakeFlutterRoot; // We can't use standard setUp because this package uses 'litetest'. void setUp() { - fakeFlutterRoot = io.Directory.systemTemp.createTempSync('flutter_tools_githooks_test'); + fakeEngineRoot = io.Directory.systemTemp.createTempSync('flutter_tools_githooks_test'); + fakeFlutterRoot = io.Directory(path.join(fakeEngineRoot.path, 'flutter')); + fakeFlutterRoot.createSync(recursive: true); } void createHostFor(String target, {DateTime? lastModified}) { - final io.Directory host = io.Directory(path.join(fakeFlutterRoot.path, 'out', target)); + final io.Directory host = io.Directory(path.join(fakeEngineRoot.path, 'out', target)); host.createSync(recursive: true); final io.File compileCommands = io.File(path.join(host.path, 'compile_commands.json')); @@ -106,7 +109,7 @@ void main() { expect( PrePushCommand.findMostRelevantCompileCommands(fakeFlutterRoot.path, verbose: false)!.path, - equals(path.join(fakeFlutterRoot.path, 'out', 'host_debug_unopt_arm64', 'compile_commands.json')), + equals(path.join(fakeEngineRoot.path, 'out', 'host_debug_unopt_arm64', 'compile_commands.json')), ); });