[native_toolchain_c] Add `libraries` and `libraryDirectories` options to `CTool` #1423

blaugold · 2024-08-13T07:58:20Z

These two new options align with the compiler/linker flags for linking.

libraries -> -l$library for gcc/clang and $library.lib for MSVC
libraryDirectories -> -L$directory for gcc/clang and /LIBPATH:$directory for MSVC

Per default, the BuildConfig.outputDirectory is added to libraryDirectories, since placing libraries directly into the outputDirectory is likely a common use case and is what CTools do.

Fixes #1419

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.

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

github-actions · 2024-08-13T10:00:36Z

PR Health

Breaking changes ✔️

Details

Package	Change	Current Version	New Version	Needed Version	Looking good?
native_assets_cli	None	0.7.3-wip	0.7.3-wip	0.7.3-wip	✔️

Changelog Entry ✔️

Details

Package	Changed Files

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

API leaks ✔️

Details

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 ✔️

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

Package publish validation ✔️

Details

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-wip	WIP (no publish necessary)
package:jnigen	0.10.0	already published at pub.dev
package:native_assets_cli	0.7.3-wip	WIP (no publish necessary)
package:objective_c	1.1.0	already published at pub.dev
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.

dcharkes

Awesome! LGTM w comments addressed! 🚀

dcharkes · 2024-12-09T08:35:24Z

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

@@ -14,7 +15,7 @@ abstract class CTool {
  /// What kind of artifact to build.
  final OutputType type;

-  /// Name of the library or executable to linkg.


build or link

(I believe linkers also inherit from ctool)

dcharkes · 2024-12-09T08:45:20Z

pkgs/native_assets_cli/test/example/native_dynamic_linking_test.dart

@@ -6,9 +6,6 @@
  'mac-os': Timeout.factor(2),
  'windows': Timeout.factor(10),
 })
-// TODO(https://github.com/dart-lang/native/issues/1415): Enable support
-// for Windows once linker flags are supported by CBuilder.
-@TestOn('!windows')


Also if (Platform.isWindows) { on line 20.

dcharkes · 2024-12-09T08:51:44Z

pkgs/native_toolchain_c/test/cbuilder/testfiles/dynamically_linked/src/dynamically_linked.c

+#include "math.h"
+
+int main() {
+  return math_add(1, 2);


It's a bit weird to expect a return code of 3 as the result of doing 1+2 because non-zero exit code usually indicate a problem. It's better to check whether the result is equal to 3 in c, and return exitcode 0 if the calculation was correct and 1 or 255 if the calculation is not correct. Or, for example printf the result and check the stdout in the test.

blaugold · 2024-12-09T18:13:41Z

This check will fail until https://dart-review.googlesource.com/c/sdk/+/381580 is in the latest dev release.

P.S.: Some projects only require approval to run checks for first time contributors. Is there a specific reason why this is not possible for this repo? 🙂

dcharkes · 2024-12-09T19:04:04Z

This check will fail until https://dart-review.googlesource.com/c/sdk/+/381580 is in the latest dev release.

Add a skip with a TODO. Otherwise the CI is going to be red all the time.

P.S.: Some projects only require approval to run checks for first time contributors. Is there a specific reason why this is not possible for this repo? 🙂

Yeah I would love to, but the Google policies don't allow it unfortunately. (There's some security concerns about being able to add arbitrary code and arbitrary GitHub actions in PRs.) It's churn for us to press the 'approve' button as well. 🙈 I'll try to be responsive on PRs. 💨

blaugold · 2024-12-09T19:29:31Z

I'll try to be responsive on PRs. 💨

And, I appreciate that! It was not meant as criticism. 😊

I'm pretty sure that I can trigger checks in the Flutter repo without approval, though, and that's Google too, but I guess different teams have different policies. 🤔😂

coveralls · 2024-12-09T20:58:47Z

coverage: 88.749% (+0.02%) from 88.732%
when pulling d6c129e on blaugold:cbuilder-dynamic-linking
into 8ea1a9d on dart-lang:main.

github-actions bot added type-infra A repository infrastructure change or enhancement package:native_assets_cli package:native_toolchain_c labels Aug 13, 2024

dcharkes reviewed Aug 13, 2024

View reviewed changes

pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart Outdated Show resolved Hide resolved

pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart Outdated Show resolved Hide resolved

blaugold mentioned this pull request Aug 21, 2024

Add native_dynamic_linking to test_data #1437

Merged

1 task

dcharkes mentioned this pull request Dec 6, 2024

[native_assets_cli] Code Assets depending on other code assets #190

Closed

3 tasks

blaugold added 2 commits December 6, 2024 21:36

Add CBuilder.dynamicallyLinkTo option

ba01bec

Add file header

f599be3

blaugold force-pushed the cbuilder-dynamic-linking branch from 8408424 to 1a11e7d Compare December 6, 2024 20:56

blaugold changed the title ~~[native_toolchain_c] Add CBuilder.dynamicallyLinkTo option~~ [native_toolchain_c] Add libraries and libraryPaths options to CBuilder Dec 6, 2024

blaugold force-pushed the cbuilder-dynamic-linking branch from 1a11e7d to 951fb4d Compare December 8, 2024 13:41

blaugold changed the title ~~[native_toolchain_c] Add libraries and libraryPaths options to CBuilder~~ [native_toolchain_c] Add libraries and libraryDirectories options to CBuilder Dec 8, 2024

blaugold changed the title ~~[native_toolchain_c] Add libraries and libraryDirectories options to CBuilder~~ [native_toolchain_c] Add libraries and libraryDirectories options to CTool Dec 8, 2024

Add libraries and libraryDirectories to CTool

94882a5

blaugold force-pushed the cbuilder-dynamic-linking branch from 951fb4d to 94882a5 Compare December 8, 2024 13:44

Update changelog

2571265

blaugold requested a review from dcharkes December 8, 2024 14:01

Update native_dynamic_linking test data

1676912

github-actions bot added the package:native_assets_builder label Dec 9, 2024

dcharkes approved these changes Dec 9, 2024

View reviewed changes

blaugold added 6 commits December 9, 2024 18:29

Fix formatting of ctool.dart

0937ee8

Reformat native_dynamic_linking_test.dart a bit

a579d3e

Run native_dynamic_linking_test.dart on windows

6b0b2c5

Fix doc comment

8f8c84a

Return wether comparison was successful

d2e13b3

Fix debug_printf in production mode

074d465

blaugold mentioned this pull request Dec 9, 2024

[native_toolchain_c] Some CTool options don't apply to linking #1793

Open

blaugold added 2 commits December 9, 2024 21:28

Make CBuilder test pass on linux

6e5d7a7

Skip native_dynamic_linking_test.dart on Windows

7e93fc5

blaugold force-pushed the cbuilder-dynamic-linking branch from 53cdbaf to 7e93fc5 Compare December 9, 2024 20:29

Disable dart test in example/build/native_dynamic_linking on windows

d6c129e

dcharkes merged commit e00afd9 into dart-lang:main Dec 10, 2024
24 checks passed

blaugold deleted the cbuilder-dynamic-linking branch December 10, 2024 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native_toolchain_c] Add `libraries` and `libraryDirectories` options to `CTool` #1423

[native_toolchain_c] Add `libraries` and `libraryDirectories` options to `CTool` #1423

blaugold commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024

dcharkes left a comment

dcharkes Dec 9, 2024

dcharkes Dec 9, 2024

dcharkes Dec 9, 2024

blaugold commented Dec 9, 2024

dcharkes commented Dec 9, 2024

blaugold commented Dec 9, 2024

coveralls commented Dec 9, 2024

[native_toolchain_c] Add libraries and libraryDirectories options to CTool #1423

[native_toolchain_c] Add libraries and libraryDirectories options to CTool #1423

Conversation

blaugold commented Aug 13, 2024 • edited Loading

github-actions bot commented Aug 13, 2024

PR Health

Breaking changes ✔️

Changelog Entry ✔️

API leaks ✔️

License Headers ✔️

Package publish validation ✔️

dcharkes left a comment

Choose a reason for hiding this comment

dcharkes Dec 9, 2024

Choose a reason for hiding this comment

dcharkes Dec 9, 2024

Choose a reason for hiding this comment

dcharkes Dec 9, 2024

Choose a reason for hiding this comment

blaugold commented Dec 9, 2024

dcharkes commented Dec 9, 2024

blaugold commented Dec 9, 2024

coveralls commented Dec 9, 2024

[native_toolchain_c] Add `libraries` and `libraryDirectories` options to `CTool` #1423

[native_toolchain_c] Add `libraries` and `libraryDirectories` options to `CTool` #1423

blaugold commented Aug 13, 2024 •

edited

Loading