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

Adds a Dart library for loading and parsing build configs #45390

Merged
zanderso merged 1 commit into from
Sep 5, 2023
Merged

Adds a Dart library for loading and parsing build configs #45390

zanderso merged 1 commit into from
Sep 5, 2023

Conversation

@zanderso
Copy link
Member

@zanderso zanderso commented Sep 1, 2023 •
edited
Loading

Also adds a test that the build configs in the repo are valid json that matches the spec.

Fleshed out @christopherfujino's code from here https://github.com/christopherfujino/flutter-engine-runner/blob/main/main.dart

@zanderso zanderso force-pushed the engine-build-configs-pkg branch from bb1f486 to 3fdb711 Compare September 1, 2023 19:43
@flutter-dashboard
Copy link

flutter-dashboard bot commented Sep 1, 2023

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #45390 at sha 3fdb711

@zanderso zanderso force-pushed the engine-build-configs-pkg branch from 3fdb711 to d6e7c15 Compare September 1, 2023 20:57
@flutter-dashboard
Copy link

flutter-dashboard bot commented Sep 1, 2023

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #45390 at sha d6e7c15

christopherfujino
christopherfujino reviewed Sep 1, 2023
View reviewed changes
christopherfujino
christopherfujino approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM other than the spacing nit

matanlurey
matanlurey suggested changes Sep 1, 2023
View reviewed changes
tools/pkg/engine_build_configs/bin/check.dart
import 'dart:io' as io;

import 'package:engine_build_configs/engine_build_configs.dart';
import 'package:engine_repo_tools/engine_repo_tools.dart';
Copy link
Contributor

@matanlurey matanlurey Sep 1, 2023

Choose a reason for hiding this comment

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

Nice! 😀

tools/pkg/engine_build_configs/lib/src/build_config.dart
final List<String>? errors;

/// Whether there were errors when loading the data for this node.
late final bool valid = errors == null;
Copy link
Contributor

@matanlurey matanlurey Sep 1, 2023

Choose a reason for hiding this comment

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

not a comment: This is so cool.

mossmana
mossmana reviewed Sep 1, 2023
View reviewed changes
Copy link
Contributor

@mossmana mossmana left a comment

Choose a reason for hiding this comment

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

This looks great so far. Just had a couple questions.

tools/pkg/engine_build_configs/bin/check.dart Outdated
));
final List<String> errors = <String>[];
final BuildConfigLoader loader = BuildConfigLoader(
errors: errors,
Copy link
Contributor

@mossmana mossmana Sep 1, 2023

Choose a reason for hiding this comment

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

Just curious why errors needs to be passed in. Is that for dependency injection? If that's the case, why just errors and not configs for example?

Copy link
Member Author

@zanderso zanderso Sep 5, 2023

Choose a reason for hiding this comment

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

Oh! This is the result of an incomplete refactor. It was previously a StringSink that needed to be injected for tests, but as a List<String> it doesn't need to be injected anymore. Thanks!

tools/pkg/engine_build_configs/test/build_config_test.dart
int main() {
test('BuildConfig parser works', () {
final BuildConfig buildConfig = BuildConfig.fromJson(
path: 'linux_test_config',
Copy link
Contributor

@mossmana mossmana Sep 1, 2023

Choose a reason for hiding this comment

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

Does the linux case cover all the bases or should other platforms also be included?

Copy link
Contributor

@christopherfujino christopherfujino Sep 2, 2023

Choose a reason for hiding this comment

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

it looks like check_build_configs.sh is essentially an integration test that will verify ALL the real configs, and run for every commit in the linux_unopt shard.

Copy link
Member Author

@zanderso zanderso Sep 5, 2023

Choose a reason for hiding this comment

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

Right. This file is just some unit tests, so the string on this line doesn't matter.

@zanderso zanderso force-pushed the engine-build-configs-pkg branch 2 times, most recently from d049480 to 00263a1 Compare September 2, 2023 04:51
@zanderso zanderso force-pushed the engine-build-configs-pkg branch from 00263a1 to 356453d Compare September 5, 2023 15:10
matanlurey
matanlurey approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@matanlurey matanlurey 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 c1a1680 into flutter:main Sep 5, 2023
@zanderso zanderso deleted the engine-build-configs-pkg branch September 5, 2023 17:00
@mossmana mossmana mentioned this pull request Sep 5, 2023
Closed
2 tasks
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 5, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 5, 2023
@engine-flutter-autoroll
c1a16808a Adds a Dart library for loading and parsing build configs (f…
ae41dd9 
…lutter/engine#45390)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 5, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 1a6b47af3eb0 to f4975e04f35e (16 revisions) (#…
d6edbfc 
…134069)

flutter/engine@1a6b47a...f4975e0

2023-09-05 [email protected] Roll Skia from c07fbf4c1d67 to a0572041af8e (3 revisions) (flutter/engine#45460)
2023-09-05 [email protected] Roll ANGLE from 9666d4d5f7c8 to dcd62fc41c3b (1 revision) (flutter/engine#45457)
2023-09-05 [email protected] [Impeller] compute path bounds once, use Skia computed bounds where possible. (flutter/engine#45456)
2023-09-05 [email protected] Roll Skia from d8ea902500a3 to c07fbf4c1d67 (2 revisions) (flutter/engine#45451)
2023-09-05 [email protected] Adds a Dart library for loading and parsing build configs (flutter/engine#45390)
2023-09-05 [email protected] Roll ANGLE from 17c4741d70dd to 9666d4d5f7c8 (1 revision) (flutter/engine#45453)
2023-09-05 [email protected] Roll Skia from 055b26152483 to d8ea902500a3 (1 revision) (flutter/engine#45448)
2023-09-05 [email protected] Roll ANGLE from e72efa276c45 to 17c4741d70dd (1 revision) (flutter/engine#45449)
2023-09-05 [email protected] Roll Fuchsia Mac SDK from qe_q1aYCmE0eC-2Yz... to bHw1LzoikQJthLkTE... (flutter/engine#45447)
2023-09-05 [email protected] Roll Skia from 9ef0225b5f8a to 055b26152483 (1 revision) (flutter/engine#45446)
2023-09-05 [email protected] Roll ANGLE from b62216047112 to e72efa276c45 (1 revision) (flutter/engine#45443)
2023-09-05 [email protected] Roll Skia from 7d0e33e32427 to 9ef0225b5f8a (1 revision) (flutter/engine#45442)
2023-09-05 [email protected] Roll ANGLE from e691a4edb19a to b62216047112 (1 revision) (flutter/engine#45441)
2023-09-05 [email protected] Roll Skia from 8206402f3c35 to 7d0e33e32427 (2 revisions) (flutter/engine#45440)
2023-09-05 [email protected] Roll ANGLE from ab9bbb9b11b3 to e691a4edb19a (1 revision) (flutter/engine#45437)
2023-09-05 [email protected] Roll Fuchsia Mac SDK from A82pOZ3-NNgfJ2Da7... to qe_q1aYCmE0eC-2Yz... (flutter/engine#45436)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from A82pOZ3-NNgf to bHw1LzoikQJt

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

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.

Reviewers

3 more reviewers

@mossmana mossmana mossmana left review comments

@christopherfujino christopherfujino christopherfujino approved these changes

@matanlurey matanlurey matanlurey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

will affect goldens

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zanderso @matanlurey @mossmana @christopherfujino