[tools] Make SnapshotType.platform non-nullable by cbracken · Pull Request #146958 · flutter/flutter

cbracken · 2024-04-17T23:14:36Z

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:

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.

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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

jmagman

If you make

flutter/packages/flutter_tools/lib/src/artifacts.dart

Line 421 in 1ce6292

TargetPlatform? platform,

required TargetPlatform platform, there's definitely no gen_snapshot places where null is passed? Hopefully it's all covered by tests.

Maybe longer-term, can we refactor this for different paths for platforms than non-platform artifacts?

cbracken · 2024-04-17T23:57:10Z

there's definitely no gen_snapshot places where null is passed?

There are still cases where a null TargetPlatform is passed to the various getArtifactPath implementations, but none where the desktop artifact path (mac, windows, linux targets) is null. i.e. getting rid of the generic gen_snapshot tool across all platforms still requires a bunch of work, but this will help clean it up for macOS, which is the key thing I'd let to get done prior to getting arm-native builds available for macOS/iOS.

And yes I definitely like the idea of splitting out the lookup of target-specific binaries from generic host-only binaries just to ensure we're not missing anything. Looks like there's an existing issue for this: #80875

cbracken · 2024-04-18T00:01:23Z

required TargetPlatform platform

I should note that while I was banging this patch up up, I did give that a shot out of curiosity, but there were ~105 errors/warnings that popped up (a ton in tests, in particular), so that's a separate, much larger chunk of work.

vashworth

LGTM

…ter#146958)

flutter/flutter@fb110b9...98685a0 2024-04-19 [email protected] Replace CocoaPods deprecated `exists?` with `exist?` (flutter/flutter#147056) 2024-04-19 [email protected] Roll Flutter Engine from 4ed7a2d6aed9 to 7fafbbf17c02 (2 revisions) (flutter/flutter#147046) 2024-04-19 [email protected] Roll Flutter Engine from 25d773ea90c4 to 4ed7a2d6aed9 (3 revisions) (flutter/flutter#147038) 2024-04-19 [email protected] Update link branches to `main` (continued) (flutter/flutter#146985) 2024-04-19 [email protected] Roll Flutter Engine from b6234dd1984e to 25d773ea90c4 (1 revision) (flutter/flutter#147035) 2024-04-19 [email protected] Roll Flutter Engine from ff471881e90a to b6234dd1984e (2 revisions) (flutter/flutter#147029) 2024-04-19 [email protected] Roll Flutter Engine from 442d14a7e840 to ff471881e90a (1 revision) (flutter/flutter#147025) 2024-04-19 [email protected] Roll Flutter Engine from 6e4a15d31769 to 442d14a7e840 (2 revisions) (flutter/flutter#147023) 2024-04-19 [email protected] Opt out users from GA3 if opted out of GA4 (flutter/flutter#146453) 2024-04-19 [email protected] Roll Flutter Engine from 61bf47d129bb to 6e4a15d31769 (1 revision) (flutter/flutter#147022) 2024-04-18 [email protected] Roll Flutter Engine from 13a6ce419664 to 61bf47d129bb (4 revisions) (flutter/flutter#147017) 2024-04-18 [email protected] Add a breadcrumb for the pub autoroller (flutter/flutter#146786) 2024-04-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add generic type for result in PopScope (#139164)" (flutter/flutter#147015) 2024-04-18 [email protected] Redundant message fix (flutter/flutter#143978) 2024-04-18 [email protected] Clean up flutterRoot (flutter/flutter#147010) 2024-04-18 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.1 to 4.3.2 (flutter/flutter#147011) 2024-04-18 [email protected] Add Swift Package Manager as new opt-in feature for iOS and macOS (flutter/flutter#146256) 2024-04-18 [email protected] Refactor framework coverage tests (flutter/flutter#146210) 2024-04-18 [email protected] Roll Flutter Engine from 46ff024bff10 to 13a6ce419664 (3 revisions) (flutter/flutter#147006) 2024-04-18 [email protected] Changing the renderer on the web target should change its build key. (flutter/flutter#147003) 2024-04-18 [email protected] Roll Flutter Engine from 6abfa565a9f9 to 46ff024bff10 (1 revision) (flutter/flutter#147005) 2024-04-18 [email protected] Add generic type for result in PopScope (flutter/flutter#139164) 2024-04-18 [email protected] Roll Flutter Engine from aa6f7411c219 to 6abfa565a9f9 (1 revision) (flutter/flutter#147002) 2024-04-18 [email protected] [tools] Make SnapshotType.platform non-nullable (flutter/flutter#146958) 2024-04-18 [email protected] Roll Flutter Engine from b8e802515b5a to aa6f7411c219 (1 revision) (flutter/flutter#146996) 2024-04-18 [email protected] Roll Flutter Engine from 2c3e9c8bfce6 to b8e802515b5a (2 revisions) (flutter/flutter#146993) 2024-04-18 [email protected] Roll Packages from d39830e to 0e3809d (9 revisions) (flutter/flutter#146992) 2024-04-18 [email protected] Fix memory leaks in navigation rail (flutter/flutter#146988) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages 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 Packages: 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

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.

flutter/flutter@fb110b9...98685a0 2024-04-19 [email protected] Replace CocoaPods deprecated `exists?` with `exist?` (flutter/flutter#147056) 2024-04-19 [email protected] Roll Flutter Engine from 4ed7a2d6aed9 to 7fafbbf17c02 (2 revisions) (flutter/flutter#147046) 2024-04-19 [email protected] Roll Flutter Engine from 25d773ea90c4 to 4ed7a2d6aed9 (3 revisions) (flutter/flutter#147038) 2024-04-19 [email protected] Update link branches to `main` (continued) (flutter/flutter#146985) 2024-04-19 [email protected] Roll Flutter Engine from b6234dd1984e to 25d773ea90c4 (1 revision) (flutter/flutter#147035) 2024-04-19 [email protected] Roll Flutter Engine from ff471881e90a to b6234dd1984e (2 revisions) (flutter/flutter#147029) 2024-04-19 [email protected] Roll Flutter Engine from 442d14a7e840 to ff471881e90a (1 revision) (flutter/flutter#147025) 2024-04-19 [email protected] Roll Flutter Engine from 6e4a15d31769 to 442d14a7e840 (2 revisions) (flutter/flutter#147023) 2024-04-19 [email protected] Opt out users from GA3 if opted out of GA4 (flutter/flutter#146453) 2024-04-19 [email protected] Roll Flutter Engine from 61bf47d129bb to 6e4a15d31769 (1 revision) (flutter/flutter#147022) 2024-04-18 [email protected] Roll Flutter Engine from 13a6ce419664 to 61bf47d129bb (4 revisions) (flutter/flutter#147017) 2024-04-18 [email protected] Add a breadcrumb for the pub autoroller (flutter/flutter#146786) 2024-04-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add generic type for result in PopScope (#139164)" (flutter/flutter#147015) 2024-04-18 [email protected] Redundant message fix (flutter/flutter#143978) 2024-04-18 [email protected] Clean up flutterRoot (flutter/flutter#147010) 2024-04-18 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.1 to 4.3.2 (flutter/flutter#147011) 2024-04-18 [email protected] Add Swift Package Manager as new opt-in feature for iOS and macOS (flutter/flutter#146256) 2024-04-18 [email protected] Refactor framework coverage tests (flutter/flutter#146210) 2024-04-18 [email protected] Roll Flutter Engine from 46ff024bff10 to 13a6ce419664 (3 revisions) (flutter/flutter#147006) 2024-04-18 [email protected] Changing the renderer on the web target should change its build key. (flutter/flutter#147003) 2024-04-18 [email protected] Roll Flutter Engine from 6abfa565a9f9 to 46ff024bff10 (1 revision) (flutter/flutter#147005) 2024-04-18 [email protected] Add generic type for result in PopScope (flutter/flutter#139164) 2024-04-18 [email protected] Roll Flutter Engine from aa6f7411c219 to 6abfa565a9f9 (1 revision) (flutter/flutter#147002) 2024-04-18 [email protected] [tools] Make SnapshotType.platform non-nullable (flutter/flutter#146958) 2024-04-18 [email protected] Roll Flutter Engine from b8e802515b5a to aa6f7411c219 (1 revision) (flutter/flutter#146996) 2024-04-18 [email protected] Roll Flutter Engine from 2c3e9c8bfce6 to b8e802515b5a (2 revisions) (flutter/flutter#146993) 2024-04-18 [email protected] Roll Packages from d39830e to 0e3809d (9 revisions) (flutter/flutter#146992) 2024-04-18 [email protected] Fix memory leaks in navigation rail (flutter/flutter#146988) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages 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 Packages: 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

cbracken requested review from jmagman and vashworth April 17, 2024 23:14

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 17, 2024

cbracken changed the title ~~[tool] Make SnapshotType.platform non-nullable~~ [tools] Make SnapshotType.platform non-nullable Apr 17, 2024

jmagman reviewed Apr 17, 2024

View reviewed changes

vashworth approved these changes Apr 18, 2024

View reviewed changes

jmagman approved these changes Apr 18, 2024

View reviewed changes

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 18, 2024

auto-submit bot merged commit c219bf7 into flutter:master Apr 18, 2024

cbracken deleted the non-nullable-gensnapshot-platform branch April 18, 2024 16:37

engine-flutter-autoroll mentioned this pull request Apr 18, 2024

Manual roll Flutter from 3882afbfa997 to c219bf73fce4 (61 revisions) flutter/packages#6566

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 18, 2024

c219bf7 [tools] Make SnapshotType.platform non-nullable (flutter/flut…

f946dcb

…ter#146958)

engine-flutter-autoroll mentioned this pull request Apr 19, 2024

Roll Flutter from fb110b98da15 to 4f7b7c93ffdc (27 revisions) flutter/packages#6576

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 19, 2024

c219bf7 [tools] Make SnapshotType.platform non-nullable (flutter/flut…

9b41ca6

…ter#146958)

engine-flutter-autoroll mentioned this pull request Apr 19, 2024

Roll Flutter from fb110b98da15 to 98685a099fdd (28 revisions) flutter/packages#6577

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 19, 2024

c219bf7 [tools] Make SnapshotType.platform non-nullable (flutter/flut…

41c97b0

…ter#146958)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[tools] Make SnapshotType.platform non-nullable#146958
auto-submit[bot] merged 1 commit intoflutter:masterfrom
cbracken:non-nullable-gensnapshot-platform

cbracken commented Apr 17, 2024 •

edited

Loading

Uh oh!

jmagman left a comment

Uh oh!

cbracken commented Apr 17, 2024 •

edited

Loading

Uh oh!

cbracken commented Apr 18, 2024 •

edited

Loading

Uh oh!

vashworth left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cbracken commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vashworth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cbracken commented Apr 17, 2024 •

edited

Loading

cbracken commented Apr 17, 2024 •

edited

Loading

cbracken commented Apr 18, 2024 •

edited

Loading