Fix building Dart Fuchsia components and packages by richkadel · Pull Request #28413 · flutter-team-archive/engine

richkadel · 2021-09-01T20:14:59Z

The previous PR did not build the Fuchsia packages correctly:

GN build rules for tests using Fuchsia SDK Dart libraries and bindings #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.

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.
- The presence of these rules does not break the existing build, since the new rules are not yet used
- This PR makes the rules available to Flutter-Fuchsia developers that require this functionality to migrate Dart Flutter-Fuchsia tests to flutter/engine.
- For example, see Draft PR Migrated integration_flutter_test/embedder (scenic integration test) from fuchsia.git #26880, which requires these new build rules. I've confirmed (via local testing) that these rules generate a loadable, runnable Dart-based Fuchsia component, in local testing of Migrated integration_flutter_test/embedder (scenic integration test) from fuchsia.git #26880.
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-09-01T20:15:49Z

@iskakaushik @arbreng - FYI, here are the updated build rules as promised.

richkadel · 2021-09-02T16:56:32Z

Oops, I somehow leaked my test code into yesterday's post of this PR. I fixed it this morning, so this commit ONLY has changes to flutter/tools, as intended.

I also added a separate commit, before the main commit, with the original versions of the GN SDK files I adapted, so anyone can see what changes I needed, to make this work for flutter/engine.

This is ready for review, assuming it passes CI.

richkadel · 2021-09-02T17:48:25Z

@chaselatta - I should have cc'ed you initially!

chaselatta · 2021-09-02T19:46:56Z

I will take a look a little later today.

chaselatta · 2021-09-02T20:07:38Z

+# # launch the vm service in the runner.
+# # This configuration is not compatible with a --release build since the
+# # profile aot runner is built without asserts.
+# dart_aot_debug_build_cfg = {


Do we still need all of these?

Technically I don't need them for the current state of things, but I assume we want the option of both jit and aot builds?

If you disagree, I'm happy to delete all of the other commented out config options. Or if you know which ones we should keep or remove, I can delete the configs we know we're not going to need.

I figured we'd talk about and resolve the TODO items after the integration tests are working.

That works for me.

chaselatta · 2021-09-02T20:12:13Z

      rebase_path(_kernel_path, root_build_dir),
    ]

+    # TODO(richkadel): Chase said these are not needed in flutter/engine tests,


They are not needed if you don't passs the --split-output-by-packages flag. If you don't pass that flag it will keep everything in a giant dill file which is how other platforms work. Fuchsia is special in that we split the packages to take advantage of the content addressed file system.

Hmm... OK I'll try that, and make sure it still works. I've been trying to keep things as close as possible to how Fuchsia works so I can eliminate variables, and focus on debugging things that are different for a reason.

I haven't tried to make this change yet, but the most recent commit (just uploaded) addresses what I think are all of the other changes.

I'll experiment with what you recommended, now that the other items are out of the way.

chaselatta · 2021-09-02T20:12:56Z

+    args += [ "--embed-sources" ]
+
+    # } else {
+    #   args += [ "--no-embed-sources" ]


Is this needed?

As above, I assume we're going to want to support AOT mode testing as well as JIT. I wanted to keep the relevant code derived from Fuchsia build rules, but comment it out, to make sure I wasn't picking up non-debug-mode (non-jit) logic when I was focusing only on getting debug/jit to work.

We'll either uncomment or remove this, once things are working, based on the team's expectations.

chaselatta · 2021-09-02T20:19:49Z

-}
+# # Builds the component in a non-product AOT build. This will
+# # launch the vm service in the runner.
+# # This configuration is not compatible with a --release build since the


Same question as above, do we still need these?

See above answers

chaselatta · 2021-09-02T20:24:45Z

    # copy the snapshot as a resource
    _snapshot_resource_target_name = "${target_name}_snapshot_resource"
+
+    # TODO(richkadel): It looks like this is the _ONLY_ place resource() is used


resource just writes to metadata and gets collected by the package

Yes, I left it in for now (but with the TODO) so the structure of files derived from Fuchsia is as similar to the original as possible. (As mentioned in my earlier comment, this helps see what did and did not change. I figure it can be optimized in a subsequent PR once we're happy that everything else is working as expected.

chaselatta · 2021-09-02T20:28:32Z

+    _convert_kernel_target_name =
+        "${_kernel_target_name}_convert_kernel_manifest"
+    _convert_kernel_manifest_file =
+        # TODO(richkadel): This is prefixed by "dartlang/", which is not found.


dartlang/ is specific to the fuchsia build

Sorry, this was a bit of a hack here. I ran into some really frustrating issues debugging some GN rules, and for this one, I couldn't figure out why I was getting the toolchain prefix, or how to remove it. Eventually I just punted, and decided to strip it out for now.

This works, but is clearly not optimal. I have no immediate solution for it though, but if you or anyone has a recommendation on how to fix the rules, I'm happy to try.

(I can give it another go, but wasted hours on just this problem already. Some GN magic still eludes me.)

Update: If/when we resolve the limitation in flutter_runner to allow loading packages that don't include a .dilplist file, I can remove all of the code related to generate_manifest, including this one. In the mean time, I'm planning to leave this as-is, if that's OK, and come back to it later, one way or the other, to resolve the TODO item.

chaselatta · 2021-09-02T20:31:00Z

@@ -1,4 +1,4 @@
-# Copyright 2013 The Flutter Authors. All rights reserved.
+# Copyright 2019 The Fuchsia Authors. All rights reserved.


The copyright needs to be 2013 for the copyright checker to work.

Right. I'll fix those. Thanks!

richkadel · 2021-09-08T00:33:19Z

@chaselatta - In addition to addressing "most" of your comments (so far), the commit I just uploaded also adds BUILD.gn rules to package up one Fuchsia-based Dart test, already present in flutter/engine. It can be built via:

$ flutter_build flutter/shell/platform/fuchsia/dart-pkg/zircon/test

And produces:

out/fuchsia_release_x64/gen/flutter/shell/platform/fuchsia/dart-pkg/zircon/test/channel_test/channel_test.far

I expanded the .far to examine it and the contents look valid. The point is not (yet) to run this test, but I wanted to test the build rules, without adding a commit dependency on the integration tests, which really depend on these improved build rules instead.

The only thing I still need to address is to try removing --split-output-by-packages and --manifest.

I addressed the copyright headers, and responded to your questions on the other items.

You'll have to tell me if the answers are satisfactory, or if you are looking for changes, but the general theme seems to be that, you're right, some things are not strictly needed, and/or are derived from Fuchsia, but I kept some build rule features so I could compare them more equally with the build rules and ninja execution of the same tests in Fuchsia (which has been helpful).

I think once we're comfortable the rules are working as expected, we can strip out more of the rules that are never likely to be needed by flutter/engine.

richkadel · 2021-09-08T00:49:08Z

Oh, one more thing. I changed where I'm putting the dart_library() rules for third_party dart library dependencies (for including dart libraries in a Fuchsia component/package). Up to this point, I was mirroring the location of the library between //third_party and //build/secondary/third_party.

This was short sighted. It adds unneeded complexity. Instead, I started (so far with a couple of additional libraries I needed for the build test I added in the latest commit) centralizing these Fuchsia-specific dart_library() rules to the existing //flutter/shell/platform/fuchsia/dart/BUILD.gn, and I set package_root to wherever the actual library is.

This saves me having to add BUILD.gn files to flutter/buildroot separately (which is a minor annoyance, no longer).

If you don't object, I will move all of the other dart_library() rules from their individual BUILD.gn files under //build/secondary to the same BUILD.gn file.

As it turns out, this should also allow me to remove find_dart_libraries.py, a python script I wrote that I felt was a bit hacky anyway... searching for the "best match" among many possible paths to find a dart library dependency.

This commit includes only the files copied from the Fuchsia GN SDK, that had to be adapted to support flutter/engine integration tests. Most of these GN rule implementations were reusable as written (modulo some directory paths); however, some rules had to be adjusted to overcome current limitations. The required changes are implemented in the succeeding git commit, which also includes the additional flutter/engine build rules needed to build Dart-based Fuchsia packages, based on the Fuchsia SDK, to be published and tested in a compatible Fuchsia instance.

The previous PR did not build the Fuchsia packages correctly: > GN build rules for tests using Fuchsia SDK Dart libraries and bindings flutter-team-archive#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.

richkadel · 2021-09-08T17:54:02Z

@iskakaushik - Just FYI, I added build rules to package up one of the existing tests as a flutter component. (I did not actually try running the component in Fuchsia, and I had to mock up a .cmx file that probably isn't valid, but I confirmed that the resulting .far appears to be structured correctly to run.) The test is not in the standard build dependencies, but you can directly invoke the build via the following command, if you're interested:

$ flutter_build flutter/shell/platform/fuchsia/dart-pkg/zircon/test

So I included this as a validation of the build rules, like we once discussed. (I think you had started something similar, using the same files (?), to at least test dart_library, if I'm not mistaken, so that's why I picked that test.)

Any concerns? I can leave it in or remove it if required.

@chaselatta - Let me know if you think this should stay or be removed.

richkadel · 2021-09-08T18:36:07Z

Oh, I see Kaushik's change to use vpython3 (which also supplied yaml) was reverted due to flakes on Windows. I removed all of the references to vpython3, and added pyyaml to DEPS, and updated the imports to load pyyaml into the path before import yaml (as done in Fuchsia today).

Confirmed the new test build rules generate a Dart Fuchsia package that appears to be structured correctly for testing on a Fuchsia platform.

richkadel · 2021-09-08T22:14:59Z

The license check finally passed in CI... just waiting out the rest of the CI checks (which all passed earlier today)

chaselatta · 2021-09-08T22:56:33Z

+# # launch the vm service in the runner.
+# # This configuration is not compatible with a --release build since the
+# # profile aot runner is built without asserts.
+# dart_aot_debug_build_cfg = {


That works for me.

chinmaygarde · 2021-09-09T20:10:50Z

Seems good to go. May I land this?

richkadel · 2021-09-09T20:54:21Z

yes, please!

…am-archive/engine#28413)

flutter-dashboard Bot added the platform-fuchsia label Sep 1, 2021

google-cla Bot added the cla: yes label Sep 1, 2021

richkadel force-pushed the build-fuchsia-dart-2 branch 2 times, most recently from f7ae543 to 3e4808f Compare September 2, 2021 16:51

arbreng requested review from arbreng, chaselatta and iskakaushik September 2, 2021 19:14

chaselatta reviewed Sep 2, 2021

View reviewed changes

richkadel force-pushed the build-fuchsia-dart-2 branch from 3e8ae90 to 5fe010d Compare September 8, 2021 17:36

Rich Kadel added 2 commits September 8, 2021 10:44

richkadel force-pushed the build-fuchsia-dart-2 branch from 5fe010d to 4e24e74 Compare September 8, 2021 17:47

richkadel force-pushed the build-fuchsia-dart-2 branch 2 times, most recently from 08d8083 to 5e9885d Compare September 8, 2021 18:33

richkadel force-pushed the build-fuchsia-dart-2 branch 3 times, most recently from 5fff63c to b3279b5 Compare September 8, 2021 21:33

Address PR feedback and add an example Dart Fuchsia test package

183b377

Confirmed the new test build rules generate a Dart Fuchsia package that appears to be structured correctly for testing on a Fuchsia platform.

richkadel force-pushed the build-fuchsia-dart-2 branch from b3279b5 to 183b377 Compare September 8, 2021 21:53

chaselatta approved these changes Sep 9, 2021

View reviewed changes

chinmaygarde 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 Sep 9, 2021

fluttergithubbot merged commit 2585976 into flutter-team-archive:master Sep 9, 2021

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Engine from 6a3b04632b57 to 65f1ca53d3af (33 revisions) flutter/flutter#89804

Closed

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

2585976 Fix building Dart Fuchsia components and packages (flutter-te…

4da214c

…am-archive/engine#28413)

engine-flutter-autoroll mentioned this pull request Sep 10, 2021

Roll Engine from 6a3b04632b57 to 57bc7414b894 (35 revisions) flutter/flutter#89807

Merged

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

2585976 Fix building Dart Fuchsia components and packages (flutter-te…

76ca45a

…am-archive/engine#28413)

mdebbar mentioned this pull request Oct 8, 2026

[gn] Use python3 as script_executable in engine/src/.gn flutter/flutter#193954

Closed

		@@ -1,4 +1,4 @@
		# Copyright 2013 The Flutter Authors. All rights reserved.
		# Copyright 2019 The Fuchsia Authors. All rights reserved.

Conversation

richkadel commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

richkadel commented Sep 1, 2021

Uh oh!

richkadel commented Sep 2, 2021

Uh oh!

richkadel commented Sep 2, 2021

Uh oh!

chaselatta commented Sep 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richkadel Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richkadel commented Sep 8, 2021

Uh oh!

richkadel commented Sep 8, 2021

Uh oh!

richkadel commented Sep 8, 2021

Uh oh!

richkadel commented Sep 8, 2021

Uh oh!

richkadel commented Sep 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Sep 9, 2021

Uh oh!

richkadel commented Sep 9, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

richkadel commented Sep 1, 2021 •

edited

Loading

richkadel Sep 2, 2021 •

edited

Loading