diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index 6625c35278177..4a49f9b591f96 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -29,6 +29,17 @@ class _ComputeJobsResult { final bool sawMalformed; } +enum _SetStatus { + Intersection, + Difference, +} + +class _SetStatusCommand { + _SetStatusCommand(this.setStatus, this.command); + final _SetStatus setStatus; + final Command command; +} + /// A class that runs clang-tidy on all or only the changed files in a git /// repo. class ClangTidy { @@ -92,14 +103,14 @@ class ClangTidy { _outSink.writeln(_linterOutputHeader); - final List changedFiles = await computeChangedFiles(); + final List filesOfInterest = await computeFilesOfInterest(); if (options.verbose) { _outSink.writeln('Checking lint in repo at ${options.repoPath.path}.'); if (options.checksArg.isNotEmpty) { _outSink.writeln('Checking for specific checks: ${options.checks}.'); } - final int changedFilesCount = changedFiles.length; + final int changedFilesCount = filesOfInterest.length; if (options.lintAll) { _outSink.writeln('Checking all $changedFilesCount files the repo dir.'); } else { @@ -109,12 +120,19 @@ class ClangTidy { } } - final List buildCommandsData = jsonDecode( + final List buildCommandsData = jsonDecode( options.buildCommandsPath.readAsStringSync(), - ) as List; - final List changedFileBuildCommands = await getLintCommandsForChangedFiles( + ) as List; + final List> shardBuildCommandsData = options + .shardCommandsPaths + .map((io.File file) => + jsonDecode(file.readAsStringSync()) as List) + .toList(); + final List changedFileBuildCommands = await getLintCommandsForFiles( buildCommandsData, - changedFiles, + filesOfInterest, + shardBuildCommandsData, + options.shardId, ); if (changedFileBuildCommands.isEmpty) { @@ -153,7 +171,7 @@ class ClangTidy { /// The files with local modifications or all the files if `lintAll` was /// specified. @visibleForTesting - Future> computeChangedFiles() async { + Future> computeFilesOfInterest() async { if (options.lintAll) { return options.repoPath .listSync(recursive: true) @@ -171,23 +189,99 @@ class ClangTidy { return repo.changedFiles; } - /// Given a build commands json file, and the files with local changes, - /// compute the lint commands to run. + /// Returns f(n) = value(n * [shardCount] + [id]). + Iterable _takeShard(Iterable values, int id, int shardCount) sync* { + int count = 0; + for (final T val in values) { + if (count % shardCount == id) { + yield val; + } + count++; + } + } + + /// This returns a `_SetStatusCommand` for each [Command] in [items]. + /// `Intersection` if the Command shows up in [items] and its filePath in all + /// [filePathSets], otherwise `Difference`. + Iterable<_SetStatusCommand> _calcIntersection( + Iterable items, Iterable> filePathSets) sync* { + bool allSetsContain(Command command) { + for (final Set filePathSet in filePathSets) { + if (!filePathSet.contains(command.filePath)) { + return false; + } + } + return true; + } + for (final Command command in items) { + if (allSetsContain(command)) { + yield _SetStatusCommand(_SetStatus.Intersection, command); + } else { + yield _SetStatusCommand(_SetStatus.Difference, command); + } + } + } + + /// Given a build commands json file's contents in [buildCommandsData], and + /// the [files] with local changes, compute the lint commands to run. If + /// build commands are supplied in [sharedBuildCommandsData] the intersection + /// of those build commands will be calculated and distributed across + /// instances via the [shardId]. @visibleForTesting - Future> getLintCommandsForChangedFiles( - List buildCommandsData, - List changedFiles, - ) async { - final List buildCommands = []; - for (final dynamic data in buildCommandsData) { - final Command command = Command.fromMap(data as Map); - final LintAction lintAction = await command.lintAction; - // Short-circuit the expensive containsAny call for the many third_party files. - if (lintAction != LintAction.skipThirdParty && command.containsAny(changedFiles)) { - buildCommands.add(command); + Future> getLintCommandsForFiles( + List buildCommandsData, + List files, + List> sharedBuildCommandsData, + int? shardId, + ) { + final List totalCommands = []; + if (sharedBuildCommandsData.isNotEmpty) { + final List buildCommands = [ + for (Object? data in buildCommandsData) + Command.fromMap((data as Map?)!) + ]; + final List> shardFilePaths = >[ + for (List list in sharedBuildCommandsData) + { + for (Object? data in list) + Command.fromMap((data as Map?)!).filePath + } + ]; + final Iterable<_SetStatusCommand> intersectionResults = + _calcIntersection(buildCommands, shardFilePaths); + for (final _SetStatusCommand result in intersectionResults) { + if (result.setStatus == _SetStatus.Difference) { + totalCommands.add(result.command); + } } + final List intersection = [ + for (_SetStatusCommand result in intersectionResults) + if (result.setStatus == _SetStatus.Intersection) result.command + ]; + // Make sure to sort results so the sharding scheme is guaranteed to work + // since we are not sure if there is a defined order in the json file. + intersection + .sort((Command x, Command y) => x.filePath.compareTo(y.filePath)); + totalCommands.addAll( + _takeShard(intersection, shardId!, 1 + sharedBuildCommandsData.length)); + } else { + totalCommands.addAll([ + for (Object? data in buildCommandsData) + Command.fromMap((data as Map?)!) + ]); } - return buildCommands; + return () async { + final List result = []; + for (final Command command in totalCommands) { + final LintAction lintAction = await command.lintAction; + // Short-circuit the expensive containsAny call for the many third_party files. + if (lintAction != LintAction.skipThirdParty && + command.containsAny(files)) { + result.add(command); + } + } + return result; + }(); } Future<_ComputeJobsResult> _computeJobs( diff --git a/tools/clang_tidy/lib/src/options.dart b/tools/clang_tidy/lib/src/options.dart index e10ca99b66ad1..1d262e3bce635 100644 --- a/tools/clang_tidy/lib/src/options.dart +++ b/tools/clang_tidy/lib/src/options.dart @@ -34,6 +34,8 @@ class Options { this.fix = false, this.errorMessage, this.warningsAsErrors, + this.shardId, + this.shardCommandsPaths = const [], StringSink? errSink, }) : checks = checksArg.isNotEmpty ? '--checks=$checksArg' : null, _errSink = errSink ?? io.stderr; @@ -64,6 +66,8 @@ class Options { ArgResults options, { required io.File buildCommandsPath, StringSink? errSink, + required List shardCommandsPaths, + int? shardId, }) { return Options( help: options['help'] as bool, @@ -76,6 +80,8 @@ class Options { fix: options['fix'] as bool, errSink: errSink, warningsAsErrors: _platformSpecificWarningsAsErrors(options), + shardCommandsPaths: shardCommandsPaths, + shardId: shardId, ); } @@ -87,14 +93,24 @@ class Options { final ArgResults argResults = _argParser.parse(arguments); String? buildCommandsPath = argResults['compile-commands'] as String?; + + String variantToBuildCommandsFilePath(String variant) => + path.join( + argResults['src-dir'] as String, + 'out', + variant, + 'compile_commands.json', + ); // 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', - ); + buildCommandsPath ??= variantToBuildCommandsFilePath(argResults['target-variant'] as String); final io.File buildCommands = io.File(buildCommandsPath); + final List shardCommands = + (argResults['shard-variants'] as String? ?? '') + .split(',') + .where((String element) => element.isNotEmpty) + .map((String variant) => + io.File(variantToBuildCommandsFilePath(variant))) + .toList(); final String? message = _checkArguments(argResults, buildCommands); if (message != null) { return Options._error(message, errSink: errSink); @@ -102,10 +118,17 @@ class Options { if (argResults['help'] as bool) { return Options._help(errSink: errSink); } + final String? shardIdString = argResults['shard-id'] as String?; + final int? shardId = shardIdString == null ? null : int.parse(shardIdString); + if (shardId != null && (shardId >= shardCommands.length || shardId < 0)) { + return Options._error('Invalid shard-id value: $shardId.', errSink: errSink); + } return Options._fromArgResults( argResults, buildCommandsPath: buildCommands, errSink: errSink, + shardCommandsPaths: shardCommands, + shardId: shardId, ); } @@ -132,6 +155,17 @@ class Options { '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 ' @@ -171,6 +205,12 @@ class Options { /// The location of the compile_commands.json file. final io.File buildCommandsPath; + /// The location of shard compile_commands.json files. + final List shardCommandsPaths; + + /// The identifier of the shard. + final int? shardId; + /// The root of the flutter/engine repository. final io.Directory repoPath = io.Directory(_engineRoot); @@ -233,6 +273,10 @@ class Options { return "ERROR: Build commands path ${buildCommandsPath.absolute.path} doesn't exist."; } + if (argResults.wasParsed('shard-variants') && !argResults.wasParsed('shard-id')) { + return 'ERROR: a `shard-id` must be specified with `shard-variants`.'; + } + return null; } } diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 06301f398e0cc..9d6738e52973c 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -193,7 +193,7 @@ Future main(List args) async { outSink: outBuffer, errSink: errBuffer, ); - final List fileList = await clangTidy.computeChangedFiles(); + final List fileList = await clangTidy.computeFilesOfInterest(); expect(fileList.length, greaterThan(1000)); }); @@ -205,10 +205,75 @@ Future main(List args) async { outSink: outBuffer, errSink: errBuffer, ); - final List fileList = await clangTidy.computeChangedFiles(); + final List fileList = await clangTidy.computeFilesOfInterest(); expect(fileList.length, lessThan(300)); }); + test('Sharding', () async { + final StringBuffer outBuffer = StringBuffer(); + final StringBuffer errBuffer = StringBuffer(); + final ClangTidy clangTidy = ClangTidy( + buildCommandsPath: io.File(buildCommands), + lintAll: true, + outSink: outBuffer, + errSink: errBuffer, + ); + Map makeBuildCommandEntry(String filePath) => { + 'directory': '/unused', + 'command': '../../buildtools/mac-x64/clang/bin/clang $filePath', + 'file': filePath, + }; + final List filePaths = [ + for (int i = 0; i < 10; ++i) '/path/to/a/source_file_$i.cc' + ]; + final List buildCommandsData = + filePaths.map((String e) => makeBuildCommandEntry(e)).toList(); + final List shardBuildCommandsData = + filePaths.sublist(6).map((String e) => makeBuildCommandEntry(e)).toList(); + + { + final List commands = await clangTidy.getLintCommandsForFiles( + buildCommandsData, + filePaths.map((String e) => io.File(e)).toList(), + >[shardBuildCommandsData], + 0, + ); + final Iterable commandFilePaths = commands.map((Command e) => e.filePath); + expect(commands.length, equals(8)); + expect(commandFilePaths.contains('/path/to/a/source_file_0.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_1.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_2.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_3.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_4.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_5.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_6.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_7.cc'), false); + expect(commandFilePaths.contains('/path/to/a/source_file_8.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_9.cc'), false); + } + { + final List commands = await clangTidy.getLintCommandsForFiles( + buildCommandsData, + filePaths.map((String e) => io.File(e)).toList(), + >[shardBuildCommandsData], + 1, + ); + + final Iterable commandFilePaths = commands.map((Command e) => e.filePath); + expect(commands.length, equals(8)); + expect(commandFilePaths.contains('/path/to/a/source_file_0.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_1.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_2.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_3.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_4.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_5.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_6.cc'), false); + expect(commandFilePaths.contains('/path/to/a/source_file_7.cc'), true); + expect(commandFilePaths.contains('/path/to/a/source_file_8.cc'), false); + expect(commandFilePaths.contains('/path/to/a/source_file_9.cc'), true); + } + }); + test('No Commands are produced when no files changed', () async { final StringBuffer outBuffer = StringBuffer(); final StringBuffer errBuffer = StringBuffer(); @@ -226,9 +291,11 @@ Future main(List args) async { 'file': filePath, }, ]; - final List commands = await clangTidy.getLintCommandsForChangedFiles( + final List commands = await clangTidy.getLintCommandsForFiles( buildCommandsData, [], + >[], + null, ); expect(commands, isEmpty); @@ -253,9 +320,11 @@ Future main(List args) async { 'file': filePath, }, ]; - final List commands = await clangTidy.getLintCommandsForChangedFiles( + final List commands = await clangTidy.getLintCommandsForFiles( buildCommandsData, [io.File(filePath)], + >[], + null, ); expect(commands, isNotEmpty);