Skip to content

[native_assets_builder] build.dart -> hook/build.dart #1018

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.

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

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented Mar 15, 2024

Notes:

  • Should keep existing toplevel build.dart scripts working by trying both hook/build.dart and build.dart.
  • Does not migrate the examples yet, so that we don't have to do breaking-change is true in the ci.
    • Will do this after rolling into Dart.
  • native_toolchain_c had a this.dartBuildFiles = const ['build.dart'],. Updating that to hook/build.dart would not work for unmigrated scripts, because files listed as dependencies that don't exist will also trigger a rebuild, e.g. it is considered deleting a file. It would also break on any kind of invocation from hook/link.dart. So, I've removed the default value for now. It's not pretty though. Ideas @mosuem?

Copy link

PR Health

Breaking changes ✔️

Details
Package Change Current Version New Version Needed Version Looking good?
native_assets_cli None 0.5.0-wip 0.5.0-wip 0.5.0-wip ✔️

Changelog Entry ✔️

Details
Package Changed Files

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

Coverage ⚠️

Details
File Coverage
pkgs/native_assets_cli/example/local_asset/build.dart 💔 Not covered
pkgs/native_assets_cli/example/native_add_library/build.dart 💔 Not covered
pkgs/native_assets_cli/example/use_dart_api/build.dart 💔 Not covered
pkgs/native_assets_cli/lib/src/api/build.dart 💚 100 %

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

License Headers ✔️

Details
// Copyright (c) 2024, 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/ffi/example/main.dart
pkgs/ffigen/example/libclang-example/generated_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/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/example/swift/swift_api_bindings.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_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_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_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_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_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_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.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/pdfbox_plugin/lib/pdfbox_plugin.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/PDDocument.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/_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/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.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/tool/command_runner.dart

Package publish validation ✔️

Details
Package Version Status
package:ffi 2.1.2 already published at pub.dev
package:ffigen 12.0.0-wip WIP (no publish necessary)
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.5.0-wip WIP (no publish necessary)

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

@mosuem
Copy link
Member

mosuem commented Mar 15, 2024

Updating that to hook/build.dart would not work for unmigrated scripts.

I don't understand what the problem is. A rebuild would be triggered once? That doesn't sound too bad?

@dcharkes
Copy link
Collaborator Author

dcharkes commented Mar 15, 2024

Updating that to hook/build.dart would not work for unmigrated scripts.

I don't understand what the problem is. A rebuild would be triggered once? That doesn't sound too bad?

dart run has the old native_assets_builder baked in. So it won't check hook/build.dart from the examples. This will break the dart --enable-experiment=native-assets run ... steps on the CI until we roll this PR actually into the Dart SDK.

- run: dart --enable-experiment=native-assets test
working-directory: pkgs/${{ matrix.package }}/example/native_add_app/
if: ${{ matrix.package == 'native_assets_cli' && matrix.sdk == 'dev' && !matrix.breaking-change }}
- run: dart --enable-experiment=native-assets run
working-directory: pkgs/${{ matrix.package }}/example/native_add_app/
if: ${{ matrix.package == 'native_assets_cli' && matrix.sdk == 'dev' && !matrix.breaking-change }}
- run: dart --enable-experiment=native-assets build bin/native_add_app.dart
working-directory: pkgs/${{ matrix.package }}/example/native_add_app/
if: ${{ matrix.package == 'native_assets_cli' && matrix.sdk == 'dev' && !matrix.breaking-change }}
- run: ./native_add_app.exe
working-directory: pkgs/${{ matrix.package }}/example/native_add_app/bin/native_add_app/
if: ${{ matrix.package == 'native_assets_cli' && matrix.sdk == 'dev' && !matrix.breaking-change }}
- run: dart --enable-experiment=native-assets test
working-directory: pkgs/${{ matrix.package }}/example/use_dart_api/
if: ${{ matrix.package == 'native_assets_cli' && matrix.sdk == 'dev' && !matrix.breaking-change }}

@coveralls
Copy link

coveralls commented Mar 15, 2024

Coverage Status

coverage: 92.104% (-0.007%) from 92.111%
when pulling a4d305a on hook-build-dot-dart
into 7e1e2b7 on main.

@auto-submit auto-submit bot merged commit ba431cb into main Mar 19, 2024
@auto-submit auto-submit bot deleted the hook-build-dot-dart branch March 19, 2024 10:30
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Mar 19, 2024
dart-lang/native#1018
dart-lang/native#1019

Change-Id: I0371ebbbba872bf90514f0f2e255957f6a89c8c5
Cq-Include-Trybots: luci.dart.try:pkg-linux-debug-try,pkg-linux-release-arm64-try,pkg-linux-release-try,pkg-mac-release-arm64-try,pkg-mac-release-try,pkg-win-release-arm64-try,pkg-win-release-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357605
Reviewed-by: Moritz Sümmermann <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
HosseinYousefi pushed a commit that referenced this pull request Mar 21, 2024
* #823

Notes:

* Should keep existing toplevel `build.dart` scripts working by trying both `hook/build.dart` and `build.dart`.
* Does not migrate the examples yet, so that we don't have to do `breaking-change` is true in the ci.
   * Will do this after rolling into Dart.
* `native_toolchain_c` had a `this.dartBuildFiles = const ['build.dart'],`. Updating that to `hook/build.dart` would not work for unmigrated scripts, because files listed as dependencies that don't exist will also trigger a rebuild, e.g. it is considered deleting a file. It would also break on any kind of invocation from `hook/link.dart`. So, I've removed the default value for now. It's not pretty though. Ideas @mosuem?
dcharkes added a commit to flutter/flutter that referenced this pull request Mar 25, 2024
Roll of a bunch of breaking changes from the native_assets_builder and
native_assets_cli upstream. Most notably:

* dart-lang/native#946
* dart-lang/native#1018
* dart-lang/native#1019

This PR also updates the template in `flutter create
--template=package_ffi` to use the rewritten API.

This PR does not change any functionality in Flutter.

For reference, the same roll in the Dart SDK:

* https://dart-review.googlesource.com/c/sdk/+/357605
* https://dart-review.googlesource.com/c/sdk/+/357623

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants