-
Notifications
You must be signed in to change notification settings - Fork 6k
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 #52885
Conversation
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 both host architectures. Differences introduced in arm64 host build: Prior to this patch we emitted: * gen_snapshot_arm64 (arch: x64, target_arch: simarm64) After this patch, we emit: * gen_snapshot_arm64 (arch: x64, target_arch: simarm64) * gen_snapshot_arm64 (arch: arm64, target_arch: arm64) Differences introduced in x64 host build: Prior to this patch we emitted: * gen_snapshot_x64 (arch: x64, target_arch: x64) After this patch, we emit: * gen_snapshot_x64 (arch: x64, target_arch: x64) * gen_snapshot_x64 (arch: arm64, target_arch: simx64) 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
There's a bunch of common logic between this and the universal framework scripts that we should really extract into a library. I'll take a look and send a followup patch for the relevant bits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the tests are happy
@@ -416,6 +416,7 @@ | |||
../../../flutter/sky/tools/create_embedder_framework.py | |||
../../../flutter/sky/tools/create_full_ios_framework.py | |||
../../../flutter/sky/tools/create_ios_framework.py | |||
../../../flutter/sky/tools/create_macos_binary.py | |||
../../../flutter/sky/tools/create_macos_framework.py | |||
../../../flutter/sky/tools/create_macos_gen_snapshots.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DBC
What's the difference between create_macos_binary.py
and create_macos_gen_snapshots.py
? Do we need both of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting -- create_macos_gen_snapshots is old and was used for iOS (and is hardcoded full of armv7 stuff); I guess we never cleaned it up?
I'll send out a patch to kill it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create_macos_gen_snapshots
is used in the generator which processes the output of the builds
engine/ci/builders/mac_host_engine.json
Line 560 in 7a309e5
"script": "flutter/sky/tools/create_macos_gen_snapshots.py" |
To avoid the timeout and because an hour is really quite long to wait for the builds to clear, maybe the snapshots should be created in different builds, and then processed with the generator at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there should be examples of using global generators already in the ios/macos build json files, and I think that can work here if we know how to split the different targets across different builders.
Reason for revert: while this patch worked fine, it pushed the mac build bot over its time limit. Previous builds were just squeaking under the wire with a minute or two to spare, but I'm seeing timeouts on the mac_host_engine host_release shard after this commit. e.g. https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20mac_host_engine/10667/overview Looking at ci.yaml I see the timeout is set to 240, but the timeout on the failing shard is clearly 60 mins. See: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/344592/overview |
This reverts commit 4e33c10.
Reverts: #52885 Initiated by: cbracken Reason for reverting: while this patch worked fine, it pushed the mac build bot over its time limit. Previous builds were just squeaking under the wire but seeing timeouts on the mac_host_engine host_release shard after this commit. e.g. https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20mac_host_engine/10667/overview Looking at ci.yaml I see the timeout is set to 240, but the timeout on the failing shard is c Original PR Author: cbracken Reviewed By: {jmagman} This change reverts the following previous change: 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: https://github.com/flutter/engine/blob/6fa734d686888a39add026a2a98d6ec311c23efb/tools/gn#L502-L505 Issue: flutter/flutter#101138 Issue: flutter/flutter#69157 Related issue: flutter/flutter#103386 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…148595) flutter/engine@93f1b5a...552a965 2024-05-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fixes MatrixFilterContents rendering/coverage (#52880)" (flutter/engine#52918) 2024-05-18 [email protected] Fixes MatrixFilterContents rendering/coverage (flutter/engine#52880) 2024-05-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[macOS] Generate universal gen_snapshots (#52885)" (flutter/engine#52913) 2024-05-17 [email protected] [macOS] Generate universal gen_snapshots (flutter/engine#52885) 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 [email protected],[email protected],[email protected] 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
Ooof. Unfortunately, I think the subbuild timeout is hardcoded in the recipes. We need a case like this but for no rbe: https://flutter.googlesource.com/recipes/+/refs/heads/main/recipe_modules/shard_util/api.py#404. |
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
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: https://github.com/flutter/engine/blob/6fa734d686888a39add026a2a98d6ec311c23efb/tools/gn#L502-L505 Issue: flutter/flutter#101138 Issue: flutter/flutter#69157 Related issue: flutter/flutter#103386 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Previously, the
gen_snapshot_arm64
andgen_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
andgen_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:
After this patch, we emit:
x64 host build
Prior to this patch we emitted:
After this patch, we emit:
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
Issue: flutter/flutter#101138
Issue: flutter/flutter#69157
Related issue: flutter/flutter#103386
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.