Support `@Native` fields and `addressOf` #860

simolus3 · 2023-12-13T14:18:41Z

This adds support for native fields with an approach similar to what we're doing for functions.
This also enables taking the address of functions and fields by generating a call to Native.addressOf.

I'm opening this as a WIP because it depends on SDK changes that haven't landed yet, afterwards the CI may have to be changed to test ffigen with beta or dev builds (or we'll just wait a little longer).

I wasn't sure where to put tests for this - it looks like the only tests using @Native are related to ObjectiveC at the moment. Do we add new tests to code_generator_tests/expected_bindings?

Closes #852.

dcharkes · 2023-12-13T14:24:21Z

You're on a roll @simolus3 ! 🔥

I'm opening this as a WIP because it depends on SDK changes that haven't landed yet, afterwards the CI may have to be changed to test ffigen with beta or dev builds (or we'll just wait a little longer).

As long as the FFIgen version itself is a WIP or dev release we can track an SDK constraint that is a dev/beta.

When FFIgen was it's own repo, we had a stable branch for making releases against the stable branch. https://github.com/dart-lang/ffigen/wiki/Branches I haven't thought about a process yet in the mono-repo world.

cc @liamappelbe looking at the FFIgen changelog, we have Fix objc_msgSend being used on arm64 platforms where it's not available. Maybe we should release a 11.0 stable, and then land this afterwards as 12.0.0-wip

github-actions · 2023-12-14T23:08:48Z

PR Health

Breaking changes ✔️

Details

Package	Change	Current Version	New Version	Needed Version	Looking good?
ffigen	Breaking	11.0.0	12.0.0-dev	12.0.0	✔️

Changelog Entry ✔️

Details

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️

Details

File	Coverage
pkgs/ffigen/example/ffinative/lib/generated_bindings.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/compound.dart	💔 98 % ⬇️ 0 %
pkgs/ffigen/lib/src/code_generator/func.dart	💚 80 % ⬆️ 1 %
pkgs/ffigen/lib/src/code_generator/global.dart	💚 98 % ⬆️ 1 %
pkgs/ffigen/lib/src/code_generator/imports.dart	💚 81 % ⬆️ 0 %
pkgs/ffigen/lib/src/code_generator/library.dart	💚 91 % ⬆️ 2 %
pkgs/ffigen/lib/src/code_generator/pointer.dart	💚 47 % ⬆️ 16 %
pkgs/ffigen/lib/src/code_generator/utils.dart	💚 95 % ⬆️ 2 %
pkgs/ffigen/lib/src/code_generator/writer.dart	💚 93 % ⬆️ 1 %
pkgs/ffigen/lib/src/header_parser/sub_parsers/var_parser.dart	💚 95 % ⬆️ 10 %
pkgs/ffigen/lib/src/header_parser/type_extractor/extractor.dart	💚 78 % ⬆️ 0 %
pkgs/ffigen/lib/src/header_parser/utils.dart	💚 88 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check

The license workflow has encountered an exception and did not complete.

Package publish validation ✔️

Details

Package	Version	Status
package:ffigen	12.0.0-dev	ready to publish
package:jni	0.8.0-wip	WIP (no publish necessary)
package:jnigen	0.8.0-wip	WIP (no publish necessary)
package:native_assets_builder	0.3.1-wip	WIP (no publish necessary)
package:native_assets_cli	0.3.3-wip	WIP (no publish necessary)
package:native_toolchain_c	0.3.3	already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

dcharkes · 2023-12-21T15:49:49Z

Everything should be landed in 3.3.0-252.0.dev, so you should be able to update SDK constraints and the GitHub CI setup to that once it is available on https://dart.dev/get-dart/archive.

dcharkes · 2023-12-29T09:18:47Z

Everything should be landed in 3.3.0-252.0.dev, so you should be able to update SDK constraints and the GitHub CI setup to that once it is available on https://dart.dev/get-dart/archive.

3.3.0-256.0.dev is available.

simolus3 · 2023-12-29T17:11:17Z

It seems to work with that version, thanks. Could you apply the skip-breaking-check label to make the health check happy? 11.0.0 hasn't been released yet, and it seems weird to go two major versions up just because the pending release has two breaking changes.

coveralls · 2023-12-29T17:38:04Z

coverage: 92.743% (+0.05%) from 92.693%
when pulling cb88681 on simolus3:native-fields
into 985ddd0 on dart-lang:main.

dcharkes

LGTM! Thanks @simolus3 ❤️

pkgs/ffigen/example/ffinative/headers/example.h

dcharkes · 2024-01-02T09:24:49Z

pkgs/ffigen/example/ffinative/lib/generated_bindings.dart


 /// Adds 2 integers.
 @ffi.Native<ffi.Int Function(ffi.Int, ffi.Int)>(
-    symbol: 'sum', assetId: 'package:ffinative_example/generated_bindings.dart')
+    assetId: 'package:ffinative_example/generated_bindings.dart')


Nice, reusing the symbol.

Side note which does not have to be addressed in this CL: Should we consider generating

@DefaultAsset('package:ffinative_example/generated_bindings.dart') library;

instead of an assetId in every @Native?

I think we should do that, especially considering that there can only be a single assetId in the config file at the moment. But even if we want different asset ids based on symbol patterns, we can always specify the id for those symbols only.

Please file an issue for us to do this later, or add it in this PR.

I've removed the assetId to generate a @DefaultAsset annotation instead.

pkgs/ffigen/example/ffinative/pubspec.yaml

pkgs/ffigen/lib/src/code_generator/func.dart

pkgs/ffigen/lib/src/code_generator/global.dart

pkgs/ffigen/test/code_generator_tests/expected_bindings/_expected_global_native_bindings.dart

pkgs/ffigen/test/code_generator_tests/code_generator_test.dart

pkgs/ffigen/example/ffinative/lib/generated_bindings.dart

dcharkes · 2024-01-02T09:50:36Z

FYI @mannprerak2, happy to get your feedback on this PR as well!

And, happy new year to you all! 🎉

simolus3 · 2024-01-02T18:43:14Z

Happy new year to all of you from me as well!

If we do a v11 release before this, the skip-breaking-check should probably be removed and I'll add a v12 entry after #872. skip-license-check might come in handy for the clang bindings which don't have the regular Dart license though.

mannprerak2

LGTM with minor nit 👍🏻

P.S Happy new year 🎉

mannprerak2 · 2024-01-02T18:48:20Z

pkgs/ffigen/lib/src/code_generator/writer.dart

-    return 'late final ${w._symbolAddressVariableName} = ${w._symbolAddressClassName}(this);';
+    final className = w._symbolAddressClassName;
+    final fieldName = w._symbolAddressVariableName;
+
+    if (hasNonNativeAddress) {
+      return 'late final $className $fieldName = $className(this);';
+    } else {
+      return 'const $className $fieldName = $className();';
+    }


Can we skip the type specifier here? I feel it's better to avoid changes in generated code unless neccessary.

pkgs/ffigen/test/code_generator_tests/expected_bindings/_expected_global_native_bindings.dart

pkgs/ffigen/test/code_generator_tests/code_generator_test.dart

Release 11 stable with the latest fixes on Dart 3.2 before starting to depend on a Dart 3.3-dev version. Should be merged after: * #869 And before: * #860

dcharkes · 2024-01-03T10:14:58Z

If we do a v11 release before this, the skip-breaking-check should probably be removed and I'll add a v12 entry after #872. skip-license-check might come in handy for the clang bindings which don't have the regular Dart license though.

I've released v11 and removed the label.

@mannprerak2 Thanks! I would have missed the array thing!

dcharkes · 2024-01-03T12:24:36Z

Resolving dependencies...
The current Dart SDK version is 3.3.0-174.3.beta.

Because ffigen requires SDK version >=3.3.0-252.0.dev, version solving failed.

Try setting the SDK in https://github.com/dart-lang/native/blob/main/.github/workflows/health.yaml

https://github.com/dart-lang/ecosystem/tree/main/pkgs/firehose#publishing-from-a-specific-version-of-the-sdk

github-actions · 2024-01-03T12:41:51Z

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:ffigen	12.0.0-dev	ready to publish	`ffigen-v12.0.0-dev`
package:jni	0.8.0-wip	WIP (no publish necessary)
package:jnigen	0.8.0-wip	WIP (no publish necessary)
package:native_assets_builder	0.3.1-wip	WIP (no publish necessary)
package:native_assets_cli	0.3.3-wip	WIP (no publish necessary)
package:native_toolchain_c	0.3.3	already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

dcharkes · 2024-01-03T15:39:08Z

.github/workflows/ffigen.yml

@@ -28,7 +28,8 @@ jobs:
    strategy:
      fail-fast: false
      matrix:
-        sdk: [3.2.0]
+        sdk: [dev]
+#        sdk: [3.2.0]


Maybe update the commented stuff to 3.3.0, and file an issue to update this when 3.3.0 beta is released.

Done - #876.

dcharkes · 2024-01-03T15:40:00Z

.github/workflows/health.yaml

@@ -10,5 +10,6 @@ jobs:
      coverage_web: false
      checks: "version,changelog,license,do-not-submit,breaking"
      use-flutter: true
+      sdk: dev


Does it work if you put an actual version number here instead of "dev"?

I think the health check is running as intended, only the publish validation workflow needed the dev version. The health check fails due to a missing license on a golden file which never had a license header (and on the libclang bindings file, which probably shouldn't have a Dart license header).

Right, that should not have a header file.

Though I'm still seeing Dart versio issues on the CI?

Done, found 52 files without license headers Collecting packages without changed changelogs: Done, found 5 packages. Collecting files without license headers in those packages: Done, found 0 packages with a need for a changelog. Look for changes in pkgs/ffigen with base ../../../base_repo/pkgs/ffigen Error: Exception: The current Dart SDK version is 3.2.3. Because ffigen requires SDK version >=3.3.0-252.0.dev <4.0.0, version solving failed. You can try the following suggestion to make the pubspec resolve: * Try using the Dart SDK version: 3.3.0. See https://dart.dev/get-dart. [...] Changed 55 dependencies! 1 package has newer versions incompatible with dependency constraints. Try `dart pub outdated` for more information. Preparing . Copying sources from . Preparing package dependencies for local package . Resolving dependencies... The current Dart SDK version is 3.2.3. Because ffigen requires SDK version >=3.3.0-252.0.dev <4.0.0, version solving failed. You can try the following suggestion to make the pubspec resolve: * Try using the Dart SDK version: 3.3.0. See https://dart.dev/get-dart. Unhandled exception: PathNotFoundException: Cannot open file, path = 'pkgs/ffigen/report.json' (OS Error: No such file or directory, errno = 2) #0 _File.throwIfError (dart:io/file_impl.dart:675:7) #1 _File.openSync (dart:io/file_impl.dart:490:5) #2 _File.readAsBytesSync (dart:io/file_impl.dart:574:18) #3 _File.readAsStringSync (dart:io/file_impl.dart:624:18) #4 Health.breakingCheck (package:firehose/src/health/health.dart:137:41) #5 Health.healthCheck (package:firehose/src/health/health.dart:82:30) <asynchronous suspension> #6 main (file:///opt/hostedtoolcache/flutter/stable-3.16.5-x64/.pub-cache/hosted/pub.dev/firehose-0.4.2/bin/health.dart:32:3) <asynchronous suspension> Error: Process completed with exit code 255.

https://github.com/dart-lang/native/actions/runs/7402310627/job/20139919918?pr=860

@mosuem Any guidance on how to make the health check happy?

We have files in third_party/ folders which require a different license than the Dart one

In various places the health check fails to resolve the Dart SDK version.

(I'm almost inclined to ignore or disable the health check, as it seems too strict for this repo.)

I will check it out. Thanks for dogfooding the health workflow :)

dcharkes

@simolus3 The CI issues have been resolved. So we can land this after the last small nits are addressed. 🚀

.github/workflows/ffigen.yml

dcharkes · 2024-01-08T13:37:50Z

pkgs/ffigen/example/ffinative/lib/generated_bindings.dart


 /// Adds 2 integers.
 @ffi.Native<ffi.Int Function(ffi.Int, ffi.Int)>(
-    symbol: 'sum', assetId: 'package:ffinative_example/generated_bindings.dart')
+    assetId: 'package:ffinative_example/generated_bindings.dart')


Please file an issue for us to do this later, or add it in this PR.

dcharkes · 2024-01-08T13:43:25Z

pkgs/ffigen/test/code_generator_tests/expected_bindings/_expected_unions_bindings.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file


Shouldn't only the new files have 2024? We don't need to update existing headers.

The problem is that the header for these goldens in defined in code_generator_test.dart and shared between all of them. So whenever we add a new test, the health check will complain about a newly added file with an old year in its header and we have to update all of them. Since this PR has skipped those checks I have reverted these changes and instead given the new tests a 2023 header as well.

Maybe we should make it conditional later, but lets leave that for another PR and lets get this in!

dcharkes · 2024-01-09T08:09:21Z

Thanks @simolus3! 🚀

…, shelf, tools, webdev Revisions updated by `dart tools/rev_sdk_deps.dart`. async (https://github.com/dart-lang/async/compare/9924570..e83d054): e83d054 2024-01-08 Futuristic Goo Fix typo (dart-lang/async#262) ecosystem (https://github.com/dart-lang/ecosystem/compare/dc44e82..1e2785d): 1e2785d 2024-01-09 Jacob MacDonald fix saving of comment ids to disk (dart-lang/ecosystem#221) 244a28d 2024-01-09 Moritz Update publish.yaml (dart-lang/ecosystem#217) bab9833 2024-01-09 Moritz Fix health commenting (dart-lang/ecosystem#219) f87e6f4 2024-01-08 Moritz Update health workflow (dart-lang/ecosystem#216) a58c7d8 2024-01-03 Moritz Fix `labeler.yml` (dart-lang/ecosystem#214) leak_tracker (https://github.com/dart-lang/leak_tracker/compare/3d4c0d6..4a5b077): 4a5b077 2024-01-09 Polina Cherkasova Enhance scripting. (dart-lang/leak_tracker#204) e7094f4 2024-01-08 Polina Cherkasova Ignore test helpers. (dart-lang/leak_tracker#196) 6591934 2024-01-03 Polina Cherkasova Handle deprecation in Flutter. (dart-lang/leak_tracker#203) markdown (https://github.com/dart-lang/markdown/compare/d2e7903..7fdfa55): 7fdfa55 2024-01-01 dependabot[bot] Bump actions/stale from 8.0.0 to 9.0.0 (dart-lang/markdown#571) 5fab3a7 2023-12-19 Alex Li ✨ Introduce AlertBlockSyntax (dart-lang/markdown#570) native (https://github.com/dart-lang/native/compare/0605d9a..14f6da1): 14f6da1d 2024-01-09 Simon Binder Support `@Native` fields and `addressOf` (dart-lang/native#860) protobuf (https://github.com/dart-lang/protobuf/compare/20ec685..a293fb9): a293fb9 2024-01-08 Ömer Sinan Ağacan Handle deprecated options in grpc services and methods, enum types and values, messages (google/protobuf.dart#908) 9a408a7 2024-01-08 Ömer Sinan Ağacan Generate docs of enums and rpc clients, some refactoring (google/protobuf.dart#909) c4fd596 2024-01-06 Ömer Sinan Ağacan Export GeneratedMessageGenericExtensions in generated files (google/protobuf.dart#907) shelf (https://github.com/dart-lang/shelf/compare/733588f..823966f): 823966f 2024-01-03 Moritz Fix `labeler.yml` (dart-lang/shelf#403) tools (https://github.com/dart-lang/tools/compare/2f59ab4..8ffc077): 8ffc077 2024-01-03 Moritz Fix `labeler.yml` (dart-lang/tools#224) webdev (https://github.com/dart-lang/webdev/compare/b2405cb..c08a65c): c08a65c9 2024-01-09 Elliott Brooks Loosen `vm_service` constraints and prepare DWDS for release to 23.1.1 (dart-lang/webdev#2329) 651bdae6 2024-01-08 Derek Xu Make FrontendServerClient start the frontend server from AOT snapshot by default (dart-lang/webdev#2263) 4d1de266 2024-01-03 Elliott Brooks Prepare DWDS for release to version 23.1.0 (dart-lang/webdev#2328) Change-Id: I4d7fd994cc54ac2d72335c3ebf40710f3bd020e6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345366 Commit-Queue: Konstantin Shcheglov <scheglov@google.com> Auto-Submit: Devon Carew <devoncarew@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

simolus3 added 3 commits December 13, 2023 15:05

Implement native fields and addressOf

ddb0a4a

Add changelog entry

99366b8

Fix missing type name

b9d738c

github-actions bot added the package:ffigen label Dec 13, 2023

simolus3 changed the title ~~Support @Native field and addressOf~~ Support @Native fields and addressOf Dec 13, 2023

dcharkes mentioned this pull request Dec 13, 2023

[ffi] @Native for global fixed-size arrays dart-lang/sdk#54337

Closed

Support array fields

01e7752

simolus3 added 2 commits December 21, 2023 15:43

Merge remote-tracking branch 'upstream/main' into native-fields

fac544f

Include asset id if desired

d3e9841

simolus3 added 2 commits December 21, 2023 17:04

Add generation tests

305c357

Fix license headers

5e4b97d

simolus3 marked this pull request as ready for review December 21, 2023 16:11

Use dev sdk for tests

f521df2

github-actions bot added the type-infra A repository infrastructure change or enhancement label Dec 24, 2023

Use self imports

bc19c46

Raise SDK constraints

44be567

Update expectations

d4b1388

dcharkes added the skip-breaking-check label Dec 30, 2023

dcharkes mentioned this pull request Jan 2, 2024

[ffigen] Release 11 Stable #872

Merged

dcharkes approved these changes Jan 2, 2024

View reviewed changes

simolus3 added 2 commits January 2, 2024 19:23

Review feedback

b9a548f

Happy new year dear health check

cb88681

mannprerak2 approved these changes Jan 2, 2024

View reviewed changes

auto-submit bot pushed a commit that referenced this pull request Jan 3, 2024

[ffigen] Release 11 Stable (#872)

0605d9a

Release 11 stable with the latest fixes on Dart 3.2 before starting to depend on a Dart 3.3-dev version. Should be merged after: * #869 And before: * #860

dcharkes removed the skip-breaking-check label Jan 3, 2024

simolus3 added 2 commits January 3, 2024 12:44

Merge remote-tracking branch 'upstream/main' into native-fields

a9a1255

More review feedback

7a858de

dcharkes mentioned this pull request Jan 3, 2024

[firehose] Should use the SDK specified in pubspec for validation dart-lang/ecosystem#215

Open

simolus3 added 4 commits January 3, 2024 13:30

Fix tests

2f96aaa

Use dev SDK

0abde28

Bring back upper SDK bound

4021f28

Remove version from health job

81bce63

dcharkes reviewed Jan 3, 2024

View reviewed changes

Add comments for 3.3 release

127e5b1

simolus3 mentioned this pull request Jan 3, 2024

Revert CI to use a beta/stable Dart version after Dart 3.3 is released #876

Open

dcharkes added the skip-license-check label Jan 4, 2024

Update health.yaml

ba586af

dcharkes approved these changes Jan 8, 2024

View reviewed changes

Revert license header change in test

3c5bb80

dcharkes merged commit 14f6da1 into dart-lang:main Jan 9, 2024
16 checks passed

simolus3 deleted the native-fields branch January 9, 2024 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `@Native` fields and `addressOf` #860

Support `@Native` fields and `addressOf` #860

simolus3 commented Dec 13, 2023

dcharkes commented Dec 13, 2023

github-actions bot commented Dec 14, 2023 •

edited

Loading

dcharkes commented Dec 21, 2023

dcharkes commented Dec 29, 2023

simolus3 commented Dec 29, 2023

coveralls commented Dec 29, 2023 •

edited

Loading

dcharkes left a comment

dcharkes Jan 2, 2024

simolus3 Jan 2, 2024

dcharkes Jan 8, 2024

simolus3 Jan 8, 2024

dcharkes commented Jan 2, 2024

simolus3 commented Jan 2, 2024 •

edited

Loading

mannprerak2 left a comment •

edited

Loading

mannprerak2 Jan 2, 2024

dcharkes commented Jan 3, 2024

dcharkes commented Jan 3, 2024

github-actions bot commented Jan 3, 2024

dcharkes Jan 3, 2024

simolus3 Jan 3, 2024

dcharkes Jan 3, 2024

simolus3 Jan 3, 2024

dcharkes Jan 4, 2024 •

edited

Loading

mosuem Jan 4, 2024

dcharkes left a comment

dcharkes Jan 8, 2024

dcharkes Jan 8, 2024

simolus3 Jan 8, 2024

dcharkes Jan 9, 2024

dcharkes commented Jan 9, 2024

		@@ -1,4 +1,4 @@
		// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
		// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file

Support @Native fields and addressOf #860

Support @Native fields and addressOf #860

Conversation

simolus3 commented Dec 13, 2023

dcharkes commented Dec 13, 2023

github-actions bot commented Dec 14, 2023 • edited Loading

PR Health

Breaking changes ✔️

Changelog Entry ✔️

Coverage ⚠️

Package publish validation ✔️

dcharkes commented Dec 21, 2023

dcharkes commented Dec 29, 2023

simolus3 commented Dec 29, 2023

coveralls commented Dec 29, 2023 • edited Loading

dcharkes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcharkes commented Jan 2, 2024

simolus3 commented Jan 2, 2024 • edited Loading

mannprerak2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcharkes commented Jan 3, 2024

dcharkes commented Jan 3, 2024

github-actions bot commented Jan 3, 2024

Package publishing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcharkes Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcharkes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcharkes commented Jan 9, 2024

Support `@Native` fields and `addressOf` #860

Support `@Native` fields and `addressOf` #860

github-actions bot commented Dec 14, 2023 •

edited

Loading

coveralls commented Dec 29, 2023 •

edited

Loading

simolus3 commented Jan 2, 2024 •

edited

Loading

mannprerak2 left a comment •

edited

Loading

dcharkes Jan 4, 2024 •

edited

Loading