[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.
diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md
index 3f31a49..9db94dd 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 7954fd0..c67fb96 100644
--- a/script/tool/lib/src/format_command.dart
+++ b/script/tool/lib/src/format_command.dart
@@ -7,17 +7,31 @@
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 @@
}) : 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 @@
// 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 @@
final Iterable<String> clangFiles = _getPathsWithExtensions(
files, <String>{'.h', '.m', '.mm', '.cc', '.cpp'});
if (clangFiles.isNotEmpty) {
- print('Formatting .cc, .cpp, .h, .m, and .mm files...');
- final Iterable<List<String>> batches = partition(clangFiles, 100);
- int exitCode = 0;
- for (final List<String> batch in batches) {
- 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;
- }
+ 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'), <String>['-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 @@
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',
- <String>['-jar', googleFormatterPath, '--replace', ...javaFiles],
- workingDir: packagesDir);
+ final int exitCode = await _runBatched(
+ java, <String>['-jar', googleFormatterPath, '--replace'],
+ files: javaFiles);
if (exitCode != 0) {
printError('Failed to format Java files: exit code $exitCode.');
throw ToolExit(_exitJavaFormatFailed);
@@ -156,9 +179,8 @@
print('Formatting .dart files...');
// `flutter format` doesn't require the project to actually be a Flutter
// project.
- final int exitCode = await processRunner.runAndStream(
- flutterCommand, <String>['format', ...dartFiles],
- workingDir: packagesDir);
+ final int exitCode = await _runBatched(flutterCommand, <String>['format'],
+ files: dartFiles);
if (exitCode != 0) {
printError('Failed to format Dart files: exit code $exitCode.');
throw ToolExit(_exitFlutterFormatFailed);
@@ -166,7 +188,12 @@
}
}
- 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 @@
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 @@
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;
+ }
}
diff --git a/script/tool/pubspec.yaml b/script/tool/pubspec.yaml
index 6273fe9..7dadc59 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 fabef31..4728c31 100644
--- a/script/tool/test/format_command_test.dart
+++ b/script/tool/test/format_command_test.dart
@@ -19,8 +19,8 @@
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 @@
mockPlatform = MockPlatform();
packagesDir = createPackagesDirectory(fileSystem: fileSystem);
processRunner = RecordingProcessRunner();
- final FormatCommand analyzeCommand = FormatCommand(
+ analyzeCommand = FormatCommand(
packagesDir,
processRunner: processRunner,
platform: mockPlatform,
@@ -37,7 +37,7 @@
// 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 @@
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 @@
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall(
- 'flutter',
- <String>['format', ..._getAbsolutePaths(pluginDir, files)],
+ getFlutterCommand(mockPlatform),
+ <String>[
+ 'format',
+ ..._getPackagesDirRelativePaths(pluginDir, files)
+ ],
packagesDir.path),
]));
});
@@ -85,9 +114,8 @@
];
createFakePlugin('a_plugin', packagesDir, extraFiles: files);
- processRunner.mockProcessesForExecutable['flutter'] = <io.Process>[
- MockProcess.failing()
- ];
+ processRunner.mockProcessesForExecutable[getFlutterCommand(mockPlatform)] =
+ <io.Process>[MockProcess.failing()];
Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['format'], errorHandler: (Error e) {
@@ -118,19 +146,20 @@
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',
@@ -150,10 +179,66 @@
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,
+ containsAllInOrder(<Matcher>[
contains('Failed to format Java files: exit code 1.'),
]));
});
+ 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 @@
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 @@
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 @@
<String>[
'-i',
'--style=Google',
- ..._getAbsolutePaths(pluginDir, clangFiles)
+ ..._getPackagesDirRelativePaths(pluginDir, clangFiles)
],
packagesDir.path),
ProcessCall(
- 'flutter',
- <String>['format', ..._getAbsolutePaths(pluginDir, dartFiles)],
+ getFlutterCommand(mockPlatform),
+ <String>[
+ 'format',
+ ..._getPackagesDirRelativePaths(pluginDir, dartFiles)
+ ],
packagesDir.path),
ProcessCall(
'java',
@@ -259,7 +405,7 @@
'-jar',
javaFormatPath,
'--replace',
- ..._getAbsolutePaths(pluginDir, javaFiles)
+ ..._getPackagesDirRelativePaths(pluginDir, javaFiles)
],
packagesDir.path),
]));
@@ -348,4 +494,97 @@
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),
+ ));
+ });
}
