Repository navigation
Conversation
bdero
added
the
tool
…ok_config Appends a FlutterExtension to every build and link hook invocation so hooks can read impellerc and libtessellator paths from the hook config, resolved through the tool's own artifact lookup, including under --local-engine. The extension's engineVersion invalidates the hook cache on SDK upgrades and local engine rebuilds.
![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 introduces the flutter_hook_config dependency and adds support for supplying engine host-tool paths (such as impellerc and libtessellator) to build and link hooks via a new FlutterExtension. It implements createFlutterExtension to resolve these tools to absolute file URIs and derives a stable local engine version based on the tools' paths, sizes, and modification times to manage hook cache invalidation. Unit tests are added to verify this behavior. There are no review comments, and I have no feedback to provide.
2247a94 to
4eda8e3
Compare
| sha256: af801f91e3e433a939551e601c386ee59683d045ae6148896983c75655ec79e5 | ||
| url: "https://pub.dev" | ||
| source: hosted | ||
| version: "0.1.0" |
There was a problem hiding this comment.
Since this is not 1.0. Should passing this config this be gated behind a flutter experiment flag? flutter config --enable-flutter-hooks-config
To make the the package 1.0 we should answer some questions:
- Is the current API future-proof? Or should things be nested (e.g. we have two artifact paths, should the be nested in an artifacts constructor and in a nested json map).
- Is the JSON future-proof? Do we promise to never break the JSON format ever? (If yes, okay. If no, is the package working with nullable fields e.g. it doesn't crash if it's not supplied?)
There was a problem hiding this comment.
Should we just mark the package as 1.0? It's a trivial package, IMO. If we put it behind a feature flag, I can't actually use it.
There was a problem hiding this comment.
I don't see why we'd ever need to break it. Perhaps we would add more later, though. But even that would be exceedingly rare, since the current cadence of adding new executables to the archive is probably once every five years.
There was a problem hiding this comment.
Mmmm on second thought... I could probably use for flutter_gpu_shaders and such. It's already using data assets anyhow (which is working wonderfully for flutter_scene by the way), which is also behind a flag. 🤷♂️
Maybe the flag isn't so bad. But curious what you think.
There was a problem hiding this comment.
A feature flag will allow us to get all the pieces in place and refactor on the go if needed. And then once the work concludes we bump to 1.0 and remove the flag. 👍
| @@ -0,0 +1,64 @@ | |||
| // Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
We should probably also add an integration test in one of the existing tests that run hooks to see that the files are available.
For example in packages/flutter_tools/test/integration.shard/isolated/native_assets_flutter_run_test.dart or flutter_built_test
In the packages/flutter_tools/test/integration.shard/isolated/native_assets_test_utils.dart createTestProject you can modify the hook to throw an error on if the flutter config is not available.
(And then test both with the experiment on and off if we add an experiment flag.)
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
3 participants
Fixes #186299
Passes a
FlutterExtension(frompackage:flutter_hook_config, added in flutter/core-packages#19) to every build and link hook invocation. Hooks that consume engine host tools can now readinput.config.flutter.impellercandinput.config.flutter.libtessellatorinstead of re-implementing artifact discovery, and the paths resolve through the tool's ownArtifactslookup, so--local-enginepropagates to hooks instead of silently using stale cached binaries.The extension also carries an
engineVersion.hooks_runnerkeys the hook cache on the hook config, so hooks re-run when the SDK is upgraded even though the tool paths are unchanged. For a stock SDK this is the engine revision. Local engine builds have no revision (Environment.engineVersionis null by design), so the version is derived from the host tools' paths, sizes, and mtimes, which invalidates the hook cache when a local engine is rebuilt.Supersedes #186300, which passed the same paths as environment variables. The typed extension participates in hook caching and needs no env-var plumbing.
Pre-launch Checklist
///).