Expand the .ci.yaml and builder.json linter (#161991)

matanlurey · web-flow · commit 74ab5937383b · 2025-01-22T18:41:02.000Z
Closes flutter/flutter#161823. Other than the integration test included, example output looks like this: ```dart # Assuming you are in $ENGINE/src/flutter % dart tools/pkg/engine_build_configs/bin/check.dart --help -v, --verbose Enable noisier diagnostic output -h, --help Output usage information. --engine-src-path=</path/to/engine/src> (defaults to "/Users/matanl/Developer/flutter/engine/src") % dart tools/pkg/engine_build_configs/bin/check.dart ✅ Loaded build configs under ci/builders ✅ .ci.yaml at .ci.yaml is valid ✅ All configuration files are valid ✅ All builds within a builder are uniquely named ✅ All build names must have a conforming prefix ✅ All builder files conform to release_build standards ``` I allow-listed a single case that needs to be moved, but I think this provides value already.
diff --git 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
@@ -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<String> 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<String> 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<String> 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<String> args) {
   // Treat it as an error if no build configs were found. The caller likely
   // expected to find some.
   final Map<String, BuilderConfig> 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<String> 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<String> 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<String> 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 = <String>[];
+  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 = <String>[];
+    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
@@ -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,15 +93,15 @@ 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".',
       );
       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;
-  }
-  if (stringNode is! y.YamlScalar) {
-    return null;
-  }
-  final y.YamlScalar stringScalar = stringNode;
-  if (stringScalar.value is! String) {
-    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<T>() {
+    final node = this;
+    if (node is! y.YamlScalar) {
+      return null;
+    }
+    final Object? value = node.value;
+    if (value is! T) {
+      return null;
+    }
+    return value;
   }
-  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
@@ -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
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
