[flutter_plugin_tools] Support format on Windows (#4150)

Allows `format` to run successfully on Windows:
- Ensures that no calls exceed the command length limit.
- Allows specifying a `java` path to make it easier to run without a system Java (e.g., by pointing to the `java` binary in an Android Studio installation).
- Adds clear error messages when `java` or `clang-format` is missing since it's very non-obvious what's wrong otherwise.

Bumps the version, which I intended to do in the previous PR but apparently didn't push to the PR.

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
 

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',
+        defaultsTo: 'clang-format', help: 'Path to "clang-format" executable.');
-        help: 'Path to executable of clang-format.');
+    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<String> files = await _getFilteredFilePaths(getFiles());
+    final Iterable<String> 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<String> clangFiles = _getPathsWithExtensions(
         files, <String>{'.h', '.m', '.mm', '.cc', '.cpp'});
     if (clangFiles.isNotEmpty) {
+      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 Iterable<List<String>> batches = partition(clangFiles, 100);
+      final int exitCode = await _runBatched(
-      int exitCode = 0;
+          getStringArg('clang-format'), <String>['-i', '--style=Google'],
-      for (final List<String> batch in batches) {
+          files: clangFiles);
-        batch.sort(); // For ease of testing; partition changes the order.
-        exitCode = await processRunner.runAndStream(
-            getStringArg('clang-format'),
-            <String>['-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.');
@@ -138,10 +153,18 @@ class FormatCommand extends PluginCommand {
     final Iterable<String> javaFiles =
         _getPathsWithExtensions(files, <String>{'.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',
+      final int exitCode = await _runBatched(
-          <String>['-jar', googleFormatterPath, '--replace', ...javaFiles],
+          java, <String>['-jar', googleFormatterPath, '--replace'],
-          workingDir: packagesDir);
+          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(
+      final int exitCode = await _runBatched(flutterCommand, <String>['format'],
-          flutterCommand, <String>['format', ...dartFiles],
+          files: dartFiles);
-          workingDir: packagesDir);
       if (exitCode != 0) {
         printError('Failed to format Dart files: exit code $exitCode.');
         throw ToolExit(_exitFlutterFormatFailed);
@@ -166,7 +188,12 @@ class FormatCommand extends PluginCommand {
     }
   }
 
-  Future<Iterable<String>> _getFilteredFilePaths(Stream<File> files) async {
+  /// Given a stream of [files], returns the paths of any that are not in known
+  /// locations to ignore, relative to [relativeTo].
+  Future<Iterable<String>> _getFilteredFilePaths(
+    Stream<File> files, {
+    required Directory relativeTo,
+  }) async {
     // Returns a pattern to check for [directories] as a subset of a file path.
     RegExp pathFragmentForDirectories(List<String> 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<bool> _hasDependency(String command) async {
+    try {
+      final io.ProcessResult result =
+          await processRunner.run(command, <String>['--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<int> _runBatched(
+    String command,
+    List<String> arguments, {
+    required Iterable<String> 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<List<String>> batches =
+        _partitionFileList(files, maxStringLength: batchMaxTotalLength);
+    for (final List<String> batch in batches) {
+      batch.sort(); // For ease of testing.
+      final int exitCode = await processRunner.runAndStream(
+          command, <String>[...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<List<String>> _partitionFileList(Iterable<String> files,
+      {required int maxStringLength}) {
+    final List<List<String>> batches = <List<String>>[<String>[]];
+    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(<String>[]);
+        currentBatchTotalLength = 0;
+      }
+      batches.last.add(file);
+      currentBatchTotalLength += length;
+    }
+    return batches;
+  }
 }

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

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<void> 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<String> _getAbsolutePaths(
+  /// Returns a modified version of a list of [relativePaths] that are relative
+  /// to [package] to instead be relative to [packagesDir].
+  List<String> _getPackagesDirRelativePaths(
       Directory package, List<String> 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<String> _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 <String>[
+      for (int i = filenameBase; i < filenameBase + count; ++i)
+        path.join('a' * padding, '$i.dart'),
+    ];
+  }
+
   test('formats .dart files', () async {
     const List<String> files = <String>[
       'lib/a.dart',
@@ -71,8 +97,11 @@ void main() {
         processRunner.recordedCalls,
         orderedEquals(<ProcessCall>[
           ProcessCall(
-              'flutter',
+              getFlutterCommand(mockPlatform),
-              <String>['format', ..._getAbsolutePaths(pluginDir, files)],
+              <String>[
+                'format',
+                ..._getPackagesDirRelativePaths(pluginDir, files)
+              ],
               packagesDir.path),
         ]));
   });
@@ -85,9 +114,8 @@ void main() {
     ];
     createFakePlugin('a_plugin', packagesDir, extraFiles: files);
 
-    processRunner.mockProcessesForExecutable['flutter'] = <io.Process>[
+    processRunner.mockProcessesForExecutable[getFlutterCommand(mockPlatform)] =
-      MockProcess.failing()
+        <io.Process>[MockProcess.failing()];
-    ];
     Error? commandError;
     final List<String> output = await runCapturingPrint(
         runner, <String>['format'], errorHandler: (Error e) {
@@ -118,19 +146,20 @@ void main() {
     expect(
         processRunner.recordedCalls,
         orderedEquals(<ProcessCall>[
+          const ProcessCall('java', <String>['--version'], null),
           ProcessCall(
               'java',
               <String>[
                 '-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<String> files = <String>[
       '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<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          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<String> files = <String>[
+      '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'] = <io.Process>[
+      MockProcess.succeeding(), // check for working java
+      MockProcess.failing(), // format
+    ];
+    Error? commandError;
+    final List<String> output = await runCapturingPrint(
+        runner, <String>['format'], errorHandler: (Error e) {
+      commandError = e;
+    });
+
     expect(commandError, isA<ToolExit>());
     expect(
         output,
@@ -154,6 +210,35 @@ void main() {
         ]));
   });
 
+  test('honors --java flag', () async {
+    const List<String> files = <String>[
+      '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, <String>['format', '--java=/path/to/java']);
+
+    expect(
+        processRunner.recordedCalls,
+        orderedEquals(<ProcessCall>[
+          const ProcessCall('/path/to/java', <String>['--version'], null),
+          ProcessCall(
+              '/path/to/java',
+              <String>[
+                '-jar',
+                javaFormatPath,
+                '--replace',
+                ..._getPackagesDirRelativePaths(pluginDir, files)
+              ],
+              packagesDir.path),
+        ]));
+  });
+
   test('formats c-ish files', () async {
     const List<String> files = <String>[
       'ios/Classes/Foo.h',
@@ -174,12 +259,69 @@ void main() {
     expect(
         processRunner.recordedCalls,
         orderedEquals(<ProcessCall>[
+          const ProcessCall('clang-format', <String>['--version'], null),
           ProcessCall(
               'clang-format',
               <String>[
                 '-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<String> files = <String>[
+      'linux/foo_plugin.cc',
+      'macos/Classes/Foo.h',
+    ];
+    createFakePlugin('a_plugin', packagesDir, extraFiles: files);
+
+    processRunner.mockProcessesForExecutable['clang-format'] = <io.Process>[
+      MockProcess.failing()
+    ];
+    Error? commandError;
+    final List<String> output = await runCapturingPrint(
+        runner, <String>['format'], errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          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<String> files = <String>[
+      'windows/foo_plugin.cpp',
+    ];
+    final Directory pluginDir = createFakePlugin(
+      'a_plugin',
+      packagesDir,
+      extraFiles: files,
+    );
+
+    await runCapturingPrint(
+        runner, <String>['format', '--clang-format=/path/to/clang-format']);
+
+    expect(
+        processRunner.recordedCalls,
+        orderedEquals(<ProcessCall>[
+          const ProcessCall(
+              '/path/to/clang-format', <String>['--version'], null),
+          ProcessCall(
+              '/path/to/clang-format',
+              <String>[
+                '-i',
+                '--style=Google',
+                ..._getPackagesDirRelativePaths(pluginDir, files)
               ],
               packagesDir.path),
         ]));
@@ -193,7 +335,8 @@ void main() {
     createFakePlugin('a_plugin', packagesDir, extraFiles: files);
 
     processRunner.mockProcessesForExecutable['clang-format'] = <io.Process>[
-      MockProcess.failing()
+      MockProcess.succeeding(), // check for working clang-format
+      MockProcess.failing(), // format
     ];
     Error? commandError;
     final List<String> output = await runCapturingPrint(
@@ -246,12 +389,15 @@ void main() {
               <String>[
                 '-i',
                 '--style=Google',
-                ..._getAbsolutePaths(pluginDir, clangFiles)
+                ..._getPackagesDirRelativePaths(pluginDir, clangFiles)
               ],
               packagesDir.path),
           ProcessCall(
-              'flutter',
+              getFlutterCommand(mockPlatform),
-              <String>['format', ..._getAbsolutePaths(pluginDir, dartFiles)],
+              <String>[
+                '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<String> batch1 =
+        _get99CharacterPathExtraFiles(pluginName, batchSize + 1);
+    final String extraFile = batch1.removeLast();
+
+    createFakePlugin(
+      pluginName,
+      packagesDir,
+      extraFiles: <String>[...batch1, extraFile],
+    );
+
+    await runCapturingPrint(runner, <String>['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),
+              <String>[
+                '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<String> batch =
+        _get99CharacterPathExtraFiles(pluginName, batchSize + 1);
+
+    createFakePlugin(
+      pluginName,
+      packagesDir,
+      extraFiles: batch,
+    );
+
+    await runCapturingPrint(runner, <String>['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<String> batch1 =
+        _get99CharacterPathExtraFiles(pluginName, batchSize + 1);
+    final String extraFile = batch1.removeLast();
+
+    createFakePlugin(
+      pluginName,
+      packagesDir,
+      extraFiles: <String>[...batch1, extraFile],
+    );
+
+    await runCapturingPrint(runner, <String>['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),
+              <String>[
+                'format',
+                '$pluginName/$extraFile',
+              ],
+              packagesDir.path),
+        ));
+  });
 }