diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 3f31a4953f..9db94dda37 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,4 +1,4 @@ -## NEXT +## 0.4.0 - Modified the output format of many commands - **Breaking change**: `firebase-test-lab` no longer supports `*_e2e.dart` @@ -10,6 +10,7 @@ - Deprecated `--plugins` in favor of new `--packages`. `--plugins` continues to work for now, but will be removed in the future. - Make `drive-examples` device detection robust against Flutter tool banners. +- `format` is now supported on Windows. ## 0.3.0 diff --git a/script/tool/lib/src/format_command.dart b/script/tool/lib/src/format_command.dart index 7954fd044c..c67fb96d28 100644 --- a/script/tool/lib/src/format_command.dart +++ b/script/tool/lib/src/format_command.dart @@ -7,17 +7,31 @@ import 'dart:io' as io; import 'package:file/file.dart'; import 'package:http/http.dart' as http; +import 'package:meta/meta.dart'; import 'package:platform/platform.dart'; -import 'package:quiver/iterables.dart'; import 'common/core.dart'; import 'common/plugin_command.dart'; import 'common/process_runner.dart'; +/// In theory this should be 8191, but in practice that was still resulting in +/// "The input line is too long" errors. This was chosen as a value that worked +/// in practice in testing with flutter/plugins, but may need to be adjusted +/// based on further experience. +@visibleForTesting +const int windowsCommandLineMax = 8000; + +/// This value is picked somewhat arbitrarily based on checking `ARG_MAX` on a +/// macOS and Linux machine. If anyone encounters a lower limit in pratice, it +/// can be lowered accordingly. +@visibleForTesting +const int nonWindowsCommandLineMax = 1000000; + const int _exitClangFormatFailed = 3; const int _exitFlutterFormatFailed = 4; const int _exitJavaFormatFailed = 5; const int _exitGitFailed = 6; +const int _exitDependencyMissing = 7; 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'); @@ -32,8 +46,9 @@ class FormatCommand extends PluginCommand { }) : super(packagesDir, processRunner: processRunner, platform: platform) { argParser.addFlag('fail-on-change', hide: true); argParser.addOption('clang-format', - defaultsTo: 'clang-format', - help: 'Path to executable of clang-format.'); + defaultsTo: 'clang-format', help: 'Path to "clang-format" executable.'); + argParser.addOption('java', + defaultsTo: 'java', help: 'Path to "java" executable.'); } @override @@ -52,7 +67,8 @@ class FormatCommand extends PluginCommand { // 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()); + final Iterable files = + await _getFilteredFilePaths(getFiles(), relativeTo: packagesDir); await _formatDart(files); await _formatJava(files, googleFormatterPath); await _formatCppAndObjectiveC(files); @@ -112,19 +128,18 @@ class FormatCommand extends PluginCommand { 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; - } + final String clangFormat = getStringArg('clang-format'); + if (!await _hasDependency(clangFormat)) { + printError( + 'Unable to run \'clang-format\'. Make sure that it is in your ' + 'path, or provide a full path with --clang-format.'); + throw ToolExit(_exitDependencyMissing); } + + print('Formatting .cc, .cpp, .h, .m, and .mm files...'); + final int exitCode = await _runBatched( + getStringArg('clang-format'), ['-i', '--style=Google'], + files: clangFiles); if (exitCode != 0) { printError( 'Failed to format C, C++, and Objective-C files: exit code $exitCode.'); @@ -138,10 +153,18 @@ class FormatCommand extends PluginCommand { final Iterable javaFiles = _getPathsWithExtensions(files, {'.java'}); if (javaFiles.isNotEmpty) { + final String java = getStringArg('java'); + if (!await _hasDependency(java)) { + printError( + 'Unable to run \'java\'. Make sure that it is in your path, or ' + 'provide a full path with --java.'); + throw ToolExit(_exitDependencyMissing); + } + print('Formatting .java files...'); - final int exitCode = await processRunner.runAndStream('java', - ['-jar', googleFormatterPath, '--replace', ...javaFiles], - workingDir: packagesDir); + final int exitCode = await _runBatched( + java, ['-jar', googleFormatterPath, '--replace'], + files: javaFiles); if (exitCode != 0) { printError('Failed to format Java files: exit code $exitCode.'); throw ToolExit(_exitJavaFormatFailed); @@ -156,9 +179,8 @@ class FormatCommand extends PluginCommand { print('Formatting .dart files...'); // `flutter format` doesn't require the project to actually be a Flutter // project. - final int exitCode = await processRunner.runAndStream( - flutterCommand, ['format', ...dartFiles], - workingDir: packagesDir); + final int exitCode = await _runBatched(flutterCommand, ['format'], + files: dartFiles); if (exitCode != 0) { printError('Failed to format Dart files: exit code $exitCode.'); throw ToolExit(_exitFlutterFormatFailed); @@ -166,7 +188,12 @@ class FormatCommand extends PluginCommand { } } - Future> _getFilteredFilePaths(Stream files) async { + /// Given a stream of [files], returns the paths of any that are not in known + /// locations to ignore, relative to [relativeTo]. + Future> _getFilteredFilePaths( + Stream files, { + required Directory relativeTo, + }) async { // Returns a pattern to check for [directories] as a subset of a file path. RegExp pathFragmentForDirectories(List directories) { String s = path.separator; @@ -177,8 +204,10 @@ class FormatCommand extends PluginCommand { return RegExp('(?:^|$s)${path.joinAll(directories)}$s'); } + final String fromPath = relativeTo.path; + return files - .map((File file) => file.path) + .map((File file) => path.relative(file.path, from: fromPath)) .where((String path) => // Ignore files in build/ directories (e.g., headers of frameworks) // to avoid useless extra work in local repositories. @@ -212,4 +241,74 @@ class FormatCommand extends PluginCommand { return javaFormatterPath; } + + /// Returns true if [command] can be run successfully. + Future _hasDependency(String command) async { + try { + final io.ProcessResult result = + await processRunner.run(command, ['--version']); + if (result.exitCode != 0) { + return false; + } + } on io.ProcessException { + // Thrown when the binary is missing entirely. + return false; + } + return true; + } + + /// Runs [command] on [arguments] on all of the files in [files], batched as + /// necessary to avoid OS command-line length limits. + /// + /// Returns the exit code of the first failure, which stops the run, or 0 + /// on success. + Future _runBatched( + String command, + List arguments, { + required Iterable files, + }) async { + final int commandLineMax = + platform.isWindows ? windowsCommandLineMax : nonWindowsCommandLineMax; + + // Compute the max length of the file argument portion of a batch. + // Add one to each argument's length for the space before it. + final int argumentTotalLength = + arguments.fold(0, (int sum, String arg) => sum + arg.length + 1); + final int batchMaxTotalLength = + commandLineMax - command.length - argumentTotalLength; + + // Run the command in batches. + final List> batches = + _partitionFileList(files, maxStringLength: batchMaxTotalLength); + for (final List batch in batches) { + batch.sort(); // For ease of testing. + final int exitCode = await processRunner.runAndStream( + command, [...arguments, ...batch], + workingDir: packagesDir); + if (exitCode != 0) { + return exitCode; + } + } + return 0; + } + + /// Partitions [files] into batches whose max string length as parameters to + /// a command (including the spaces between them, and between the list and + /// the command itself) is no longer than [maxStringLength]. + List> _partitionFileList(Iterable files, + {required int maxStringLength}) { + final List> batches = >[[]]; + int currentBatchTotalLength = 0; + for (final String file in files) { + final int length = file.length + 1 /* for the space */; + if (currentBatchTotalLength + length > maxStringLength) { + // Start a new batch. + batches.add([]); + currentBatchTotalLength = 0; + } + batches.last.add(file); + currentBatchTotalLength += length; + } + return batches; + } } diff --git a/script/tool/pubspec.yaml b/script/tool/pubspec.yaml index 6273fe9bf2..7dadc598d4 100644 --- a/script/tool/pubspec.yaml +++ b/script/tool/pubspec.yaml @@ -1,7 +1,7 @@ name: flutter_plugin_tools description: Productivity utils for flutter/plugins and flutter/packages repository: https://github.com/flutter/plugins/tree/master/script/tool -version: 0.3.0 +version: 0.4.0 dependencies: args: ^2.1.0 diff --git a/script/tool/test/format_command_test.dart b/script/tool/test/format_command_test.dart index fabef31a1b..4728c31365 100644 --- a/script/tool/test/format_command_test.dart +++ b/script/tool/test/format_command_test.dart @@ -19,8 +19,8 @@ void main() { late FileSystem fileSystem; late MockPlatform mockPlatform; late Directory packagesDir; - late p.Context path; late RecordingProcessRunner processRunner; + late FormatCommand analyzeCommand; late CommandRunner runner; late String javaFormatPath; @@ -29,7 +29,7 @@ void main() { mockPlatform = MockPlatform(); packagesDir = createPackagesDirectory(fileSystem: fileSystem); processRunner = RecordingProcessRunner(); - final FormatCommand analyzeCommand = FormatCommand( + analyzeCommand = FormatCommand( packagesDir, processRunner: processRunner, platform: mockPlatform, @@ -37,7 +37,7 @@ void main() { // Create the java formatter file that the command checks for, to avoid // a download. - path = analyzeCommand.path; + final p.Context path = analyzeCommand.path; javaFormatPath = path.join(path.dirname(path.fromUri(mockPlatform.script)), 'google-java-format-1.3-all-deps.jar'); fileSystem.file(javaFormatPath).createSync(recursive: true); @@ -46,13 +46,39 @@ void main() { runner.addCommand(analyzeCommand); }); - List _getAbsolutePaths( + /// Returns a modified version of a list of [relativePaths] that are relative + /// to [package] to instead be relative to [packagesDir]. + List _getPackagesDirRelativePaths( Directory package, List relativePaths) { + final p.Context path = analyzeCommand.path; + final String relativeBase = + path.relative(package.path, from: packagesDir.path); return relativePaths - .map((String relativePath) => path.join(package.path, relativePath)) + .map((String relativePath) => path.join(relativeBase, relativePath)) .toList(); } + /// Returns a list of [count] relative paths to pass to [createFakePlugin] + /// with name [pluginName] such that each path will be 99 characters long + /// relative to [packagesDir]. + /// + /// This is for each of testing batching, since it means each file will + /// consume 100 characters of the batch length. + List _get99CharacterPathExtraFiles(String pluginName, int count) { + final int padding = 99 - + pluginName.length - + 1 - // the path separator after the plugin name + 1 - // the path separator after the padding + 10; // the file name + const int filenameBase = 10000; + + final p.Context path = analyzeCommand.path; + return [ + for (int i = filenameBase; i < filenameBase + count; ++i) + path.join('a' * padding, '$i.dart'), + ]; + } + test('formats .dart files', () async { const List files = [ 'lib/a.dart', @@ -71,8 +97,11 @@ void main() { processRunner.recordedCalls, orderedEquals([ ProcessCall( - 'flutter', - ['format', ..._getAbsolutePaths(pluginDir, files)], + getFlutterCommand(mockPlatform), + [ + 'format', + ..._getPackagesDirRelativePaths(pluginDir, files) + ], packagesDir.path), ])); }); @@ -85,9 +114,8 @@ void main() { ]; createFakePlugin('a_plugin', packagesDir, extraFiles: files); - processRunner.mockProcessesForExecutable['flutter'] = [ - MockProcess.failing() - ]; + processRunner.mockProcessesForExecutable[getFlutterCommand(mockPlatform)] = + [MockProcess.failing()]; Error? commandError; final List output = await runCapturingPrint( runner, ['format'], errorHandler: (Error e) { @@ -118,19 +146,20 @@ void main() { expect( processRunner.recordedCalls, orderedEquals([ + const ProcessCall('java', ['--version'], null), ProcessCall( 'java', [ '-jar', javaFormatPath, '--replace', - ..._getAbsolutePaths(pluginDir, files) + ..._getPackagesDirRelativePaths(pluginDir, files) ], packagesDir.path), ])); }); - test('fails if Java formatter fails', () async { + test('fails with a clear message if Java is not in the path', () 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', @@ -146,6 +175,33 @@ void main() { commandError = e; }); + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains( + 'Unable to run \'java\'. Make sure that it is in your path, or ' + 'provide a full path with --java.'), + ])); + }); + + 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.succeeding(), // check for working java + MockProcess.failing(), // format + ]; + Error? commandError; + final List output = await runCapturingPrint( + runner, ['format'], errorHandler: (Error e) { + commandError = e; + }); + expect(commandError, isA()); expect( output, @@ -154,6 +210,35 @@ void main() { ])); }); + test('honors --java flag', () 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', '--java=/path/to/java']); + + expect( + processRunner.recordedCalls, + orderedEquals([ + const ProcessCall('/path/to/java', ['--version'], null), + ProcessCall( + '/path/to/java', + [ + '-jar', + javaFormatPath, + '--replace', + ..._getPackagesDirRelativePaths(pluginDir, files) + ], + packagesDir.path), + ])); + }); + test('formats c-ish files', () async { const List files = [ 'ios/Classes/Foo.h', @@ -174,12 +259,69 @@ void main() { expect( processRunner.recordedCalls, orderedEquals([ + const ProcessCall('clang-format', ['--version'], null), ProcessCall( 'clang-format', [ '-i', '--style=Google', - ..._getAbsolutePaths(pluginDir, files) + ..._getPackagesDirRelativePaths(pluginDir, files) + ], + packagesDir.path), + ])); + }); + + test('fails with a clear message if clang-format is not in the path', + () 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( + 'Unable to run \'clang-format\'. Make sure that it is in your ' + 'path, or provide a full path with --clang-format.'), + ])); + }); + + test('honors --clang-format flag', () async { + const List files = [ + 'windows/foo_plugin.cpp', + ]; + final Directory pluginDir = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: files, + ); + + await runCapturingPrint( + runner, ['format', '--clang-format=/path/to/clang-format']); + + expect( + processRunner.recordedCalls, + orderedEquals([ + const ProcessCall( + '/path/to/clang-format', ['--version'], null), + ProcessCall( + '/path/to/clang-format', + [ + '-i', + '--style=Google', + ..._getPackagesDirRelativePaths(pluginDir, files) ], packagesDir.path), ])); @@ -193,7 +335,8 @@ void main() { createFakePlugin('a_plugin', packagesDir, extraFiles: files); processRunner.mockProcessesForExecutable['clang-format'] = [ - MockProcess.failing() + MockProcess.succeeding(), // check for working clang-format + MockProcess.failing(), // format ]; Error? commandError; final List output = await runCapturingPrint( @@ -246,12 +389,15 @@ void main() { [ '-i', '--style=Google', - ..._getAbsolutePaths(pluginDir, clangFiles) + ..._getPackagesDirRelativePaths(pluginDir, clangFiles) ], packagesDir.path), ProcessCall( - 'flutter', - ['format', ..._getAbsolutePaths(pluginDir, dartFiles)], + getFlutterCommand(mockPlatform), + [ + 'format', + ..._getPackagesDirRelativePaths(pluginDir, dartFiles) + ], packagesDir.path), ProcessCall( 'java', @@ -259,7 +405,7 @@ void main() { '-jar', javaFormatPath, '--replace', - ..._getAbsolutePaths(pluginDir, javaFiles) + ..._getPackagesDirRelativePaths(pluginDir, javaFiles) ], packagesDir.path), ])); @@ -348,4 +494,97 @@ void main() { contains('Unable to determine diff.'), ])); }); + + test('Batches moderately long file lists on Windows', () async { + mockPlatform.isWindows = true; + + const String pluginName = 'a_plugin'; + // -1 since the command itself takes some length. + const int batchSize = (windowsCommandLineMax ~/ 100) - 1; + + // Make the file list one file longer than would fit in the batch. + final List batch1 = + _get99CharacterPathExtraFiles(pluginName, batchSize + 1); + final String extraFile = batch1.removeLast(); + + createFakePlugin( + pluginName, + packagesDir, + extraFiles: [...batch1, extraFile], + ); + + await runCapturingPrint(runner, ['format']); + + // Ensure that it was batched... + expect(processRunner.recordedCalls.length, 2); + // ... and that the spillover into the second batch was only one file. + expect( + processRunner.recordedCalls, + contains( + ProcessCall( + getFlutterCommand(mockPlatform), + [ + 'format', + '$pluginName\\$extraFile', + ], + packagesDir.path), + )); + }); + + // Validates that the Windows limit--which is much lower than the limit on + // other platforms--isn't being used on all platforms, as that would make + // formatting slower on Linux and macOS. + test('Does not batch moderately long file lists on non-Windows', () async { + const String pluginName = 'a_plugin'; + // -1 since the command itself takes some length. + const int batchSize = (windowsCommandLineMax ~/ 100) - 1; + + // Make the file list one file longer than would fit in a Windows batch. + final List batch = + _get99CharacterPathExtraFiles(pluginName, batchSize + 1); + + createFakePlugin( + pluginName, + packagesDir, + extraFiles: batch, + ); + + await runCapturingPrint(runner, ['format']); + + expect(processRunner.recordedCalls.length, 1); + }); + + test('Batches extremely long file lists on non-Windows', () async { + const String pluginName = 'a_plugin'; + // -1 since the command itself takes some length. + const int batchSize = (nonWindowsCommandLineMax ~/ 100) - 1; + + // Make the file list one file longer than would fit in the batch. + final List batch1 = + _get99CharacterPathExtraFiles(pluginName, batchSize + 1); + final String extraFile = batch1.removeLast(); + + createFakePlugin( + pluginName, + packagesDir, + extraFiles: [...batch1, extraFile], + ); + + await runCapturingPrint(runner, ['format']); + + // Ensure that it was batched... + expect(processRunner.recordedCalls.length, 2); + // ... and that the spillover into the second batch was only one file. + expect( + processRunner.recordedCalls, + contains( + ProcessCall( + getFlutterCommand(mockPlatform), + [ + 'format', + '$pluginName/$extraFile', + ], + packagesDir.path), + )); + }); }