Add `native_dynamic_linking` to `test_data` #1437

blaugold · 2024-08-20T15:22:16Z

This PR adds test data for dart-lang/sdk#56459.

CL that adds tests in Dart SDK that use the test_data: https://dart-review.googlesource.com/c/sdk/+/381580

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

mosuem · 2024-08-21T08:13:36Z

pkgs/native_assets_builder/test_data/native_dynamic_linking/hook/build.dart

+  });
+}
+
+extension on BuildConfig {


Code like this, switching over OSs, seems like something which should actually live in a helper package. Or is it specific to this example?

Or is this covered by #190?

I've started working on an option for CBuilder that adds the necessary flags (#1423) but have held off until both embedders have landed the changes necessary for dynamic linking to work (standalone Dart: https://dart-review.googlesource.com/c/sdk/+/381580, Flutter: flutter/flutter#153054).

github-actions · 2024-08-21T08:37:11Z

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
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart

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/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/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/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_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

This check can be disabled by tagging the PR with skip-license-check.

Package publish validation ✔️

Package	Version	Status
package:ffi	2.1.3	already published at pub.dev
package:ffigen	14.0.0-wip	WIP (no publish necessary)
package:jni	0.11.0	already published at pub.dev
package:jnigen	0.11.0	already published at pub.dev
package:native_assets_cli	0.8.0-wip	WIP (no publish necessary)
package:objective_c	2.0.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.

coveralls · 2024-08-21T11:31:50Z

coverage: 91.964% (+0.05%) from 91.918%
when pulling 421b123 on blaugold:dynamic-linking-test-data
into 4a18bf2 on dart-lang:main.

dcharkes

Please exercise the test data in the tests:

run the hook
check if there's more than one asset
try to dlopen (probably requires spawning a new Dart process with the right path).

After that, LGTM! 🚀

dcharkes · 2024-08-22T13:03:59Z

pkgs/native_assets_builder/test_data/native_dynamic_linking/hook/build.dart

+import 'package:native_assets_cli/native_assets_cli.dart';
+import 'package:native_toolchain_c/native_toolchain_c.dart';
+
+void main(List<String> args) async {


Please exercise the test data in a new test in pkgs/native_assets_builder/test/

Invoke the hook

Then try to dlopen the dylib. (This will most likely require starting a new Dart process to open the dylib in so that PATH can be set correctly for that Dart process.)

dcharkes · 2024-09-05T06:40:09Z

pkgs/native_assets_builder/test/test_data/native_dynamic_linking_test.dart

+          '-Dpackage_root=${testPackageUri.toFilePath()}',
+          '-Dtarget_os=${OSImpl.current}',
+          '-Dversion=${HookConfigImpl.latestVersion}',
+          '-Dlink_mode_preference=dynamic',


add -Dlinking_enabled=0 (see fixes to tests in #1507)

Or possibly better, use BuildConfig constructor and write the config to a file and pass that in.

auto-submit · 2024-09-05T14:15:27Z

auto label is removed for dart-lang/native/1437, 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.

Add native_dynamic_linking to test_data

227dcd6

github-actions bot added the package:native_assets_builder label Aug 20, 2024

mosuem reviewed Aug 21, 2024

View reviewed changes

Fix GHA workflow

5a5d615

github-actions bot added the type-infra A repository infrastructure change or enhancement label Aug 21, 2024

blaugold requested a review from mosuem August 21, 2024 11:23

mosuem requested a review from dcharkes August 21, 2024 11:56

dcharkes reviewed Aug 22, 2024

View reviewed changes

Add test

663502f

blaugold requested a review from dcharkes September 5, 2024 06:13

dcharkes approved these changes Sep 5, 2024

View reviewed changes

Use BuildConfig to write config file

421b123

blaugold requested a review from dcharkes September 5, 2024 14:05

dcharkes approved these changes Sep 5, 2024

View reviewed changes

dcharkes added the autosubmit label Sep 5, 2024

auto-submit bot removed the autosubmit label Sep 5, 2024

dcharkes merged commit b18d32f into dart-lang:main Sep 5, 2024
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `native_dynamic_linking` to `test_data` #1437

Add `native_dynamic_linking` to `test_data` #1437

blaugold commented Aug 20, 2024 •

edited

Loading

mosuem Aug 21, 2024

mosuem Aug 21, 2024

blaugold Aug 21, 2024

github-actions bot commented Aug 21, 2024 •

edited

Loading

coveralls commented Aug 21, 2024 •

edited

Loading

dcharkes left a comment

dcharkes Aug 22, 2024

dcharkes Sep 5, 2024

blaugold Sep 5, 2024

auto-submit bot commented Sep 5, 2024

Add native_dynamic_linking to test_data #1437

Add native_dynamic_linking to test_data #1437

Conversation

blaugold commented Aug 20, 2024 • edited Loading

mosuem Aug 21, 2024

Choose a reason for hiding this comment

mosuem Aug 21, 2024

Choose a reason for hiding this comment

blaugold Aug 21, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2024 • edited Loading

PR Health

coveralls commented Aug 21, 2024 • edited Loading

dcharkes left a comment

Choose a reason for hiding this comment

dcharkes Aug 22, 2024

Choose a reason for hiding this comment

dcharkes Sep 5, 2024

Choose a reason for hiding this comment

blaugold Sep 5, 2024

Choose a reason for hiding this comment

auto-submit bot commented Sep 5, 2024

Add `native_dynamic_linking` to `test_data` #1437

Add `native_dynamic_linking` to `test_data` #1437

blaugold commented Aug 20, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

coveralls commented Aug 21, 2024 •

edited

Loading