Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ffigen] fix syntax for leaf ffiNative functions #861

Merged
merged 5 commits into from
Dec 15, 2023
Merged

[ffigen] fix syntax for leaf ffiNative functions #861

merged 5 commits into from
Dec 15, 2023

Conversation

miDeb
Copy link
Contributor

@miDeb miDeb commented Dec 13, 2023

I noticed that for leaf functions the Native annotation was being generated like this: @ffi.Native<ffi.Int Function(tjhandle , ffi.Int , ffi.Int )>(symbol: 'tj3Set'isLeaf:true), with a missing comma between the symbol name and isLeaf: true.

This PR adds the comma back. I also modified a test to also check the generation with ffiNatives enabled.

miDeb added 2 commits December 14, 2023 00:01
@miDeb
[ffigen] fix syntax for leaf ffiNative functions
473746a 
Fixes a missing comma in front of isLeaf: true, which is only needed for ffiNatives.
@miDeb
test ffiNative function binding generation
43de373
@miDeb
Copy link
Contributor Author

miDeb commented Dec 13, 2023

It looks like running test/regen.dart doesn't update the expected generated bindings (which I only got working after adding -I/usr/lib/clang/16/include to a bunch of config files)? In the end I patched test_utils to write the expected file.

@dcharkes
Copy link
Collaborator

Thanks @miDeb!

It looks like running test/regen.dart doesn't update the expected generated bindings

It might be that this script doesn't cover all tests. Also, that script doesn't seem to be exercised on the CI. So maybe it only used to work for the devs who used that script. I have filed #862 to track this.

dcharkes
dcharkes approved these changes Dec 14, 2023
View reviewed changes
Copy link
Collaborator

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dcharkes dcharkes mentioned this pull request Dec 14, 2023
Merged
auto-submit bot pushed a commit that referenced this pull request Dec 14, 2023
@dcharkes
[infra] Skip coverage in health check (#863)
01f3e06 
We already have coveralls check coverage for the various packages.

The coverage check is blocking various PRs on this repo:

- #861
- #858
@dcharkes
Copy link
Collaborator

@miDeb Could you try merging the last commit from master to see if the health check passes now?

@github-actions GitHub Actions
Copy link

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

PR Health

Package publish validation ✔️

Details
Package Version Status
package:jni 0.8.0-wip WIP (no publish necessary)
package:jnigen 0.8.0-wip WIP (no publish necessary)
package:native_assets_cli 0.3.3-wip WIP (no publish necessary)
package:ffigen 11.0.0-wip WIP (no publish necessary)
package:native_toolchain_c 0.3.3-wip WIP (no publish necessary)
package:native_assets_builder 0.3.1-wip WIP (no publish necessary)

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

License Headers ✔️

Details 
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/test/jackson_core_test/third_party/c_based/dart_bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/test/jackson_core_test/third_party/dart_only/dart_bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/_init.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/example/objective_c/avf_audio_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/ffinative/lib/generated_bindings.dart
pkgs/ffigen/example/swift/swift_api_bindings.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/lib/src/code_generator/utils.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
tools/delete_pubspec_overrides.dart

Changelog Entry ✔️

Details
Package Changed Files

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

Breaking changes ✔️

Details
Package Change Current Version New Version Needed Version Looking good?
jni None 0.8.0-wip 0.8.0-wip 0.8.0-wip ✔️
jnigen None 0.8.0-wip 0.8.0-wip 0.8.0-wip ✔️
native_assets_cli None 0.3.3-wip 0.3.3-wip 0.3.3-wip ✔️
ffigen None 11.0.0-wip 11.0.0-wip 11.0.0-wip ✔️
native_toolchain_c None 0.3.3-wip 0.3.3-wip 0.3.3-wip ✔️
native_assets_builder None 0.3.1-wip 0.3.1-wip 0.3.1-wip ✔️

@miDeb
Copy link
Contributor Author

miDeb commented Dec 14, 2023

Other test expectations don't have a license header either, should I just ignore this?

@miDeb
Copy link
Contributor Author

miDeb commented Dec 14, 2023

Regarding the changelog, is this worth mentioning?

@dcharkes
Copy link
Collaborator

This health check is new, and strict apparently. If you can add it to the expect file, I think it's fine to add it.

Yeah it's fine to add it to the change log.

@miDeb
Copy link
Contributor Author

miDeb commented Dec 14, 2023

If you can add it to the expect file, I think it's fine to add it.

I added it to all expected files in the generator test for consistency. To keep the test passing I had to update it as well.

I also added it to the changelog.

@auto-submit auto-submit bot removed the autosubmit label Dec 15, 2023
@auto-submit Auto-submit
Copy link

auto-submit bot commented Dec 15, 2023

auto label is removed for dart-lang/native/861, due to This PR has not met approval requirements for merging. The PR author is not a member of dart-team and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@dcharkes dcharkes merged commit 2b1fbf8 into dart-lang:main Dec 15, 2023
12 checks passed
@dcharkes
Copy link
Collaborator

Thanks @miDeb ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcharkes dcharkes dcharkes approved these changes

Assignees
No one assigned
Labels
package:ffigen
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@miDeb @dcharkes