Repository navigation
[flutter_tools] Replace KernelAsset with Flutter-owned NativeAssetsMa… - #192800
auto-submit[bot] merged 3 commits into
Conversation
github-actions
Bot
added
tool
…rings The manifest records paths on the target device (e.g. the `@rpath` install name of a framework on iOS/macOS). Representing them as a host `Uri` and round-tripping through `Uri.file`/`toFilePath` rewrote them with host path semantics, turning `@rpath/Foo.framework/Foo` into `@rpath\Foo.framework\Foo` on Windows hosts. That broke both `DarwinAddToAppNativeAssets.parseNativeAssetsManifest` and the XCFramework creation in `build_swift_package.dart`, which splits the parsed path into a framework directory and a binary name. Also restore parsing of `system` paths in `parseNativeAssetsManifest`, which were silently dropped. Adds a round-trip test for `NativeAssetsManifest` that fails on Windows hosts with the previous representation.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors native assets handling by introducing a structured NativeAssetsManifest class and NativeAssetPath hierarchy to manage the native assets manifest, replacing manual JSON manipulation. It also refactors FlutterCodeAsset to track os and architecture independently instead of using a combined Target object, updating platform-specific copying and location resolution logic accordingly. Review feedback suggests throwing a FormatException for malformed manifest JSON instead of silently ignoring entries, and removing redundant wildcard patterns in switch expressions over sealed classes and enums to preserve compile-time exhaustiveness checks.
| factory NativeAssetsManifest.fromJson(Map<String, Object?> json) { | ||
| final Object? nativeAssetsJson = json[_nativeAssetsKey]; | ||
| if (nativeAssetsJson is! Map<String, Object?>) { | ||
| return const NativeAssetsManifest(assets: <String, Map<String, NativeAssetPath>>{}); | ||
| } | ||
| final assets = <String, Map<String, NativeAssetPath>>{}; | ||
| for (final MapEntry<String, Object?>(key: targetString, value: targetAssets) | ||
| in nativeAssetsJson.entries) { | ||
| if (targetAssets is! Map<String, Object?>) { | ||
| continue; | ||
| } | ||
| final targetMap = <String, NativeAssetPath>{}; | ||
| for (final MapEntry<String, Object?>(key: assetId, value: pathInfo) in targetAssets.entries) { | ||
| if (pathInfo is List<Object?>) { | ||
| targetMap[assetId] = NativeAssetPath.fromJson(pathInfo); | ||
| } | ||
| } | ||
| assets[targetString] = targetMap; | ||
| } | ||
| return NativeAssetsManifest(assets: assets); | ||
| } |
There was a problem hiding this comment.
In NativeAssetsManifest.fromJson, if targetAssets is not a Map<String, Object?> or pathInfo is not a List<Object?>, the code currently silently ignores them (using continue or skipping the entry). Silently ignoring malformed entries in a manifest can lead to hard-to-debug runtime issues where assets are missing without any build-time warning. Consider throwing a FormatException to fail fast and provide clear feedback when the manifest JSON is malformed.
factory NativeAssetsManifest.fromJson(Map<String, Object?> json) {
final Object? nativeAssetsJson = json[_nativeAssetsKey];
if (nativeAssetsJson is! Map<String, Object?>) {
return const NativeAssetsManifest(assets: <String, Map<String, NativeAssetPath>>{});
}
final assets = <String, Map<String, NativeAssetPath>>{};
for (final MapEntry<String, Object?>(key: targetString, value: targetAssets)
in nativeAssetsJson.entries) {
if (targetAssets is! Map<String, Object?>) {
throw FormatException('Invalid native assets map for target "$targetString": $targetAssets');
}
final targetMap = <String, NativeAssetPath>{};
for (final MapEntry<String, Object?>(key: assetId, value: pathInfo) in targetAssets.entries) {
if (pathInfo is! List<Object?>) {
throw FormatException('Invalid path info for asset "$assetId": $pathInfo');
}
targetMap[assetId] = NativeAssetPath.fromJson(pathInfo);
}
assets[targetString] = targetMap;
}
return NativeAssetsManifest(assets: assets);
}| return switch (linkMode) { | ||
| DynamicLoadingSystem(:final uri) => FlutterCodeAssetTargetLocation( | ||
| runtimePath: NativeAssetSystemPath.fromFileUri(uri), | ||
| ), | ||
| LookupInExecutable() => const FlutterCodeAssetTargetLocation( | ||
| runtimePath: NativeAssetInExecutable(), | ||
| ), | ||
| LookupInProcess() => const FlutterCodeAssetTargetLocation(runtimePath: NativeAssetInProcess()), | ||
| DynamicLoadingBundled() => bundledLocationCallback(asset), | ||
| _ => throw Exception('Unsupported asset link mode ${linkMode.runtimeType} in asset $asset'), | ||
| }; |
There was a problem hiding this comment.
Since LinkMode is a sealed class, the switch expression is already exhaustive. Including a wildcard _ pattern is redundant and prevents the compiler from checking for exhaustiveness if a new subclass of LinkMode is added in the future. Removing the wildcard pattern allows you to leverage Dart's compile-time exhaustiveness checks fully.
return switch (linkMode) {
DynamicLoadingSystem(:final uri) => FlutterCodeAssetTargetLocation(
runtimePath: NativeAssetSystemPath.fromFileUri(uri),
),
LookupInExecutable() => const FlutterCodeAssetTargetLocation(
runtimePath: NativeAssetInExecutable(),
),
LookupInProcess() => const FlutterCodeAssetTargetLocation(runtimePath: NativeAssetInProcess()),
DynamicLoadingBundled() => bundledLocationCallback(asset),
};There was a problem hiding this comment.
it is sealed, but we don't have all cases! Stop hallucinating!
| return switch (targetOS) { | ||
| OS.windows || OS.linux => _assetTargetLocationsWindowsLinux(codeAssets, absolutePath), | ||
| OS.macOS => assetTargetLocationsMacOS(codeAssets, absolutePath), | ||
| OS.iOS => assetTargetLocationsIOS(codeAssets), | ||
| OS.android => assetTargetLocationsAndroid(codeAssets), | ||
| _ => throw UnimplementedError('This should be unreachable.'), | ||
| }; |
There was a problem hiding this comment.
Since OS is an enum and all its values are covered, the switch expression is exhaustive. The wildcard _ pattern is redundant and prevents compile-time exhaustiveness checks if a new OS is added to the enum in the future. Consider removing the wildcard pattern.
return switch (targetOS) {
OS.windows || OS.linux => _assetTargetLocationsWindowsLinux(codeAssets, absolutePath),
OS.macOS => assetTargetLocationsMacOS(codeAssets, absolutePath),
OS.iOS => assetTargetLocationsIOS(codeAssets),
OS.android => assetTargetLocationsAndroid(codeAssets),
};
| for (final MapEntry<String, NativeAssetPath>(key: assetId, value: path) | ||
| in targetAssets.entries) { | ||
| if (path | ||
| case NativeAssetAbsolutePath(path: final pathString) || |
There was a problem hiding this comment.
Nit: Once https://dart-review.googlesource.com/c/sdk/+/548220 lands, these will likely require types to be specified.
| final Map<String, Map<String, NativeAssetPath>> assets; | ||
|
|
||
| Map<String, Object> toJson() => <String, Object>{ | ||
| _formatVersionKey: const <int>[1, 0, 0], |
There was a problem hiding this comment.
Nit: should this be in a constant somewhere for discoverability?
dcharkes
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
2 participants
Since #159322, Flutter no longer embeds native assets into dill files. Instead,
flutter_toolsserializes anative_assets.jsonmanifest that is parsed by the C++ engine. However, the format was still using the same toJson as for the dill files, making the serialization and deserialization asymmetric.Moreover, because of this asymmetry, a lot of classes were imported from
hooks_runneronly to construct objects and then to destruct them again and construct them in a different way for the Flutter internal treatment of the data.This PR removes the use of all of those classes, and adds the json serialization.
The PR is almost code size neutral: +426 -376, but the added code is much simpler to understand (data classes and a toJson) while the removed code was ugly shuffling things across classes defined in different packages.
Pre-launch Checklist
///).