Skip to content

Commit

Permalink
[tool] Make SnapshotType.platform non-nullable
Browse files Browse the repository at this point in the history
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
  • Loading branch information
cbracken committed Apr 17, 2024
1 parent 500ed0b commit 265902e
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 12 deletions.
10 changes: 5 additions & 5 deletions packages/flutter_tools/lib/src/artifacts.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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!);
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/lib/src/base/build.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import 'process.dart';
class SnapshotType {
SnapshotType(this.platform, this.mode);

final TargetPlatform? platform;
final TargetPlatform platform;
final BuildMode mode;

@override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,6 @@ const List<String> kDefaultClang = <String>[
];

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;
Expand Down

0 comments on commit 265902e

Please sign in to comment.