[flutter_hook_config] Add the flutter_hook_config package#11701
Conversation
| - any-glob-to-any-file: | ||
| - packages/file_selector/**/* | ||
|
|
||
| 'p: flutter_hook_config': |
There was a problem hiding this comment.
Note: label needs to be created
|
Note for shipping this: Also flagging: the top-level |
8af09f1 to
fa73758
Compare
fa73758 to
e4b62e5
Compare
| # | ||
| # Name/Organization <email address> | ||
|
|
||
| Google Inc. |
There was a problem hiding this comment.
Added this following the template of other new packages. I assume this is correct but not 100% sure. I'm CLA signed of course, but I no longer work at Google.
e4b62e5 to
e9f14eb
Compare
e9f14eb to
0264309
Compare
0264309 to
674a6a7
Compare
674a6a7 to
db28bc4
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces the flutter_hook_config package, a package:hooks protocol extension that allows the Flutter SDK to provide engine host-tool paths like impellerc and libtessellator to build and link hooks. The PR includes the package's core configuration logic, a ProtocolExtension implementation, documentation, and unit tests. A review comment suggests improving the assertions in the setupFlutter method to explicitly verify that provided URIs use the 'file' scheme, ensuring more descriptive error messages for developers.
db28bc4 to
097ace0
Compare
| /// | ||
| /// When the SDK is invoked with `--local-engine`, this points at the locally | ||
| /// built `impellerc`, matching the engine the app is being built against. | ||
| Uri get impellerc => _filePath(_impellercKey); |
There was a problem hiding this comment.
Are these two related? Do they change together? If yes they should maybe be nested in an object together.
Also, does every Flutter have impeller? E.g. With third-party forks of Flutter is there ever a possibility there's no impeller? If yes, then that wrapper object should be nullable. And if it's not null both fields are non-nullably provided.
Do we need a some kind of version number for this? e.g. if any of these files change or the underlying formats change, you'd want the hook to be rerun.
There was a problem hiding this comment.
Also, does every Flutter have impeller? With third-party forks of Flutter is there ever a possibility there's no impeller? If yes, then that wrapper object should be nullable. And if it's not null both fields are non-nullably provided.
impellerc is used for both Impeller and Skia. Even if one were to do the heavy work of ripping out Impeller from the engine (since we don't even ship Skia on iOS), features like the Fragment Program API/RuntimeEffect use impellerc to compile shaders regardless of the graphics backend (impellerc transpiles to SkSL for Skia).
The impellerc dependency is baked elsewhere in flutter_tools already.
There was a problem hiding this comment.
Do we need a some kind of version number for this? e.g. if any of these files change or the underlying formats change, you'd want the hook to be rerun.
Hmm yes that's a good point and we should think about this... Since the artifacts are per-engine build, maybe the right thing to do would be to add a config.flutter.engineVersion, so that the cache key gets invalidated whenever the engine artifacts change. I can just grab the engine revision from Cache.engineRevision in flutter_tools.
Does that seem right?
| sdk: ^3.9.0 | ||
|
|
||
| dependencies: | ||
| hooks: ^1.0.0 |
There was a problem hiding this comment.
Side note: I'm publishing a 2.0.0 soon, because we need to add a method to the ProtocolExtension (dart-lang/native#3358).
097ace0 to
bb9071f
Compare
bb9071f to
be4ada9
Compare
A package:hooks protocol extension that lets the Flutter SDK expose engine host-tool paths (impellerc, libtessellator) to build and link hooks in a typed way, replacing the per-package SDK-cache-walking that packages like flutter_gpu_shaders do today and making the --local-engine override propagate to hooks for free. Towards flutter/flutter#186299
be4ada9 to
9d5bfd0
Compare
Towards flutter/flutter#186299
This came out of an exchange with @dcharkes here: flutter/flutter#186300 (comment)
Adds
flutter_hook_config, apackage:hooksprotocol extension owned by Flutter.The intent:
flutter_toolsconstructs aFlutterExtensionand passes it to every build and link hook invocation, populating it with engine host-tool paths resolved throughArtifacts.getHostArtifact(so the--local-engineoverride applies, same as for flutter_tools' own build steps). Build hooks then read those paths through a typed accessor instead of re-deriving them by walking the SDK cache directory, which is what packages likeflutter_gpu_shadersdo today (and which misses--local-engine).What a Flutter build hook looks like with it:
v1 exposes
impellercandlibtessellator; more host artifacts can be added later as null-checked fields. Pure Dart, depends onhooks: ^1.0.0, starts at0.1.0while the shape stabilizes. Theflutter_toolsside lands in a separate flutter/flutter PR and depends on this being published first.Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2