Generate gen_snapshot_armv7 and gen_snapshot_arm64 #22818

jmagman · 2020-12-02T05:12:11Z

Description

Make local engine builds mimic artifacts by creating gen_snapshot_armv7 and gen_snapshot_arm64 for iOS as appropriate.

I put the logic in shell/platform/darwin/ios/BUILD.gn to mirror where it is in the engine recipe. Let me know if there's a better spot for it.

Related Issues

Fixes flutter/flutter#38933

Split was introduced in flutter/flutter#37445 and #10430

jmagman · 2020-12-02T18:30:59Z

FAILED: clang_x64/gen_snapshot_arm64 
python ../../flutter/sky/tools/create_macos_gen_snapshots.py --dst /opt/s/w/ir/cache/builder/src/out/ios_debug/clang_x64 --arm64-out-dir /opt/s/w/ir/cache/builder/src/out/ios_debug
Cannot find gen_snapshot at /opt/s/w/ir/cache/builder/src/out/ios_debug/clang_x64/gen_snapshot

Back to the drawing board.

cbracken

LGTM other than the gen_snapshot isfile check!

jmagman · 2020-12-02T18:54:11Z

Let's see if it likes this logic in lib/snapshot/BUILD.gn better.

jmagman · 2020-12-02T19:27:46Z

Let's see if it likes this logic in lib/snapshot/BUILD.gn better.

It did! https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20iOS%20Engine/11902/overview

cbracken

re-lgtm!

…ngine#22818)

* d6beaed Roll Fuchsia Linux SDK from gkfmiRsIl... to un3JixwuO... (flutter/engine#22744) * 8832b48 Roll Skia from 888c5d3e57eb to 51b74afb84d4 (12 revisions) (flutter/engine#22746) * e890901 Don't register CanvasKit with `define` (flutter/engine#22745) * 3c51679 Roll Skia from 51b74afb84d4 to 452369182f6e (1 revision) (flutter/engine#22749) * 5bf6533 Introduce a delegate class for gpu metal rendering (flutter/engine#22611) * 5131aa4 Roll Skia from 452369182f6e to f2efb80bc316 (4 revisions) (flutter/engine#22750) * 7b5f79f fuchsia: Ensure full-screen input interceptor (flutter/engine#22687) * cec8a6e Manual roll of Dart SDK from ce76503f5b46 to dcd5a8f005a (flutter/engine#22766) * 001a511 Roll Fuchsia Linux SDK from un3JixwuO... to Bnaeivv07... (flutter/engine#22757) * b9615b1 Roll Fuchsia Mac SDK from 36uDTGJQp... to qpkZl0s5J... (flutter/engine#22753) * c4c4763 Roll Skia from f2efb80bc316 to 8d78da910e45 (5 revisions) (flutter/engine#22754) * dbd1abe Roll Dart SDK from dcd5a8f005a2 to 960620d2e811 (794 revisions) (flutter/engine#22768) * 1c2a6bd Fix the unchecked conversion warning for searchPaths in PlayStoreDynamicFeatureManager (flutter/engine#22654) * 81af789 add file package to deps in prep for glob update (flutter/engine#22770) * a35e3fe Let FlutterFragment not pop the whole activity by default when more fragments are in the activity (flutter/engine#22692) * adb3312 Revert "Introduce a delegate class for gpu metal rendering (#22611)" (flutter/engine#22775) * bcc8832 Cleanup dart_runner examples & tests. (flutter/engine#22769) * 609307d Roll Skia from 8d78da910e45 to fd41d878b13d (20 revisions) (flutter/engine#22772) * 587c023 [web] Add new line break type (prohibited) (flutter/engine#22771) * 6b2ed2b Roll Skia from fd41d878b13d to 70fe17e12f38 (6 revisions) (flutter/engine#22776) * 7910a17 Roll Dart SDK from 960620d2e811 to 7a2a3968ef53 (12 revisions) (flutter/engine#22778) * f4ada80 Roll Skia from 70fe17e12f38 to 4c6f57a23e63 (1 revision) (flutter/engine#22781) * 3101dff [web] Optimize Matrix4.identity (flutter/engine#22622) * a4ce848 Add FlutterPlayStoreSplitApplication for simpler opt in to Split AOT (flutter/engine#22752) * 747b791 Add file.dart to DEPS (flutter/engine#22794) * 40fa345 Fix race condition in key event handling on Android (flutter/engine#22658) * d2ad441 Fix PlatformDispatcher.locale to return something meaningful when there are no locales. (flutter/engine#22608) * b9a0b5e Roll Skia from 4c6f57a23e63 to a927771c9cce (10 revisions) (flutter/engine#22802) * 96d63e5 Roll Dart SDK from 7a2a3968ef53 to e9a03fd98faa (5 revisions) (flutter/engine#22801) * cdf72da Roll Skia from a927771c9cce to 7b776b514933 (3 revisions) (flutter/engine#22803) * a0c8b67 Roll buildroot and benchmark (flutter/engine#22804) * c3c3ec6 Roll Fuchsia Mac SDK from qpkZl0s5J... to 7O11wjLVX... (flutter/engine#22805) * 6625308 Revert "Roll buildroot and benchmark (#22804)" (flutter/engine#22816) * 64d9add Add a golden scenario test for fallback font rendering on iOS take 3 (flutter/engine#22736) * 7d7a260 Add static text trait to plain semantics object with label in iOS (flutter/engine#22811) * 22e1143 Roll Skia from 7b776b514933 to c504ecda03b8 (6 revisions) (flutter/engine#22808) * 65254eb Roll Dart SDK from e9a03fd98faa to 5acaa5f14b03 (1 revision) (flutter/engine#22810) * 3926b21 Roll Fuchsia Linux SDK from Bnaeivv07... to W14Qninrb... (flutter/engine#22817) * 5eb505f Roll Fuchsia Mac SDK from 7O11wjLVX... to Z_-ciOYM9... (flutter/engine#22820) * d85cb10 add trace kernel flag to allowlist (flutter/engine#22812) * 14cb066 [embedder] Compositor can specify that no backing stores be cached (flutter/engine#22780) * eb6eabc Reland "Introduce a delegate class for gpu metal rendering (#22611)" (flutter/engine#22777) * 644dd65 Temporarily reduce e2e test matrix to stop flaky web engine builds (flutter/engine#22824) * 105004d Stop using the List constructor. (flutter/engine#22793) * 34f49a1 Roll Dart SDK from 5acaa5f14b03 to cfaa7606cbf5 (2 revisions) (flutter/engine#22827) * 6c8342f Revert "Fix race condition in key event handling on Android (#22658)" (flutter/engine#22823) * 1c2a8f9 Roll Skia from c504ecda03b8 to 9443d58af292 (16 revisions) (flutter/engine#22828) * b63e911 Better handle image codec instantiation failure (flutter/engine#22809) * 1358fda Generate Maven metadata files for engine artifacts (flutter/engine#22685) * 079c669 Generate gen_snapshot_armv7 and gen_snapshot_arm64 (flutter/engine#22818) * fcbfa9f Split AOT Engine Runtime (flutter/engine#22624) * 24d289e Roll Fuchsia Linux SDK from W14Qninrb... to M_8svVndh... (flutter/engine#22842) * 78b567f Reland: "Fix race condition in key event handling on Android (#22658)" (flutter/engine#22834) * 7d32cea (MacOS) Add FlutterGLCompositor with support for rendering multiple layers (flutter/engine#22782) * a713174 Roll Skia from 9443d58af292 to c7112edbe0f4 (10 revisions) (flutter/engine#22839) * bee352c Roll Dart SDK from cfaa7606cbf5 to 97cfd05b3cb3 (2 revisions) (flutter/engine#22840) * e5f510f [web] Fix event transform between mousedown/up due to mouse move event (flutter/engine#22813) * 04b98dc Roll Fuchsia Mac SDK from Z_-ciOYM9... to DRN4P3zbe... (flutter/engine#22841) * 0e3b2cf Roll Skia from c7112edbe0f4 to d39aec0e40ec (17 revisions) (flutter/engine#22844) * e71c6f4 leaving only html tests (flutter/engine#22846) * 3773835 Make CkPicture resurrectable (flutter/engine#22807) * bd394a1 Roll Skia from d39aec0e40ec to 38921cafe1bb (7 revisions) (flutter/engine#22847) * 66f44c6 Roll Dart SDK from 97cfd05b3cb3 to a37a4d42e53d (4 revisions) (flutter/engine#22849) * bdadaad Add delayed event delivery for Linux. (flutter/engine#22577) * 48befc5 More rename from GPU thread to raster thread (flutter/engine#22819) * 9b1b7f6 Roll Skia from 38921cafe1bb to abcc1ecdfd0c (8 revisions) (flutter/engine#22851) * 14a6fd9 Fix NPE when platform plugin delegate is null (flutter/engine#22852)

zanderso

dbc

zanderso · 2021-01-14T18:12:10Z

lib/snapshot/BUILD.gn

@@ -197,6 +197,29 @@ bin_to_linkable("platform_strong_dill_linkable") {
  executable = false
 }

+action("create_arm_gen_snapshot") {


@jmagman @cbracken This rule must declare a dependency on some rule that causes gen_snapshot to end up at the expected location. Otherwise GN/Ninja might try to run this rule before gen_snapshot is there.

I had to reopen flutter/flutter#38933 since it didn't work the next time I ran it. That would explain it...

I am poking around something related, and will make a concrete suggestion shortly.

When performing artifact lookups for Artifact.genSnapshot, a TargetPlatform is used to determine the name of the tool, typically `gen_snapshot_$TARGET_ARCH`. Formerly, this tool was always named `gen_snapshot`. The astute reader may ask "but Chris, didn't we support TWO target architectures on iOS and therefore need TWO gen_snapshot binaries?" Yes, we did support both armv7 and arm64 target architectures on iOS. No, we didn't have two gen_snapshot binaries. At the time, the bitness of the gen_snapshot tool needed to match the bitness of the target architecture. As such we did *build* two gen_snapshots: * A 32-bit x86 binary that emitted armv7 AOT code * A 64-bit x64 binary that emitted arm64 AOT code However, to avoid having to do a lot of work plumbing through suffixed gen_snapshot names, the author of that work elected to "cleverly" lipo the two together into a single multi-architecture macOS binary. See: flutter/engine#4948 This was later remediated over the course of several patches, including: See: flutter#37445 See: flutter/engine#10430 See: flutter/engine#22818 However, there were still cases (notably --local-engine workflows in the tool) where we weren't computing the target platform and thus referenced the generic gen_snapshot tool. See: flutter#38933 Fixed in: flutter/engine#28345 The test removed in this PR, which ensured that null SnapshotType.platform was supported was introduced in flutter#11924 as a followup to flutter#11820 when the snapshotting logic was originally extracted to the `GenSnapshot` class. Since there are no longer any cases where TargetPlatform isn't passed when looking up Artifacts.genSnapshot, we can safely make the platform non-nullable and remove the test. This is pre-factoring towrards the removal of the generic gen_snapshot artifact, which is pre-factoring towards the goal of building gen_snapshot binaries with an arm64 host architecture, and eliminate the need to use Rosetta during iOS and macOS Flutter builds. Issue: flutter#101138 Issue: flutter#103386

When performing artifact lookups for `Artifact.genSnapshot` for macOS desktop builds, a `TargetPlatform` is used to determine the name of the tool, typically `gen_snapshot_$TARGET_ARCH`. Formerly, this tool was always named `gen_snapshot`. The astute reader may ask "but Chris, didn't we support TWO target architectures on iOS and therefore need TWO `gen_snapshot` binaries?" Yes, we did support both armv7 and arm64 target architectures on iOS. But no, we didn't initially have two `gen_snapshot` binaries. We did *build* two `gen_snapshots`: * A 32-bit x86 binary that emitted armv7 AOT code * A 64-bit x64 binary that emitted arm64 AOT code At the time, the bitness of the `gen_snapshot` tool needed to match the bitness of the target architecture, and to avoid having to do a lot of work plumbing through suffixed `gen_snapshot` names, the author of that work (who, as evidenced by this patch, is still paying for his code crimes) elected to "cleverly" lipo the two together into a single multi-architecture macOS binary still named `gen_snapshot`. See: flutter/engine#4948 This was later remediated over the course of several patches, including: * flutter/engine#10430 * flutter/engine#22818 * #37445 However, there were still cases (notably `--local-engine` workflows in the tool) where we weren't computing the target platform and thus referenced the generic `gen_snapshot` tool. See: #38933 Fixed in: flutter/engine#28345 The test removed in this PR, which ensured that null `SnapshotType.platform` was supported was introduced in #11924 as a followup to #11820 when the snapshotting logic was originally extracted to the `GenSnapshot` class, and most invocations still passed a null target platform. Since there are no longer any cases where `TargetPlatform` isn't passed when looking up `Artifact.genSnapshot`, we can safely make the platform non-nullable and remove the test. This is pre-factoring towards the removal of the generic `gen_snapshot` artifact from the macOS host binaries (which are currently unused since we never pass a null `TargetPlatform`), which is pre-factoring towards the goal of building `gen_snapshot` binaries with an arm64 host architecture, and eliminate the need to use Rosetta during iOS and macOS Flutter builds. Part of: #101138 Umbrella issue: #103386 Umbrella issue: #69157 No new tests since the behaviour is enforced by the compiler.

When performing artifact lookups for `Artifact.genSnapshot` for macOS desktop builds, a `TargetPlatform` is used to determine the name of the tool, typically `gen_snapshot_$TARGET_ARCH`. Formerly, this tool was always named `gen_snapshot`. The astute reader may ask "but Chris, didn't we support TWO target architectures on iOS and therefore need TWO `gen_snapshot` binaries?" Yes, we did support both armv7 and arm64 target architectures on iOS. But no, we didn't initially have two `gen_snapshot` binaries. We did *build* two `gen_snapshots`: * A 32-bit x86 binary that emitted armv7 AOT code * A 64-bit x64 binary that emitted arm64 AOT code At the time, the bitness of the `gen_snapshot` tool needed to match the bitness of the target architecture, and to avoid having to do a lot of work plumbing through suffixed `gen_snapshot` names, the author of that work (who, as evidenced by this patch, is still paying for his code crimes) elected to "cleverly" lipo the two together into a single multi-architecture macOS binary still named `gen_snapshot`. See: flutter/engine#4948 This was later remediated over the course of several patches, including: * flutter/engine#10430 * flutter/engine#22818 * flutter#37445 However, there were still cases (notably `--local-engine` workflows in the tool) where we weren't computing the target platform and thus referenced the generic `gen_snapshot` tool. See: flutter#38933 Fixed in: flutter/engine#28345 The test removed in this PR, which ensured that null `SnapshotType.platform` was supported was introduced in flutter#11924 as a followup to flutter#11820 when the snapshotting logic was originally extracted to the `GenSnapshot` class, and most invocations still passed a null target platform. Since there are no longer any cases where `TargetPlatform` isn't passed when looking up `Artifact.genSnapshot`, we can safely make the platform non-nullable and remove the test. This is pre-factoring towards the removal of the generic `gen_snapshot` artifact from the macOS host binaries (which are currently unused since we never pass a null `TargetPlatform`), which is pre-factoring towards the goal of building `gen_snapshot` binaries with an arm64 host architecture, and eliminate the need to use Rosetta during iOS and macOS Flutter builds. Part of: flutter#101138 Umbrella issue: flutter#103386 Umbrella issue: flutter#69157 No new tests since the behaviour is enforced by the compiler.

Generate gen_snapshot_armv7 and gen_snapshot_arm64

f8f5f4c

jmagman self-assigned this Dec 2, 2020

flutter-dashboard bot added the platform-ios label Dec 2, 2020

googlebot added the cla: yes label Dec 2, 2020

jmagman requested review from cbracken and liamappelbe December 2, 2020 05:15

jmagman marked this pull request as draft December 2, 2020 18:29

jmagman removed request for cbracken and liamappelbe December 2, 2020 18:29

cbracken approved these changes Dec 2, 2020

View reviewed changes

Move to snapshot/BUILD.gn

6fdd1f5

jmagman marked this pull request as ready for review December 2, 2020 19:27

cbracken approved these changes Dec 2, 2020

View reviewed changes

jmagman merged commit 079c669 into flutter:master Dec 2, 2020

jmagman deleted the gen_snap_arm branch December 2, 2020 20:43

engine-flutter-autoroll mentioned this pull request Dec 4, 2020

Roll Engine from 20caf5496951 to 14a6fd97ca12 (67 revisions) flutter/flutter#71699

Merged

engine-flutter-autoroll mentioned this pull request Dec 4, 2020

Roll Engine from 20caf5496951 to 6a5971dac761 (70 revisions) flutter/flutter#71701

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2020

079c669 Generate gen_snapshot_armv7 and gen_snapshot_arm64 (flutter/e…

e5e855a

…ngine#22818)

cbracken pushed a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2020

079c669 Generate gen_snapshot_armv7 and gen_snapshot_arm64 (flutter/e…

9c6d275

…ngine#22818)

zanderso reviewed Jan 14, 2021

View reviewed changes

jmagman mentioned this pull request Aug 27, 2021

organize snapshot/BUILD.gn targets and fix create_arm_gen_snapshot #28345

Merged

8 tasks

cbracken mentioned this pull request Apr 17, 2024

[tools] Make SnapshotType.platform non-nullable flutter/flutter#146958

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Generate gen_snapshot_armv7 and gen_snapshot_arm64 #22818

Generate gen_snapshot_armv7 and gen_snapshot_arm64 #22818

Uh oh!

jmagman commented Dec 2, 2020

Uh oh!

jmagman commented Dec 2, 2020

Uh oh!

cbracken left a comment

Uh oh!

jmagman commented Dec 2, 2020

Uh oh!

jmagman commented Dec 2, 2020

Uh oh!

cbracken left a comment

Uh oh!

zanderso left a comment

Uh oh!

zanderso Jan 14, 2021

Uh oh!

jmagman Jan 14, 2021

Uh oh!

zanderso Jan 14, 2021

Uh oh!

Uh oh!

Generate gen_snapshot_armv7 and gen_snapshot_arm64 #22818

Generate gen_snapshot_armv7 and gen_snapshot_arm64 #22818

Uh oh!

Conversation

jmagman commented Dec 2, 2020

Description

Related Issues

Uh oh!

jmagman commented Dec 2, 2020

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman commented Dec 2, 2020

Uh oh!

jmagman commented Dec 2, 2020

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

zanderso Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!