GN build rules for tests using Fuchsia SDK Dart libraries and bindings #27996

richkadel · 2021-08-10T18:13:07Z

Most of these new rules replicate the basic build rules and scripts
built into the fuchsia.git, for Fuchsia in-tree development. Additional
rules and scripts were added to generate dart_library targets for the
Fuchsia Dart APIs included in the Fuchsia SDK, and fidl_library
bindings for Dart.

These changes provide the required build foundation for
#26880 and should support other ongoing efforts to improve
flutter/engine testing for Fuchsia, directly from the flutter/engine
repo.

Example usage:

import("//flutter/tools/fuchsia/dart/dart_library.gni")
import("//flutter/tools/fuchsia/flutter/flutter_component.gni")

dart_library("lib") {
  package_name = "parent-view"
  sources = [ "main.dart" ]

  deps = [
    "//flutter/tools/fuchsia/dart:fuchsia_services",
    "//flutter/tools/fuchsia/dart:zircon",
    "//flutter/tools/fuchsia/fidl:fuchsia.sys",
    "//flutter/tools/fuchsia/fidl:fuchsia.ui.app",
    "//flutter/tools/fuchsia/fidl:fuchsia.ui.views",
    "//third_party/dart/third_party/pkg/args",
  ]
}

flutter_component("parent-view") {
  manifest = "meta/parent_view.cmx"
  main_package = "parent-view"
  deps = [ ":lib" ]
}

Fixes: flutter/flutter#87999

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

richkadel · 2021-08-10T18:14:39Z

cc: @chinmaygarde @arbreng @iskakaushik

Most of these new rules replicate the basic build rules and scripts built into the fuchsia.git, for Fuchsia in-tree development. Additional rules and scripts were added to generate `dart_library` targets for the Fuchsia Dart APIs included in the Fuchsia SDK, and `fidl_library` bindings for Dart. These changes provide the required build foundation for flutter#26880 and should support other ongoing efforts to improve flutter/engine testing for Fuchsia, directly from the flutter/engine repo. Example usage: ```gn import("//flutter/tools/fuchsia/dart/dart_library.gni") import("//flutter/tools/fuchsia/flutter/flutter_component.gni") dart_library("lib") { package_name = "parent-view" sources = [ "main.dart" ] deps = [ "//flutter/tools/fuchsia/dart:fuchsia_services", "//flutter/tools/fuchsia/dart:zircon", "//flutter/tools/fuchsia/fidl:fuchsia.sys", "//flutter/tools/fuchsia/fidl:fuchsia.ui.app", "//flutter/tools/fuchsia/fidl:fuchsia.ui.views", "//third_party/dart/third_party/pkg/args", ] } flutter_component("parent-view") { manifest = "meta/parent_view.cmx" main_package = "parent-view" deps = [ ":lib" ] } ```

This is a work in progress, with changes that successfully compile the embedder test. This commit leverages new build rules, added in PR flutter#27996, that enable building flutter tests as Fuchsia components, and dependencies on Dart libraries from the Fuchsia SDK (Fuchsia Dart APIs and Dart FIDL bindings). Notable: * Adds dart third_party library "quiver" version 2.1.5 to support a dependency from a Fuchsia Dart library required by the integration test. * Leverages the recent addition of TestWithEnvironment to the Fuchsia SDK, and blends this class with Googletest's (gtest) `testing::Test`, from Flutter's pre-existing gtest (third_party dependency).

iskakaushik

@richkadel left a few initial comments. Would it be possible to add a test dart component that is built as a part of this PR?

iskakaushik · 2021-08-12T16:08:50Z

tools/fuchsia/executable_action.gni

+      deps = []
+    }
+
+    if (defined(invoker.inputs)) {


do we want to give the same treatment to invoker.args?

Done, and I moved executable_action.gni to //tools/fuchsia because it's actually not fuchsia-specific I think.

iskakaushik · 2021-08-12T16:09:47Z

tools/fuchsia/fidl/BUILD.gn

@@ -0,0 +1,24 @@
+# Copyright 2018 The Fuchsia Authors. All rights reserved.


this copy right message is inconsistent with the rest of the repo, it has to be:

// Copyright 2013 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file.

Could you please change every copyright in this PR to be the above?

iskakaushik · 2021-08-12T16:14:55Z

tools/fuchsia/fidl/gen_response_file.py

@@ -0,0 +1,122 @@
+#!/usr/bin/env python3.8


I haven't look too closely at how fidl is setup, i'm assuming this is mostly using the same logic as in the fuchsia tree? please let me know if you'd like me to review the fidl stuff more carefully.

Yes, almost all of the logic was copied (and minimally tailored) from fuchsia.git. More on this in my other response, below.

I don't think you need to review the details. If you think this is helpful: you could try a recursive diff of the additional files in //flutter/tools/fuchsia/*/* with Fuchsia's //build/*/ directory to confirm they closely match. That could give you more confidence.

iskakaushik · 2021-08-12T16:15:17Z

tools/fuchsia/flutter/config.gni

+import("//flutter/tools/fuchsia/flutter/flutter_build_config.gni")
+
+declare_args() {
+  # If set to true, will force the runenrs to be built in


typo: runners

iskakaushik · 2021-08-12T16:18:50Z

tools/fuchsia/flutter/flutter_build_config.gni

@@ -0,0 +1,56 @@
+# Copyright 2020 The Fuchsia Authors. All rights reserved.


I like this classification mechanism, makes it clearer than the obscure rules we have now. We do have more modes than what is enumerated below though, see: go/flutter-fuchsia-packaging.

This is copied from fuchsia.git, but we can add to it, if that's useful.

iskakaushik · 2021-08-12T16:22:49Z

tools/fuchsia/sdk/sdk_targets.gni

+  if (defined(meta.dart_library_null_safe) && meta.dart_library_null_safe) {
+    _dart_language_version = "2.12"
+  } else {
+    _dart_language_version = "2.0"


do we need to support pre null-safe versions of dart?

It seems like we do? One example, fuchsia_vfs depends on quiver v2.1.5, which (according to fuchsia.git's BUILD.gn, that is replicated in flutter/buildroot secondary/third_party/pkg/quiver/) depends on Dart language version 2.0 (not null safe). The Fuchsia SDK includes the dart_library_null_safe property, so I don't want to ignore it, even if current Fuchsia Dart SDK libraries always set this to true for now.

iskakaushik · 2021-08-12T16:24:25Z

tools/fuchsia/sdk/component.gni

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import("cmc.gni")


can we bring in the gn-sdk dep to get some of these rules? Some of them feel like they should be provided by the sdk.

So it turns out that the gn-sdk is not really helpful to Flutter.

For context, there are some recent learnings, a Fuchsia SDK design review, and approved design doc (on the Fuchsia side) that resulted in this approach. (The design doc was specific to adding TestWithEnvironment to the SDK, but the proposal was the result of the guidance I received, specific to this Flutter integration testing effort, which I've summarized below.):

The gn-sdk was really set up more as a "chromium-sdk". In fact, the gn-sdk is a supposed to be a superset of the core-sdk, but it isn't. Some things are explicitly removed from gn-sdk, including (surprisingly) the Fuchsia Dart APIs! So "upgrading" to gn-sdk resulted in losing those APIs.

When I discussed this with the SDK team, they did not want to add the Dart APIs to the gn-sdk. I expect the SDK's to be restructured in the future, but for now, we want the core-sdk.

Second reason: Even if we had the gn-sdk PLUS the Dart APIs from core, the gn-sdk included only TWO files that I had to manually copy into src/flutter/tools/fuchsia/. Most of the other additional to src/flutter/tools/fuchsia/ come directly from fuchsia.git (with some minor tailoring) and were not available via either SDK.

Also, Fuchsia is not planning to add any more GN rules to any SDK in the future. They don't expect any project other that Flutter is likely to need similar build rules, using GN. So I was asked to use whatever existing support I could find in existing Flutter GN rules, and add (copy from fuchsia.git) whatever was missing.

As best I could tell, flutter/engine didn't appear to have existing support for Dart-based Fuchsia components, I had to bring in build rules from fuchsia.git.

Due to the complexity of these rules, I preferred to keep the original fuchsia.git build structure and logic as close to the original as possible, which probably resulted in some duplication of existing logic in flutter/engine, but it seemed like the duplication was minimal, and the additional logic accounted for the vast majority of what I had to add.

richkadel · 2021-08-12T18:08:41Z

@richkadel left a few initial comments. Would it be possible to add a test dart component that is built as a part of this PR?

Well... originally that would have been the tests in #26880. I am building that PR with the rules in this PR. But #26880 is not ready.

Since the build rules are essentially done, and it seemed like you could use them to enable the other Fuchsia tests, I split the rules out of #26880 into this separate PR so you could take advantage of them now.

I can't currently prioritize writing a standalone test. It could be really easy (copy the parent-view or child-view from #26880 into a different location, perhaps) but other than confirming the build rules can be imported and used, and don't crash (which I already know because I'm using them), I don't know how to make a test that could actually be more useful.

If you're able to use these rules to build your own test, maybe one of your tests could be the test to accompany this PR? I'm open to suggestions, but have to prioritize the integration test right now.

richkadel · 2021-08-12T18:12:04Z

I pushed changes to address the recent feedback.

This is a work in progress, with changes that successfully compile the embedder test. This commit leverages new build rules, added in PR flutter#27996, that enable building flutter tests as Fuchsia components, and dependencies on Dart libraries from the Fuchsia SDK (Fuchsia Dart APIs and Dart FIDL bindings). Notable: * Adds dart third_party library "quiver" version 2.1.5 to support a dependency from a Fuchsia Dart library required by the integration test. * Leverages the recent addition of TestWithEnvironment to the Fuchsia SDK, and blends this class with Googletest's (gtest) `testing::Test`, from Flutter's pre-existing gtest (third_party dependency).

iskakaushik

LGTM. I also tested that these work with the following commit.

This is a work in progress, with changes that successfully compile the embedder test. This commit leverages new build rules, added in PR flutter#27996, that enable building flutter tests as Fuchsia components, and dependencies on Dart libraries from the Fuchsia SDK (Fuchsia Dart APIs and Dart FIDL bindings). Notable: * Adds dart third_party library "quiver" version 2.1.5 to support a dependency from a Fuchsia Dart library required by the integration test. * Leverages the recent addition of TestWithEnvironment to the Fuchsia SDK, and blends this class with Googletest's (gtest) `testing::Test`, from Flutter's pre-existing gtest (third_party dependency).

… bindings (flutter/engine#27996)

This is a work in progress, with changes that successfully compile the embedder test. This commit leverages new build rules, added in PR flutter#27996, that enable building flutter tests as Fuchsia components, and dependencies on Dart libraries from the Fuchsia SDK (Fuchsia Dart APIs and Dart FIDL bindings). Notable: * Adds dart third_party library "quiver" version 2.1.5 to support a dependency from a Fuchsia Dart library required by the integration test. * Leverages the recent addition of TestWithEnvironment to the Fuchsia SDK, and blends this class with Googletest's (gtest) `testing::Test`, from Flutter's pre-existing gtest (third_party dependency).

The previous PR did not build the Fuchsia packages correctly: GN build rules for tests using Fuchsia SDK Dart libraries and bindings flutter#27996 The new changes bring in more of the Fuchsia GN-SDK build rules. Some changes to these rules were required because the fuchsia.git-derived flutter/dart build rules depend on a different implementation of similar-sounding rules (fuchsia.git <> Fuchsia GN SDK, regarding build rule implementations).

The previous PR did not build the Fuchsia packages correctly: > GN build rules for tests using Fuchsia SDK Dart libraries and bindings flutter#27996 The new changes bring in more of the Fuchsia GN-SDK build rules. Some changes to these rules were required because the fuchsia.git-derived flutter/dart build rules depend on a different implementation of similar-sounding rules (fuchsia.git <> Fuchsia GN SDK, regarding build rule implementations). I renamed flutter/tools/fuchsia/sdk to flutter/tools/fuchsia/gn-sdk to be more clear that GN SDK was the original source. Also, I realized there were still some references to python3.8 and python3_action. Now that flutter is building with python3 by default, that action is no longer needed, so I've removed it. Fixes: flutter/flutter#87999 (because the original fix had deficiencies) Tested in Fuchsia and confirmed the dart package/component launches and runs.

This is a work in progress, with changes that successfully compile the embedder test. This commit leverages new build rules, added in PR flutter#27996, that enable building flutter tests as Fuchsia components, and dependencies on Dart libraries from the Fuchsia SDK (Fuchsia Dart APIs and Dart FIDL bindings). Notable: * Adds dart third_party library "quiver" version 2.1.5 to support a dependency from a Fuchsia Dart library required by the integration test. * Leverages the recent addition of TestWithEnvironment to the Fuchsia SDK, and blends this class with Googletest's (gtest) `testing::Test`, from Flutter's pre-existing gtest (third_party dependency).

The previous PR did not build the Fuchsia packages correctly: > GN build rules for tests using Fuchsia SDK Dart libraries and bindings flutter#27996 The new changes bring in more of the Fuchsia GN-SDK build rules. Some changes to these rules were required because the fuchsia.git-derived flutter/dart build rules depend on a different implementation of similar-sounding rules (fuchsia.git <> Fuchsia GN SDK, regarding build rule implementations). I renamed flutter/tools/fuchsia/sdk to flutter/tools/fuchsia/gn-sdk to be more clear that GN SDK was the original source. Also, I realized there were still some references to python3.8 and python3_action. Now that flutter is building with python3 by default, that action is no longer needed, so I've removed it. Fixes: flutter/flutter#87999 (because the original fix had deficiencies) Tested in Fuchsia and confirmed the dart package/component launches and runs.

This is a work in progress, with changes that successfully compile the embedder test. This commit leverages new build rules, added in PR flutter#27996, that enable building flutter tests as Fuchsia components, and dependencies on Dart libraries from the Fuchsia SDK (Fuchsia Dart APIs and Dart FIDL bindings). Notable: * Adds dart third_party library "quiver" version 2.1.5 to support a dependency from a Fuchsia Dart library required by the integration test. * Leverages the recent addition of TestWithEnvironment to the Fuchsia SDK, and blends this class with Googletest's (gtest) `testing::Test`, from Flutter's pre-existing gtest (third_party dependency).

The previous PR did not build the Fuchsia packages correctly: > GN build rules for tests using Fuchsia SDK Dart libraries and bindings flutter#27996 The new changes bring in more of the Fuchsia GN-SDK build rules. Some changes to these rules were required because the fuchsia.git-derived flutter/dart build rules depend on a different implementation of similar-sounding rules (fuchsia.git <> Fuchsia GN SDK, regarding build rule implementations). I renamed flutter/tools/fuchsia/sdk to flutter/tools/fuchsia/gn-sdk to be more clear that GN SDK was the original source. Also, I realized there were still some references to python3.8 and python3_action. Now that flutter is building with python3 by default, that action is no longer needed, so I've removed it. Fixes: flutter/flutter#87999 (because the original fix had deficiencies) Tested in Fuchsia and confirmed the dart package/component launches and runs.

This is a work in progress, with changes that successfully compile the embedder test. This commit leverages new build rules, added in PR flutter#27996, that enable building flutter tests as Fuchsia components, and dependencies on Dart libraries from the Fuchsia SDK (Fuchsia Dart APIs and Dart FIDL bindings). Notable: * Adds dart third_party library "quiver" version 2.1.5 to support a dependency from a Fuchsia Dart library required by the integration test. * Leverages the recent addition of TestWithEnvironment to the Fuchsia SDK, and blends this class with Googletest's (gtest) `testing::Test`, from Flutter's pre-existing gtest (third_party dependency).

flutter#27996)

flutter-dashboard bot added the platform-fuchsia label Aug 10, 2021

google-cla bot added the cla: yes label Aug 10, 2021

richkadel changed the title ~~Build fuchsia dart~~ GN build rules for tests using Fuchsia SDK Dart libraries and bindings Aug 10, 2021

chandarrengoog requested review from iskakaushik, arbreng and chinmaygarde August 10, 2021 18:47

richkadel force-pushed the build-fuchsia-dart branch from a549782 to c4d26e8 Compare August 10, 2021 21:42

richkadel added 2 commits August 10, 2021 14:43

Removed "Adapted from $FUCHSIA..." comments and cleaned up TODOs

c5586fd

richkadel force-pushed the build-fuchsia-dart branch from c4d26e8 to c5586fd Compare August 10, 2021 21:44

richkadel mentioned this pull request Aug 10, 2021

Migrated integration_flutter_test/embedder (scenic integration test) from fuchsia.git #26880

Merged

9 tasks

iskakaushik reviewed Aug 12, 2021

View reviewed changes

Addressed code review comments

e5efb61

iskakaushik approved these changes Aug 13, 2021

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 13, 2021

fluttergithubbot merged commit 1fe5b16 into flutter:master Aug 13, 2021

engine-flutter-autoroll mentioned this pull request Aug 13, 2021

Roll Engine from b3248764e4dd to b288e8a84c89 (16 revisions) flutter/flutter#88184

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 13, 2021

1fe5b16 GN build rules for tests using Fuchsia SDK Dart libraries and…

7b9c309

… bindings (flutter/engine#27996)

engine-flutter-autoroll mentioned this pull request Aug 13, 2021

Roll Engine from b3248764e4dd to 6227940c3a06 (17 revisions) flutter/flutter#88186

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 13, 2021

1fe5b16 GN build rules for tests using Fuchsia SDK Dart libraries and…

451b864

… bindings (flutter/engine#27996)

engine-flutter-autoroll mentioned this pull request Aug 14, 2021

Roll Engine from b3248764e4dd to a447901bc58b (30 revisions) flutter/flutter#88210

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 14, 2021

1fe5b16 GN build rules for tests using Fuchsia SDK Dart libraries and…

97ac21d

… bindings (flutter/engine#27996)

engine-flutter-autoroll mentioned this pull request Aug 14, 2021

Roll Engine from b3248764e4dd to b1ccc4175c9e (33 revisions) flutter/flutter#88212

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 14, 2021

1fe5b16 GN build rules for tests using Fuchsia SDK Dart libraries and…

b2a4e45

… bindings (flutter/engine#27996)

engine-flutter-autoroll mentioned this pull request Aug 14, 2021

Roll Engine from b3248764e4dd to a447901bc58b (30 revisions) flutter/flutter#88213

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 14, 2021

1fe5b16 GN build rules for tests using Fuchsia SDK Dart libraries and…

2b83556

… bindings (flutter/engine#27996)

richkadel mentioned this pull request Sep 1, 2021

Fix building Dart Fuchsia components and packages #28413

Merged

8 tasks

filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022

GN build rules for tests using Fuchsia SDK Dart libraries and bindings (

b571209

flutter#27996)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GN build rules for tests using Fuchsia SDK Dart libraries and bindings #27996

GN build rules for tests using Fuchsia SDK Dart libraries and bindings #27996

richkadel commented Aug 10, 2021 •

edited

richkadel commented Aug 10, 2021

iskakaushik left a comment

iskakaushik Aug 12, 2021

richkadel Aug 12, 2021

iskakaushik Aug 12, 2021

richkadel Aug 12, 2021

iskakaushik Aug 12, 2021

richkadel Aug 12, 2021

iskakaushik Aug 12, 2021

richkadel Aug 12, 2021

iskakaushik Aug 12, 2021

richkadel Aug 12, 2021

iskakaushik Aug 12, 2021

richkadel Aug 12, 2021

iskakaushik Aug 12, 2021

richkadel Aug 12, 2021

richkadel commented Aug 12, 2021

richkadel commented Aug 12, 2021

iskakaushik left a comment

		@@ -0,0 +1,24 @@
		# Copyright 2018 The Fuchsia Authors. All rights reserved.

		@@ -0,0 +1,56 @@
		# Copyright 2020 The Fuchsia Authors. All rights reserved.

GN build rules for tests using Fuchsia SDK Dart libraries and bindings #27996

GN build rules for tests using Fuchsia SDK Dart libraries and bindings #27996

Conversation

richkadel commented Aug 10, 2021 • edited

Pre-launch Checklist

richkadel commented Aug 10, 2021

iskakaushik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richkadel commented Aug 12, 2021

richkadel commented Aug 12, 2021

iskakaushik left a comment

Choose a reason for hiding this comment

richkadel commented Aug 10, 2021 •

edited