Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@zanderso
Copy link
Member

Towards flutter/flutter#132807.

This is needed so the engine tool can map the names of CI builders listed in .ci.yaml to the names of the configurations in the build config json files in ci/builders.

@zanderso zanderso force-pushed the ci-config branch 2 times, most recently from 1f81089 to e6775f7 Compare February 20, 2024 17:52
@zanderso
Copy link
Member Author

@CaseyHillers Wondering if there's already a reasonably standalone package that I can pull in for parsing ci.yaml instead of writing this?

// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:yaml/yaml.dart' as y;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a few months, I maintained package:cocoon_scheduler which contained Dart helpers for stuff like this used by Cocoon. I ended up removing it in flutter/cocoon#1254 as Cocoon ended up being the only customer, and it was annoying to roll the versions.

For simplicity, would you be blocked from importing the Cocoon server into the engine? We could set up a script to publish new versions to pub.dev. If it pulls in too much, we could move https://github.com/flutter/cocoon/tree/main/app_dart/lib/src/model/ci_yaml into a separate package.

\cc @keyonghan

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how extensive the transitive dependencies will be, since they would all need to be explicitly brought in via the DEPS file. If it's just a few things, then that's fine, but if the cocoon scheduler has a big list of transitive deps, then that would be a lot of trouble.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CaseyHillers I'm having trouble finding where in https://github.com/flutter/cocoon/tree/main/app_dart/lib/src/model/ci_yaml the yaml file is parsed. It looks like the code in that directory is mostly poking around in the fields of a protobuf.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how extensive the transitive dependencies will be, since they would all need to be explicitly brought in via the DEPS file. If it's just a few things, then that's fine, but if the cocoon scheduler has a big list of transitive deps, then that would be a lot of trouble.

Is there a way we can estimate that by importing it in pubspec via a git package?

@CaseyHillers I'm having trouble finding where in https://github.com/flutter/cocoon/tree/main/app_dart/lib/src/model/ci_yaml the yaml file is parsed. It looks like the code in that directory is mostly poking around in the fields of a protobuf.

ci.yaml is proto backed, so the pipeline looks like package:yaml -> toJson() -> package:protobuf mergeFromProtoJson -> ci_yaml.dart

https://github.com/flutter/cocoon/blob/main/app_dart/lib/src/service/scheduler.dart#L272

For Cocoon, it reads the file content over the network, then

    final String configContent = await githubFileContent(
      commit.slug,
      '.ci.yaml',
      ref: commit.sha!,
    );
    final YamlMap configYaml = loadYaml(configContent) as YamlMap;
    final pb.SchedulerConfig schedulerConfig = pb.SchedulerConfig()
      ..mergeFromProto3Json(configYaml)
      ..writeToBuffer();

import 'package:source_span/source_span.dart';
import 'package:yaml/yaml.dart' as y;

void main() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test to validate the logic when other unrelated configs exist? What if there exists a malformed config there? Do we want to ignore them whenever the desired ones are valid?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if any config looks invalid, then it's okay for the whole parse to fail. The whoel input .ci.yaml has to be valid since it would fail in presubmit if it's invalid, anyway. I added another test with both a valid and invalid target to check that behavior.

Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zanderso zanderso merged commit 78082a2 into flutter:main Feb 21, 2024
@zanderso zanderso deleted the ci-config branch February 21, 2024 22:56
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 22, 2024
…143911)

flutter/engine@bf5c003...7eeb697

2024-02-22 [email protected] Add view focus direction detection to flutter web. (flutter/engine#50843)
2024-02-22 [email protected] Add a similar `runIf` guard to `web_engine` as web framework. (flutter/engine#50846)
2024-02-22 [email protected] Add scheduleWarmUpFrame (flutter/engine#50570)
2024-02-22 [email protected] Remove/reduce unused or private methods and add tests to `SkiaGoldClient` (flutter/engine#50844)
2024-02-22 [email protected] Clean up contributing formatting, add a Skia gold callout (flutter/engine#50828)
2024-02-21 [email protected] Make Android Studio be able to run android embedding unit tests out of the box (flutter/engine#50840)
2024-02-21 [email protected] Shift some deps to //flutter/third_party (flutter/engine#50830)
2024-02-21 [email protected] Make the view focus binding report focus transitions across elements. (flutter/engine#50610)
2024-02-21 [email protected] [macOS] Wrap FlutterEngineTest in autoreleasepool (flutter/engine#50832)
2024-02-21 [email protected] Update the vulkan_glfw sample for the latest roll of vulkan-deps (flutter/engine#50839)
2024-02-21 [email protected] Roll Skia from 8fa858855820 to 57490c8d257e (6 revisions) (flutter/engine#50833)
2024-02-21 [email protected] Starts a .ci.yaml parser (flutter/engine#50783)
2024-02-21 [email protected] Roll Dart SDK from f344e2266468 to 0f0f7400c38a (6 revisions) (flutter/engine#50837)
2024-02-21 [email protected] [Windows] Fix top-level message procedure order (flutter/engine#50797)
2024-02-21 [email protected] Tweak verbose log messages in ImageReaderSurfaceProducer (flutter/engine#50831)
2024-02-21 [email protected] Add a throw statement for imgtestAdd non 0 exit codes. (flutter/engine#50829)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants