DEV/flutter_flutter
1
0
Fork 0
mirror of https://github.com/flutter/flutter.git synced 2026-02-20 02:29:02 +08:00
Code Issues Packages Projects Releases Wiki Activity

Enable header_guard_check (and --fix) in ci/format (flutter/engine#50102)

Browse Source 
Closes https://github.com/flutter/flutter/issues/133415.

This includes a performance optimization for the runner itself, where
`--include=...`s are used as a base to check, versus checking _every_
file and seeing if it exists in a list. This works better with what is
expected in `ci/bin/format.dart` without any contract change.

Also added a few more functional tests of the system.
This commit is contained in:
Matan Lurey 2024-01-26 16:32:14 -08:00 committed by GitHub
parent 7d955bfd7c
commit a50711f4f4
5 changed files with 316 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

104
engine/src/flutter/ci/bin/format.dart
View File

@ -39,11 +39,13 @@ enum MessageType {
}
enum FormatCheck {
clang,
gn,
java,
python,
whitespace,
header,
// Run clang after the header check.
clang,
}
FormatCheck nameToFormatCheck(String name) {
@ -58,6 +60,8 @@ FormatCheck nameToFormatCheck(String name) {
return FormatCheck.python;
case 'whitespace':
return FormatCheck.whitespace;
case 'header':
return FormatCheck.header;
default:
throw FormattingException('Unknown FormatCheck type $name');
}
@ -75,6 +79,8 @@ String formatCheckToName(FormatCheck check) {
return 'Python';
case FormatCheck.whitespace:
return 'Trailing whitespace';
case FormatCheck.header:
return 'Header guards';
}
}
@ -167,6 +173,14 @@ abstract class FormatChecker {
allFiles: allFiles,
messageCallback: messageCallback,
);
case FormatCheck.header:
return HeaderFormatChecker(
processManager: processManager,
baseGitRef: baseGitRef,
repoDir: repoDir,
allFiles: allFiles,
messageCallback: messageCallback,
);
}
}
@ -944,6 +958,94 @@ class WhitespaceFormatChecker extends FormatChecker {
}
}
final class HeaderFormatChecker extends FormatChecker {
HeaderFormatChecker({
required super.baseGitRef,
required super.repoDir,
super.processManager,
super.allFiles,
super.messageCallback,
});
// $ENGINE/third_party/dart/tools/sdks/dart-sdk/bin/dart
late final String _dartBin = path.join(
repoDir.absolute.parent.path,
'third_party',
'dart',
'tools',
'sdks',
'dart-sdk',
'bin',
'dart',
);
// $ENGINE/src/flutter/tools/bin/main.dart
late final String _headerGuardCheckBin = path.join(
repoDir.absolute.path,
'tools',
'header_guard_check',
'bin',
'main.dart',
);
@override
Future<bool> checkFormatting() async {
final List<String> include = <String>[];
if (!allFiles) {
include.addAll(await getFileList(<String>[
'*.h',
]));
if (include.isEmpty) {
message('No header files with changes, skipping header guard check.');
return true;
}
}
final List<String> args = <String>[
_dartBin,
_headerGuardCheckBin,
...include.map((String f) => '--include=$f'),
];
// TIP: --exclude is encoded into the tool itself.
// see tools/header_guard_check/lib/header_guard_check.dart
final ProcessRunnerResult result = await _processRunner.runProcess(args);
if (result.exitCode != 0) {
error('Header check failed. The following files have incorrect header guards:');
message(result.stdout);
return false;
}
return true;
}
@override
Future<bool> fixFormatting() async {
final List<String> include = <String>[];
if (!allFiles) {
include.addAll(await getFileList(<String>[
'*.h',
]));
if (include.isEmpty) {
message('No header files with changes, skipping header guard fix.');
return true;
}
}
final List<String> args = <String>[
_dartBin,
_headerGuardCheckBin,
'--fix',
...include.map((String f) => '--include=$f'),
];
// TIP: --exclude is encoded into the tool itself.
// see tools/header_guard_check/lib/header_guard_check.dart
final ProcessRunnerResult result = await _processRunner.runProcess(args);
if (result.exitCode != 0) {
error('Header check fix failed:');
message(result.stdout);
return false;
}
return true;
}
}
Future<String> _getDiffBaseRevision(ProcessManager processManager, Directory repoDir) async {
final ProcessRunner processRunner = ProcessRunner(
defaultWorkingDirectory: repoDir,

41
engine/src/flutter/ci/test/format_test.dart
View File

@ -34,6 +34,21 @@ final FileContentPair pythonContentPair = FileContentPair(
final FileContentPair whitespaceContentPair = FileContentPair(
'int main() {\n return 0; \n}\n',
'int main() {\n return 0;\n}\n');
final FileContentPair headerContentPair = FileContentPair(
<String>[
'#ifndef FOO_H_',
'#define FOO_H_',
'',
'#endif // FOO_H_',
].join('\n'),
<String>[
'#ifndef FLUTTER_FORMAT_TEST_H_',
'#define FLUTTER_FORMAT_TEST_H_',
'',
'#endif // FLUTTER_FORMAT_TEST_H_',
'',
].join('\n'),
);
class TestFileFixture {
TestFileFixture(this.type) {
@ -62,6 +77,10 @@ class TestFileFixture {
final io.File whitespaceFile = io.File('${repoDir.path}/format_test.c');
whitespaceFile.writeAsStringSync(whitespaceContentPair.original);
files.add(whitespaceFile);
case target.FormatCheck.header:
final io.File headerFile = io.File('${repoDir.path}/format_test.h');
headerFile.writeAsStringSync(headerContentPair.original);
files.add(headerFile);
}
}
@ -118,6 +137,11 @@ class TestFileFixture {
content,
whitespaceContentPair.formatted,
);
case target.FormatCheck.header:
return FileContentPair(
content,
headerContentPair.formatted,
);
}
});
}
@ -210,4 +234,21 @@ void main() {
fixture.gitRemove();
}
});
test('Can fix header guard formatting errors', () {
final TestFileFixture fixture = TestFileFixture(target.FormatCheck.header);
try {
fixture.gitAdd();
io.Process.runSync(
formatterPath, <String>['--check', 'header', '--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();
}
});
}

3
engine/src/flutter/tools/header_guard_check/README.md
View File

@ -22,9 +22,6 @@ If the header file does not follow this pattern, the tool will print an error
message and exit with a non-zero exit code. For more information about why we
use this pattern, see [the Google C++ style guide](https://google.github.io/styleguide/cppguide.html#The__define_Guard).
> [!IMPORTANT]
> This is a prototype tool and is not yet integrated into the engine's CI.
## Automatic fixes
The tool can automatically fix header files that do not follow the pattern:

81
engine/src/flutter/tools/header_guard_check/lib/header_guard_check.dart
View File

@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import 'dart:collection';
import 'dart:io' as io;
import 'package:args/args.dart';
@ -15,12 +16,14 @@ import 'src/header_file.dart';
@immutable
final class HeaderGuardCheck {
/// Creates a new header guard checker.
const HeaderGuardCheck({
HeaderGuardCheck({
required this.source,
required this.exclude,
this.include = const <String>[],
this.fix = false,
});
StringSink? stdOut,
StringSink? stdErr,
}) : _stdOut = stdOut ?? io.stdout, _stdErr = stdErr ?? io.stderr;
/// Parses the command line arguments and creates a new header guard checker.
factory HeaderGuardCheck.fromCommandLine(List<String> arguments) {
@ -45,14 +48,20 @@ final class HeaderGuardCheck {
/// Path directories to exclude from the check.
final List<String> exclude;
/// Stdout.
final StringSink _stdOut;
/// Stderr.
final StringSink _stdErr;
/// Runs the header guard check.
Future<int> run() async {
final List<HeaderFile> badFiles = _checkFiles(_findIncludedHeaderFiles()).toList();
if (badFiles.isNotEmpty) {
io.stdout.writeln('The following ${badFiles.length} files have invalid header guards:');
_stdOut.writeln('The following ${badFiles.length} files have invalid header guards:');
for (final HeaderFile headerFile in badFiles) {
io.stdout.writeln(' ${headerFile.path}');
_stdOut.writeln(' ${headerFile.path}');
}
// If we're fixing, fix the files.
@ -61,7 +70,7 @@ final class HeaderGuardCheck {
headerFile.fix(engineRoot: source.flutterDir.path);
}
io.stdout.writeln('Fixed ${badFiles.length} files.');
_stdOut.writeln('Fixed ${badFiles.length} files.');
return 0;
}
@ -72,32 +81,33 @@ final class HeaderGuardCheck {
}
Iterable<io.File> _findIncludedHeaderFiles() sync* {
final io.Directory dir = source.flutterDir;
for (final io.FileSystemEntity entity in dir.listSync(recursive: true)) {
if (entity is! io.File) {
continue;
}
if (!entity.path.endsWith('.h')) {
continue;
}
if (!_isIncluded(entity.path) || _isExcluded(entity.path)) {
continue;
}
yield entity;
final Queue<String> queue = Queue<String>();
final Set<String> yielded = <String>{};
if (include.isEmpty) {
queue.add(source.flutterDir.path);
} else {
queue.addAll(include);
}
}
bool _isIncluded(String path) {
for (final String includePath in include) {
final String relativePath = p.relative(includePath, from: source.flutterDir.path);
if (p.isWithin(relativePath, path) || p.equals(relativePath, path)) {
return true;
while (queue.isNotEmpty) {
final String path = queue.removeFirst();
if (path.endsWith('.h')) {
if (!_isExcluded(path) && yielded.add(path)) {
yield io.File(path);
}
} else if (io.FileSystemEntity.isDirectorySync(path)) {
if (_isExcluded(path)) {
continue;
}
final io.Directory directory = io.Directory(path);
for (final io.FileSystemEntity entity in directory.listSync(recursive: true)) {
if (entity is io.File && entity.path.endsWith('.h')) {
queue.add(entity.path);
}
}
} else {
// Neither a header file nor a directory that might contain header files.
}
}
return include.isEmpty;
}
bool _isExcluded(String path) {
@ -114,27 +124,32 @@ final class HeaderGuardCheck {
for (final io.File header in headers) {
final HeaderFile headerFile = HeaderFile.parse(header.path);
if (headerFile.pragmaOnce != null) {
io.stderr.writeln(headerFile.pragmaOnce!.message('Unexpected #pragma once'));
_stdErr.writeln(headerFile.pragmaOnce!.message('Unexpected #pragma once'));
yield headerFile;
continue;
}
if (headerFile.guard == null) {
io.stderr.writeln('Missing header guard in ${headerFile.path}');
_stdErr.writeln('Missing header guard in ${headerFile.path}');
yield headerFile;
continue;
}
final String expectedGuard = headerFile.computeExpectedName(engineRoot: source.flutterDir.path);
if (headerFile.guard!.ifndefValue != expectedGuard) {
io.stderr.writeln(headerFile.guard!.ifndefSpan!.message('Expected #ifndef $expectedGuard'));
_stdErr.writeln(headerFile.guard!.ifndefSpan!.message('Expected #ifndef $expectedGuard'));
yield headerFile;
continue;
}
if (headerFile.guard!.defineValue != expectedGuard) {
io.stderr.writeln(headerFile.guard!.defineSpan!.message('Expected #define $expectedGuard'));
_stdErr.writeln(headerFile.guard!.defineSpan!.message('Expected #define $expectedGuard'));
yield headerFile;
continue;
}
if (headerFile.guard!.endifValue != expectedGuard) {
io.stderr.writeln(headerFile.guard!.endifSpan!.message('Expected #endif // $expectedGuard'));
_stdErr.writeln(headerFile.guard!.endifSpan!.message('Expected #endif // $expectedGuard'));
yield headerFile;
continue;
}
}
}

124
engine/src/flutter/tools/header_guard_check/test/header_guard_check_test.dart Normal file
View File

@ -0,0 +1,124 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import 'dart:io' as io;
import 'package:engine_repo_tools/engine_repo_tools.dart';
import 'package:header_guard_check/header_guard_check.dart';
import 'package:litetest/litetest.dart';
import 'package:path/path.dart' as p;
Future<int> main(List<String> args) async {
void withTestRepository(String path, void Function(io.Directory) fn) {
// Create a temporary directory and delete it when we're done.
final io.Directory tempDir = io.Directory.systemTemp.createTempSync('header_guard_check_test');
final io.Directory repoDir = io.Directory(p.join(tempDir.path, path));
repoDir.createSync(recursive: true);
try {
fn(repoDir);
} finally {
tempDir.deleteSync(recursive: true);
}
}
group('HeaderGuardCheck', () {
test('by default checks all files', () {
withTestRepository('engine/src', (io.Directory repoDir) {
final io.Directory flutterDir = io.Directory(p.join(repoDir.path, 'flutter'));
flutterDir.createSync(recursive: true);
final io.File file1 = io.File(p.join(flutterDir.path, 'foo.h'));
file1.createSync(recursive: true);
final io.File file2 = io.File(p.join(flutterDir.path, 'bar.h'));
file2.createSync(recursive: true);
final io.File file3 = io.File(p.join(flutterDir.path, 'baz.h'));
file3.createSync(recursive: true);
final StringBuffer stdOut = StringBuffer();
final StringBuffer stdErr = StringBuffer();
final HeaderGuardCheck check = HeaderGuardCheck(
source: Engine.fromSrcPath(repoDir.path),
exclude: const <String>[],
stdOut: stdOut,
stdErr: stdErr,
);
expect(check.run(), completion(1));
expect(stdOut.toString(), contains('foo.h'));
expect(stdOut.toString(), contains('bar.h'));
expect(stdOut.toString(), contains('baz.h'));
});
});
test('if --include is provided, checks specific files', () {
withTestRepository('engine/src', (io.Directory repoDir) {
final io.Directory flutterDir = io.Directory(p.join(repoDir.path, 'flutter'));
flutterDir.createSync(recursive: true);
final io.File file1 = io.File(p.join(flutterDir.path, 'foo.h'));
file1.createSync(recursive: true);
final io.File file2 = io.File(p.join(flutterDir.path, 'bar.h'));
file2.createSync(recursive: true);
final io.File file3 = io.File(p.join(flutterDir.path, 'baz.h'));
file3.createSync(recursive: true);
final StringBuffer stdOut = StringBuffer();
final StringBuffer stdErr = StringBuffer();
final HeaderGuardCheck check = HeaderGuardCheck(
source: Engine.fromSrcPath(repoDir.path),
include: <String>[file1.path, file3.path],
exclude: const <String>[],
stdOut: stdOut,
stdErr: stdErr,
);
expect(check.run(), completion(1));
expect(stdOut.toString(), contains('foo.h'));
expect(stdOut.toString(), contains('baz.h'));
// TODO(matanlurey): https://github.com/flutter/flutter/issues/133569).
if (stdOut.toString().contains('bar.h')) {
// There is no not(contains(...)) matcher.
fail('bar.h should not be checked. Output: $stdOut');
}
});
});
test('if --include is provided, checks specific directories', () {
withTestRepository('engine/src', (io.Directory repoDir) {
final io.Directory flutterDir = io.Directory(p.join(repoDir.path, 'flutter'));
flutterDir.createSync(recursive: true);
// Create a sub-directory called "impeller".
final io.Directory impellerDir = io.Directory(p.join(flutterDir.path, 'impeller'));
impellerDir.createSync(recursive: true);
// Create one file in both the root and in impeller.
final io.File file1 = io.File(p.join(flutterDir.path, 'foo.h'));
file1.createSync(recursive: true);
final io.File file2 = io.File(p.join(impellerDir.path, 'bar.h'));
file2.createSync(recursive: true);
final StringBuffer stdOut = StringBuffer();
final StringBuffer stdErr = StringBuffer();
final HeaderGuardCheck check = HeaderGuardCheck(
source: Engine.fromSrcPath(repoDir.path),
include: <String>[impellerDir.path],
exclude: const <String>[],
stdOut: stdOut,
stdErr: stdErr,
);
expect(check.run(), completion(1));
expect(stdOut.toString(), contains('bar.h'));
// TODO(matanlurey): https://github.com/flutter/flutter/issues/133569).
if (stdOut.toString().contains('foo.h')) {
// There is no not(contains(...)) matcher.
fail('foo.h should not be checked. Output: $stdOut');
}
});
});
});
return 0;
}