diff --git a/script/tool/lib/src/format_command.dart b/script/tool/lib/src/format_command.dart index 5f060d715b..9d39d93b91 100644 --- a/script/tool/lib/src/format_command.dart +++ b/script/tool/lib/src/format_command.dart @@ -14,6 +14,11 @@ import 'common/core.dart'; import 'common/plugin_command.dart'; import 'common/process_runner.dart'; +const int _exitClangFormatFailed = 3; +const int _exitFlutterFormatFailed = 4; +const int _exitJavaFormatFailed = 5; +const int _exitGitFailed = 6; + final Uri _googleFormatterUrl = Uri.https('github.com', '/google/google-java-format/releases/download/google-java-format-1.3/google-java-format-1.3-all-deps.jar'); @@ -43,14 +48,18 @@ class FormatCommand extends PluginCommand { Future run() async { final String googleFormatterPath = await _getGoogleFormatterPath(); - await _formatDart(); - await _formatJava(googleFormatterPath); - await _formatCppAndObjectiveC(); + // This class is not based on PackageLoopingCommand because running the + // formatters separately for each package is an order of magnitude slower, + // due to the startup overhead of the formatters. + final Iterable files = await _getFilteredFilePaths(getFiles()); + await _formatDart(files); + await _formatJava(files, googleFormatterPath); + await _formatCppAndObjectiveC(files); if (getBoolArg('fail-on-change')) { final bool modified = await _didModifyAnything(); if (modified) { - throw ToolExit(1); + throw ToolExit(exitCommandFoundErrors); } } } @@ -60,9 +69,12 @@ class FormatCommand extends PluginCommand { 'git', ['ls-files', '--modified'], workingDir: packagesDir, - exitOnError: true, logOnError: true, ); + if (modifiedFiles.exitCode != 0) { + printError('Unable to determine changed files.'); + throw ToolExit(_exitGitFailed); + } print('\n\n'); @@ -79,66 +91,105 @@ class FormatCommand extends PluginCommand { 'pub global run flutter_plugin_tools format" or copy-paste ' 'this command into your terminal:'); - print('patch -p1 <['diff'], workingDir: packagesDir, - exitOnError: true, logOnError: true, ); + if (diff.exitCode != 0) { + printError('Unable to determine diff.'); + throw ToolExit(_exitGitFailed); + } + print('patch -p1 < _formatCppAndObjectiveC() async { - print('Formatting all .cc, .cpp, .mm, .m, and .h files...'); - final Iterable allFiles = [ - ...await _getFilesWithExtension('.h'), - ...await _getFilesWithExtension('.m'), - ...await _getFilesWithExtension('.mm'), - ...await _getFilesWithExtension('.cc'), - ...await _getFilesWithExtension('.cpp'), - ]; - // Split this into multiple invocations to avoid a - // 'ProcessException: Argument list too long'. - final Iterable> batches = partition(allFiles, 100); - for (final List batch in batches) { - await processRunner.runAndStream(getStringArg('clang-format'), - ['-i', '--style=Google', ...batch], - workingDir: packagesDir, exitOnError: true); + Future _formatCppAndObjectiveC(Iterable files) async { + final Iterable clangFiles = _getPathsWithExtensions( + files, {'.h', '.m', '.mm', '.cc', '.cpp'}); + if (clangFiles.isNotEmpty) { + print('Formatting .cc, .cpp, .h, .m, and .mm files...'); + final Iterable> batches = partition(clangFiles, 100); + int exitCode = 0; + for (final List batch in batches) { + batch.sort(); // For ease of testing; partition changes the order. + exitCode = await processRunner.runAndStream( + getStringArg('clang-format'), + ['-i', '--style=Google', ...batch], + workingDir: packagesDir); + if (exitCode != 0) { + break; + } + } + if (exitCode != 0) { + printError( + 'Failed to format C, C++, and Objective-C files: exit code $exitCode.'); + throw ToolExit(_exitClangFormatFailed); + } } } - Future _formatJava(String googleFormatterPath) async { - print('Formatting all .java files...'); - final Iterable javaFiles = await _getFilesWithExtension('.java'); - await processRunner.runAndStream('java', - ['-jar', googleFormatterPath, '--replace', ...javaFiles], - workingDir: packagesDir, exitOnError: true); + Future _formatJava( + Iterable files, String googleFormatterPath) async { + final Iterable javaFiles = + _getPathsWithExtensions(files, {'.java'}); + if (javaFiles.isNotEmpty) { + print('Formatting .java files...'); + final int exitCode = await processRunner.runAndStream('java', + ['-jar', googleFormatterPath, '--replace', ...javaFiles], + workingDir: packagesDir); + if (exitCode != 0) { + printError('Failed to format Java files: exit code $exitCode.'); + throw ToolExit(_exitJavaFormatFailed); + } + } } - Future _formatDart() async { - // This actually should be fine for non-Flutter Dart projects, no need to - // specifically shell out to dartfmt -w in that case. - print('Formatting all .dart files...'); - final Iterable dartFiles = await _getFilesWithExtension('.dart'); - if (dartFiles.isEmpty) { - print( - 'No .dart files to format. If you set the `--exclude` flag, most likey they were skipped'); - } else { - await processRunner.runAndStream( + Future _formatDart(Iterable files) async { + final Iterable dartFiles = + _getPathsWithExtensions(files, {'.dart'}); + if (dartFiles.isNotEmpty) { + print('Formatting .dart files...'); + // `flutter format` doesn't require the project to actually be a Flutter + // project. + final int exitCode = await processRunner.runAndStream( 'flutter', ['format', ...dartFiles], - workingDir: packagesDir, exitOnError: true); + workingDir: packagesDir); + if (exitCode != 0) { + printError('Failed to format Dart files: exit code $exitCode.'); + throw ToolExit(_exitFlutterFormatFailed); + } } } - Future> _getFilesWithExtension(String extension) async => - getFiles() - .where((File file) => p.extension(file.path) == extension) - .map((File file) => file.path) - .toList(); + Future> _getFilteredFilePaths(Stream files) async { + // Returns a pattern to check for [directories] as a subset of a file path. + RegExp pathFragmentForDirectories(List directories) { + final String s = p.separator; + return RegExp('(?:^|$s)${p.joinAll(directories)}$s'); + } + + return files + .map((File file) => file.path) + .where((String path) => + // Ignore files in build/ directories (e.g., headers of frameworks) + // to avoid useless extra work in local repositories. + !path.contains( + pathFragmentForDirectories(['example', 'build'])) && + // Ignore files in Pods, which are not part of the repository. + !path.contains(pathFragmentForDirectories(['Pods'])) && + // Ignore .dart_tool/, which can have various intermediate files. + !path.contains(pathFragmentForDirectories(['.dart_tool']))) + .toList(); + } + + Iterable _getPathsWithExtensions( + Iterable files, Set extensions) { + return files.where((String path) => extensions.contains(p.extension(path))); + } Future _getGoogleFormatterPath() async { final String javaFormatterPath = p.join( diff --git a/script/tool/test/format_command_test.dart b/script/tool/test/format_command_test.dart new file mode 100644 index 0000000000..e7f4d795eb --- /dev/null +++ b/script/tool/test/format_command_test.dart @@ -0,0 +1,344 @@ +// 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; + +import 'package:args/command_runner.dart'; +import 'package:file/file.dart'; +import 'package:file/memory.dart'; +import 'package:flutter_plugin_tools/src/common/core.dart'; +import 'package:flutter_plugin_tools/src/format_command.dart'; +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; + +import 'mocks.dart'; +import 'util.dart'; + +void main() { + late FileSystem fileSystem; + late Directory packagesDir; + late RecordingProcessRunner processRunner; + late CommandRunner runner; + late String javaFormatPath; + + setUp(() { + fileSystem = MemoryFileSystem(); + packagesDir = createPackagesDirectory(fileSystem: fileSystem); + processRunner = RecordingProcessRunner(); + final FormatCommand analyzeCommand = + FormatCommand(packagesDir, processRunner: processRunner); + + // Create the java formatter file that the command checks for, to avoid + // a download. + javaFormatPath = p.join(p.dirname(p.fromUri(io.Platform.script)), + 'google-java-format-1.3-all-deps.jar'); + fileSystem.file(javaFormatPath).createSync(recursive: true); + + runner = CommandRunner('format_command', 'Test for format_command'); + runner.addCommand(analyzeCommand); + }); + + List _getAbsolutePaths( + Directory package, List relativePaths) { + return relativePaths + .map((String path) => p.join(package.path, path)) + .toList(); + } + + test('formats .dart files', () async { + const List files = [ + 'lib/a.dart', + 'lib/src/b.dart', + 'lib/src/c.dart', + ]; + final Directory pluginDir = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: files, + ); + + await runCapturingPrint(runner, ['format']); + + expect( + processRunner.recordedCalls, + orderedEquals([ + ProcessCall( + 'flutter', + ['format', ..._getAbsolutePaths(pluginDir, files)], + packagesDir.path), + ])); + }); + + test('fails if flutter format fails', () async { + const List files = [ + 'lib/a.dart', + 'lib/src/b.dart', + 'lib/src/c.dart', + ]; + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + + processRunner.mockProcessesForExecutable['flutter'] = [ + MockProcess.failing() + ]; + Error? commandError; + final List output = await runCapturingPrint( + runner, ['format'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('Failed to format Dart files: exit code 1.'), + ])); + }); + + test('formats .java files', () async { + const List files = [ + 'android/src/main/java/io/flutter/plugins/a_plugin/a.java', + 'android/src/main/java/io/flutter/plugins/a_plugin/b.java', + ]; + final Directory pluginDir = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: files, + ); + + await runCapturingPrint(runner, ['format']); + + expect( + processRunner.recordedCalls, + orderedEquals([ + ProcessCall( + 'java', + [ + '-jar', + javaFormatPath, + '--replace', + ..._getAbsolutePaths(pluginDir, files) + ], + packagesDir.path), + ])); + }); + + test('fails if Java formatter fails', () async { + const List files = [ + 'android/src/main/java/io/flutter/plugins/a_plugin/a.java', + 'android/src/main/java/io/flutter/plugins/a_plugin/b.java', + ]; + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + + processRunner.mockProcessesForExecutable['java'] = [ + MockProcess.failing() + ]; + Error? commandError; + final List output = await runCapturingPrint( + runner, ['format'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('Failed to format Java files: exit code 1.'), + ])); + }); + + test('formats c-ish files', () async { + const List files = [ + 'ios/Classes/Foo.h', + 'ios/Classes/Foo.m', + 'linux/foo_plugin.cc', + 'macos/Classes/Foo.h', + 'macos/Classes/Foo.mm', + 'windows/foo_plugin.cpp', + ]; + final Directory pluginDir = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: files, + ); + + await runCapturingPrint(runner, ['format']); + + expect( + processRunner.recordedCalls, + orderedEquals([ + ProcessCall( + 'clang-format', + [ + '-i', + '--style=Google', + ..._getAbsolutePaths(pluginDir, files) + ], + packagesDir.path), + ])); + }); + + test('fails if clang-format fails', () async { + const List files = [ + 'linux/foo_plugin.cc', + 'macos/Classes/Foo.h', + ]; + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + + processRunner.mockProcessesForExecutable['clang-format'] = [ + MockProcess.failing() + ]; + Error? commandError; + final List output = await runCapturingPrint( + runner, ['format'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains( + 'Failed to format C, C++, and Objective-C files: exit code 1.'), + ])); + }); + + test('skips known non-repo files', () async { + const List skipFiles = [ + '/example/build/SomeFramework.framework/Headers/SomeFramework.h', + '/example/Pods/APod.framework/Headers/APod.h', + '.dart_tool/internals/foo.cc', + '.dart_tool/internals/Bar.java', + '.dart_tool/internals/baz.dart', + ]; + const List clangFiles = ['ios/Classes/Foo.h']; + const List dartFiles = ['lib/a.dart']; + const List javaFiles = [ + 'android/src/main/java/io/flutter/plugins/a_plugin/a.java' + ]; + final Directory pluginDir = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: [ + ...skipFiles, + // Include some files that should be formatted to validate that it's + // correctly filtering even when running the commands. + ...clangFiles, + ...dartFiles, + ...javaFiles, + ], + ); + + await runCapturingPrint(runner, ['format']); + + expect( + processRunner.recordedCalls, + containsAll([ + ProcessCall( + 'clang-format', + [ + '-i', + '--style=Google', + ..._getAbsolutePaths(pluginDir, clangFiles) + ], + packagesDir.path), + ProcessCall( + 'flutter', + ['format', ..._getAbsolutePaths(pluginDir, dartFiles)], + packagesDir.path), + ProcessCall( + 'java', + [ + '-jar', + javaFormatPath, + '--replace', + ..._getAbsolutePaths(pluginDir, javaFiles) + ], + packagesDir.path), + ])); + }); + + test('fails if files are changed with --file-on-change', () async { + const List files = [ + 'linux/foo_plugin.cc', + 'macos/Classes/Foo.h', + ]; + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + + processRunner.mockProcessesForExecutable['git'] = [ + MockProcess.succeeding(), + ]; + const String changedFilePath = 'packages/a_plugin/linux/foo_plugin.cc'; + processRunner.resultStdout = changedFilePath; + Error? commandError; + final List output = + await runCapturingPrint(runner, ['format', '--fail-on-change'], + errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('These files are not formatted correctly'), + contains(changedFilePath), + contains('patch -p1 < files = [ + 'linux/foo_plugin.cc', + 'macos/Classes/Foo.h', + ]; + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + + processRunner.mockProcessesForExecutable['git'] = [ + MockProcess.failing() + ]; + Error? commandError; + final List output = + await runCapturingPrint(runner, ['format', '--fail-on-change'], + errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('Unable to determine changed files.'), + ])); + }); + + test('reports git diff failures', () async { + const List files = [ + 'linux/foo_plugin.cc', + 'macos/Classes/Foo.h', + ]; + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + + processRunner.mockProcessesForExecutable['git'] = [ + MockProcess.succeeding(), // ls-files + MockProcess.failing(), // diff + ]; + const String changedFilePath = 'packages/a_plugin/linux/foo_plugin.cc'; + processRunner.resultStdout = changedFilePath; + Error? commandError; + final List output = + await runCapturingPrint(runner, ['format', '--fail-on-change'], + errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('These files are not formatted correctly'), + contains(changedFilePath), + contains('Unable to determine diff.'), + ])); + }); +}