From 265902e30a9002f450cf743b5df96f832cc714be Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Wed, 17 Apr 2024 15:23:22 -0700 Subject: [PATCH] [tool] Make SnapshotType.platform non-nullable 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: https://github.com/flutter/engine/pull/4948 This was later remediated over the course of several patches, including: See: https://github.com/flutter/flutter/pull/37445 See: https://github.com/flutter/engine/pull/10430 See: https://github.com/flutter/engine/pull/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: https://github.com/flutter/flutter/issues/38933 Fixed in: https://github.com/flutter/engine/pull/28345 The test removed in this PR, which ensured that null SnapshotType.platform was supported was introduced in https://github.com/flutter/flutter/pull/11924 as a followup to https://github.com/flutter/flutter/pull/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: https://github.com/flutter/flutter/issues/101138 Issue: https://github.com/flutter/flutter/issues/103386 --- packages/flutter_tools/lib/src/artifacts.dart | 10 +++++----- packages/flutter_tools/lib/src/base/build.dart | 2 +- .../test/general.shard/base/build_test.dart | 6 ------ 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/flutter_tools/lib/src/artifacts.dart b/packages/flutter_tools/lib/src/artifacts.dart index ba3703b86ef60..efb6e9b4d2639 100644 --- a/packages/flutter_tools/lib/src/artifacts.dart +++ b/packages/flutter_tools/lib/src/artifacts.dart @@ -578,7 +578,7 @@ class CachedArtifacts implements Artifacts { case TargetPlatform.linux_arm64: case TargetPlatform.windows_x64: case TargetPlatform.windows_arm64: - return _getDesktopArtifactPath(artifact, platform, mode); + return _getDesktopArtifactPath(artifact, platform!, mode); case TargetPlatform.fuchsia_arm64: case TargetPlatform.fuchsia_x64: return _getFuchsiaArtifactPath(artifact, platform!, mode!); @@ -594,18 +594,18 @@ class CachedArtifacts implements Artifacts { return _fileSystem.path.basename(_getEngineArtifactsPath(platform, mode)!); } - String _getDesktopArtifactPath(Artifact artifact, TargetPlatform? platform, BuildMode? mode) { + String _getDesktopArtifactPath(Artifact artifact, TargetPlatform platform, BuildMode? mode) { // When platform is null, a generic host platform artifact is being requested // and not the gen_snapshot for darwin as a target platform. - if (platform != null && artifact == Artifact.genSnapshot) { + if (artifact == Artifact.genSnapshot) { final String engineDir = _getEngineArtifactsPath(platform, mode)!; return _fileSystem.path.join(engineDir, _artifactToFileName(artifact, _platform)); } - if (platform != null && artifact == Artifact.flutterMacOSFramework) { + if (artifact == Artifact.flutterMacOSFramework) { final String engineDir = _getEngineArtifactsPath(platform, mode)!; return _getMacOSEngineArtifactPath(engineDir, _fileSystem, _platform); } - return _getHostArtifactPath(artifact, platform ?? _currentHostPlatform(_platform, _operatingSystemUtils), mode); + return _getHostArtifactPath(artifact, platform, mode); } String _getAndroidArtifactPath(Artifact artifact, TargetPlatform platform, BuildMode mode) { diff --git a/packages/flutter_tools/lib/src/base/build.dart b/packages/flutter_tools/lib/src/base/build.dart index 60be3c0e1d7da..09e57962c6283 100644 --- a/packages/flutter_tools/lib/src/base/build.dart +++ b/packages/flutter_tools/lib/src/base/build.dart @@ -16,7 +16,7 @@ import 'process.dart'; class SnapshotType { SnapshotType(this.platform, this.mode); - final TargetPlatform? platform; + final TargetPlatform platform; final BuildMode mode; @override diff --git a/packages/flutter_tools/test/general.shard/base/build_test.dart b/packages/flutter_tools/test/general.shard/base/build_test.dart index 20b85f4f0eb2e..f0509e7d02720 100644 --- a/packages/flutter_tools/test/general.shard/base/build_test.dart +++ b/packages/flutter_tools/test/general.shard/base/build_test.dart @@ -49,12 +49,6 @@ const List kDefaultClang = [ ]; void main() { - group('SnapshotType', () { - test('does not throw, if target platform is null', () { - expect(() => SnapshotType(null, BuildMode.release), returnsNormally); - }); - }); - group('GenSnapshot', () { late GenSnapshot genSnapshot; late Artifacts artifacts;