diff --git a/engine/src/flutter/ci/check_build_configs.sh b/engine/src/flutter/ci/check_build_configs.sh index 425e9dda5c..077c66d1d9 100755 --- a/engine/src/flutter/ci/check_build_configs.sh +++ b/engine/src/flutter/ci/check_build_configs.sh @@ -44,5 +44,5 @@ DART="${DART_BIN}/dart" cd "$SCRIPT_DIR" "$DART" \ "$SRC_DIR/flutter/tools/pkg/engine_build_configs/bin/check.dart" \ - "$SRC_DIR" + "--engine-src-path=$SRC_DIR" diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/bin/check.dart b/engine/src/flutter/tools/pkg/engine_build_configs/bin/check.dart index ef03855389..b7d135c17a 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/bin/check.dart +++ b/engine/src/flutter/tools/pkg/engine_build_configs/bin/check.dart @@ -2,34 +2,89 @@ // 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 show Directory, exitCode, stderr; +import 'dart:io' as io; +import 'package:args/args.dart'; import 'package:engine_build_configs/engine_build_configs.dart'; +import 'package:engine_build_configs/src/ci_yaml.dart'; import 'package:engine_repo_tools/engine_repo_tools.dart'; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; +import 'package:source_span/source_span.dart'; +import 'package:yaml/yaml.dart' as y; // Usage: -// $ dart bin/check.dart [/path/to/engine/src] +// $ dart bin/check.dart +// +// Or, for more options: +// $ dart bin/check.dart --help + +final _argParser = + ArgParser() + ..addFlag('verbose', abbr: 'v', help: 'Enable noisier diagnostic output', negatable: false) + ..addFlag('help', abbr: 'h', help: 'Output usage information.', negatable: false) + ..addOption( + 'engine-src-path', + valueHelp: '/path/to/engine/src', + defaultsTo: Engine.tryFindWithin()?.srcDir.path, + ); void main(List args) { - final String? engineSrcPath; - if (args.isNotEmpty) { - engineSrcPath = args[0]; - } else { - engineSrcPath = null; - } + run( + args, + stderr: io.stderr, + stdout: io.stdout, + platform: const LocalPlatform(), + setExitCode: (exitCode) { + io.exitCode = exitCode; + }, + ); +} - // Find the engine repo. - final Engine engine; - try { - engine = Engine.findWithin(engineSrcPath); - } catch (e) { - io.stderr.writeln(e); - io.exitCode = 1; +@visibleForTesting +void run( + Iterable args, { + required Platform platform, + required StringSink stderr, + required StringSink stdout, + required void Function(int) setExitCode, +}) { + y.yamlWarningCallback = (String message, [SourceSpan? span]) {}; + + final argResults = _argParser.parse(args); + if (argResults.flag('help')) { + stdout.writeln(_argParser.usage); return; } + final verbose = argResults.flag('verbose'); + void debugPrint(String output) { + if (!verbose) { + return; + } + stderr.writeln(output); + } + + void indentedPrint(Iterable errors) { + for (final error in errors) { + stderr.writeln(' $error'); + } + } + + final supportsEmojis = !platform.isWindows || platform.environment.containsKey('WT_SESSION'); + final symbolSuccess = supportsEmojis ? '✅' : '✓'; + final symbolFailure = supportsEmojis ? '❌' : 'X'; + void statusPrint(String describe, {required bool success}) { + stderr.writeln('${success ? symbolSuccess : symbolFailure} $describe'); + if (!success) { + setExitCode(1); + } + } + + final engine = Engine.fromSrcPath(argResults.option('engine-src-path')!); + debugPrint('Initializing from ${p.relative(engine.srcDir.path)}'); + // Find and parse the engine build configs. final io.Directory buildConfigsDir = io.Directory( p.join(engine.flutterDir.path, 'ci', 'builders'), @@ -39,36 +94,112 @@ void main(List args) { // Treat it as an error if no build configs were found. The caller likely // expected to find some. final Map configs = loader.configs; + + // We can't make further progress if we didn't find any configurations. + statusPrint( + 'Loaded build configs under ${p.relative(buildConfigsDir.path)}', + success: configs.isNotEmpty && loader.errors.isEmpty, + ); if (configs.isEmpty) { - io.stderr.writeln('Error: No build configs found under ${buildConfigsDir.path}'); - io.exitCode = 1; return; } - if (loader.errors.isNotEmpty) { - loader.errors.forEach(io.stderr.writeln); - io.exitCode = 1; + indentedPrint(loader.errors); + + // Find and parse the .ci.yaml configuration (for the engine). + final CiConfig? ciConfig; + { + final String ciYamlPath = p.join(engine.flutterDir.path, '.ci.yaml'); + final String realCiYaml = io.File(ciYamlPath).readAsStringSync(); + final y.YamlNode yamlNode = y.loadYamlNode(realCiYaml, sourceUrl: Uri.file(ciYamlPath)); + final loadedConfig = CiConfig.fromYaml(yamlNode); + + statusPrint('.ci.yaml at ${p.relative(ciYamlPath)} is valid', success: loadedConfig.valid); + if (!loadedConfig.valid) { + indentedPrint([loadedConfig.error!]); + ciConfig = null; + } else { + ciConfig = loadedConfig; + } } // Check the parsed build configs for validity. final List invalidErrors = checkForInvalidConfigs(configs); - if (invalidErrors.isNotEmpty) { - invalidErrors.forEach(io.stderr.writeln); - io.exitCode = 1; - } + statusPrint('All configuration files are valid', success: invalidErrors.isEmpty); + indentedPrint(invalidErrors); // We require all builds within a builder config to be uniquely named. final List duplicateErrors = checkForDuplicateConfigs(configs); - if (duplicateErrors.isNotEmpty) { - duplicateErrors.forEach(io.stderr.writeln); - io.exitCode = 1; - } + statusPrint('All builds within a builder are uniquely named', success: duplicateErrors.isEmpty); + indentedPrint(duplicateErrors); // We require all builds to be named in a way that is understood by et. final List buildNameErrors = checkForInvalidBuildNames(configs); - if (buildNameErrors.isNotEmpty) { - buildNameErrors.forEach(io.stderr.writeln); - io.exitCode = 1; + statusPrint('All build names must have a conforming prefix', success: buildNameErrors.isEmpty); + indentedPrint(buildNameErrors); + + // If we have a successfully parsed .ci.yaml, perform additional checks. + if (ciConfig == null) { + return; } + + // We require that targets that have `properties: release_build: "true"`: + // (1) Each sub-build produces artifacts (`archives: [...]`) + // (2) Each sub-build does not have `tests: [ ... ]` + final buildConventionErrors = []; + for (final MapEntry(key: _, value: target) in ciConfig.ciTargets.entries) { + final config = loader.configs[target.properties.configName]; + if (target.properties.configName == null) { + // * builder_cache targets do not have configuration files. + debugPrint(' Skipping ${target.name}: No configuration file found'); + continue; + } + + // This would fail above during the general loading. + if (config == null) { + throw StateError('Unreachable'); + } + + final configConventionErrors = []; + if (target.properties.isReleaseBuilder) { + // If there is a global generators step, assume artifacts are uploaded from the generators. + if (config.generators.isNotEmpty) { + debugPrint(' Skipping ${target.name}: Has "generators": [ ... ] which could do anything'); + continue; + } + // Check each build: it must have "archives: [ ... ]" and NOT "tests: [ ... ]" + for (final build in config.builds) { + if (build.archives.isEmpty) { + configConventionErrors.add('${build.name}: Does not have "archives: [ ... ]"'); + } + if (build.archives.any((e) => e.includePaths.isEmpty)) { + configConventionErrors.add( + '${build.name}: Has an archive with an empty "include_paths": []', + ); + } + if (build.tests.isNotEmpty) { + // TODO(matanlurey): https://github.com/flutter/flutter/issues/161990. + if (target.properties.configName == 'windows_host_engine' && + build.name == r'ci\host_debug') { + debugPrint(' Skipping: ${build.name}: Allow-listed during migration'); + } else { + configConventionErrors.add('${build.name}: Includes "tests: [ ... ]"'); + } + } + } + } + + if (configConventionErrors.isNotEmpty) { + buildConventionErrors.add( + '${p.basename(config.path)} (${target.name}, release_build = ${target.properties.isReleaseBuilder}):', + ); + buildConventionErrors.addAll(configConventionErrors.map((e) => ' $e')); + } + } + statusPrint( + 'All builder files conform to release_build standards', + success: buildConventionErrors.isEmpty, + ); + indentedPrint(buildConventionErrors); } // This check ensures that all the json files were deserialized without errors. diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/ci_yaml.dart b/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/ci_yaml.dart index f5512cfff9..25d3de2757 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/ci_yaml.dart +++ b/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/ci_yaml.dart @@ -18,6 +18,7 @@ const String _nameField = 'name'; const String _recipeField = 'recipe'; const String _propertiesField = 'properties'; const String _configNameField = 'config_name'; +const String _releaseBuildField = 'release_build'; /// A class containing the information deserialized from the .ci.yaml file. /// @@ -92,7 +93,7 @@ class CiTarget { return CiTarget._error(error); } final y.YamlMap targetMap = yaml; - final String? name = _stringOfNode(targetMap.nodes[_nameField]); + final String? name = targetMap.nodes[_nameField].readStringOrNull(); if (name == null) { final String error = targetMap.span.message( 'Expected map to contain a string value for key "$_nameField".', @@ -100,7 +101,7 @@ class CiTarget { return CiTarget._error(error); } - final String? recipe = _stringOfNode(targetMap.nodes[_recipeField]); + final String? recipe = targetMap.nodes[_recipeField].readStringOrNull(); if (recipe == null) { final String error = targetMap.span.message( 'Expected map to contain a string value for key "$_recipeField".', @@ -159,18 +160,23 @@ class CiTargetProperties { return CiTargetProperties._error(error); } final y.YamlMap propertiesMap = yaml; - final String? configName = _stringOfNode(propertiesMap.nodes[_configNameField]); - return CiTargetProperties._(configName: configName ?? ''); + return CiTargetProperties._( + configName: propertiesMap.nodes[_configNameField].readStringOrNull(), + isReleaseBuilder: propertiesMap.nodes[_releaseBuildField].readStringOrNull() == 'true', + ); } - CiTargetProperties._({required this.configName}) : error = null; + CiTargetProperties._({required this.configName, required this.isReleaseBuilder}) : error = null; - CiTargetProperties._error(this.error) : configName = ''; + CiTargetProperties._error(this.error) : configName = '', isReleaseBuilder = false; /// The name of the build configuration. If the containing [CiTarget] instance /// is using the engine_v2 recipes, then this name is the same as the name /// of the build config json file under ci/builders. - final String configName; + final String? configName; + + /// Whether this is a release builder. + final bool isReleaseBuilder; /// An error message when this instance is invalid. final String? error; @@ -179,16 +185,25 @@ class CiTargetProperties { late final bool valid = error == null; } -String? _stringOfNode(y.YamlNode? stringNode) { - if (stringNode == null) { - return null; +/// Extensions for reading and partially validating typed values from YAML. +extension on y.YamlNode? { + // TODO(matanlurey): Much of this should really be an error but that also + // predicates a lot more work put into .ci.yaml validation that is better + // served by having a dedicated library: + // https://github.com/flutter/flutter/issues/161971. + + T? _readOrNull() { + final node = this; + if (node is! y.YamlScalar) { + return null; + } + final Object? value = node.value; + if (value is! T) { + return null; + } + return value; } - if (stringNode is! y.YamlScalar) { - return null; - } - final y.YamlScalar stringScalar = stringNode; - if (stringScalar.value is! String) { - return null; - } - return stringScalar.value as String; + + /// Returns this node as a string if possible. + String? readStringOrNull() => _readOrNull(); } diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/pubspec.yaml b/engine/src/flutter/tools/pkg/engine_build_configs/pubspec.yaml index d3e29b99df..1e33345fae 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/pubspec.yaml +++ b/engine/src/flutter/tools/pkg/engine_build_configs/pubspec.yaml @@ -20,9 +20,9 @@ dependencies: path: any platform: any process_runner: any + source_span: any yaml: any dev_dependencies: process_fakes: any - source_span: any test: any diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/test/check_integration_test.dart b/engine/src/flutter/tools/pkg/engine_build_configs/test/check_integration_test.dart new file mode 100644 index 0000000000..278baec3db --- /dev/null +++ b/engine/src/flutter/tools/pkg/engine_build_configs/test/check_integration_test.dart @@ -0,0 +1,233 @@ +// 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. + +@TestOn('vm') +library; + +import 'dart:convert'; +import 'dart:io' as io; + +import 'package:path/path.dart' as p; +import 'package:platform/platform.dart'; +import 'package:test/test.dart'; + +import '../bin/check.dart' as check show run; + +void main() { + late io.Directory tmpFlutterEngineRoot; + late StringBuffer stdout; + late StringBuffer stderr; + late int exitCode; + + late io.Directory tmpFlutterEngineSrc; + late io.Directory tmpCiBuilders; + late io.File tmpCiYaml; + late List> ciYamlTargets; + + setUp(() { + tmpFlutterEngineRoot = io.Directory.systemTemp.createTempSync('check_integration_test.'); + stdout = StringBuffer(); + stderr = StringBuffer(); + exitCode = 0; + + // Create a synthetic engine directory. + tmpFlutterEngineSrc = io.Directory(p.join(tmpFlutterEngineRoot.path, 'flutter', 'src')) + ..createSync(recursive: true); + tmpCiBuilders = io.Directory(p.join(tmpFlutterEngineSrc.path, 'flutter', 'ci', 'builders')) + ..createSync(recursive: true); + tmpCiYaml = io.File(p.join(p.join(tmpFlutterEngineSrc.path, 'flutter', '.ci.yaml'))); + ciYamlTargets = []; + }); + + tearDown(() { + tmpFlutterEngineRoot.deleteSync(recursive: true); + }); + + void run(Iterable args, {Platform? platform, bool allowFailure = false}) { + check.run( + args, + stderr: stderr, + stdout: stdout, + setExitCode: (newExitCode) { + exitCode = newExitCode; + }, + platform: platform ?? FakePlatform(operatingSystem: Platform.linux), + ); + if (exitCode != 0 && !allowFailure) { + fail('$args failed: $stderr'); + } + } + + test('should produce usage on --help', () { + run(['--help']); + + expect( + stdout.toString(), + allOf([contains('--verbose'), contains('--help'), contains('--engine-src-path')]), + ); + }); + + test('fails if no build configurations were found', () { + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ Loaded build configs under')); + }); + + void addConfig( + String name, + List> builds, { + bool releaseBuild = false, + bool specifyConfig = true, + bool writeGenerators = false, + }) { + if (specifyConfig) { + io.File(p.join(tmpCiBuilders.path, '$name.json')).writeAsStringSync( + jsonEncode({ + 'builds': builds, + if (writeGenerators) + 'generators': { + 'tasks': [{}], + }, + }), + ); + } + + ciYamlTargets.add({ + 'name': name, + 'recipe': 'flutter/some_recipe', + 'properties': { + if (specifyConfig) 'config_name': name, + if (releaseBuild) 'release_build': 'true', + }, + }); + tmpCiYaml.writeAsStringSync(jsonEncode({'targets': ciYamlTargets})); + } + + test('fails if a configuration file had a deserialization error', () { + addConfig('linux_unopt', []); + + // Malform the .ci.yaml file. + tmpCiYaml.writeAsStringSync('bad{}'); + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ .ci.yaml at')); + }); + + test('fails if an individual builder has a schema error', () { + addConfig('linux_unopt', [ + {'ninja': 1234}, + ]); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ All configuration files are valid')); + }); + + test('fails if all builds within a builder are not uniquely named', () { + addConfig('linux_unopt', [ + {'name': 'foo'}, + {'name': 'foo'}, + ]); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ All builds within a builder are uniquely named')); + }); + + test('fails if a build does not use a conforming OS prefix or "ci"', () { + addConfig('linux_unopt', [ + {'name': 'not_an_os_or_ci/foo'}, + ]); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ All build names must have a conforming prefix')); + }); + + test('skips targets without a config_name', () { + addConfig('linux_unopt', []); + addConfig('linux_cache', [], specifyConfig: false); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}', '--verbose']); + + expect(stderr.toString(), contains('Skipping linux_cache')); + }); + + test('skips checking if a global "generators" field is present', () { + addConfig( + 'linux_befuzzled', + [ + {'name': 'ci/test'}, + ], + releaseBuild: true, + writeGenerators: true, + ); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}', '--verbose']); + + expect(stderr.toString(), contains('Skipping linux_befuzzled: Has "generators"')); + }); + + test('fails if a release builder omits archives', () { + addConfig('linux_engine', [ + { + 'name': 'ci/host_debug', + 'archives': [{}], + }, + ], releaseBuild: true); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ All builder files conform to release_build standards')); + }); + + test('fails if a release builder includes tests', () { + addConfig('linux_engine', [ + { + 'name': 'ci/host_debug', + 'archives': [ + { + 'include_paths': ['out/foo'], + }, + ], + 'tests': [{}], + }, + ], releaseBuild: true); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ All builder files conform to release_build standards')); + }); + + test('allows a release builder if allow-listed', () { + addConfig('windows_host_engine', [ + { + 'name': r'ci\host_debug', + 'archives': [ + { + 'include_paths': ['out/foo'], + }, + ], + 'tests': [{}], + }, + ], releaseBuild: true); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}']); + }); + + test('fails if archives.include_paths is empty', () { + addConfig('linux_engine', [ + { + 'name': 'ci/host_debug', + 'archives': [ + {'include_paths': []}, + ], + }, + ], releaseBuild: true); + + run(['--engine-src-path=${tmpFlutterEngineSrc.path}'], allowFailure: true); + + expect(stderr.toString(), contains('❌ All builder files conform to release_build standards')); + }); +} diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/test/ci_yaml_test.dart b/engine/src/flutter/tools/pkg/engine_build_configs/test/ci_yaml_test.dart index ea27f1cf91..00c32927de 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/test/ci_yaml_test.dart +++ b/engine/src/flutter/tools/pkg/engine_build_configs/test/ci_yaml_test.dart @@ -43,6 +43,7 @@ targets: recipe: engine_v2/engine_v2 properties: config_name: linux_build + release_build: "true" '''; final y.YamlNode yamlNode = y.loadYamlNode(yamlData, sourceUrl: Uri.file(ciYamlPath)); final CiConfig config = CiConfig.fromYaml(yamlNode); @@ -57,6 +58,7 @@ targets: expect(config.ciTargets['Linux linux_build']!.recipe, equals('engine_v2/engine_v2')); expect(config.ciTargets['Linux linux_build']!.properties.valid, isTrue); expect(config.ciTargets['Linux linux_build']!.properties.configName, equals('linux_build')); + expect(config.ciTargets['Linux linux_build']!.properties.isReleaseBuilder, isTrue); }); test('Invalid when targets is malformed', () {