[native_assets_builder] `build.dart` -> `hook/build.dart` by dcharkes · Pull Request #1018 · dart-lang/native

dcharkes · 2024-03-15T15:34:41Z

[native_assets_builder] Rename build.dart to hook/build.dart #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?

…rolled

github-actions · 2024-03-15T15:36:56Z

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 · 2024-03-15T15:39:53Z

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 · 2024-03-15T15:41:23Z

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.

native/.github/workflows/native.yaml

Lines 105 to 123 in 7e1e2b7

    
           - 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 · 2024-03-15T15:55:48Z

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

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]>

@mosuem

* #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?

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.  [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

dcharkes added 3 commits March 15, 2024 16:20

[native_assets_builder] Invoke hook/build.dart

561e01a

Undo changes in examples to ensure examples work until the change is …

a1f130c

…rolled

Remove default value for dartBuildFiles.

571a542

github-actions Bot added package:hooks package:native_toolchain_c package:hooks_runner labels Mar 15, 2024

dcharkes requested a review from mosuem March 15, 2024 15:36

mosuem approved these changes Mar 19, 2024

View reviewed changes

Comment thread pkgs/native_assets_builder/lib/src/package_layout/package_layout.dart

add todo

a4d305a

dcharkes added the autosubmit label Mar 19, 2024

auto-submit Bot merged commit ba431cb into main Mar 19, 2024

auto-submit Bot deleted the hook-build-dot-dart branch March 19, 2024 10:30

This was referenced Mar 22, 2024

Roll native_assets_builder to 0.5.0 flutter/flutter#143472

Merged

[deps] Bump native assets dependencies flutter/flutter#145612

Merged

Navigation Menu

Repository navigation

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

[native_assets_builder] `build.dart` -> `hook/build.dart`#1018
auto-submit[bot] merged 4 commits into
mainfrom
hook-build-dot-dart

dcharkes commented Mar 15, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Mar 15, 2024

Uh oh!

mosuem commented Mar 15, 2024

Uh oh!

dcharkes commented Mar 15, 2024 •

edited

Loading

Uh oh!

coveralls commented Mar 15, 2024 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dcharkes commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 15, 2024

PR Health

Breaking changes ✔️

Changelog Entry ✔️

Coverage ⚠️

License Headers ✔️

Package publish validation ✔️

Uh oh!

mosuem commented Mar 15, 2024

Uh oh!

dcharkes commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dcharkes commented Mar 15, 2024 •

edited

Loading

dcharkes commented Mar 15, 2024 •

edited

Loading

coveralls commented Mar 15, 2024 •

edited

Loading