Bump ffigen #180507
Bump ffigen #180507
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 updates the ffigen dependency from version 18.1.0 to 20.1.1. This constitutes a major version upgrade. My review includes a comment on pubspec.yaml highlighting the potential risk of breaking changes and the need to ensure any code generated by ffigen is updated, as no such updates are present in this PR.
pubspec.yaml
Outdated
| cli_util: 0.4.2 | ||
| dart_style: 3.1.3 | ||
| ffigen: 18.1.0 | ||
| ffigen: 20.1.1 |
There was a problem hiding this comment.
Bumping ffigen across two major versions (from 18.1.0 to 20.1.1) is a significant change that may introduce breaking changes. Since ffigen is a code generator, this update likely requires regenerating any associated bindings to prevent build failures or runtime errors from stale code. This pull request does not contain any such updated files, which could be a potential issue.
9ed8f3d to
92bcf2a
Compare
|
Typically when you update ffigen you should also regen whatever bindings it generated. From a quick search, it looks like the only places that ffigen is used (aside from the templates, which hard code a different version) is in the link_hook integration test and some Fuchsia zircon bindings. So I think to do this roll properly you'd also have to regen those, otherwise there's not really much point updating the ffigen version. dart run ffigen --config dev/integration_tests/link_hook/ffigen.yaml
dart run ffigen --config engine/src/flutter/shell/platform/fuchsia/dart-pkg/zircon_ffi/pubspec.yamlIt's also a little odd to have ffigen as a dependency rather than a dev_dependency, but it looks like this top level pubspec doesn't have a dev_dependency section, so I guess that's WAI. |
bba452c to
c5253f2
Compare
|
We should have some tests that fail if the generated bindings become outdated (e.g. through bumping the ffigen version). I added one, but I think the problem is now that the bots are not set up with the right header files? I'll have to look into that... (The Fuchsia zircon bindings use an independent ffigen version specified here, that's not affected by this bump). |
0f48abb to
b10caf5
Compare
edb8da4 to
e092676
Compare
e092676 to
1e821db
Compare
|
Since running ffigen on the bots requires some extra work I am splitting this in two: This PR bumps the ffigen version and updates the generated code. #180680 will then update the bots to run ffigen and adds the tests that will verify the generated code stays uptodate. |
flutter/flutter@3134be8...d81cd3e 2026-01-12 47866232+chunhtai@users.noreply.github.com Migrate doc and code to use sendAnnounce (flutter/flutter#180776) 2026-01-12 engine-flutter-autoroll@skia.org Roll Packages from 51fe1d9 to e57e7f4 (3 revisions) (flutter/flutter#180846) 2026-01-12 dixita0607@users.noreply.github.com Fix: Allow dartpad embed clipboard-write permission for "Copy to clipboard" functionality (flutter/flutter#178057) 2026-01-12 engine-flutter-autoroll@skia.org Roll Dart SDK from 87fbfd5381b6 to 42fd9ef68c1a (1 revision) (flutter/flutter#180836) 2026-01-12 engine-flutter-autoroll@skia.org Roll Skia from aefdde600f1e to 487a9943210b (3 revisions) (flutter/flutter#180835) 2026-01-12 goderbauer@google.com Bump ffigen (flutter/flutter#180507) 2026-01-12 engine-flutter-autoroll@skia.org Roll Skia from d42a43daa6cf to aefdde600f1e (1 revision) (flutter/flutter#180829) 2026-01-12 ahmedsameha1@gmail.com Make sure that a LongPressDraggable doesn't crash in 0x0 environment (flutter/flutter#180408) 2026-01-12 ahmedsameha1@gmail.com Make sure that a FlutterLogo doesn't crash in 0x0 environment (flutter/flutter#180617) 2026-01-12 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from T_HOCpDjLgzi1SUMq... to VYeyMPe1lyCtlcl-V... (flutter/flutter#180825) 2026-01-11 engine-flutter-autoroll@skia.org Roll Skia from f39cc645b1dd to d42a43daa6cf (2 revisions) (flutter/flutter#180819) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
It's unclear why it isn't getting picked up by the autoroller, see flutter#180503.
It's unclear why it isn't getting picked up by the autoroller, see #180503.