Don't embed unreferenced assets#179251
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request changes the logic for embedding native assets in Xcode builds. Instead of copying all assets, it now reads the native_assets.json file to determine which assets are actually referenced by the build, and only copies those. This prevents bundling of unreferenced assets, for example from a previous simulator build when creating a release build. The changes look good and address the issue described. I've added a couple of suggestions to improve the robustness of the JSON and path parsing logic.
| final nativeAssetsSpec = json.decode(nativeAssetsJson.readAsStringSync()) as Map; | ||
| for (final Object? perPlatform | ||
| in (nativeAssetsSpec['native-assets'] as Map<String, Object?>).values) { | ||
| for (final Object? asset in (perPlatform! as Map<String, Object?>).values) { |
There was a problem hiding this comment.
The JSON parsing here could be made more robust. The current implementation uses several unsafe casts (as Map, as Map<String, Object?>) and a null assertion (!) which could lead to runtime errors if native_assets.json has an unexpected structure or is empty/malformed. Consider using type checks and safer access patterns to make this code more resilient against such issues.
There was a problem hiding this comment.
It would be better to have a reusable format, I've filed dart-lang/native#2860. For now you could consider simply copy-pasting the validate method from pkg/vm/lib/native_assets/validator.dart from the Dart SDK.
For the error messages we should probably follow: // "error:" prefix makes this show up as an Xcode compilation error..
There was a problem hiding this comment.
FWIW I don't think we can import any packages into xcode_backend.dart, so even if there was a Dart package it wouldn't help for this.
But to be honest, I don't really see the value of doing a complete validation on that file. It's generated via hooks_runner in the main build, and we can reasonably expect it to be well-formed. There are plenty of places in xcode_backend.dart relying on the main build emitting files at specific paths that would crash on a mismatch. I don't see why this specifically should be checked more defensively.
I can copy and adapt the validator if you think that improves the logic here. But to me, xcode_backend.dart reads like it's specifically written to do as little as possible and doesn't import any libraries outside of the Dart SDK. Copying the entire validator (which would be around 30% of that file when added) just to be able to generate better error messages when we're unable to extract absolute framework paths just doesn't sound that helpful to me.
There was a problem hiding this comment.
I mostly agree, however, I think the whole thing should be wrapped in a try catch so that if something does go wrong unexpectedly, we have a clearer indication of why.
try {
// Parse json file
} on Exception catch (e, stackTrace) {
echo(e.toString());
echo(stackTrace.toString());
echoXcodeError('Failed to embed native assets: $e');
exit(-1);
}
jmagman
left a comment
There was a problem hiding this comment.
I think you're operating at the wrong layer here. last_build_id is created and used in service of the build system fingerprinting system, but you're hijacking that fingerprinting one layer too high.
Read about it in the long docs around here:
flutter/packages/flutter_tools/lib/src/build_system/build_system.dart
Lines 60 to 78 in d18ab0e
I suspect some native asset files (native_assets.json ?) need to be added to the inputs and/or outputs of the build system.
Example:
flutter/packages/flutter_tools/lib/src/build_system/targets/ios.dart
Lines 157 to 161 in d18ab0e
cc @dcharkes
Nice catch @simolus3! (FYI. I'm currently OOO, so I will only have time to review this in 2 weeks when I'm back.) |
dcharkes
left a comment
There was a problem hiding this comment.
Thanks @simolus3!
This still needs an integration test.
This could be done in packages/flutter_tools/test/integration.shard/isolated/native_assets_test.dart, with running a flutter run on the simulator with a native asset in a dev dependency. And subsequently running flutter build in release mode. However, that test already runs quite long.
Can we check whether the files not referenced get copied in packages/flutter_tools/test/general.shard/xcode_backend_test.dart or packages/flutter_tools/test/integration.shard/xcode_backend_test.dart instead?
LGTM once covered with test.
| final nativeAssetsSpec = json.decode(nativeAssetsJson.readAsStringSync()) as Map; | ||
| for (final Object? perPlatform | ||
| in (nativeAssetsSpec['native-assets'] as Map<String, Object?>).values) { | ||
| for (final Object? asset in (perPlatform! as Map<String, Object?>).values) { |
There was a problem hiding this comment.
It would be better to have a reusable format, I've filed dart-lang/native#2860. For now you could consider simply copy-pasting the validate method from pkg/vm/lib/native_assets/validator.dart from the Dart SDK.
For the error messages we should probably follow: // "error:" prefix makes this show up as an Xcode compilation error..
d78ee30 to
c3dca22
Compare
I've adapted existing tests in Since the existing integration tests pass (edit: whops, actually they don't), that kind of serves as a validation that we are able to parse real |
dcharkes
left a comment
There was a problem hiding this comment.
I've adapted existing tests in
packages/flutter_tools/test/general.shard/xcode_backend_test.dartto ensure we're not copying additional frameworks for macOS or iOS builds.Since the existing integration tests pas (edit: whops, actually they don't), that kind of serves as a validation that we are able to parse real
NativeAssetsManifest.jsonfiles correctly (since frameworks would not get copied otherwise).
SGTM 👍
| final nativeAssetsSpec = json.decode(nativeAssetsJson.readAsStringSync()) as Map; | ||
| for (final Object? perPlatform | ||
| in (nativeAssetsSpec['native-assets'] as Map<String, Object?>).values) { | ||
| for (final Object? asset in (perPlatform! as Map<String, Object?>).values) { |
c3dca22 to
42ce061
Compare
|
Friendly ping @jmagman |
I'll dismiss my review, over to Victoria to review.
|
I dismissed by block, but I'd like @vashworth to review this when she gets back. |
| final nativeAssetsSpec = json.decode(nativeAssetsJson.readAsStringSync()) as Map; | ||
| for (final Object? perPlatform | ||
| in (nativeAssetsSpec['native-assets'] as Map<String, Object?>).values) { | ||
| for (final Object? asset in (perPlatform! as Map<String, Object?>).values) { |
There was a problem hiding this comment.
I mostly agree, however, I think the whole thing should be wrapped in a try catch so that if something does go wrong unexpectedly, we have a clearer indication of why.
try {
// Parse json file
} on Exception catch (e, stackTrace) {
echo(e.toString());
echo(stackTrace.toString());
echoXcodeError('Failed to embed native assets: $e');
exit(-1);
}42ce061 to
cb66fc7
Compare
| echo(e.toString()); | ||
| echo(stackTrace.toString()); | ||
| echoXcodeError('Failed to embed native assets: $e'); | ||
| exitApp(1); |
There was a problem hiding this comment.
Sorry, I misread your original suggestion.
I've used exitApp because it calls exit in the real script but throws an exception that can be caught in TestContext. I'm using that for a test verifying that we exit with that message in case of a corrupt native asset manifest.
I'm still using exitApp, but I've replaced the exit code of 1 with -1 as per your original suggestion. Let me know if there's something I'm missing about exitApp being inappropriate here.
Roll Flutter from 13b2b91 to 01d37bc (74 revisions) flutter/flutter@13b2b91...01d37bc 2026-01-07 [email protected] Replace Hybrid Composition wiki page with dev-facing content (flutter/flutter#180642) 2026-01-07 [email protected] Improve code quality in `FlutterActivityTest.java` (flutter/flutter#180585) 2026-01-07 [email protected] Roll Dart SDK to 3.11.0-296.1.beta (flutter/flutter#180633) 2026-01-07 [email protected] Bump target Windows version to 10 (flutter/flutter#180624) 2026-01-07 [email protected] Run hook_user_defines and link_hook integration tests on CI (flutter/flutter#180622) 2026-01-07 [email protected] Fix division by zero in RenderTable intrinsic size methods (flutter/flutter#178217) 2026-01-07 [email protected] Remove more `requires 24` anotations (flutter/flutter#180116) 2026-01-07 [email protected] Roll Skia from 3971dbb85d45 to c5359a4d720e (1 revision) (flutter/flutter#180631) 2026-01-07 [email protected] Fix TabBar.image does not render at initialIndex for the first time (flutter/flutter#179616) 2026-01-07 [email protected] Roll Skia from 1e7ad625f6f7 to 3971dbb85d45 (3 revisions) (flutter/flutter#180627) 2026-01-07 [email protected] Unpin DDS (flutter/flutter#180571) 2026-01-07 [email protected] Fix DropdownMenuEntry.style not resolved when entry is highlighted (flutter/flutter#178873) 2026-01-07 [email protected] Remove unnecessary `@RequiresApi24` annotations from FlutterFragment methods (flutter/flutter#180117) 2026-01-07 [email protected] Roll Skia from 7fc63228056b to 1e7ad625f6f7 (1 revision) (flutter/flutter#180616) 2026-01-07 [email protected] Roll Fuchsia Linux SDK from QfR2ZFZ5kGTD3raO3... to dTvN_JVSCfGFRasvH... (flutter/flutter#180612) 2026-01-07 [email protected] [ Widget Preview ] Add support for `dart:ffi` imports (flutter/flutter#180586) 2026-01-07 [email protected] Roll Skia from eec90000a899 to 7fc63228056b (2 revisions) (flutter/flutter#180608) 2026-01-07 [email protected] Add --web-define flag for template variable injection in Flutter web builds (flutter/flutter#175805) 2026-01-07 [email protected] docs(engine): update rbe notes (flutter/flutter#180599) 2026-01-06 [email protected] Roll Skia from b6249496c230 to eec90000a899 (3 revisions) (flutter/flutter#180602) 2026-01-06 [email protected] Forward proxy 404 responses to client (flutter/flutter#179858) 2026-01-06 [email protected] Roll Dart SDK from 40a8c6188f7f to d2e3ce177270 (1 revision) (flutter/flutter#180598) 2026-01-06 [email protected] Restore CLI precedence for web headers and HTTPS over web_dev_config.yaml (flutter/flutter#179639) 2026-01-06 [email protected] Roll Skia from f5d9da13d56d to b6249496c230 (1 revision) (flutter/flutter#180596) 2026-01-06 [email protected] Enable misc leak tracking (flutter/flutter#176992) 2026-01-06 [email protected] [hooks] Don't require NDK for Android targets (flutter/flutter#180594) 2026-01-06 [email protected] [skia] Disable legacy non-const SkData APIs (flutter/flutter#179684) 2026-01-06 [email protected] Fix/ios share context menu (flutter/flutter#176199) 2026-01-06 [email protected] Roll Skia from b970aeffa66f to f5d9da13d56d (5 revisions) (flutter/flutter#180588) 2026-01-06 [email protected] Manually bump dependencies (flutter/flutter#180509) 2026-01-06 [email protected] Roll Dart SDK from 8150be8a0e48 to 40a8c6188f7f (2 revisions) (flutter/flutter#180582) 2026-01-06 [email protected] Roll Packages from 12eb764 to d3f860d (2 revisions) (flutter/flutter#180581) 2026-01-06 [email protected] Roll Fuchsia Test Scripts from MHF-UAfO6sVKqSEYk... to nR2ESa1Gd8yPcWo06... (flutter/flutter#180578) 2026-01-06 [email protected] Add tooltip support to PlatformMenuItem and PlatformMenu. (flutter/flutter#180069) 2026-01-06 [email protected] Add DropdownMenuFormField.errorBuilder (flutter/flutter#179345) 2026-01-06 [email protected] Roll Skia from 1845397e11ba to b970aeffa66f (2 revisions) (flutter/flutter#180566) 2026-01-06 [email protected] Don't embed unreferenced assets (flutter/flutter#179251) 2026-01-06 [email protected] Improve documentation about ValueNotifier's behavior (flutter/flutter#179870) 2026-01-06 [email protected] Roll Skia from 904ba00331ca to 1845397e11ba (5 revisions) (flutter/flutter#180558) 2026-01-06 [email protected] Roll Dart SDK from 2fb9ad834c4d to 8150be8a0e48 (1 revision) (flutter/flutter#180557) 2026-01-06 [email protected] Roll Skia from 98c042dde68c to 904ba00331ca (3 revisions) (flutter/flutter#180550) 2026-01-06 [email protected] Roll Dart SDK from ba9f7f790966 to 2fb9ad834c4d (2 revisions) (flutter/flutter#180548) 2026-01-06 [email protected] Roll Fuchsia Linux SDK from ubBGcRaAKWKihQ4ac... to QfR2ZFZ5kGTD3raO3... (flutter/flutter#180547) 2026-01-06 [email protected] Make sure that a DraggableScrollableSheet doesn't crash in 0x0 enviro… (flutter/flutter#180433) 2026-01-06 [email protected] Make sure that a ColorFiltered doesn't crash 0x0 environment (flutter/flutter#180307) 2026-01-06 [email protected] Make sure that a FadeInImage doesn't crash in 0x0 environment (flutter/flutter#180495) ...
Roll Flutter from 13b2b91 to 01d37bc (74 revisions) flutter/flutter@13b2b91...01d37bc 2026-01-07 [email protected] Replace Hybrid Composition wiki page with dev-facing content (flutter/flutter#180642) 2026-01-07 [email protected] Improve code quality in `FlutterActivityTest.java` (flutter/flutter#180585) 2026-01-07 [email protected] Roll Dart SDK to 3.11.0-296.1.beta (flutter/flutter#180633) 2026-01-07 [email protected] Bump target Windows version to 10 (flutter/flutter#180624) 2026-01-07 [email protected] Run hook_user_defines and link_hook integration tests on CI (flutter/flutter#180622) 2026-01-07 [email protected] Fix division by zero in RenderTable intrinsic size methods (flutter/flutter#178217) 2026-01-07 [email protected] Remove more `requires 24` anotations (flutter/flutter#180116) 2026-01-07 [email protected] Roll Skia from 3971dbb85d45 to c5359a4d720e (1 revision) (flutter/flutter#180631) 2026-01-07 [email protected] Fix TabBar.image does not render at initialIndex for the first time (flutter/flutter#179616) 2026-01-07 [email protected] Roll Skia from 1e7ad625f6f7 to 3971dbb85d45 (3 revisions) (flutter/flutter#180627) 2026-01-07 [email protected] Unpin DDS (flutter/flutter#180571) 2026-01-07 [email protected] Fix DropdownMenuEntry.style not resolved when entry is highlighted (flutter/flutter#178873) 2026-01-07 [email protected] Remove unnecessary `@RequiresApi24` annotations from FlutterFragment methods (flutter/flutter#180117) 2026-01-07 [email protected] Roll Skia from 7fc63228056b to 1e7ad625f6f7 (1 revision) (flutter/flutter#180616) 2026-01-07 [email protected] Roll Fuchsia Linux SDK from QfR2ZFZ5kGTD3raO3... to dTvN_JVSCfGFRasvH... (flutter/flutter#180612) 2026-01-07 [email protected] [ Widget Preview ] Add support for `dart:ffi` imports (flutter/flutter#180586) 2026-01-07 [email protected] Roll Skia from eec90000a899 to 7fc63228056b (2 revisions) (flutter/flutter#180608) 2026-01-07 [email protected] Add --web-define flag for template variable injection in Flutter web builds (flutter/flutter#175805) 2026-01-07 [email protected] docs(engine): update rbe notes (flutter/flutter#180599) 2026-01-06 [email protected] Roll Skia from b6249496c230 to eec90000a899 (3 revisions) (flutter/flutter#180602) 2026-01-06 [email protected] Forward proxy 404 responses to client (flutter/flutter#179858) 2026-01-06 [email protected] Roll Dart SDK from 40a8c6188f7f to d2e3ce177270 (1 revision) (flutter/flutter#180598) 2026-01-06 [email protected] Restore CLI precedence for web headers and HTTPS over web_dev_config.yaml (flutter/flutter#179639) 2026-01-06 [email protected] Roll Skia from f5d9da13d56d to b6249496c230 (1 revision) (flutter/flutter#180596) 2026-01-06 [email protected] Enable misc leak tracking (flutter/flutter#176992) 2026-01-06 [email protected] [hooks] Don't require NDK for Android targets (flutter/flutter#180594) 2026-01-06 [email protected] [skia] Disable legacy non-const SkData APIs (flutter/flutter#179684) 2026-01-06 [email protected] Fix/ios share context menu (flutter/flutter#176199) 2026-01-06 [email protected] Roll Skia from b970aeffa66f to f5d9da13d56d (5 revisions) (flutter/flutter#180588) 2026-01-06 [email protected] Manually bump dependencies (flutter/flutter#180509) 2026-01-06 [email protected] Roll Dart SDK from 8150be8a0e48 to 40a8c6188f7f (2 revisions) (flutter/flutter#180582) 2026-01-06 [email protected] Roll Packages from 12eb764 to d3f860d (2 revisions) (flutter/flutter#180581) 2026-01-06 [email protected] Roll Fuchsia Test Scripts from MHF-UAfO6sVKqSEYk... to nR2ESa1Gd8yPcWo06... (flutter/flutter#180578) 2026-01-06 [email protected] Add tooltip support to PlatformMenuItem and PlatformMenu. (flutter/flutter#180069) 2026-01-06 [email protected] Add DropdownMenuFormField.errorBuilder (flutter/flutter#179345) 2026-01-06 [email protected] Roll Skia from 1845397e11ba to b970aeffa66f (2 revisions) (flutter/flutter#180566) 2026-01-06 [email protected] Don't embed unreferenced assets (flutter/flutter#179251) 2026-01-06 [email protected] Improve documentation about ValueNotifier's behavior (flutter/flutter#179870) 2026-01-06 [email protected] Roll Skia from 904ba00331ca to 1845397e11ba (5 revisions) (flutter/flutter#180558) 2026-01-06 [email protected] Roll Dart SDK from 2fb9ad834c4d to 8150be8a0e48 (1 revision) (flutter/flutter#180557) 2026-01-06 [email protected] Roll Skia from 98c042dde68c to 904ba00331ca (3 revisions) (flutter/flutter#180550) 2026-01-06 [email protected] Roll Dart SDK from ba9f7f790966 to 2fb9ad834c4d (2 revisions) (flutter/flutter#180548) 2026-01-06 [email protected] Roll Fuchsia Linux SDK from ubBGcRaAKWKihQ4ac... to QfR2ZFZ5kGTD3raO3... (flutter/flutter#180547) 2026-01-06 [email protected] Make sure that a DraggableScrollableSheet doesn't crash in 0x0 enviro… (flutter/flutter#180433) 2026-01-06 [email protected] Make sure that a ColorFiltered doesn't crash 0x0 environment (flutter/flutter#180307) 2026-01-06 [email protected] Make sure that a FadeInImage doesn't crash in 0x0 environment (flutter/flutter#180495) ...
When a Flutter app depends on a package using hooks to add code assets, those get built to
build/native_assets/$platform, where$platformis something likeiosormacos. Crucially, there's no difference between simulator or release builds here, all native assets for a platform end up in that directory.To embed those frameworks with the app, the "sign and embed" stage of an XCode build invokes
xcode_backend.dart, which then copies all frameworks frombuild/native_assets/$targetPlatforminto$build/Runner.app/Frameworks. This is a problem when a developer runs a simulator build followed by a release build without clearing the build folder in between, since both assets would be inbuild/native_assets/iosat that point.This fixes the issue by:
native_assets.jsonfile emitted by the main build.This still needs an integration test.
Closes #178602.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.