diff --git a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart index 890a30c49b..7bf2576d07 100644 --- a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart +++ b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart @@ -343,7 +343,7 @@ class CiYaml { /// 5. [pb.Target] should not depend on self /// 6. [pb.Target] cannot have more than 1 dependency /// 7. [pb.Target] should depend on target that already exist in depedency graph, and already recorded in map [targetGraph] - /// 8. [pb.Target] has an empty runIf or the runIf includes `.ci.yaml` and `DEPS if on the engine repo. + /// 8. [pb.Target] has an empty runIf or the runIf includes `.ci.yaml` `DEPS`, and `engine/**` if validating the framework. void _validate(pb.SchedulerConfig schedulerConfig, String branch, {pb.SchedulerConfig? totSchedulerConfig}) { if (schedulerConfig.targets.isEmpty) { throw const FormatException('Scheduler config must have at least 1 target'); @@ -394,26 +394,18 @@ class CiYaml { // Verify runIf includes foundational files. if (target.runIf.isNotEmpty) { - if (isFusion && type == CiType.fusionEngine) { - // Look in different locations if fusion && engine ci.yaml - if (!target.runIf.contains('engine/src/flutter/.ci.yaml')) { - exceptions.add( - 'ERROR: ${target.name} is missing `engine/src/flutter/.ci.yaml` in runIf', - ); - } - if (!target.runIf.contains('DEPS')) { - exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf'); - } + if (isFusion) { + _verifyTargetFusionRepo(target, exceptions); } else { - // not fusion or not engine in fusion. - if (!target.runIf.contains('.ci.yaml')) { - exceptions.add('ERROR: ${target.name} is missing `.ci.yaml` in runIf'); - } - if (slug == Config.engineSlug && !target.runIf.contains('DEPS')) { - exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf'); - } + _verifyTargetClassicRepo(target, exceptions); } } + + // Verify runIfNot is never used, as it is a very confusing feature. + // https://github.com/flutter/flutter/issues/147182 + if (target.runIfNot.isNotEmpty) { + exceptions.add('ERROR: ${target.name}: Do not use runIfNot.'); + } } /// Check the dependencies for the current target if it is viable and to @@ -429,6 +421,61 @@ class CiYaml { _checkExceptions(exceptions); } + void _verifyTargetClassicRepo(pb.Target target, List exceptions) { + // These serve mostly as documentation. + assert(!isFusion, 'Expected to be called in a non-monorepo'); + assert(target.runIf.isNotEmpty, 'Expected to be called when non-empty'); + + // 1. Every target must depend on .ci.yaml at the root of the repo. + // TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser? + // See https://github.com/flutter/flutter/issues/160874. + if (!target.runIf.contains('.ci.yaml')) { + exceptions.add('ERROR: ${target.name} is missing `.ci.yaml` in runIf'); + } + + // 2. The engine repo must additionally depend on DEPS. + // TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser? + // See https://github.com/flutter/flutter/issues/160874. + if (slug == Config.engineSlug && !target.runIf.contains('DEPS')) { + exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf'); + } + } + + void _verifyTargetFusionRepo(pb.Target target, List exceptions) { + // These serve mostly as documentation. + assert(isFusion, 'Expected to be called in a fusion monorepo'); + assert(target.runIf.isNotEmpty, 'Expected to be called when non-empty'); + assert(slug == Config.flutterSlug, 'Expected to be in the combined repo'); + + // 1. Every target must depend on .ci.yaml. + // The path depends on whether the framework or engine are being validated; + // while both belong in the same (mono)repo, they have separate .ci.yaml + // files located in different paths. + // + // TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser? + // See https://github.com/flutter/flutter/issues/160874. + final ciYamlPath = switch (type) { + CiType.fusionEngine => 'engine/src/flutter/.ci.yaml', + _ => '.ci.yaml', + }; + if (!target.runIf.contains(ciYamlPath)) { + exceptions.add('ERROR: ${target.name} is missing `$ciYamlPath` in runIf'); + } + + // 2. Every target must depend on DEPS. + // TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser? + // See https://github.com/flutter/flutter/issues/160874. + if (!target.runIf.contains('DEPS')) { + exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf'); + } + + // 3. The framework tree must depend on engine/**. + final isFrameworkCiYaml = type != CiType.fusionEngine; + if (isFrameworkCiYaml && !target.runIf.contains('engine/**')) { + exceptions.add('ERROR: ${target.name} is missing `engine/**` in runIf'); + } + } + void _checkExceptions(List exceptions) { if (exceptions.isNotEmpty) { final String fullException = exceptions.reduce((String exception, _) => '$exception\n'); diff --git a/app_dart/test/model/ci_yaml/ci_yaml_test.dart b/app_dart/test/model/ci_yaml/ci_yaml_test.dart index 1f860de374..8c7f43395b 100644 --- a/app_dart/test/model/ci_yaml/ci_yaml_test.dart +++ b/app_dart/test/model/ci_yaml/ci_yaml_test.dart @@ -357,6 +357,7 @@ void main() { ); }); }); + group('Presubmit validation', () { final CiYaml totCIYaml = CiYaml( type: CiType.any, @@ -415,6 +416,339 @@ void main() { }); }); + group('validates runIf blocks', () { + test('never use runIfNot', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.Uses.runIfNot', + runIfNot: ['some-file'], + ), + ], + ), + type: CiType.any, + validate: true, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + stringContainsInOrder([ + 'Target.Uses.runIfNot', + 'Do not use runIfNot', + ]), + ), + ), + ); + }); + + group('classic repo', () { + test('must include .ci.yaml', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.Missing.ciYaml', + runIf: [ + 'some-file', + ], + ), + ], + ), + type: CiType.any, + validate: true, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + stringContainsInOrder([ + 'Target.Missing.ciYaml', + 'is missing `.ci.yaml` in runIf', + ]), + ), + ), + ); + }); + + test('framework is valid with .ci.yaml', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.OK', + runIf: [ + '.ci.yaml', + ], + ), + ], + ), + type: CiType.any, + validate: true, + ), + returnsNormally, + ); + }); + + test('engine is valid with DEPS and .ci.yaml', () { + expect( + () => CiYaml( + slug: Config.engineSlug, + branch: Config.defaultBranch(Config.engineSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.engineSlug)], + targets: [ + pb.Target( + name: 'Target.OK', + runIf: [ + '.ci.yaml', + 'DEPS', + ], + ), + ], + ), + type: CiType.any, + validate: true, + ), + returnsNormally, + ); + }); + }); + + group('fusion repo', () { + test('framework is valid with DEPS, root .ci.yaml, and engine/**', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.OK', + runIf: [ + '.ci.yaml', + 'DEPS', + 'engine/**', + ], + ), + ], + ), + type: CiType.any, + isFusion: true, + validate: true, + ), + returnsNormally, + ); + }); + + test('engine is valid with DEPS and engine .ci.yaml', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.OK', + runIf: [ + 'engine/src/flutter/.ci.yaml', + 'DEPS', + 'engine/**', + ], + ), + ], + ), + type: CiType.fusionEngine, + isFusion: true, + validate: true, + ), + returnsNormally, + ); + }); + + test('must include DEPS for the framework builds/tests', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.Missing.DEPS', + runIf: [ + '.ci.yaml', + ], + ), + ], + ), + type: CiType.any, + validate: true, + isFusion: true, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + stringContainsInOrder([ + 'Target.Missing.DEPS', + 'is missing `DEPS` in runIf', + ]), + ), + ), + ); + }); + + test('must include root .ci.yaml for the framework builds/tests', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.Missing.ciYaml', + runIf: [ + 'some-file', + ], + ), + ], + ), + type: CiType.any, + validate: true, + isFusion: true, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + stringContainsInOrder([ + 'Target.Missing.ciYaml', + 'is missing `.ci.yaml` in runIf', + ]), + ), + ), + ); + }); + + test('must include engine .ci.yaml for the engine builds/tests', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.Missing.ciYaml', + runIf: [ + 'some-file', + ], + ), + ], + ), + type: CiType.fusionEngine, + validate: true, + isFusion: true, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + stringContainsInOrder([ + 'Target.Missing.ciYaml', + 'is missing `engine/src/flutter/.ci.yaml` in runIf', + ]), + ), + ), + ); + }); + + test('must include engine sources for the framework builds/tests', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.Missing.engineSources', + runIf: [ + '.ci.yaml', + 'DEPS', + ], + ), + ], + ), + type: CiType.any, + validate: true, + isFusion: true, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + stringContainsInOrder([ + 'Target.Missing.engineSources', + 'is missing `engine/**` in runIf', + ]), + ), + ), + ); + }); + + test('must include DEPS for the engine builds/tests', () { + expect( + () => CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: pb.SchedulerConfig( + enabledBranches: [Config.defaultBranch(Config.flutterSlug)], + targets: [ + pb.Target( + name: 'Target.Missing.DEPS', + runIf: [ + 'engine/src/flutter/.ci.yaml', + ], + ), + ], + ), + type: CiType.fusionEngine, + validate: true, + isFusion: true, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + stringContainsInOrder([ + 'Target.Missing.DEPS', + 'is missing `DEPS` in runIf', + ]), + ), + ), + ); + }); + }); + }); + group('flakiness_threshold', () { test('is set', () { final ciYaml = exampleFlakyConfig; diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index d97c59b23e..7f7b77b0dc 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -60,7 +60,9 @@ targets: - name: Linux runIf runIf: - .ci.yaml + - DEPS - dev/** + - engine/** - name: Google Internal Roll postsubmit: true presubmit: false @@ -1481,11 +1483,6 @@ targets: - .ci.yaml - DEPS - dev/run_if/** - - name: Linux Conditional Presubmit (runIfNot) - presubmit: true - scheduler: luci - runIfNot: - - dev/run_if_not/** - name: Linux Postsubmit presubmit: false scheduler: luci @@ -1516,7 +1513,6 @@ targets: 'Linux Presubmit', // test: all label is present, so runIf is skipped. 'Linux Conditional Presubmit (runIf)', - 'Linux Conditional Presubmit (runIfNot)', // test: all label is present, so postsubmit is treated as presubmit. 'Linux Postsubmit', ], @@ -1537,7 +1533,6 @@ targets: 'Linux Presubmit', // test: all label is present, so runIf is skipped. 'Linux Conditional Presubmit (runIf)', - 'Linux Conditional Presubmit (runIfNot)', // test: all label is present, so postsubmit is treated as presubmit. 'Linux Postsubmit', ], @@ -1556,7 +1551,6 @@ targets: ([ // Always runs. 'Linux Presubmit', - 'Linux Conditional Presubmit (runIfNot)', ]), ); }); diff --git a/cloud_build/dashboard_build.sh b/cloud_build/dashboard_build.sh index 3df334450c..5872159e08 100755 --- a/cloud_build/dashboard_build.sh +++ b/cloud_build/dashboard_build.sh @@ -8,7 +8,7 @@ pushd dashboard > /dev/null set -e rm -rf build -flutter channel stable +flutter channel stable flutter upgrade flutter doctor flutter pub get