mirror of
https://github.com/flutter/flutter
synced 2024-10-05 15:59:49 +00:00
[tools] Make SnapshotType.platform non-nullable (#146958)
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: https://github.com/flutter/engine/pull/4948 This was later remediated over the course of several patches, including: * https://github.com/flutter/engine/pull/10430 * https://github.com/flutter/engine/pull/22818 * https://github.com/flutter/flutter/pull/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: 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, 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: https://github.com/flutter/flutter/issues/101138 Umbrella issue: https://github.com/flutter/flutter/issues/103386 Umbrella issue: https://github.com/flutter/flutter/issues/69157 No new tests since the behaviour is enforced by the compiler.
This commit is contained in:
parent
b25f90995a
commit
c219bf73fc
|
@ -578,7 +578,7 @@ class CachedArtifacts implements Artifacts {
|
||||||
case TargetPlatform.linux_arm64:
|
case TargetPlatform.linux_arm64:
|
||||||
case TargetPlatform.windows_x64:
|
case TargetPlatform.windows_x64:
|
||||||
case TargetPlatform.windows_arm64:
|
case TargetPlatform.windows_arm64:
|
||||||
return _getDesktopArtifactPath(artifact, platform, mode);
|
return _getDesktopArtifactPath(artifact, platform!, mode);
|
||||||
case TargetPlatform.fuchsia_arm64:
|
case TargetPlatform.fuchsia_arm64:
|
||||||
case TargetPlatform.fuchsia_x64:
|
case TargetPlatform.fuchsia_x64:
|
||||||
return _getFuchsiaArtifactPath(artifact, platform!, mode!);
|
return _getFuchsiaArtifactPath(artifact, platform!, mode!);
|
||||||
|
@ -594,18 +594,18 @@ class CachedArtifacts implements Artifacts {
|
||||||
return _fileSystem.path.basename(_getEngineArtifactsPath(platform, mode)!);
|
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
|
// When platform is null, a generic host platform artifact is being requested
|
||||||
// and not the gen_snapshot for darwin as a target platform.
|
// 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)!;
|
final String engineDir = _getEngineArtifactsPath(platform, mode)!;
|
||||||
return _fileSystem.path.join(engineDir, _artifactToFileName(artifact, _platform));
|
return _fileSystem.path.join(engineDir, _artifactToFileName(artifact, _platform));
|
||||||
}
|
}
|
||||||
if (platform != null && artifact == Artifact.flutterMacOSFramework) {
|
if (artifact == Artifact.flutterMacOSFramework) {
|
||||||
final String engineDir = _getEngineArtifactsPath(platform, mode)!;
|
final String engineDir = _getEngineArtifactsPath(platform, mode)!;
|
||||||
return _getMacOSEngineArtifactPath(engineDir, _fileSystem, _platform);
|
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) {
|
String _getAndroidArtifactPath(Artifact artifact, TargetPlatform platform, BuildMode mode) {
|
||||||
|
|
|
@ -16,7 +16,7 @@ import 'process.dart';
|
||||||
class SnapshotType {
|
class SnapshotType {
|
||||||
SnapshotType(this.platform, this.mode);
|
SnapshotType(this.platform, this.mode);
|
||||||
|
|
||||||
final TargetPlatform? platform;
|
final TargetPlatform platform;
|
||||||
final BuildMode mode;
|
final BuildMode mode;
|
||||||
|
|
||||||
@override
|
@override
|
||||||
|
|
|
@ -49,12 +49,6 @@ const List<String> kDefaultClang = <String>[
|
||||||
];
|
];
|
||||||
|
|
||||||
void main() {
|
void main() {
|
||||||
group('SnapshotType', () {
|
|
||||||
test('does not throw, if target platform is null', () {
|
|
||||||
expect(() => SnapshotType(null, BuildMode.release), returnsNormally);
|
|
||||||
});
|
|
||||||
});
|
|
||||||
|
|
||||||
group('GenSnapshot', () {
|
group('GenSnapshot', () {
|
||||||
late GenSnapshot genSnapshot;
|
late GenSnapshot genSnapshot;
|
||||||
late Artifacts artifacts;
|
late Artifacts artifacts;
|
||||||
|
|
Loading…
Reference in a new issue