Add linking for `native_toolchain_c` and linux #987

mosuem · 2024-03-11T13:28:59Z

Requires #827 to be merged.

Extend API by providing a CLinker, supporting development only on linux for now. Other platforms to be added in future PRs.

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.

commit c24e422 Author: Moritz <mosum@google.com> Date: Tue Apr 16 10:46:50 2024 +0200 Unify `configFile` commit 53ab489 Author: Moritz <mosum@google.com> Date: Tue Apr 16 10:16:26 2024 +0200 Changes as per review commit 95a6479 Author: Moritz <mosum@google.com> Date: Mon Apr 15 16:48:49 2024 +0200 Copy over more files commit c33d5dd Author: Moritz <mosum@google.com> Date: Mon Apr 15 15:02:10 2024 +0200 Fix linking id check commit b88c152 Author: Moritz <mosum@google.com> Date: Fri Apr 12 15:55:56 2024 +0200 Fix dep commit 3d8ed4a Author: Moritz <mosum@google.com> Date: Fri Apr 12 15:17:52 2024 +0200 Fix import commit 3955ea9 Author: Moritz <mosum@google.com> Date: Fri Apr 12 15:09:59 2024 +0200 Export ResourceIdentifiers

commit 725bdd7 Author: Moritz <mosum@google.com> Date: Tue Apr 16 15:58:21 2024 +0200 Make resources nullable commit c24e422 Author: Moritz <mosum@google.com> Date: Tue Apr 16 10:46:50 2024 +0200 Unify `configFile` commit 53ab489 Author: Moritz <mosum@google.com> Date: Tue Apr 16 10:16:26 2024 +0200 Changes as per review commit 95a6479 Author: Moritz <mosum@google.com> Date: Mon Apr 15 16:48:49 2024 +0200 Copy over more files commit c33d5dd Author: Moritz <mosum@google.com> Date: Mon Apr 15 15:02:10 2024 +0200 Fix linking id check commit b88c152 Author: Moritz <mosum@google.com> Date: Fri Apr 12 15:55:56 2024 +0200 Fix dep commit 3d8ed4a Author: Moritz <mosum@google.com> Date: Fri Apr 12 15:17:52 2024 +0200 Fix import commit 3955ea9 Author: Moritz <mosum@google.com> Date: Fri Apr 12 15:09:59 2024 +0200 Export ResourceIdentifiers

dcharkes

Add linking for native_toolchain_c and linux

[native_toolchain_c] Linker support for Linux

dcharkes · 2024-04-17T12:05:55Z

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

@@ -19,7 +17,10 @@ void main(List<String> arguments) async {
        'src/native_add.c',
        'src/native_multiply.c',
      ],
-      dartBuildFiles: ['hook/build.dart'],
+      dartBuildFiles: [
+        'hook/build.dart',


Why do we want to rerun the build hook if the link hook changed?

dcharkes · 2024-04-17T12:10:12Z

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

-      os: config.targetOS,
-    ),
-    linkInPackage: packageName,
+NativeCodeAsset placeholderAsset(HookConfig config) {


This should not be needed anymore.

Am I correct in thinking that:

The unit tests for the package here can land without SDK changes.

Then we land the SDK CL in which this test_data is used in dartdev unit tests and we actually add a new asset in link.

Then we land another PR in the dart-lang/native that enables the examples.

If I am not mistaken, we should not need to land the placeholder workaround.

dcharkes · 2024-04-17T12:11:43Z

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

+  /// Get the linker flags for the specified [linker].
+  ///
+  /// Throws if the [linker] is not supported.
+  Iterable<String> preflags(Tool linker) =>


Document pre-what.

dcharkes · 2024-04-17T12:11:56Z

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

+  /// Get the linker flags for the specified [linker].
+  ///
+  /// Throws if the [linker] is not supported.
+  Iterable<String> flags(Tool linker) => _toLinkerSyntax(linker, [


Should this be postFlags? And post-what?

dcharkes · 2024-04-17T12:14:01Z

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

+          linkerTool == clang ||
+          linkerTool == gcc ||
+          linkerTool == gnuLinker) {
+        await runClangLike(compiler: linker_);


compiler: linker_ looks odd, maybe rename the param to tool so it's tool: linker_.

dcharkes · 2024-04-17T12:15:48Z

pkgs/native_toolchain_c/lib/src/native_toolchain/recognizer.dart

-      tool = gnuLinker;
+
+    //TODO: Make this logic more correct
+    if (filePath.endsWith(os.executableFileName('clang'))) {


Why do we recognize a linker with a compiler executable name?

dcharkes · 2024-04-17T12:17:50Z

pkgs/native_toolchain_c/test/clinker/testfiles/linker/test1.c

@@ -0,0 +1,6 @@
+#include <stdio.h>


Don't commit binary files (test.a, test1.o, test2.o).

Add git ignores.

dcharkes · 2024-04-17T12:18:45Z

pkgs/native_toolchain_c/test/clinker/objects_test.dart

+              sources: [
+                'test/clinker/testfiles/linker/test1.o',
+                'test/clinker/testfiles/linker/test2.o',
+              ].map((e) => packageUri.resolve(e).toFilePath()).toList())


].map((e) => packageUri.resolve(e).toFilePath()).toList(),) for better formatting

dcharkes · 2024-04-17T12:22:42Z

pkgs/native_toolchain_c/lib/src/native_toolchain/recognizer.dart

 class LinkerRecognizer implements ToolResolver {
  final Uri uri;
+  final OS os;


This field is not used, resolve has a variable os that shadows it.

dcharkes · 2024-04-17T12:23:19Z

pkgs/native_toolchain_c/pubspec.yaml

@@ -1,7 +1,7 @@
 name: native_toolchain_c
 description: >-
  A library to invoke the native C compiler installed on the host machine.
-version: 0.4.1
+version: 0.4.2-wip


(I can't leave comments on binary files.)

Don't commit test.so, add a git ignore.

dcharkes · 2024-04-17T12:25:49Z

pkgs/native_assets_builder/test_data/treeshaking_native_assets/hook/link.dart

+      //TODO: expose name from `placeholderAsset`
+      assetName: 'src/${packageName}_bindings.dart',
+      linkerOptions: LinkerOptions.treeshake(
+          symbols: config.resources


🤯 😄 🚀

mosuem added 30 commits November 6, 2023 14:40

Initial commit

5424b7e

Merge branch 'main' into addLinkingScript

4993919

Fixes

86108c4

Merge branch 'main' into addLinkingScript

a739eb8

Works now

3025ce5

Start writing tests

4f77986

Fixing test

f5aef83

Merge branch 'main' into addLinkingScript

f8d7498

Fix merge errors

ed23630

Fix issues

470e356

Fix path

ded78ff

Fix paths

5ff082f

Fix paths

4755b64

Rename asset

b376ae1

Format

5c205b7

Add buildType

30e639f

Rename

cd92f28

Remove asset type

217a340

Add step serialization

d7bae1a

Merge branch 'main' into addLinkingScript

e4c5793

Fix overrides

f916a26

Don't check web coverage

c2f8ecb

Merge fixes

f0668f1

Switch to const constructors

83c6cdc

Don't fail fast

6bfa254

Add override to link

e710239

Small fixes

a8651cc

Working on it

9d6c1fb

Merge branch 'main' into addLinkingScript

3edfc66

Fixes

a9967d1

mosuem added 7 commits April 12, 2024 09:19

Fix API

21402dc

Merge branch 'addLinkingScript' into exposeLinkFromCtoolchain

00d33b5

Make arguments optional

4b0df6a

Merge branch 'addLinkingScript' into exposeLinkFromCtoolchain

3b6a980

Revert linkable assets

c19dc83

Merge branch 'addLinkingScript' into exposeLinkFromCtoolchain

d98bce9

Fixes

b5a8e5f

github-actions bot added the package:native_assets_builder label Apr 12, 2024

mosuem marked this pull request as ready for review April 12, 2024 11:59

mosuem added 4 commits April 12, 2024 14:02

Add newline

f5653c2

Export ResourceIdentifiers

3955ea9

Fix import

3d8ed4a

Fix dep

b88c152

mosuem requested a review from dcharkes April 15, 2024 11:48

mosuem added 5 commits April 15, 2024 15:02

Fix linking id check

c33d5dd

Copy over more files

95a6479

Changes as per review

53ab489

Unify configFile

c24e422

github-actions bot added the package:native_assets_cli label Apr 16, 2024

mosuem added 6 commits April 16, 2024 15:58

Make resources nullable

725bdd7

Fix treeshake options

d79e192

Ass whole archive option

a26f11b

Remove unneeded method

1a1bb92

Merge branch 'addLinkingScript' into exposeLinkFromCtoolchain

c31408a

github-actions bot removed the package:native_assets_cli label Apr 17, 2024

dcharkes reviewed Apr 17, 2024

View reviewed changes

Base automatically changed from addLinkingScript to main May 8, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add linking for `native_toolchain_c` and linux #987

Add linking for `native_toolchain_c` and linux #987

mosuem commented Mar 11, 2024 •

edited

dcharkes left a comment

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

dcharkes Apr 17, 2024

Add linking for native_toolchain_c and linux #987

Are you sure you want to change the base?

Add linking for native_toolchain_c and linux #987

Conversation

mosuem commented Mar 11, 2024 • edited

dcharkes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add linking for `native_toolchain_c` and linux #987

Add linking for `native_toolchain_c` and linux #987

mosuem commented Mar 11, 2024 •

edited