Support native assets

[dcharkes]

The Dart VM expects a native_assets.yaml next to packages_config.json to contain the mapping for @FfiNative and @Native FFI functions when running a script or spawning an isolate.

When a program compiles to kernel eagerly (before invoking the Dart VM), it should add that native_assets.yaml to the kernel compilation invocation.

package:test does manual compilation to kernel, so it needs to provide this mapping.

After https://dart-review.googlesource.com/c/sdk/+/267340, the native_assets.yaml will be created by dartdev when doing dart test or dart run test.

I'm not entirely sure how to test this PR.

I can't land the dartdev logic in https://dart-review.googlesource.com/c/sdk/+/267340 without doing dependency overrides for package:test and package:test_core, but I don't want to land dependency overrides pointing to non-merged code in the SDK.

I can't easily add a unit test that compiles a dynamic library and adds a native_assets.yaml manually on this repo. I would need to be able to invoke bin/test.dart instead of dart run test:test from an example project that provides a dylib. But package:test doesn't seem to support that:

dacoharkes@dacoharkes-linux:~/src/dart-lang/test/pkgs/test/test/runner/native_assets/test_projects/native_add$ dart ../../../../../bin/test.dart
00:01 +0 -1: loading test/native_add_test.dart [E]                                                                                      
  Failed to load "test/native_add_test.dart":
  Couldn't resolve the package 'native_add' in 'package:native_add/native_add.dart'.
  test/native_add_test.dart:5:8: Error: Not found: 'package:native_add/native_add.dart'
  import 'package:native_add/native_add.dart';

I know it works locally with a patched package:test and patched dartdev.

Any suggestions for how to test this?

(For reference: the unit tests in the VM that get a manually provided native_assets.yaml: https://dart-review.googlesource.com/c/sdk/+/264842)

[jakemac53]

Should this apply to both AOT and JIT modes? We also support compiling with dart compile exe now (via --compiler exe). That goes through a different code path that also probably has to be updated.

[dcharkes]

Should this apply to both AOT and JIT modes? We also support compiling with dart compile exe now (via --compiler exe). That goes through a different code path that also probably has to be updated.

Ah, I'm not aware of that code path, met me try!

[jakemac53]

Should this be a configurable path? Or just wait to allow that until/if it is needed later?

The package config isn't always necessarily what you think it is... tools could copy it to a new location and modify it before invoking us etc.

[dcharkes]

The use case I'm thinking about is dart test which is equal to dart run test:test. In this case dart run/dart test will run themselves in JIT mode use the .dart_tools/package_config.json and .dart_tools/native_assets.yaml.

(dart test/foo_test.dart already works after .dart_tools/package_config.json and .dart_tools/native_assets.yaml are present. Basically running a test that way skips the whole runner infrastructure.)

tools could [...]

I hope these tools would then still run dart pub get + dart test/dart run test which would then result in the same layout. Are you aware of tools that do different things?

[jakemac53]

So this package actually reads the Isolate.packageConfig to get the location. Users can customize what package config they pass, it's just that by default it will be .dart_tool/package_config.json. But we are not opinionated today and I would like to keep it that way if possible.

There is also one very concrete place this will happen which is internally, the package config files are always manually passed. I don't believe that internal hits this code path today though (it uses pre-compiled mode or runs directly from source), and I don't know what our plans are there long term.

[jakemac53]

I hope these tools would then still run dart pub get + dart test/dart run test which would then result in the same layout. Are you aware of tools that do different things?

I am not sure if it is still the case but for a long time flutter used to mess with the package config after pub get, to inject some references to generated packages. It did cause problems for build_runner etc or other tools that expect the .dart_tool/package_config.json file to be the source of truth, or the pubspec.yaml or pubspec.lock files to agree with the package_config.json file (which was the case with build_runner).

Also as soon as you step outside of the pub ecosystem all these assumptions no longer apply.

[dcharkes]

Yes, for Flutter I have implementations for this in flutter_tools. (It does cross-compiling, Dart doesn't. It bundles libraries differently per target OS as well, so it also transforms the paths where libraries are found for the target bundles.)

[jakemac53]

Just a random thought here, but would it make sense to add a new (optional) field to the package_config.json which lists the location of the native_assets.yaml file? That way we need no changes to this package at all, and internally it should also be more straightforward.

Any command that supports reading that file would just get it from the package config, which is always already supplied.

[dcharkes]

Good thought. @jonasfj and discussed embedding the native assets mapping inside the package config earlier.

Unfortunately package_config.json is produced by pub on pub get, this file is produced on dart run, dart compile, flutter run, dart test, flutter pub test which is later in the pipeline. Only those later stages of the pipeline know where a file is shipped. (For example an Android bundle in the case of Flutter, in which case the paths of dylibs are inside that Android bundle.) So pub cant produce this file, only the embedders can. And pub doesn't know if and when embedders are going to produce this file.

(Side note, I believe the pub authors don't like us modifying package_config.json after pub created that file.)

[jonasfj]

I believe the pub authors don't like us modifying package_config.json after pub created that file.

Yeah, ideally it's best not to do this. It messes up timestamps. We can add support for it -- but I would suggest just writing it next to package_config.json in .dart_tool/native_assets.yaml.

[dcharkes]

Should this apply to both AOT and JIT modes? We also support compiling with dart compile exe now (via --compiler exe). That goes through a different code path that also probably has to be updated.

Ah, I'm not aware of that code path, met me try!

This is not going to work just yet.

dart compile exe produces a single executable. In order to support adding native code to single executable we need to support using static libraries and using the native linker. Tracked in:

• dart compile exe support static linking of C code sdk#47718

To support dynamic libraries, we need to introduce a new command dart build that produces a directory (bundle) with both dynamic libraries and the executable. This is being tracked in:

• Adding dart build sdk#51165

I have added a check to dart compile exe which errors out if there are any native assets. So in my dartdev prototype the test correctly fails when running dart run test:test --compiler exe: 'dart compile exe' does not yet support native artifacts.

I can follow up with package:test support for AOT when either one of those issues is addressed. (edit: the fact that it picks up on that it doesn't support native assets yet means we can rely on dart compile implementation for native assets when that lands in dartdev. So that will just start working once dart-lang/sdk#47718 lands. For dart bundle we would need to modify package:test to use dart build instead of dart compile.)

[jakemac53]

I can't easily add a unit test that compiles a dynamic library and adds a native_assets.yaml manually on this repo. I would need to be able to invoke bin/test.dart instead of dart run test:test from an example project that provides a dylib. But package:test doesn't seem to support that:

Why do you need to invoke bin/test.dart?

My guess is that you haven't ran a dart pub get in that example directory, it seems like it is loading the package config from package:test and not from the new package you created.

Ultimately though I would add this test to the integration_tests directory, that is where we store this style of test, and it won't accidentally pick up package configs from the package it is nested under. You should be able to create a pubspec_overrides.yaml in that package to override package:test to be a path dependency (you should also do this for test_api and test_core).

[dcharkes]

Why do you need to invoke bin/test.dart?

dart test will do the native assets mapping once we land support in the Dart SDK, but I can't test dart test in the Dart SDK without this fix in package:test.

Before landing native assets mapping in the Dart SDK for dart test, dart test will just ignore native assets, so no native assets will be provided. The only workaround is to not run dart test but dart path/to/a/script.dart, as the VM (not dartdev) is configured to pick up native_assets.yaml. The path/to/a/script.dart should not be the test/my_test.dart because then we don't exercise the package:test runner. I want to invoke te runner so that we exercise the code path that is modified in this PR, but not via dart test or dart run because that requires us to land the Dart SDK changes first.

I need to land this PR first and roll it into the SDK and add a test there. And later a test here once an SDK has been released with dart test changes also add a test here so we test with dart test. (Or somehow do bin/test.dart as described above.)

integration_tests

Here is what the test would look like once dart test is providing the mapping in the Dart SDK:

https://github.com/dart-lang/test/compare/native-assets-test

[jakemac53]

dart test will do the native assets mapping once we land support in the Dart SDK, but I can't test dart test in the Dart SDK without this fix in package:test.

package:test itself does not require any native assets, so this shouldn't matter right?

I need to land this PR first and roll it into the SDK and add a test there. And later a test here once an SDK has been released with dart test changes also add a test here so we test with dart test. (Or somehow do bin/test.dart as described above.)

Ok so maybe this explains things for me, is the issue that the native_assets.yaml file won't exist at all in any current sdks? Or the functionality to read it doesn't exist?

[jakemac53]

I am just going to check out your branch and see if I can get things working locally :)

[jakemac53]

I tried building the latest sdk and running dart pub get, dart run test/native_add_test.dart, and dart compile exe native_add_test.dart and none of them created the native_assets.yaml file?

I wanted to try just checking in this file into the integration test, at least until the SDK can actually build it as a part of dart test?

[dcharkes]

I tried building the latest sdk and running dart pub get, dart run test/native_add_test.dart, and dart compile exe native_add_test.dart and none of them created the native_assets.yaml file?

Correct, none of the released SDKs will produce native_assets.yaml until I land some version of https://dart-review.googlesource.com/c/sdk/+/267340.

Ok so maybe this explains things for me, is the issue that the native_assets.yaml file won't exist at all in any current sdks?

Correct. You need some version of https://dart-review.googlesource.com/c/sdk/+/26734.

Or the functionality to read it doesn't exist?

Incorrect, that exists. See the tests that manually provide a native_assets.yaml in https://github.com/dart-lang/sdk/tree/main/tests/ffi/native_assets.

I wanted to try just checking in this file into the integration test, at least until the SDK can actually build it as a part of dart test?

Checking in won't work fully, we need to provide an absolute path, a target (linux_x64) and an actual dynamic library:

format-version:
  - 1
  - 0
  - 0
native-assets:
  linux_x64:
    package:native_add/src/native_add_bindings_generated.dart:
      - absolute
      - /usr/local/google/home/dacoharkes/src/dart-lang/test/pkgs/test/test/runner/native_assets/test_projects/native_add/out/libnative_add.so

Moreover, before landing some version of https://dart-review.googlesource.com/c/sdk/+/26734 that file is to going to be ignored when doing dart test or dart run, it's only going to be consumed when doing dart path/to/a/script.dart. However in that case we don't need this PR, because only when going through the package:test runner do we do the manual kernel compilation that this PR modifies. Hence why we would need to invoke bin/test.dart manually if providing native_assets.yaml manually.

[jakemac53]

Ok, so given all that it sounds like we can't land any test of the functionality in this repo until the SDK changes are landed.

I am hesitant to land anything that is enabled by default but not tested though (on CI). I wouldn't want a new SDK to drop that creates this file but then the way we use it is somehow broken and people get broken.

Could we guard this code behind an environment variable so that you can enable it explicitly in the SDK tests? I am not sure if our CI setup there supports setting an environment variable for a test.

[dcharkes]

I am hesitant to land anything that is enabled by default but not tested though (on CI). I wouldn't want a new SDK to drop that creates this file but then the way we use it is somehow broken and people get broken.

In the CL that would start creating this file, would also land a unit test that exercises dart test. The tests are here: https://dart-review.googlesource.com/c/sdk/+/267340/40/pkg/dartdev/test/native_assets/test_test.dart. They are currently skipped. When this lands, I can unskip them in that CL.

Could we guard this code behind an environment variable so that you can enable it explicitly in the SDK tests? I am not sure if our CI setup there supports setting an environment variable for a test.

That could work, as I'm invoking dart test manually with Process.run in the unit tests.

I think a better solution might be to gate all native assets logic in the SDK behind an experimental flag, that way the native_assets.yaml file will not be produced if the user doesn't opt in.

[dcharkes]

I think a better solution might be to gate all native assets logic in the SDK behind an experimental flag, that way the native_assets.yaml file will not be produced if the user doesn't opt in.

Hm, dart test arguments get forwarded directly to package:test's bin entry-point. But package:test doesn't recognize the --enable-experiment=xxx flag.
Would we want to recognize experimental flags in package test?
If no, we need to filter that out before forwarding the arguments from dartdev to package:test.
(edit: with the latter in https://dart-review.googlesource.com/c/sdk/+/267340/41 dart-lang/test/integration_tests/native_add$ dart test --enable-experiment=native-assets succeeds, dart-lang/test/integration_tests/native_add$ dart test fails.)

(Side note: We want a experimental flag this feature, since it's an experiment we can't really experiment with if it only runs on my machine locally. So we want to land some stuff behind a flag. So it's not just for this PR.)

[jakemac53]

Experiments are supported at least to some extent, IIRC you need to pass them before the test command, so like dart --enable-experiment=some-experiment test?

[jakemac53]

Would the file potentially still exist but with stale contents though if somebody ran with the experiment once and then without it? We could parse the VM arguments to see if this experiment is enabled though and guard the logic behind that.

[jakemac53]

See https://github.com/dart-lang/test/blob/master/pkgs/test_core/lib/src/util/dart.dart#L75

[dcharkes]

Experiments are supported at least to some extent, IIRC you need to pass them before the test command, so like dart --enable-experiment=some-experiment test?

Good catch. dart compile takes experiment flags after the compile, due to being able to compile for a different experiment set than the dart invocation. dart run can forward experiment flags to the script being invoked. However, for dart run / dart test behavior itself we should use the current dart experiments.

Would the file potentially still exist but with stale contents though if somebody ran with the experiment once and then without it? We could parse the VM arguments to see if this experiment is enabled though and guard the logic behind that.
See https://github.com/dart-lang/test/blob/master/pkgs/test_core/lib/src/util/dart.dart#L75

Perfect! That was indeed an issue.

https://dart-review.googlesource.com/c/sdk/+/267340/42 with https://github.com/dart-lang/test/tree/native-assets-test

$ dart-lang/test/integration_tests/native_add$ dart test
[fail]
$ dart-lang/test/integration_tests/native_add$ dart --enable-experiment=native-assets test
[success]
$ dart-lang/test/integration_tests/native_add$ dart test
[fail]

[jakemac53]

Yeah the way experiments can/should be specified in general is pretty weird and seemingly inconsistent to the end user, although it can usually be explained with reason, it isn't obvious :).

Fwiw for package:test we rely on the argument being set for the actual test runner script because we spawn Isolates to run tests, and we can't enable an experiment just for an Isolate.

[jakemac53]

I think this PR is pretty much ready to go but I think we want to land #1974 first, cc @natebosch . Then we will want to update the pubspec/changelog here.

[jakemac53]

The versioning here is slightly complicated so I will push up a pubspec/changelog change

[dcharkes]

Ready to merge from my side if you're happy with it.

[jakemac53]

@natebosch good on your end?