-
Notifications
You must be signed in to change notification settings - Fork 100
Fix .ci.yaml validation for Fusion (the monorepo)
#4137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1e4135e
fb36ba5
d470bd1
44df905
0da4e33
5ea0d11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<String> 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<String> 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', | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can these be added automatically by the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed as flutter/flutter#160874 |
||
| 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<String> exceptions) { | ||
| if (exceptions.isNotEmpty) { | ||
| final String fullException = exceptions.reduce((String exception, _) => '$exception\n'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please leave a TODO with a link to an issue reminding us to clean this up once we're ready to archive the engine repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed as flutter/flutter#160874