From a50711f4f417c0d596c79b38070c8ff0da54aba2 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 26 Jan 2024 16:32:14 -0800 Subject: [PATCH] Enable `header_guard_check` (and `--fix`) in `ci/format` (flutter/engine#50102) 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. --- engine/src/flutter/ci/bin/format.dart | 104 ++++++++++++++- engine/src/flutter/ci/test/format_test.dart | 41 ++++++ .../tools/header_guard_check/README.md | 3 - .../lib/header_guard_check.dart | 81 +++++++----- .../test/header_guard_check_test.dart | 124 ++++++++++++++++++ 5 files changed, 316 insertions(+), 37 deletions(-) create mode 100644 engine/src/flutter/tools/header_guard_check/test/header_guard_check_test.dart diff --git a/engine/src/flutter/ci/bin/format.dart b/engine/src/flutter/ci/bin/format.dart index 3eebfb23896..207c152a6f2 100644 --- a/engine/src/flutter/ci/bin/format.dart +++ b/engine/src/flutter/ci/bin/format.dart @@ -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 checkFormatting() async { + final List include = []; + if (!allFiles) { + include.addAll(await getFileList([ + '*.h', + ])); + if (include.isEmpty) { + message('No header files with changes, skipping header guard check.'); + return true; + } + } + final List args = [ + _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 fixFormatting() async { + final List include = []; + if (!allFiles) { + include.addAll(await getFileList([ + '*.h', + ])); + if (include.isEmpty) { + message('No header files with changes, skipping header guard fix.'); + return true; + } + } + final List args = [ + _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 _getDiffBaseRevision(ProcessManager processManager, Directory repoDir) async { final ProcessRunner processRunner = ProcessRunner( defaultWorkingDirectory: repoDir, diff --git a/engine/src/flutter/ci/test/format_test.dart b/engine/src/flutter/ci/test/format_test.dart index c603e800eb2..f31a944bffe 100644 --- a/engine/src/flutter/ci/test/format_test.dart +++ b/engine/src/flutter/ci/test/format_test.dart @@ -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( + [ + '#ifndef FOO_H_', + '#define FOO_H_', + '', + '#endif // FOO_H_', + ].join('\n'), + [ + '#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, ['--check', 'header', '--fix'], + workingDirectory: repoDir.path, + ); + final Iterable files = fixture.getFileContents(); + for (final FileContentPair pair in files) { + expect(pair.original, equals(pair.formatted)); + } + } finally { + fixture.gitRemove(); + } + }); } diff --git a/engine/src/flutter/tools/header_guard_check/README.md b/engine/src/flutter/tools/header_guard_check/README.md index 20ec093564f..a2118b563c9 100644 --- a/engine/src/flutter/tools/header_guard_check/README.md +++ b/engine/src/flutter/tools/header_guard_check/README.md @@ -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: diff --git a/engine/src/flutter/tools/header_guard_check/lib/header_guard_check.dart b/engine/src/flutter/tools/header_guard_check/lib/header_guard_check.dart index f7880c7fe56..d9b925715cb 100644 --- a/engine/src/flutter/tools/header_guard_check/lib/header_guard_check.dart +++ b/engine/src/flutter/tools/header_guard_check/lib/header_guard_check.dart @@ -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 [], 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 arguments) { @@ -45,14 +48,20 @@ final class HeaderGuardCheck { /// Path directories to exclude from the check. final List exclude; + /// Stdout. + final StringSink _stdOut; + + /// Stderr. + final StringSink _stdErr; + /// Runs the header guard check. Future run() async { final List 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 _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 queue = Queue(); + final Set yielded = {}; + 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; } } } diff --git a/engine/src/flutter/tools/header_guard_check/test/header_guard_check_test.dart b/engine/src/flutter/tools/header_guard_check/test/header_guard_check_test.dart new file mode 100644 index 00000000000..cf4ed35435b --- /dev/null +++ b/engine/src/flutter/tools/header_guard_check/test/header_guard_check_test.dart @@ -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 main(List 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 [], + 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: [file1.path, file3.path], + exclude: const [], + 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: [impellerDir.path], + exclude: const [], + 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; +}