From 925bea899f5e8681f93c223b6512d52222910a39 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Fri, 17 Feb 2023 14:15:46 -0800 Subject: [PATCH] [pigeon] Validate generated files in CI (#3224) * Add autoformatting option to generation * Extract to helper * Add validation * Test change to demonstrate behavior * Actually fail when validation fails * Revert "Test change to demonstrate behavior" This reverts commit 7c47d56a6d6b5892b41977df64dab377c5cdd701. --- packages/pigeon/tool/generate.dart | 34 ++++++++- packages/pigeon/tool/run_tests.dart | 84 ++++++++++++++++++++- packages/pigeon/tool/shared/generation.dart | 24 ++++++ 3 files changed, 138 insertions(+), 4 deletions(-) diff --git a/packages/pigeon/tool/generate.dart b/packages/pigeon/tool/generate.dart index d04b486590b29..946a69c4c6763 100644 --- a/packages/pigeon/tool/generate.dart +++ b/packages/pigeon/tool/generate.dart @@ -12,19 +12,47 @@ import 'dart:io' show Platform, exit; +import 'package:args/args.dart'; import 'package:path/path.dart' as p; import 'shared/generation.dart'; +const String _helpFlag = 'help'; +const String _formatFlag = 'format'; + Future main(List args) async { + final ArgParser parser = ArgParser() + ..addFlag(_formatFlag, abbr: 'f', help: 'Autoformats after generation.') + ..addFlag(_helpFlag, + negatable: false, abbr: 'h', help: 'Print this reference.'); + + final ArgResults argResults = parser.parse(args); + if (argResults.wasParsed(_helpFlag)) { + print(''' +usage: dart run tool/generate.dart [options] + +${parser.usage}'''); + exit(0); + } + final String baseDir = p.dirname(p.dirname(Platform.script.toFilePath())); print('Generating platform_test/ output...'); - final int exitCode = await generatePigeons(baseDir: baseDir); - if (exitCode == 0) { + final int generateExitCode = await generatePigeons(baseDir: baseDir); + if (generateExitCode == 0) { print('Generation complete!'); } else { print('Generation failed; see above for errors.'); + exit(generateExitCode); + } + + if (argResults.wasParsed(_formatFlag)) { + print('Formatting generated output...'); + final int formatExitCode = + await formatAllFiles(repositoryRoot: p.dirname(p.dirname(baseDir))); + if (formatExitCode != 0) { + print('Formatting failed; see above for errors.'); + exit(formatExitCode); + } } - exit(exitCode); } diff --git a/packages/pigeon/tool/run_tests.dart b/packages/pigeon/tool/run_tests.dart index dab54247a30ad..f7bc3530005b6 100644 --- a/packages/pigeon/tool/run_tests.dart +++ b/packages/pigeon/tool/run_tests.dart @@ -9,8 +9,11 @@ /// /// For any use other than CI, use test.dart instead. //////////////////////////////////////////////////////////////////////////////// -import 'dart:io' show Platform, exit; +import 'dart:io'; +import 'package:path/path.dart' as p; + +import 'shared/generation.dart'; import 'shared/test_runner.dart'; import 'shared/test_suites.dart'; @@ -29,6 +32,77 @@ void _validateTestCoverage(List> shards) { } } +Future _validateGeneratedTestFiles() async { + final String baseDir = p.dirname(p.dirname(Platform.script.toFilePath())); + final String repositoryRoot = p.dirname(p.dirname(baseDir)); + final String relativePigeonPath = p.relative(baseDir, from: repositoryRoot); + + print('Validating generated files:'); + print(' Generating output...'); + final int generateExitCode = await generatePigeons(baseDir: baseDir); + if (generateExitCode != 0) { + print('Generation failed; see above for errors.'); + exit(generateExitCode); + } + + print(' Formatting output...'); + final int formatExitCode = + await formatAllFiles(repositoryRoot: repositoryRoot); + if (formatExitCode != 0) { + print('Formatting failed; see above for errors.'); + exit(formatExitCode); + } + + print(' Checking for changes...'); + final List modifiedFiles = await _modifiedFiles( + repositoryRoot: repositoryRoot, relativePigeonPath: relativePigeonPath); + + if (modifiedFiles.isEmpty) { + return; + } + + print('The following files are not updated, or not formatted correctly:'); + modifiedFiles.map((String line) => ' $line').forEach(print); + + print('\nTo fix run "dart run tool/generate.dart --format" from the pigeon/ ' + 'directory, or apply the diff with the command below.\n'); + + final ProcessResult diffResult = await Process.run( + 'git', + ['diff', relativePigeonPath], + workingDirectory: repositoryRoot, + ); + if (diffResult.exitCode != 0) { + print('Unable to determine diff.'); + exit(1); + } + print('patch -p1 <> _modifiedFiles( + {required String repositoryRoot, + required String relativePigeonPath}) async { + final ProcessResult result = await Process.run( + 'git', + ['ls-files', '--modified', relativePigeonPath], + workingDirectory: repositoryRoot, + ); + if (result.exitCode != 0) { + print('Unable to determine changed files.'); + print(result.stdout); + print(result.stderr); + exit(1); + } + return (result.stdout as String) + .split('\n') + .map((String line) => line.trim()) + .where((String line) => line.isNotEmpty) + .toList(); +} + Future main(List args) async { // Run most tests on Linux, since Linux tends to be the easiest and cheapest. const List linuxHostTests = [ @@ -85,6 +159,14 @@ Future main(List args) async { ], ]); + // Ensure that all generated files are up to date. This is run only on Linux + // both to avoid duplication of work, and to avoid issues if different CI + // configurations have different setups (e.g., different clang-format versions + // or no clang-format at all). + if (Platform.isLinux) { + await _validateGeneratedTestFiles(); + } + final List testsToRun; if (Platform.isMacOS) { if (Platform.environment['LUCI_CI'] != null) { diff --git a/packages/pigeon/tool/shared/generation.dart b/packages/pigeon/tool/shared/generation.dart index 9cc1eedfe091b..bc5674bf17b7b 100644 --- a/packages/pigeon/tool/shared/generation.dart +++ b/packages/pigeon/tool/shared/generation.dart @@ -2,9 +2,13 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:io' show Platform; + import 'package:path/path.dart' as p; import 'package:pigeon/pigeon.dart'; +import 'process_utils.dart'; + enum GeneratorLanguages { cpp, java, @@ -166,3 +170,23 @@ Future runPigeon({ swiftOptions: const SwiftOptions(), )); } + +/// Runs the repository tooling's format command on this package. +/// +/// This is intended for formatting generated autoput, but since there's no +/// way to filter to specific files in with the repo tooling it runs over the +/// entire package. +Future formatAllFiles({required String repositoryRoot}) { + final String dartCommand = Platform.isWindows ? 'dart.exe' : 'dart'; + return runProcess( + dartCommand, + [ + 'run', + 'script/tool/bin/flutter_plugin_tools.dart', + 'format', + '--packages=pigeon', + ], + streamOutput: false, + workingDirectory: repositoryRoot, + logFailure: true); +}