-
Notifications
You must be signed in to change notification settings - Fork 53
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
[native_assets_cli] Redesign API step 1 #946
Conversation
PR HealthBreaking changes ✔️Details
Changelog Entry ✔️Details
Changes to files need to be accounted for in their respective changelogs. Coverage
|
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/native_assets_cli.dart | 💔 Not covered |
pkgs/native_assets_cli/lib/native_assets_cli_internal.dart | 💔 Not covered |
pkgs/native_assets_cli/lib/src/api/architecture.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/api/asset.dart | 💔 Not covered |
pkgs/native_assets_cli/lib/src/api/build.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/api/build_config.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/api/build_mode.dart | 💔 Not covered |
pkgs/native_assets_cli/lib/src/api/build_output.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/api/c_compiler_config.dart | 💔 Not covered |
pkgs/native_assets_cli/lib/src/api/data_asset.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/api/ios_sdk.dart | 💔 Not covered |
pkgs/native_assets_cli/lib/src/api/link_mode_preference.dart | 💔 Not covered |
pkgs/native_assets_cli/lib/src/api/native_code_asset.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/api/os.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/architecture.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/asset.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/build_config.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/build_mode.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/build_output.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/c_compiler_config.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/data_asset.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/dependencies.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/ios_sdk.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/link_mode_preference.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/metadata.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/native_code_asset.dart | 💚 95 % |
pkgs/native_assets_cli/lib/src/model/os.dart | 💚 100 % |
pkgs/native_assets_cli/lib/src/model/target.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.
@lrhn You suggested adding a @mkustermann This PR (finally) addresses #3 (comment). PTAL. @mosuem PTAL. I'll probably shoot in a meeting to discuss the design as well. |
My original suggestion was to make config & output extensible tree structures, e.g.: ///// Core API (possibly in one package)
// An object operating on a JSON-like data structure.
class Config {
final Map map;
}
// An object operating on a JSON-like data structure.
class Output {
final Map map;
}
// Basic things that every config should have, e.g. what assets the
// `dart build` / `flutter build` wants.
// Avoid using an enum, as we may add more later.
extension on Config {
GeneralConfig get general => GeneralConfig(map['general'] ?? {});
}
extension type GeneralConfig(Map map) {
String outputPath => map['output'];
Set<String> get assetTypes => Set(map['assetTypes']!);
}
///// Possibly in a native assets package:
// Extend the configuration with native library specific settings
extension on Config {
bool get buildNative => general.assetTypes.contains('native');
NativeConfig get native => NativeConfig(map.putIfAbsent('native', () => []));
}
// Provide access to abi / cflags / paths to compiler/linker/...
extension type NativeConfig(Map map) {
Abi get abi => Abi(map['abi']);
}
// Extend the output to support adding native assets.
extension on Output {
NativeAssets get native => NativeAssets(map.putIfAbsent('nativeAssets', () => {}));
}
// Have simple methods that build up the native assets.
extension type NativeAssets(Map map) {
void addSystemInstalledLibrary(String assetId, Abi abi, ...) { ... }
}
///// Possibly in a android assets package:
// Extend the configuration with android specifics.
extension on Config {
bool get buildAndroid => assetTypes.contains('android');
AndroidConfig get android => AndroidConfig(map.putIfAbsent('android', () => []));
}
extension type AndroidConfig(Map map) {
int get apiLevel => Abi(map['apiLevel']);
}
// Extend the output to support adding native assets.
extension on Output {
AndroidAssets get androidAssets => AndroidAssets(map.putIfAbsent('androidAssets', () => {}));
}
// Have simple methods that build up the android-specific assets.
extension type AndroidAssets(Map map) {
void addJarFile(...) { ... }
void addProguardRules(...) { ... }
} That would allow adding more asset types later on (e.g. jar files, images, ...) by
So the minimal one needs is a 3 liner to get started with native assets: main(args) {
final (config, output) = loadConfig(args);
output.native.addSystemInstalledLibrary('<asset-id>', 'libexif.so');
writeOutput(config.outputPath, output.map);
} or main(args) async {
await build(args, (config, output) {
output.native.addSystemInstalledLibrary('<asset-id>', 'libexif.so');
});
} By not having a monolithic main(args) async {
await build(args, (config, output) {
if (config.buildNative) {
CBuilder(config.general, config.native, output.native).buildLibrary('<asset-id>', 'foo.c');
}
if (config.buildAndroid) {
AndroidBuilder(config.general, config.android, output.android).addJar(...);
}
});
} i.e. one can pass the sub-parts of |
Adding the assets in extensions in different packages would mean that we would not share the serializing/deserializing code between the flutter_tools/dartdev and build.dart scripts. And that native_assets_cli does not need to have a minor version increase. This is in the same spirit "Stop letting native_assets_builder depend on native_assets_cli." in #949 but splits However, I think the API becomes harder to discover. One would have to know which helper packages there, add them to the pubspec,
👍 Applied, I've removed Note that that means we're passing in config and output as two objects to most helper methods (CBuilder, AssetDownloader).
Note from @lrhn: He's worried when he sees Thanks for your input so far! I've scheduled a follow up discussion! |
There are multiple differences between the `native_assets.yaml` that is embedded in the kernel file and the `build_output.yaml -> assets`. * Based on the discussions on #955 and #946, it is clear that the `path` for assets should be in `Asset`, not in `AssetPath` for the file-path the asset has after the `build.dart` run. When the embedders (Flutter/Dart) embed the native assets mapping then the `path`s start representing the path on the system where the Dart/Flutter app is running. This should be embedded in the path-type there. * The kernel info does not contain link mode (currently), as static linking is not supported. It's not clear that if we support static linking whether any information should be embedded in the kernel info at all. * The native_assets.yaml for the kernel file is laid out for easy lookup at runtime (keyed on Target). * We want to change the `Asset`s to output OS and Architecture instead of Target. Therefore we should not share the data structure between `Asset`s and `KernelAsset`s (new name for the entries that are embedded via native_assets.yaml in a kernel file.) This means that `dartdev` and `flutter_tools` will have to start converting `Asset`s to `KernelAsset`s when making the `native_assets.yaml` file. Therefore this is a breaking change and will have to be rolled into Dart and flutter/flutter manually.
Thanks @mosuem! I like the new shorter names! Happy to merge this if you are happy with this! |
Manual roll of dart-lang/native#946. Change-Id: I6d4fd28f4174307024526dd73ff29b63e570bfd7 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-win-release-try,pkg-mac-release-try,pkg-win-release-arm64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357623 Reviewed-by: Moritz Sümmermann <[email protected]> Commit-Queue: Daco Harkes <[email protected]>
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
This PR completely redesigns the API of
native_assets_cli
:BuildConfig
now has getters/methods instead of fields.BuildOutput
has add methods.Dependency
andMetadata
classes are hidden, the contents are available in methods.final
classes to ensure the casts succeed.build.dart
writers should not be distracted in the API with methods only useful forpackage:native_assets_builder
. Neither do we want to duplicate the logic betweenpackage:native_assets_cli
andpackage:native_assets_builder
.Asset
is renamed toNativeCodeAsset
.NativeCodeAsset
s haveOS
&Architecture
instead ofTarget
.Architecture
can be omitted in dry runs.DataAsset
s. [native_assets_cli] data assets #154BuildConfig
supplies a list supported asset types. [native_assets_cli] Open ended asset types list #968Target
from the public API. All APIs useArchitecture
andOS
now.PathType
is replaced with aLinkMode
hierarchy. Only system dylibs have a path now.Asset
s have an optionalfile
which designates the file on the file system afterbuild.dart
invocation.BuildConfig
s main constructor is now the one that takes the command-line arguments.Asset.id
is now automatically created frompackage
andname
.This PR also revamps the protocol. However it does so in a non-breaking way by:
build_config.yaml
(only extending). So oldbuild.dart
can parse it from a newer SDK.build_config.yaml
s. So newbuild.dart
can parse it form an older SDK.build_output.yaml
s if the build_config was an old version. So old SDKs can parse from newerbuild.dart
.build_output.yaml
s. So new SDKs can parse from olderbuild.dart
.Suggested review order:
build.dart
writers:pkgs/native_assets_cli/lib/src/api/*
+pkgs/native_assets_cli/example/*
.pkgs/native_assets_cli/lib/src/model/*
.pkgs/native_assets_cli/lib/src/model/build_config_CHANGELOG.md
andpkgs/native_assets_cli/lib/src/model/build_output_CHANGELOG.md
.This PR will need manual rolling:
native_assets_cli_internal
classes are now renamed toImpl
, this will need to be taken into account when rolling.Related issues:
build.dart
#882Closes: #1001
Closes: #188
Closes: #25