Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[macOS] Generate universal gen_snapshots #53524

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Jun 24, 2024

This reverts commit ac4c31a (#52913).
This relands commit 4e33c10 (#52885).

Previously, the gen_snapshot_arm64 and gen_snapshot_x64 binaries used by the tool were all built for x64 architecture. As such, developers building apps with Flutter rely on Rosetta translation with every build.

This refactors the gen_snapshot build rules on macOS hosts to consistently produce gen_snapshot_arm64 and gen_snapshot_x64 binaries with the target architecture of the build but with as universal binaries with both host architectures.

arm64 host build

Prior to this patch we emitted:

  • gen_snapshot_arm64 (arch: x64, target_arch: simarm64)

After this patch, we emit:

  • artifacts_x64/gen_snapshot_arm64 (arch: x64, target_arch: simarm64)
  • artifacts_arm64/gen_snapshot_arm64 (arch: arm64, target_arch: arm64)
  • gen_snapshot_arm64 (universal binary composed of both of the above)

x64 host build

Prior to this patch we emitted:

  • gen_snapshot_x64 (arch: x64, target_arch: x64)

After this patch, we emit:

  • artifacts_x64/gen_snapshot_x64 (arch: x64, target_arch: x64)
  • artifacts_arm64/gen_snapshot_x64 (arch: arm64, target_arch: simx64)
  • gen_snapshot_x64 (universal binary composed of both of the above)

Note that host builds on macOS currently default to a host architecture of x64 (can be overridden via --force-mac-arm64) regardless of host architecture and thus, the build itself relies on Rosetta translation when invoked on Apple Silicon arm64 hardware. This is to ensure a consistent build in CI regardless of bot architecture. See:

engine/tools/gn

Lines 502 to 505 in 6fa734d

# TODO(cbracken):
# https://github.com/flutter/flutter/issues/103386
if get_host_os() == 'mac' and not args.force_mac_arm64:
gn_args['host_cpu'] = 'x64'

Issue: flutter/flutter#101138
Issue: flutter/flutter#69157

Related issue: flutter/flutter#103386

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 the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I'm pretty sure something infra-related will go wrong with this reland, but I'm still not quite sure what it is.
  • 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.

This reverts commit ac4c31a. (flutter#52913)
This relands commit 4e33c10. (flutter#52885)

Previously, the `gen_snapshot_arm64` and `gen_snapshot_x64` binaries used by the tool were all built for x64 architecture. As such, developers building apps with Flutter rely on Rosetta translation with every build.

This refactors the gen_snapshot build rules on macOS hosts to consistently produce `gen_snapshot_arm64` and `gen_snapshot_x64` binaries with the target architecture of the build but with as universal binaries with both host architectures.

Prior to this patch we emitted:
* gen_snapshot_arm64 (arch: x64, target_arch: simarm64)

After this patch, we emit:
* artifacts_x64/gen_snapshot_arm64 (arch: x64, target_arch: simarm64)
* artifacts_arm64/gen_snapshot_arm64 (arch: arm64, target_arch: arm64)
* gen_snapshot_arm64 (universal binary composed of both of the above)

Prior to this patch we emitted:
* gen_snapshot_x64 (arch: x64, target_arch: x64)

After this patch, we emit:
* artifacts_x64/gen_snapshot_x64 (arch: x64, target_arch: x64)
* artifacts_arm64/gen_snapshot_x64 (arch: arm64, target_arch: simx64)
* gen_snapshot_x64 (universal binary composed of both of the above)

Note that host builds on macOS currently default to a host architecture of x64 (can be overridden via `--force-mac-arm64`) regardless of host architecture and thus, the build itself relies on Rosetta translation when invoked on Apple Silicon arm64 hardware. This is to ensure a consistent build in CI regardless of bot architecture. See: https://github.com/flutter/engine/blob/6fa734d686888a39add026a2a98d6ec311c23efb/tools/gn#L502-L505

Issue: flutter/flutter#101138
Issue: flutter/flutter#69157

Related issue: flutter/flutter#103386
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@stuartmorgan
Copy link
Contributor

test-exempt: configuration change

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 24, 2024
@auto-submit auto-submit bot merged commit 552ca5b into flutter:main Jun 24, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2024
@jmagman
Copy link
Member

jmagman commented Jun 24, 2024

Passed in post-submit with longer timeout: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20mac_host_engine/11165/overview

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 25, 2024
…150762)

flutter/engine@be7db94...afa7ce1

2024-06-25 skia-flutter-autoroll@skia.org Roll Dart SDK from bb18127b2a8e to b5fc85cfcf1b (1 revision) (flutter/engine#53552)
2024-06-25 skia-flutter-autoroll@skia.org Roll Skia from 5feca3095719 to 335200e57c26 (1 revision) (flutter/engine#53549)
2024-06-25 skia-flutter-autoroll@skia.org Roll Dart SDK from c187d4b3ec88 to bb18127b2a8e (1 revision) (flutter/engine#53547)
2024-06-25 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from _6HNhJ6G59VMceKoN... to WUN7NQK04NjF9fRmf... (flutter/engine#53545)
2024-06-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Re-re-land "Upgrade all[most] androidx dependencies to latest" (#53532)" (flutter/engine#53546)
2024-06-25 bdero@google.com Bump impeller-cmake-example (flutter/engine#53538)
2024-06-24 skia-flutter-autoroll@skia.org Roll Skia from e20c8b0bac0c to 5feca3095719 (1 revision) (flutter/engine#53544)
2024-06-24 ditman@gmail.com [web] Reland "Fix focus management for text fields (#51009)" (flutter/engine#53537)
2024-06-24 skia-flutter-autoroll@skia.org Roll Dart SDK from 5df89347bddf to c187d4b3ec88 (1 revision) (flutter/engine#53542)
2024-06-24 chris@bracken.jp [macOS] Generate universal gen_snapshots (flutter/engine#53524)
2024-06-24 34871572+gmackall@users.noreply.github.com Re-re-land "Upgrade all[most] androidx dependencies to latest" (flutter/engine#53532)
2024-06-24 skia-flutter-autoroll@skia.org Roll Skia from 1948fd53e280 to e20c8b0bac0c (1 revision) (flutter/engine#53540)
2024-06-24 skia-flutter-autoroll@skia.org Roll Skia from 0fa58b6ddba0 to 1948fd53e280 (2 revisions) (flutter/engine#53536)
2024-06-24 skia-flutter-autoroll@skia.org Roll Skia from ea84df425483 to 0fa58b6ddba0 (3 revisions) (flutter/engine#53535)
2024-06-24 30870216+gaaclarke@users.noreply.github.com [Impeller] added a fallback that will make sure the blur fragment shader doesn't overflow (flutter/engine#53466)
2024-06-24 jonnywang@google.com [fuchsia] Update Fuchsia API level to 19 (flutter/engine#53494)
2024-06-24 skia-flutter-autoroll@skia.org Roll Dart SDK from 95470b2cac1f to 5df89347bddf (1 revision) (flutter/engine#53534)
2024-06-24 skia-flutter-autoroll@skia.org Roll Skia from f6b4344d73cc to ea84df425483 (1 revision) (flutter/engine#53531)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from _6HNhJ6G59VM to WUN7NQK04NjF

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 jimgraham@google.com,rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@jfahrenkrug
Copy link

@cbracken Thanks for this fix! Do we have an idea which Flutter version this will be released in?

@zanderso
Copy link
Member

AFAICT, the zip archives produced by the GN rules for the gen_snapshot archive aren't actually being uploaded to storage and pulled by the tool.

See the build config file here where a different script is used to produce gen_snapshot.zip files. Those are the ones that get uploaded to storage.

I think the fix is to just upload the gen_snapshot.zip files produced by the individual builds instead of using a global generator after the fact. However, I suspect that GN rule that creates the gen_snpashot.zip file will have to also embed an entitlements.txt file like create_macos_gen_snapshot.py does here.

@jmagman
Copy link
Member

jmagman commented Jul 10, 2024

AFAICT, the zip archives produced by the GN rules for the gen_snapshot archive aren't actually being uploaded to storage and pulled by the tool.

@cbracken, when you take a look at this can you also add an arch check to the framework so we can make sure the right ones are getting pulled in?
https://github.com/flutter/flutter/blob/32c0c39ea5d81f0c9b3e51a513c76c4ca3d5d111/packages/flutter_tools/test/general.shard/artifacts_test.dart#L45

Here's an arch check example
https://github.com/flutter/flutter/blob/32c0c39ea5d81f0c9b3e51a513c76c4ca3d5d111/packages/flutter_tools/test/host_cross_arch.shard/ios_content_validation_test.dart#L314-L318

cbracken added a commit to cbracken/flutter_engine that referenced this pull request Jul 17, 2024
In flutter#53524, we started producing universal `gen_snapshot_arm64` and
`gen_snapshot_x64` binaries. This migrates away from bundling
`gen_snapshot` binaries with x64-only host architecture to universal
binaries that bundle both x64 and arm64 host architectures.

Issue: flutter/flutter#101138
Issue: flutter/flutter#69157
auto-submit bot pushed a commit that referenced this pull request Jul 17, 2024
In #53524, we started producing universal `gen_snapshot_arm64` and `gen_snapshot_x64` binaries. This migrates away from bundling `gen_snapshot` binaries with x64-only host architecture to universal binaries that bundle both x64 and arm64 host architectures.

Also adds a dependency on `flutter/lib/snapshot:create_macos_gen_snapshots` to the profile build, where it was previously missing. Presumably, `gen_snapshot` was being built as a side-effect of one of the other targets.

Issue: flutter/flutter#101138
Issue: flutter/flutter#69157

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
5 participants