refactor: migrate ffigen configs to Dart and upgrade to ^20.0.0#324
refactor: migrate ffigen configs to Dart and upgrade to ^20.0.0#324mosuem wants to merge 5 commits into
Conversation
- Upgrade ffigen dependency to ^20.0.0 in pubspec.yaml - Consolidate YAML ffigen configs into tool/ffigen.dart - Update tool/update-bindings.sh to run dart run tool/ffigen.dart - Re-generate native bindings
|
Why not use YAML files? |
https://pub.dev/packages/ffigen#yaml-configuration-reference It's being deprecated :) |
HamdaanAliQuatil
left a comment
HamdaanAliQuatil
left a comment
There was a problem hiding this comment.
Moving the ffigen configuration to Dart makes sense since YAML configuration is being phased out.
I left comments on three things that currently make this harder to audit: typedefs: includeAll is greatly expanding the generated surface, the Flutter demo cleanup is unrelated, and the hooks major upgrade should be separated unless ffigen 20 requires it.
Could we keep this PR focused on the Dart configuration migration, the ffigen upgrade, the update script, and the necessary regenerated bindings? Changes to generated native bindings are much easier to review when the surrounding diff has one clear purpose.
| include: Declarations.includeSet({'webcrypto_get_CBB_size'}), | ||
| ), | ||
| structs: const Structs(dependencies: CompoundDependencies.opaque), | ||
| typedefs: const Typedefs(include: Declarations.includeAll), |
There was a problem hiding this comment.
This is pulling in far more than the YAML config did. The regenerated wrapper binding gains 141 typedefs, while the BoringSSL binding gains hundreds of typedefs and more than 100 unrelated opaque declarations. The set of bound functions itself has not changed.
Can we remove includeAll here and in the second config below, and let ffigen generate only the typedefs required by the selected declarations?
|
|
||
| @override | ||
| _MyAppState createState() => _MyAppState(); | ||
| MyAppState createState() => MyAppState(); |
There was a problem hiding this comment.
Could we leave the demo changes out of this PR? They aren’t needed for the ffigen migration.
If the private return type was causing a lint, the narrower fix is:
State<MyApp> createState() => _MyAppState();
That keeps the state class private. The scrolling, spacing and deprecated-API cleanup would be easier to review separately.
| dependencies: | ||
| ffi: ^2.0.0 | ||
| hooks: ^1.0.0 | ||
| hooks: ^2.0.0 |
There was a problem hiding this comment.
Is the hooks 2.x upgrade required by ffigen 20? This changes a runtime dependency, while the rest of the PR is a code-generation tooling migration.
If it is independent, can we update it separately?
3 participants
Migrated YAML ffigen configurations to Dart configurations in
tool/ffigen.dart, updatedffigento^20.0.0, updatedtool/update-bindings.sh, and re-generated native bindings.