Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dcharkes
Copy link
Contributor

@dcharkes dcharkes commented Nov 20, 2024

This PR introduces a NativeAssetsManifest.json next to the AssetManifest.json and FontManifest.json. This removes the need for embedding the native assets mapping inside the kernel file and will enable decoupling native assets building and bundling from the kernel compilation in flutter tools. This will then allow us to remove dry-run from the build hook protocol.

(It also means all isolate groups will have the same native assets. However, since Flutter does not support Isolate.spawnUri from kernel files anyways, this is not a regression.)

This manifest is parsed eagerly on startup by the engine in a manner similar to how the font manifest is parsed. The manifest contents need to be available in the callback for resolving assets, which does not have access to the engine. Therefore the parsed manifest is NativeAssetsManager stored in the IsolateGroupData. The engine passes it in on isolate group creation, and the FFI callbacks access it from the isolate group data.

Issue:

Related PRs:

Follow up work:

  • This PR does not yet remove the engine callbacks registered via the dart_api that rely on kernel embedding. If we were to do that in this PR, it would require a manual roll of the engine into flutter/flutter with the PR that switches flutter_tools to emit the native assets manifest instead of embedding in kernel, and a manual roll into g3 to switch emitting a manifest instead of embedding in kernel. A TODO is left in the code for those callbacks to be removed.

Testing

Most of this PR cannot be tested in isolation. The code in this PR is heavily exercised in the follow up flutter_tools PR which creates the NativeAssetsManifest.json and removes the embedding of native assets in kernel files.

  • This PR adds a unit test for parsing the JSON manifest.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. 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.

@dcharkes
Copy link
Contributor Author

Thanks for the quick review @jonahwilliams! 🙏

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have at least a simple unit test of the native_assets class.

i.e. show that it can parse the JSON document, return the right strings, print the debug string. Doesn't haven't to actually read from disk or anything.

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dcharkes dcharkes added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2024
@dcharkes
Copy link
Contributor Author

Thanks @jonahwilliams! 🙏

@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 21, 2024

auto label is removed for flutter/engine/56727, due to - The status or check suite Mac mac_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2024
@dcharkes dcharkes added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2024
@auto-submit auto-submit bot merged commit 764b91d into main Nov 21, 2024
36 checks passed
@auto-submit auto-submit bot deleted the native-assets-not-kernel-2 branch November 21, 2024 18:01
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 21, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 21, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 22, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 22, 2024
…159308)

flutter/engine@d1a0806...6f941c9

2024-11-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[Impeller] support GLES 3.0 MSAA without extension. (#56705)"
(flutter/engine#56741)
2024-11-21 [email protected] Roll Dart SDK from
dde57dc75c15 to b36e4d731d67 (1 revision) (flutter/engine#56723)
2024-11-21 [email protected] [native assets] Consume
`NativeAssetsManifest.json` (flutter/engine#56727)
2024-11-21 [email protected] [Impeller] support GLES 3.0 MSAA
without extension. (flutter/engine#56705)
2024-11-21 [email protected] Updated some
impeller benchmark urls (flutter/engine#56721)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure
that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 27, 2024
…embedding (#159322)

This PR introduces a `NativeAssetsManifest.json` next to the
`AssetManifest.bin` and `FontManifest.json`. This removes the need for
embedding the native assets mapping inside the kernel file and enables
decoupling native assets building and bundling from the kernel
compilation in flutter tools. This means `flutter run` no longer does a
dry run of `hook/build.dart` hooks.

(It also means all isolate groups will have the same native assets.
However, since Flutter does not support `Isolate.spawnUri` from kernel
files, this is not a regression.)

Implementation details:

* g3 is still using kernel embedding.
#142016 introduced an argument to
embed a `native_assets.yaml` inside `flutter attach` and `flutter run`
(the outer flutter process), but it is not used in `flutter assemble`
(the inner process when doing `flutter run`). So, those arguments need
to still be respected. However, all other logic related to embedding a
yaml encoding in the kernel file has been removed.
* All dry-run logic has been removed. 🎉 
* The `KernelSnapshot` target no longer depends on the
`InstallCodeAssets` target. Instead, the various OS-specific
"BundleAsset" targets now depend on the `InstallCodeAssets` target. The
`InstallCodeAssets` invokes the build hooks and produces the
`NativeAssetsManifest.json`. The various "BundleAsset" commands
synchronize the `NativeAssetsManifest.json` to the app bundle.
* `InstallCodeAssets` produces a `native_assets.json`, which is renamed
to `NativeAssetsManifest.json` in the various "Bundle" targets. This
means that all unit tests of the "Bundle" targets now need to create
this file. (Similar to how `app.dill` is expected to exist because
`KernelSnapshot` is a dependency of the "Bundle" targets.)
* Because dynamic libraries need to be code signed (at least on iOS and
MacOS), the bundling of the dylibs is _not_ migrated to reuse
`_updateDevFS` (which is used for ordinary assets). Only the 2nd and 3rd
invocation of `flutter assemble` from `xcodebuild` has access to the
code signing identity.

Relevant tests:

* test/integration.shard/isolated/native_assets_test.dart - runs
`flutter run` with native assets including hot restart and hot reload.

TODO:

* Undo engine-roll in this PR after engine has rolled in.

Issue:

* #154425

Related PRs:

* https://dart-review.googlesource.com/c/sdk/+/388161
* flutter/engine#56727

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
…6727)

This PR introduces a `NativeAssetsManifest.json` next to the `AssetManifest.json` and `FontManifest.json`. This removes the need for embedding the native assets mapping inside the kernel file and will enable decoupling native assets building and bundling from the kernel compilation in flutter tools. This will then allow us to remove dry-run from the build hook protocol.

(It also means all isolate groups will have the same native assets. However, since Flutter does not support `Isolate.spawnUri` from kernel files anyways, this is not a regression.)

This manifest is parsed eagerly on startup by the engine in a manner similar to how the font manifest is parsed. The manifest contents need to be available in the callback for resolving assets, which does not have access to the engine. Therefore the parsed manifest is `NativeAssetsManager` stored in the `IsolateGroupData`. The engine passes it in on isolate group creation, and the FFI callbacks access it from the isolate group data.

Issue:

* flutter#154425

Related PRs:

* https://dart-review.googlesource.com/c/sdk/+/388161

Follow up work:

* This PR does not yet remove the engine callbacks registered via the dart_api that rely on kernel embedding. If we were to do that in this PR, it would require a manual roll of the engine into flutter/flutter with the PR that switches flutter_tools to emit the native assets manifest instead of embedding in kernel, and a manual roll into g3 to switch emitting a manifest instead of embedding in kernel. A TODO is left in the code for those callbacks to be removed.

## Testing

Most of this PR cannot be tested in isolation. The code in this PR is heavily exercised in the follow up flutter_tools PR which creates the `NativeAssetsManifest.json` and removes the embedding of native assets in kernel files.

* This PR adds a unit test for parsing the JSON manifest.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants