Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 143 additions & 3 deletions ci/bin/format.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ enum MessageType {
}

enum FormatCheck {
dart,
gn,
java,
python,
Expand All @@ -53,6 +54,8 @@ FormatCheck nameToFormatCheck(String name) {
switch (name) {
case 'clang':
return FormatCheck.clang;
case 'dart':
return FormatCheck.dart;
case 'gn':
return FormatCheck.gn;
case 'java':
Expand All @@ -72,6 +75,8 @@ String formatCheckToName(FormatCheck check) {
switch (check) {
case FormatCheck.clang:
return 'C++/ObjC/Shader';
case FormatCheck.dart:
return 'Dart';
case FormatCheck.gn:
return 'GN';
case FormatCheck.java:
Expand All @@ -87,8 +92,7 @@ String formatCheckToName(FormatCheck check) {

List<String> formatCheckNames() {
return FormatCheck.values
.map<String>((FormatCheck check) =>
check.toString().replaceFirst('$FormatCheck.', ''))
.map<String>((FormatCheck check) => check.name)
.toList();
}

Expand Down Expand Up @@ -141,6 +145,14 @@ abstract class FormatChecker {
allFiles: allFiles,
messageCallback: messageCallback,
);
case FormatCheck.dart:
return DartFormatChecker(
processManager: processManager,
baseGitRef: baseGitRef,
repoDir: repoDir,
allFiles: allFiles,
messageCallback: messageCallback,
);
case FormatCheck.gn:
return GnFormatChecker(
processManager: processManager,
Expand Down Expand Up @@ -786,6 +798,133 @@ class GnFormatChecker extends FormatChecker {
}
}

/// Checks the format of any .dart files using the "dart format" command.
class DartFormatChecker extends FormatChecker {
DartFormatChecker({
super.processManager,
required super.baseGitRef,
required Directory repoDir,
super.allFiles,
super.messageCallback,
}) : super(
repoDir: repoDir,
) {
// $ENGINE/flutter/third_party/dart/tools/sdks/dart-sdk/bin/dart
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct, but FYI this is the prebuilt dev channel Dart SDK, and not the prebuilt Dart SDK whose version matches the hash in the DEPS file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zanderso For my own understanding: What's the difference between the two and where is the version of either of these specified?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The versions are specified in the DEPS file:

The prebuilt dev channel SDK is here:

engine/DEPS

Line 431 in 4d00124

{'dep_type': 'cipd', 'packages': [{'package': 'dart/dart-sdk/${{platform}}', 'version': 'git_revision:7fce3544047c41018b8c00b4453c0262f463dd74'}]},

vs.

here we download a prebuilt Dart SDK that matches the version of the Dart SDK source that is pulled into the engine:

engine/DEPS

Line 435 in 4d00124

# Prebuilt Dart SDK of the same revision as the Dart SDK source checkout

The version of the Dart SDK that the framework will be using is the second one.

_dartBin = path.join(
repoDir.absolute.parent.path,
'flutter',
'third_party',
'dart',
'tools',
'sdks',
'dart-sdk',
'bin',
Platform.isWindows ? 'dart.exe' : 'dart',
);
}

late final String _dartBin;

@override
Future<bool> checkFormatting() async {
message('Checking Dart formatting...');
return (await _runDartFormat(fixing: false)) == 0;
}

@override
Future<bool> fixFormatting() async {
message('Fixing Dart formatting...');
await _runDartFormat(fixing: true);
// The dart formatter shouldn't fail when fixing errors.
return true;
}

Future<int> _runDartFormat({required bool fixing}) async {
final List<String> filesToCheck = await getFileList(<String>['*.dart']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit:

// Todo: we probably should just do this on single list of files rather than forking X processes.

  • dart formatter is pretty quick
  • engineers probably aren't going to make huge changes
  • most of the time is probably spent in process start

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this modeled after the other language formatters in this file, which all use this pattern. It has the advantage of allowing us to print a nice diff between expected formatting vs. actual formatting. It is probably faster to just pass the list of files directly to the formatter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing a nice diff seems like a good motivation to follow the same pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to print a nice diff. We just need to tell people the files that are failing format checking. They should be running et format before they put the PR up (which in the engine's case is true).


final List<String> cmd = <String>[
_dartBin,
'format',
'--set-exit-if-changed',
'--show=none',
if (!fixing) '--output=show',
if (fixing) '--output=write',
];
final List<WorkerJob> jobs = <WorkerJob>[];
for (final String file in filesToCheck) {
jobs.add(WorkerJob(<String>[...cmd, file]));
}
final ProcessPool dartFmt = ProcessPool(
processRunner: _processRunner,
printReport: namedReport('dart format'),
);

Iterable<WorkerJob> incorrect;
if (!fixing) {
final Stream<WorkerJob> completedJobs = dartFmt.startWorkers(jobs);
final List<WorkerJob> diffJobs = <WorkerJob>[];
await for (final WorkerJob completedJob in completedJobs) {
if (completedJob.result.exitCode == 1) {
diffJobs.add(
WorkerJob(
<String>[
'git',
'diff',
'--no-index',
'--no-color',
'--ignore-cr-at-eol',
'--',
completedJob.command.last,
'-',
],
stdinRaw: codeUnitsAsStream(completedJob.result.stdoutRaw),
),
);
}
}
final ProcessPool diffPool = ProcessPool(
processRunner: _processRunner,
printReport: namedReport('diff'),
);
final List<WorkerJob> completedDiffs = await diffPool.runToCompletion(diffJobs);
incorrect = completedDiffs.where((WorkerJob job) {
return job.result.exitCode != 0;
});
} else {
final List<WorkerJob> completedJobs = await dartFmt.runToCompletion(jobs);
incorrect = completedJobs.where((WorkerJob job) => job.result.exitCode == 1);
}

reportDone();

if (incorrect.isNotEmpty) {
final bool plural = incorrect.length > 1;
if (fixing) {
message('Fixing ${incorrect.length} dart file${plural ? 's' : ''}'
' which ${plural ? 'were' : 'was'} formatted incorrectly.');
} else {
error('Found ${incorrect.length} Dart file${plural ? 's' : ''}'
' which ${plural ? 'were' : 'was'} formatted incorrectly.');
stdout.writeln('To fix, run `et format` or:');
stdout.writeln();
stdout.writeln('git apply <<DONE');
for (final WorkerJob job in incorrect) {
stdout.write(job.result.stdout
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}')
.replaceFirst('b/-', 'b/${job.command[job.command.length - 2]}')
.replaceFirst(RegExp('\\+Formatted \\d+ files? \\(\\d+ changed\\) in \\d+.\\d+ seconds.\n'), '')
);
}
stdout.writeln('DONE');
stdout.writeln();
}
} else {
message('All dart files formatted correctly.');
}
return incorrect.length;
}
}

/// Checks the format of any .py files using the "yapf" command.
class PythonFormatChecker extends FormatChecker {
PythonFormatChecker({
Expand Down Expand Up @@ -1144,7 +1283,8 @@ Future<int> main(List<String> arguments) async {
parser.addMultiOption('check',
abbr: 'c',
allowed: formatCheckNames(),
defaultsTo: formatCheckNames(),
// TODO(goderbauer): Enable dart by default when we turned on the formatter.
defaultsTo: formatCheckNames()..remove(FormatCheck.dart.name),
help: 'Specifies which checks will be performed. Defaults to all checks. '
'May be specified more than once to perform multiple types of checks. ');
parser.addFlag('verbose', help: 'Print verbose output.', defaultsTo: verbose);
Expand Down
27 changes: 27 additions & 0 deletions ci/test/format_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ final FileContentPair ccContentPair = FileContentPair(
'int main(){return 0;}\n', 'int main() {\n return 0;\n}\n');
final FileContentPair hContentPair =
FileContentPair('int\nmain\n()\n;\n', 'int main();\n');
final FileContentPair dartContentPair = FileContentPair(
'enum \n\nfoo {\n entry1,\n entry2,\n}', 'enum foo { entry1, entry2 }\n');
final FileContentPair gnContentPair = FileContentPair(
'test\n(){testvar=true}\n', 'test() {\n testvar = true\n}\n');
final FileContentPair javaContentPair = FileContentPair(
Expand Down Expand Up @@ -60,6 +62,10 @@ class TestFileFixture {
final io.File hFile = io.File('${repoDir.path}/format_test.h');
hFile.writeAsStringSync(hContentPair.original);
files.add(hFile);
case target.FormatCheck.dart:
final io.File dartFile = io.File('${repoDir.path}/format_test.dart');
dartFile.writeAsStringSync(dartContentPair.original);
files.add(dartFile);
case target.FormatCheck.gn:
final io.File gnFile = io.File('${repoDir.path}/format_test.gn');
gnFile.writeAsStringSync(gnContentPair.original);
Expand Down Expand Up @@ -116,6 +122,11 @@ class TestFileFixture {
? ccContentPair.formatted
: hContentPair.formatted,
);
case target.FormatCheck.dart:
return FileContentPair(
content,
dartContentPair.formatted,
);
case target.FormatCheck.gn:
return FileContentPair(
content,
Expand Down Expand Up @@ -166,6 +177,22 @@ void main() {
}
});

test('Can fix Dart formatting errors', () {
final TestFileFixture fixture = TestFileFixture(target.FormatCheck.dart);
try {
fixture.gitAdd();
io.Process.runSync(formatterPath, <String>['--check', 'dart', '--fix'],
workingDirectory: repoDir.path);

final Iterable<FileContentPair> files = fixture.getFileContents();
for (final FileContentPair pair in files) {
expect(pair.original, equals(pair.formatted));
}
} finally {
fixture.gitRemove();
}
});

test('Can fix GN formatting errors', () {
final TestFileFixture fixture = TestFileFixture(target.FormatCheck.gn);
try {
Expand Down
Loading