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.

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

Ensure the NativeAssetsBuildRunner.link() produces a BuildResult with all assets #1633

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

mkustermann
Copy link
Member

Before this change a bundling tool would

  • call buildResult = nativeAssetsBuildRunner.build(...)
  • call `linkResult = nativeAssetsBuildRunner.link(buildResult: buildResult, ...)
  • get all assets via allAssets = [...buildResult.assets, ...linkResult.assets]

=> This PR makes the link() command produce a LinkResult containing
all assets of the final application.

=> One could view this as the assets from build() without a linker to go to
a default linker that just emits all of it's inputs.

=> This allows the link() step to also perform the application-wide
validation steps (not just over all linker outputs but across build &
link outputs)

Bundling tools now only have to deal with

  • buildResult in JIT mode (where linking is not enabled)
  • linkResult in AOT mode (where linking is enabled)

Instead of dealing with both buildResult and linkResult in AOT mode.

(In some sense one actually would prefer a NativeAssetsBuilder.build and
NativeAssetsBuilder.buildAndLink instead of the current API that exposes the
intermediary result from build(linkingEnabled: true) just to be explicitly
passed on to link later on)

… all assets

Before this change a bundling tool would

  * call `buildResult = nativeAssetsBuildRunner.build(...)`
  * call `linkResult = nativeAssetsBuildRunner.link(buildResult: buildResult, ...)
  * get all assets via `allAssets = [...buildResult.assets, ...linkResult.assets]`

=> This PR makes the `link()` command produce a `LinkResult` containing
   all assets of the final application.

=> One could view this as the assets from `build()` without a linker to go to
   a default linker that just emits all of it's inputs.

=> This allows the `link()` step to also perform the application-wide
   validation steps (not just over all linker outputs but across build &
   link outputs)

Bundling tools now only have to deal with
  * `buildResult` in JIT mode (where linking is not enabled)
  * `linkResult` in AOT mode (where linking is enabled)

Instead of dealing with both `buildResult` and `linkResult` in AOT mode.
Copy link

github-actions bot commented Oct 7, 2024

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

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

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// 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/ffigen/example/libclang-example/generated_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/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_enum_int_mimic_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/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/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/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart
Package publish validation ✔️
Package Version Status
package:ffi 2.1.3 already published at pub.dev
package:ffigen 15.0.0-wip WIP (no publish necessary)
package:jni 0.12.0 already published at pub.dev
package:jnigen 0.12.0 already published at pub.dev
package:native_assets_cli 0.9.0-wip WIP (no publish necessary)
package:objective_c 2.1.0-wip WIP (no publish necessary)
package:swift2objc 0.0.1-wip WIP (no publish necessary)
package:swiftgen 0.0.1-wip WIP (no publish necessary)

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

@mkustermann
Copy link
Member Author

Will update the tests & changelog

@dcharkes
Copy link
Collaborator

dcharkes commented Oct 7, 2024

(In some sense one actually would prefer a NativeAssetsBuilder.build and
NativeAssetsBuilder.buildAndLink instead of the current API that exposes the
intermediary result from build(linkingEnabled: true) just to be explicitly
passed on to link later on)

Now that we don't actually run the build step before kernel compilation of the Dart code in any of the embedders (after I made kernel concatenation work for both JIT/AOT), we could actually do that. 👍

@mkustermann
Copy link
Member Author

Now that we don't actually run the build step before kernel compilation of the Dart code in any of the embedders (after I made kernel concatenation work for both JIT/AOT), we could actually do that. 👍

Yes, we could make that changes. Though if we ever allowed hook/build.dart to output dart code that is used to run/build the dart application, then we'd need the separation.

For now I'll just keep like this to not get too much off-trail (of making the system more flexible).

@mkustermann
Copy link
Member Author

Thanks, @dcharkes !

@mkustermann mkustermann merged commit 6595118 into main Oct 7, 2024
31 checks passed
@mkustermann mkustermann deleted the make-link-result-contain-all-assets branch October 7, 2024 12:40
@coveralls
Copy link

Coverage Status

coverage: 90.47% (+0.001%) from 90.469%
when pulling f056c8f on make-link-result-contain-all-assets
into cbfe69e on main.

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.

3 participants