[native_assets_cli] `BuildConfig.linkingEnabled` #1273

dcharkes · 2024-07-09T13:15:27Z

Step 1 of

[native_assets_cli] Stop running link hooks in JIT mode. #1252

Naming bike shedding:

BuildConfig.hasLinkPhase?
BuildConfig.willLink?
BuildConfig.isLinkHookEnabled?

github-actions · 2024-07-09T13:17:42Z

PR Health

Breaking changes ✔️

Details

Package	Change	Current Version	New Version	Needed Version	Looking good?
native_assets_cli	Breaking	0.6.2-wip	0.7.0-wip	0.7.0-wip	✔️

Changelog Entry ✔️

Details

Package	Changed Files

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

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/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/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_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/native_assets_cli/lib/src/api/builder.dart
pkgs/native_assets_cli/lib/src/api/linker.dart
pkgs/swiftgen/swift2objc/lib/src/transformer/_core/unique_namer.dart

Package publish validation ✔️

Details

Package	Version	Status
package:ffi	2.1.3-wip	WIP (no publish necessary)
package:ffigen	13.0.0-wip	WIP (no publish necessary)
package:jni	0.9.3	already published at pub.dev
package:jnigen	0.10.0-wip	WIP (no publish necessary)
package:native_assets_cli	0.7.0-wip	WIP (no publish necessary)
package:objective_c	1.1.0-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-07-09T13:25:57Z

coverage: 92.211% (+0.01%) from 92.201%
when pulling dcd64e9 on no-link-in-jit
into b054170 on main.

HosseinYousefi

LGTM!

Mandatory bikeshedding:

Between the options, I also prefer hasLinkPhase.

One possible confusion with the name might be that we're using two words, "phase" and "hook". Of course these are different concepts. XYZ hook gets run in the XYZ phase. But we don't mention "phase" in the docs that often.

Another option is to go with an enum with two options (buildStrategy/linkStrategy?):

buildOnly/noLink
buildAndLink/willLink

pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart

pkgs/native_assets_cli/test/api/build_config_test.dart

pkgs/native_assets_cli/test/model/checksum_test.dart

pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart

mosuem · 2024-07-09T14:56:31Z

Step 1 of

[native_assets_cli] Stop running link hooks in JIT mode. #1252

Naming bike shedding:

BuildConfig.hasLinkPhase?

BuildConfig.willLink?

BuildConfig.isLinkHookEnabled?

IMO, phase sounds too technical for no reason, willLink could be misleading (what if there is no link hook?), so isLinkEnabled or similar should be fine.

HosseinYousefi · 2024-07-09T15:14:42Z

what if there is no link hook?

Good point, but the same thing could be said about the other names. The only way to mitigate this confusion completely would be to use verbs that show possibility instead of down right stating that things will be linked. For example supportsLinkHook or canBeLinked.

mosuem · 2024-07-09T15:19:46Z

the same thing could be said about the other names. The only way to mitigate this confusion completely would be to use verbs that show possibility instead of down right stating that things will be linked.

I agree, I think my proposed isLinkEnabled also works in this regard. It does not promise that linking will happen, only that it is enabled in the config.

dcharkes · 2024-07-09T15:42:19Z

the same thing could be said about the other names. The only way to mitigate this confusion completely would be to use verbs that show possibility instead of down right stating that things will be linked.

I agree, I think my proposed isLinkEnabled also works in this regard. It does not promise that linking will happen, only that it is enabled in the config.

Much better! Bouncing off this with some other wording that feels slightly more natural:

linkingEnabled
linkHooksEnabled

I think, I lean towards the first.

HosseinYousefi · 2024-07-09T16:03:37Z

I agree, I think my proposed isLinkEnabled also works in this regard. It does not promise that linking will happen, only that it is enabled in the config.

To me, it sounds like there is already a link hook and it is "enabled" not that it's "possible to be enabled".

Some other suggestions:

supportsLinking / linkingSupported
allowsLinking / linkingAllowed
canLink

mosuem · 2024-07-09T16:08:26Z

For me, support/allow/can sounds more like a general option, less like a switch.

+1 for linkingEnabled

dcharkes · 2024-07-09T16:11:35Z

Hooks are called/invoked/run or not called/invoked/run.

I think the "enabled" is whether they will be called/invoked/run.

"allow" is for access control, "enable" is for possibility. So, I think linkingEnabled is better than linkingAllowed.

canLink

I like this, together with addAsset(..., {bool linkInPackage}). "can" and "enabled" are the same thing to me, but can definitely omits the possibility that something has already happened. And it's concise. 👌

HosseinYousefi · 2024-07-09T16:14:32Z

For me, support/allow/can sounds more like a general option, less like a switch.

+1 for linkingEnabled

I don't want to keep bike-shedding, so feel free to go with that name. However "enabled" suggests that there already is a linking step involved, so I "must", build linkable things.

Something like linkingAvailable suggests that I have the possibility of building linkable things.

dcharkes · 2024-07-09T16:24:30Z

linkingAvailable

I also like this one!

"available" and "enabled" is maybe better than "can", because can leaves it open if you as hook author can invoke linking or if the system might possibly be linking.

"available" is maybe better than "enabled" because it clearly states that you can use something from your hook. While enabled could mean something has already happened.

(Okay, I'm going to keep changing my mind in this conversation.)

@mosuem linkingAvailable?

mosuem · 2024-07-10T08:25:17Z

There is a distinction between the link.dart file and the linking step. The link.dart file is certainly always available, but not always enabled, and the linking step is not always available. So I would say that either linkingStepAvailable/linkingPhaseAvailable or linkingEnabled is fine by me.

I do think linkingEnabled is shorter and simpler, as we don't need to distinguish between link hook file and phase for enabled, but we do for available. Maybe I am overthinking this.

dcharkes · 2024-07-10T08:32:48Z

I was asking our trusty assistant earlier:

Option	Focus	Nuance	Clarity
linkingAvailable	Capability & Presence	Suggests linking is an option, but not necessarily the default or always guaranteed.	Very clear
canLink	Capability & Potential	Emphasizes the possibility of linking, but other factors might influence the outcome.	Clear
linkingEnabled	Configuration & Intent	Indicates linking is explicitly allowed and expected due to the build configuration.	Very clear

The Dart and Flutter SDK have the capability to link in some configurations (the configurations in which Dart is build in AOT mode). So seeing from the point of view that build hooks are invoked with a variety of different BuildConfigs, "enabled" seems to be a more fitting word than "available".

I don't want to keep bike-shedding, so feel free to go with that name. However "enabled" suggests that there already is a linking step involved, so I "must", build linkable things.

I will add a doc comment to clarify that one "may" build linkable things instead of "must".

Thanks for all the input!

HosseinYousefi · 2024-07-10T08:32:53Z

There is a distinction between the link.dart file and the linking step. The link.dart file is certainly always available, but not always enabled, and the linking step is not always available. So I would say that either linkingStepAvailable/linkingPhaseAvailable or linkingEnabled is fine by me.

Good point, I didn't think of it that way.

I do think linkingEnabled is shorter and simpler, as we don't need to distinguish between link hook file and phase for enabled, but we do for available. Maybe I am overthinking this.

I think we all like linkingEnabled enough to go with that.

[native_assets_cli] BuildConfig.hasLinkPhase

cf9d944

dcharkes requested review from HosseinYousefi and mosuem July 9, 2024 13:15

github-actions bot added package:native_assets_cli package:native_toolchain_c package:native_assets_builder labels Jul 9, 2024

missing copyright header

5514671

dcharkes added 3 commits July 9, 2024 15:29

only use linkInPackage if hasLinkPhase

5286730

exercise not linking build hook

33eda09

add documentation on when link hooks are run

846ff19

HosseinYousefi approved these changes Jul 9, 2024

View reviewed changes

hasLinkPhase -> linkingAvailable and address comments

520fb6c

dcharkes added 2 commits July 10, 2024 10:37

linkingAvailable -> linkingEnabled

e92a9e9

more doc comments

dcd64e9

mosuem approved these changes Jul 10, 2024

View reviewed changes

dcharkes changed the title ~~[native_assets_cli] BuildConfig.hasLinkPhase~~ [native_assets_cli] BuildConfig.linkingEnabled Jul 10, 2024

dcharkes merged commit 81dd2b4 into main Jul 10, 2024
32 checks passed

dcharkes deleted the no-link-in-jit branch July 10, 2024 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native_assets_cli] `BuildConfig.linkingEnabled` #1273

[native_assets_cli] `BuildConfig.linkingEnabled` #1273

dcharkes commented Jul 9, 2024

github-actions bot commented Jul 9, 2024 •

edited

Loading

coveralls commented Jul 9, 2024 •

edited

Loading

HosseinYousefi left a comment

mosuem commented Jul 9, 2024

HosseinYousefi commented Jul 9, 2024

mosuem commented Jul 9, 2024

dcharkes commented Jul 9, 2024

HosseinYousefi commented Jul 9, 2024 •

edited

Loading

mosuem commented Jul 9, 2024

dcharkes commented Jul 9, 2024

HosseinYousefi commented Jul 9, 2024

dcharkes commented Jul 9, 2024

mosuem commented Jul 10, 2024

dcharkes commented Jul 10, 2024

HosseinYousefi commented Jul 10, 2024

[native_assets_cli] BuildConfig.linkingEnabled #1273

[native_assets_cli] BuildConfig.linkingEnabled #1273

Conversation

dcharkes commented Jul 9, 2024

github-actions bot commented Jul 9, 2024 • edited Loading

PR Health

Breaking changes ✔️

Changelog Entry ✔️

License Headers ✔️

Package publish validation ✔️

coveralls commented Jul 9, 2024 • edited Loading

HosseinYousefi left a comment

Choose a reason for hiding this comment

mosuem commented Jul 9, 2024

HosseinYousefi commented Jul 9, 2024

mosuem commented Jul 9, 2024

dcharkes commented Jul 9, 2024

HosseinYousefi commented Jul 9, 2024 • edited Loading

mosuem commented Jul 9, 2024

dcharkes commented Jul 9, 2024

HosseinYousefi commented Jul 9, 2024

dcharkes commented Jul 9, 2024

mosuem commented Jul 10, 2024

dcharkes commented Jul 10, 2024

HosseinYousefi commented Jul 10, 2024

[native_assets_cli] `BuildConfig.linkingEnabled` #1273

[native_assets_cli] `BuildConfig.linkingEnabled` #1273

github-actions bot commented Jul 9, 2024 •

edited

Loading

coveralls commented Jul 9, 2024 •

edited

Loading

HosseinYousefi commented Jul 9, 2024 •

edited

Loading