Build rules for scenario_app #27302

dnfield · 2021-07-10T07:24:18Z

Makes scenario_app part of the GN build.

This deletes shell scripts that were used solely by developers/internal scripts to compile assets, since that is all now taken care of by GN/Ninja.

It leaves scripts that are currently used by CI. Once this change is in place, CI can be changed to avoid using those scripts, in particular ones that directly invoke ninja/autoninja, which is incompatible with upcoming verisons of depot_tools/LUCI.

High level overview of changes:

New build rules added to create dart kernel and AOT/JIT gen_snapshot artifacts for Android/iOS
New build rule that invokes gradle and tries to get it to use the out/android_whatever directory for build output.
Updates to gradle scripts to avoid copying things into the source tree, e.g. pulling flutter.jar and libapp.so from the out directory instead of from the source tree. This means that the gradlew builds will no longer work without specifying these properties.
Copy the scenario app into the out direcotry using copy_trees to avoid mutating the source tree during build.
Create the App.framework directly in out/ios_whatever/scenario_app/Scenarios instead of in the source tree.
Updates to the Scenario xcodeproj to refer to Flutter.xcframework from two directories up (e.g. out/ios_whatever/scenario_app/Scenarios/../../Flutter.xcframework) to avoid needless copying.
Delete the compile_[ios|android]_[jit|aot].sh scripts.
Update the run_and_build_[ios|android]tests.sh and the build[ios|android]_tests.sh scripts to use the GN/ninja built artifacts to avoid disruptions on CI

These builds also avoid needing a host toolchain build by using prebuilt dart sdk.

While working on this patch, I discovered that the scenario_app FTL android test is completely broken. That's fixed separately in #27295.

This work should help unblock adding a test harness for #27141

Related bugs:
fixes flutter/flutter#53455
part of flutter/flutter#84618
part of flutter/flutter#84290

/cc @godofredoc

chinmaygarde

Sorely needed. LGTM with some nits and comments.

chinmaygarde · 2021-07-11T01:04:38Z

build/dart/rules.gni

+
+  kernel_target = "_${target_name}_kernel"
+  snapshot_target = "_${target_name}_snapshot"
+  is_aot =


Nit: Perhaps add assert(!is_fuchsia) here as this would be invalid on that target.

@richkadel: FYI, you may run into such assumptions while working with engine GN rules for Dart. We can patch them later though. The issue is that the assumption of AOT being tied to a specific runtime mode does not hold on Fuchsia.

Fuchsia has its own template(s) for this.

Perhaps we could add it as an arg and default it to this, and let fuchsia specify the arg differently?

flutter_runtime_mode can also be jit_release. It might simplify things for Fuchsia to use that to signal that a release build is using JIT rather than AOT (if it isn't like that already.)

For now I'm just moving this to //flutter/testing/scenario_app/runtime_mode.gni.

Errr... wrong comment thread - I moved it from common.gni. I'll add the assert.

chinmaygarde · 2021-07-11T01:07:51Z

build/dart/rules.gni

+#
+# Invoker must supply dart_main and package_config. Invoker may optionally
+# supply aot as a boolean and product as a boolean.
+template("dart_snapshot") {


This is an unfortunate repetition of the rule (in //flutter/testing/testing.gni) used by the gtest harness for Dart fixtures. Can we dry this up (in a later patch if you wish)? If anything, //build/dart/rules.gni is the right spot for such rules.

I think this will lead to template name collision if both gnis are included from a .GN definition.

There are now several places where we have rules like this - I'd like to clean them up separately to avoid potential issues that only get caught in post submit :\

One thing I can see is that the testing rules lack special handling for iOS, which will need to be addressed.

chinmaygarde · 2021-07-11T01:12:11Z

build/dart/rules.gni

+    main_dart = rebase_path(invoker.main_dart)
+    package_config = rebase_path(invoker.package_config)
+
+    platform_dill = "$root_out_dir/flutter_patched_sdk/platform_strong.dill"


Minor nit: Presumably, these are outputs of the //flutter/lib/snapshot dep below. Can we use get_label_info with target_gen_dir to determine the location of these artifacts? Right now, these paths are hardcoded in unrelated rules.

This isn't in the target_gen_dir of that target - it's hard coded in there.

Changing that is possible, but right now recipes depend on this location being where it is.

I have a goal of cleaning up the way recipes get artifacts though - and once that's all set, we can start changing these things more easily.

chinmaygarde · 2021-07-11T01:14:11Z

build/dart/rules.gni

+# supply aot as a boolean and product as a boolean.
+template("dart_snapshot") {
+  assert(defined(invoker.main_dart), "main_dart is a required parameter.")
+  assert(defined(invoker.sources), "sources is a required parameter.")


Why must the sources be explicitly specified even though the script action supports depfiles? Seems redundant. The main_dart and the package configuration ought to sufficient.

Yeah this is wrong, removed it.

chinmaygarde · 2021-07-11T01:16:07Z

build/dart/rules.gni

+    outputs = [ kernel_output ]
+
+    depfile = "$kernel_output.d"
+    abs_depfile = rebase_path(depfile)


Note: Not that it matters here, but using absolute paths trips up compiler proxies. If the script supports relative depfile paths, I'd recommend just using relative paths from the get go.

This was cargo-culted from another template - it seems to make dart happy, which doesn't support compiler proxies so... I guess it's ok?

chinmaygarde · 2021-07-11T01:18:07Z

build/dart/rules.gni

+      "--output=" + rebase_path(kernel_output),
+    ]
+
+    if (is_debug) {


I am confused if you meant is_debug to mean unopt instead of the debug runtime mode.

We should probably rename this to is_unopt. But that's what it means - it's distinct from the runtime mode.

chinmaygarde · 2021-07-11T01:22:34Z

common/config.gni

  flutter_prebuilt_dart_sdk = false
 }

+is_aot = flutter_runtime_mode == "profile" || flutter_runtime_mode == "release"


This doesn't hold for Fuchsia. I don't think we should add this to the common config if its not actually true everywhere.

zanderso · 2021-07-11T05:10:37Z

build/dart/rules.gni

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

 # This file has rules for making Dart packages and Dart-based Mojo applications.


Consider updating this comment.

Missed this earlier - done

zanderso · 2021-07-11T05:19:00Z

build/dart/rules.gni

+      args += [ "--enable_asserts" ]
+    }
+
+    if (!is_aot) {


Think positive: if (is_aot) { ... } else { ... }

zanderso · 2021-07-11T05:22:20Z

testing/scenario_app/BUILD.gn

+  } else if (is_ios) {
+    _flutter_assets_dir =
+        "$root_out_dir/scenario_app/Scenarios/App.framework/flutter_assets"
+  }


Suggest adding a final else clause that contains only an assert(false, error message).

zanderso · 2021-07-11T05:35:10Z

build/dart/rules.gni

+
+  kernel_target = "_${target_name}_kernel"
+  snapshot_target = "_${target_name}_snapshot"
+  is_aot =


flutter_runtime_mode can also be jit_release. It might simplify things for Fuchsia to use that to signal that a release build is using JIT rather than AOT (if it isn't like that already.)

zanderso · 2021-07-11T05:41:47Z

build/dart/rules.gni

+    vm_args = [
+      "--depfile=$abs_depfile",
+      "--depfile_output_filename=$rebased_output",
+      "--disable-dart-dev",


Does this flag need to come first?

I don't know - it's how the VM gets invoked today though and it doesn't complain?

Just asked around. It did have to come first at some point in the past, but it can go anywhere now.

zanderso · 2021-07-11T05:50:47Z

build/dart/rules.gni

+import("//flutter/common/config.gni")
+import("//third_party/dart/build/dart/dart_action.gni")
+
+# Creates a dart kernel (dill) file suitalbe for use with gen_snapshot.


It looks like the template does more than just generate the .dill file. For AOT, it generates the AOT snapshot, and in non-AOT it generates an app-jit snapshot.

Updated the comment.

zanderso · 2021-07-11T05:54:15Z

build/dart/rules.gni

+    package_config = rebase_path(invoker.package_config)
+
+    platform_dill = "$root_out_dir/flutter_patched_sdk/platform_strong.dill"
+    gen_kernel_script = "//third_party/dart/pkg/vm/bin/gen_kernel.dart"


Why gen_kernel and not //flutter/flutter_frontend_server? Using the flutter_frontend_server would more closely mimic how the flutter_tool compiles Dart: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/compile.dart#L268

Good point - done. Although this is a small change in behavior, since before the scenario app was not using the flutter_frontend_server.

zanderso · 2021-07-12T16:56:19Z

build/dart/rules.gni

+    flutter_patched_sdk = rebase_path("$root_out_dir/flutter_patched_sdk")
+
+    deps = [
+      "//flutter/flutter_frontend_server:frontend_server",


This was previously only built when targeting the host. It's built for those configurations as an app-jit snapshot for fast startup because it's distributed as an engine artifact for use by end-users. During an iOS/Android build, if it's only being used for a small number of targets, then it will take longer to build the app-jit snapshot than the time it saves by starting up more quickly. That being the case, I'd suggest running this from source using prebuilt_dart_action().

Cool, will change - I noticed the build time went up doing it this way :\

zanderso · 2021-07-12T16:58:01Z

build/dart/rules.gni

+    vm_args = [
+      "--depfile=$abs_depfile",
+      "--depfile_output_filename=$rebased_output",
+      "--disable-dart-dev",


Just asked around. It did have to come first at some point in the past, but it can go anywhere now.

zanderso · 2021-07-12T17:03:25Z

testing/scenario_app/runtime_mode.gni

+
+import("//flutter/common/config.gni")
+
+is_aot = flutter_runtime_mode == "profile" || flutter_runtime_mode == "release"


Is it possible to assert at the top-level of this file that the target OS is iOS or Android?

zanderso · 2021-07-12T17:06:53Z

sky/packages/sky_engine/lib/_embedder.yaml

-# This file is suitable for use within the tree. A different _embedder.yaml
-# is generated by the BUILD.gn in this directory. Changes here must be
-# mirrored there.
+# This file is generated by //flutter/sky/packages/sky_engine:_embedder_yaml


I'm a bit confused by this comment. Is the behavior any different than before this change? If so, are there some changes to the BUILD.gn file that didn't get pushed?

Also, weren't there some other users of this file aside from the scenario apps? If not, and this file is only ever used out of the build output directory, and not directly, then this file is more like a template, and so we should rename it to something like _embedder.yaml.tmpl.

Something is weird with my git checkout here - this file shouldn't hav ebeen modified.

blasten · 2021-07-12T17:30:33Z

build/dart/rules.gni

+import("//flutter/common/config.gni")
+import("//third_party/dart/build/dart/dart_action.gni")
+
+# Creates a dart kernel (dill) file suitalbe for use with gen_snapshot, as well


nit: suitable

blasten · 2021-07-12T17:42:27Z

testing/scenario_app/android/BUILD.gn

+    "app/src/main/java/dev/flutter/scenarios/TextPlatformViewActivity.java",
+    "app/src/main/java/dev/flutter/scenarios/TextPlatformViewFactory.java",
+    "app/src/main/java/io/flutter/plugins/GeneratedPluginRegistrant.java",
+    "build.gradle",


what do you think about globbing under app/src?

GN does not like globs.

(gradle will still build the files anyway, but GN won't be aware of changes to unmentioned ones dirtying the build)

blasten · 2021-07-12T17:49:58Z

testing/scenario_app/android/run_gradle.py

+# found in the LICENSE file.
+
+"""
+Invokes gradlew for building the scenario_app from GN/Ninja.


does gn have a way to run a target? If yes, is this wrapper script still needed?

GN knows how to run compiler binaries with some help, and it knows how to run python scripts.

This is actually really hacky - we shouldn't be invoking gradle from GN, but refactoring that will take more work and we're no worse off than we were before invoking gradle from a shell script.

does it know how to run a bash script? If that's the case, then gradlew directly should work, no?

That doesn't work on Windows.

It could use gradlew.bat on Windows

Which is all this python script is doing

blasten · 2021-07-12T17:50:40Z

testing/scenario_app/run_android_tests.sh

 cd "$SCRIPT_DIR/android"
 GRADLE_USER_HOME="$PWD/android/gradle-home/.cache"
-set -o pipefail && ./gradlew app:verifyDebugAndroidTestScreenshotTest --gradle-user-home "$GRADLE_USER_HOME"
+set -o pipefail && ./gradlew app:verifyDebugAndroidTestScreenshotTest \


it can be moved to gn too, right?

This actually runs unit tests. GN builds things, it doesn't run them.

zanderso

LGTM

dnfield · 2021-07-13T16:11:13Z

@blasten @jmagman - I'm going to land this, happy to follow up if there are any outstanding issues/improvements wanted on Android/iOS

dnfield · 2021-07-13T16:11:49Z

I also strongly suspect there are variants of this built on post not built in presubmits - I'll be watching to make sure this passes.

zanderso · 2021-07-13T16:19:45Z

@dnfield Please also keep an eye on the overall time on the builders. This should probably have little or no effect, but let's make sure.

This reverts commit bc2cf93.

…r#27358)" This reverts commit 99f8791.

* Revert "Revert "Build rules for scenario_app (#27302)" (#27358)" This reverts commit 99f8791. * Ignore *.*framework in copy_trees

dnfield · 2021-07-13T17:38:23Z

Bots are taking slightly longer with an initial run on this - although it's hard to say why. For example, bot update took longer on this patch, but this patch has nothing to do with that step.

Is there some dashboard where I can see the timeframes of this stuff? @keyonghan might know - it's a little hard to track this through the LUCI Milo UI.

zanderso · 2021-07-13T17:41:47Z

@dnfield Long Running Builders in the go/flutter-infra-metrics dashboard. You can play with the filters on the chart to select the builders and timeframe that you're interested in.

This reverts commit bc2cf93.

* Revert "Revert "Build rules for scenario_app (flutter#27302)" (flutter#27358)" This reverts commit 99f8791. * Ignore *.*framework in copy_trees

This reverts commit bc2cf93.

* Revert "Revert "Build rules for scenario_app (flutter#27302)" (flutter#27358)" This reverts commit 99f8791. * Ignore *.*framework in copy_trees

Build rules for scenario_app

578460b

dnfield requested review from blasten, chinmaygarde, jmagman and zanderso July 10, 2021 07:24

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

chinmaygarde approved these changes Jul 11, 2021

View reviewed changes

zanderso reviewed Jul 11, 2021

View reviewed changes

dnfield added 3 commits July 12, 2021 08:03

Merge remote-tracking branch 'upstream/master' into scenario_pub

7e1d933

review

71f998d

review

3354f7c

zanderso reviewed Jul 12, 2021

View reviewed changes

blasten reviewed Jul 12, 2021

View reviewed changes

dnfield added 3 commits July 12, 2021 10:47

Revert unintended change, use frontend from source

c91a053

assert

097d1d3

typo

c19070a

blasten reviewed Jul 12, 2021

View reviewed changes

dnfield added 3 commits July 12, 2021 11:03

format

1d5e61f

Merge remote-tracking branch 'upstream/master' into scenario_pub

c38c2ee

one more comment

06178f2

dnfield requested review from blasten and zanderso July 12, 2021 23:36

zanderso approved these changes Jul 13, 2021

View reviewed changes

dnfield merged commit bc2cf93 into flutter:master Jul 13, 2021

dnfield deleted the scenario_pub branch July 13, 2021 16:12

zanderso added a commit that referenced this pull request Jul 13, 2021

Revert "Build rules for scenario_app (#27302)"

133a407

This reverts commit bc2cf93.

zanderso mentioned this pull request Jul 13, 2021

Revert "Build rules for scenario_app" #27358

Merged

zanderso added a commit that referenced this pull request Jul 13, 2021

Revert "Build rules for scenario_app (#27302)" (#27358)

99f8791

This reverts commit bc2cf93.

dnfield added a commit to dnfield/engine that referenced this pull request Jul 13, 2021

Revert "Revert "Build rules for scenario_app (flutter#27302)" (flutte…

98bffb5

…r#27358)" This reverts commit 99f8791.

dnfield added a commit that referenced this pull request Jul 13, 2021

Reland build rules for scenario app (#27360)

c74f25f

* Revert "Revert "Build rules for scenario_app (#27302)" (#27358)" This reverts commit 99f8791. * Ignore *.*framework in copy_trees

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

Roll Engine from 07d04041d58e to c74f25f4cb0a (8 revisions) flutter/flutter#86366

Merged

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

bc2cf93 Build rules for scenario_app (flutter/engine#27302)

79361c9

jmagman mentioned this pull request Jul 29, 2021

iOS Scenario App tests isn't running during the Mac iOS Engine build flutter/flutter#87308

Closed

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

Build rules for scenario_app (flutter#27302)

4010690

moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021

Revert "Build rules for scenario_app (flutter#27302)" (flutter#27358)

a830f32

This reverts commit bc2cf93.

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Build rules for scenario_app (flutter#27302)

3cfa975

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Revert "Build rules for scenario_app (flutter#27302)" (flutter#27358)

1985815

This reverts commit bc2cf93.

jmagman mentioned this pull request Jul 8, 2022

Add scenario_app target to iOS scenario test README #34552

Closed

8 tasks


		import("//flutter/common/config.gni")

		is_aot = flutter_runtime_mode == "profile" \|\| flutter_runtime_mode == "release"

Build rules for scenario_app #27302

Build rules for scenario_app #27302

Uh oh!

Conversation

dnfield commented Jul 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde left a comment

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!

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!

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!

dnfield commented Jul 10, 2021 •

edited

Loading