Skip to content

Commit

Permalink
Enabling pre-push checks on Windows (flutter#36123)
Browse files Browse the repository at this point in the history 
Re-submit the changes to enable windows pre-push checks.

This patch changes how `ci/bin/format.dart` generate diffs from `diff` and `patch` commands to `git diff` and `git apply` in order to have a common method for these operations on all platforms. Windows installations don't have diff and patch commands available by default and many implementations which provide such commands work differently than the UN*X tools. Git however works consistently across all platforms.

Additionally, this patch also changes the python executable in some of the pre-push components affected by this to `vpython3` to continue the effort started at flutter/flutter#108474 and I also removed the `--no-sound-null-safety` parameter in the ci/format.sh, ci/format.bat files

NOTE: Since the original patch caused some issues, I suggest that this should be tested more carefully before it is merged.

### Issues fixed by this PR
* flutter/flutter#108122
* flutter/flutter#107920
* flutter/flutter#86506
* flutter/flutter#106615

### [flutter/tests] repo impact
None.

writing and running engine tests.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
  • Loading branch information
@mtolmacs
mtolmacs authored Jun 21, 2023
1 parent 08c6507 commit 08aaa88
Show file tree
Hide file tree
Showing 10 changed files with 426 additions and 52 deletions.
168 changes: 121 additions & 47 deletions ci/bin/format.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
//
// Run with --help for usage.

// TODO(gspencergoog): Support clang formatting on Windows.
// TODO(gspencergoog): Support Java formatting on Windows.

import 'dart:io';

import 'package:args/args.dart';
Expand Down Expand Up @@ -84,14 +81,9 @@ String formatCheckToName(FormatCheck check) {
}

List<String> formatCheckNames() {
List<FormatCheck> allowed;
if (!Platform.isWindows) {
allowed = FormatCheck.values;
} else {
allowed = <FormatCheck>[FormatCheck.gn, FormatCheck.whitespace];
}
return allowed
.map<String>((FormatCheck check) => check.toString().replaceFirst('$FormatCheck.', ''))
return FormatCheck.values
.map<String>((FormatCheck check) =>
check.toString().replaceFirst('$FormatCheck.', ''))
.toList();
}

Expand Down Expand Up @@ -226,7 +218,7 @@ abstract class FormatChecker {
);
final List<WorkerJob> jobs = patches.map<WorkerJob>((String patch) {
return WorkerJob(
<String>['patch', '-p0'],
<String>['git', 'apply', '--ignore-space-change'],
stdinRaw: codeUnitsAsStream(patch.codeUnits),
);
}).toList();
Expand Down Expand Up @@ -318,6 +310,8 @@ class ClangFormatChecker extends FormatChecker {
clangOs = 'linux-x64';
} else if (Platform.isMacOS) {
clangOs = 'mac-x64';
} else if (Platform.isWindows) {
clangOs = 'windows-x64';
} else {
throw FormattingException(
"Unknown operating system: don't know how to run clang-format here.");
Expand Down Expand Up @@ -401,9 +395,23 @@ class ClangFormatChecker extends FormatChecker {
await for (final WorkerJob completedJob in completedClangFormats) {
if (completedJob.result.exitCode == 0) {
diffJobs.add(
WorkerJob(<String>['diff', '-u', completedJob.command.last, '-'],
WorkerJob(<String>[
'git',
'diff',
'--no-index',
'--no-color',
'--ignore-cr-at-eol',
'--',
completedJob.command.last,
'-',
],
stdinRaw: codeUnitsAsStream(completedJob.result.stdoutRaw)),
);
} else {
final String formatterCommand = completedJob.command.join(' ');
error("Formatter command '$formatterCommand' failed with exit code "
'${completedJob.result.exitCode}. Command output follows:\n\n'
'${completedJob.result.output}');
}
}
final ProcessPool diffPool = ProcessPool(
Expand All @@ -425,9 +433,11 @@ class ClangFormatChecker extends FormatChecker {
' which ${plural ? 'were' : 'was'} formatted incorrectly.');
stdout.writeln('To fix, run:');
stdout.writeln();
stdout.writeln('patch -p0 <<DONE');
stdout.writeln('git apply <<DONE');
for (final WorkerJob job in failed) {
stdout.write(job.result.stdout);
stdout.write(job.result.stdout
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}')
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}'));
}
stdout.writeln('DONE');
stdout.writeln();
Expand All @@ -436,7 +446,9 @@ class ClangFormatChecker extends FormatChecker {
message('Completed checking ${diffJobs.length} C++/ObjC/Shader files with no formatting problems.');
}
return failed.map<String>((WorkerJob job) {
return job.result.stdout;
return job.result.stdout
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}')
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}');
}).toList();
}
}
Expand Down Expand Up @@ -536,16 +548,30 @@ class JavaFormatChecker extends FormatChecker {
processRunner: _processRunner,
printReport: namedReport('Java format'),
);
final Stream<WorkerJob> completedClangFormats = formatPool.startWorkers(formatJobs);
final Stream<WorkerJob> completedJavaFormats = formatPool.startWorkers(formatJobs);
final List<WorkerJob> diffJobs = <WorkerJob>[];
await for (final WorkerJob completedJob in completedClangFormats) {
await for (final WorkerJob completedJob in completedJavaFormats) {
if (completedJob.result.exitCode == 0) {
diffJobs.add(
WorkerJob(
<String>['diff', '-u', completedJob.command.last, '-'],
<String>[
'git',
'diff',
'--no-index',
'--no-color',
'--ignore-cr-at-eol',
'--',
completedJob.command.last,
'-',
],
stdinRaw: codeUnitsAsStream(completedJob.result.stdoutRaw),
),
);
} else {
final String formatterCommand = completedJob.command.join(' ');
error("Formatter command '$formatterCommand' failed with exit code "
'${completedJob.result.exitCode}. Command output follows:\n\n'
'${completedJob.result.output}');
}
}
final ProcessPool diffPool = ProcessPool(
Expand All @@ -567,9 +593,11 @@ class JavaFormatChecker extends FormatChecker {
' which ${plural ? 'were' : 'was'} formatted incorrectly.');
stdout.writeln('To fix, run:');
stdout.writeln();
stdout.writeln('patch -p0 <<DONE');
stdout.writeln('git apply <<DONE');
for (final WorkerJob job in failed) {
stdout.write(job.result.stdout);
stdout.write(job.result.stdout
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}')
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}'));
}
stdout.writeln('DONE');
stdout.writeln();
Expand All @@ -578,7 +606,9 @@ class JavaFormatChecker extends FormatChecker {
message('Completed checking ${diffJobs.length} Java files with no formatting problems.');
}
return failed.map<String>((WorkerJob job) {
return job.result.stdout;
return job.result.stdout
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}')
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}');
}).toList();
}
}
Expand Down Expand Up @@ -627,45 +657,90 @@ class GnFormatChecker extends FormatChecker {
final List<String> cmd = <String>[
gnBinary.path,
'format',
if (!fixing) '--dry-run',
if (!fixing) '--stdin',
];
final List<WorkerJob> jobs = <WorkerJob>[];
for (final String file in filesToCheck) {
jobs.add(WorkerJob(<String>[...cmd, file]));
if (fixing) {
jobs.add(WorkerJob(
<String>[...cmd, file],
name: <String>[...cmd, file].join(' '),
));
} else {
final WorkerJob job = WorkerJob(
cmd,
stdinRaw: codeUnitsAsStream(
File(path.join(repoDir.absolute.path, file)).readAsBytesSync(),
),
name: <String>[...cmd, file].join(' '),
);
jobs.add(job);
}
}
final ProcessPool gnPool = ProcessPool(
processRunner: _processRunner,
printReport: namedReport('gn format'),
);
final List<WorkerJob> completedJobs = await gnPool.runToCompletion(jobs);
reportDone();
final List<String> incorrect = <String>[];
for (final WorkerJob job in completedJobs) {
if (job.result.exitCode == 2) {
incorrect.add(' ${job.command.last}');
}
if (job.result.exitCode == 1) {
// GN has exit code 1 if it had some problem formatting/checking the
// file.
throw FormattingException(
'Unable to format ${job.command.last}:\n${job.result.output}',
final Stream<WorkerJob> completedJobs = gnPool.startWorkers(jobs);
final List<WorkerJob> diffJobs = <WorkerJob>[];
await for (final WorkerJob completedJob in completedJobs) {
if (completedJob.result.exitCode == 0) {
diffJobs.add(
WorkerJob(
<String>[
'git',
'diff',
'--no-index',
'--no-color',
'--ignore-cr-at-eol',
'--',
completedJob.name.split(' ').last,
'-'
],
stdinRaw: codeUnitsAsStream(completedJob.result.stdoutRaw),
),
);
} else {
final String formatterCommand = completedJob.command.join(' ');
error("Formatter command '$formatterCommand' failed with exit code "
'${completedJob.result.exitCode}. Command output follows:\n\n'
'${completedJob.result.output}');
}
}
if (incorrect.isNotEmpty) {
final bool plural = incorrect.length > 1;
final ProcessPool diffPool = ProcessPool(
processRunner: _processRunner,
printReport: namedReport('diff'),
);
final List<WorkerJob> completedDiffs =
await diffPool.runToCompletion(diffJobs);
final Iterable<WorkerJob> failed = completedDiffs.where((WorkerJob job) {
return job.result.exitCode != 0;
});
reportDone();
if (failed.isNotEmpty) {
final bool plural = failed.length > 1;
if (fixing) {
message('Fixed ${incorrect.length} GN file${plural ? 's' : ''}'
message('Fixed ${failed.length} GN file${plural ? 's' : ''}'
' which ${plural ? 'were' : 'was'} formatted incorrectly.');
} else {
error('Found ${incorrect.length} GN file${plural ? 's' : ''}'
' which ${plural ? 'were' : 'was'} formatted incorrectly:');
incorrect.forEach(stderr.writeln);
error('Found ${failed.length} GN file${plural ? 's' : ''}'
' which ${plural ? 'were' : 'was'} formatted incorrectly.');
stdout.writeln('To fix, run:');
stdout.writeln();
stdout.writeln('git apply <<DONE');
for (final WorkerJob job in failed) {
stdout.write(job.result.stdout
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}')
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}'));
}
stdout.writeln('DONE');
stdout.writeln();
}
} else {
message('All GN files formatted correctly.');
message('Completed checking ${completedDiffs.length} GN files with no '
'formatting problems.');
}
return incorrect.length;
return failed.length;
}
}

Expand All @@ -684,7 +759,7 @@ class PythonFormatChecker extends FormatChecker {
yapfBin = File(path.join(
repoDir.absolute.path,
'tools',
'yapf.sh',
Platform.isWindows ? 'yapf.bat' : 'yapf.sh',
));
_yapfStyle = File(path.join(
repoDir.absolute.path,
Expand Down Expand Up @@ -937,8 +1012,7 @@ Future<int> main(List<String> arguments) async {
allowed: formatCheckNames(),
defaultsTo: formatCheckNames(),
help: 'Specifies which checks will be performed. Defaults to all checks. '
'May be specified more than once to perform multiple types of checks. '
'On Windows, only whitespace and gn checks are currently supported.');
'May be specified more than once to perform multiple types of checks. ');
parser.addFlag('verbose', help: 'Print verbose output.', defaultsTo: verbose);

late final ArgResults options;
Expand Down
11 changes: 10 additions & 1 deletion ci/format.bat
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,16 @@ where /q git || ECHO Error: Unable to find git in your PATH. && EXIT /B 1

SET repo_dir=%SRC_DIR%\flutter
SET ci_dir=%repo_dir%\ci
SET dart_sdk_path=%SRC_DIR%\third_party\dart\tools\sdks\dart-sdk

REM Determine which platform we are on and use the right prebuilt Dart SDK
IF "%PROCESSOR_ARCHITECTURE%"=="AMD64" (
SET dart_sdk_path=%SRC_DIR%\flutter\prebuilts\windows-x64\dart-sdk
) ELSE IF "%PROCESSOR_ARCHITECTURE%"=="ARM64" (
SET dart_sdk_path=%SRC_DIR%\flutter\prebuilts\windows-arm64\dart-sdk
) ELSE (
ECHO "Windows x86 (32-bit) is not supported" && EXIT /B 1
)

SET dart=%dart_sdk_path%\bin\dart.exe

cd "%ci_dir%"
Expand Down
13 changes: 13 additions & 0 deletions ci/pubspec.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,26 @@ dependencies:
process_runner: any
process: any

dev_dependencies:
async_helper: any
expect: any
litetest: any

dependency_overrides:
args:
path: ../../third_party/dart/third_party/pkg/args
async:
path: ../../third_party/dart/third_party/pkg/async
async_helper:
path: ../../third_party/dart/pkg/async_helper
collection:
path: ../../third_party/dart/third_party/pkg/collection
expect:
path: ../../third_party/dart/pkg/expect
file:
path: ../../third_party/pkg/file/packages/file
litetest:
path: ../testing/litetest
meta:
path: ../../third_party/dart/pkg/meta
path:
Expand All @@ -42,3 +53,5 @@ dependency_overrides:
path: ../../third_party/pkg/process
process_runner:
path: ../../third_party/pkg/process_runner
smith:
path: ../../third_party/dart/pkg/smith
Loading

0 comments on commit 08aaa88

Please sign in to comment.