Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[native_assets_cli] Add DartCApi #1937

Closed
wants to merge 1 commit into from
Closed

[native_assets_cli] Add DartCApi #1937

wants to merge 1 commit into from

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented Jan 23, 2025

This PR adds an (optional) path to the Dart C API. Both Dart and Flutter will be able to provide this.

For C code that's compiled in the hook, the path can be used directly.

For C code that's precompiled and pulled in via from a CDN, the version number should be checked in the hook.

Does not yet update the example in pkgs/native_assets_cli/example/build/use_dart_api/ because it's run against the released Dart and Flutter on CI. We should update it after Dart and Flutter pass in the API.

Bug: #839

Copy link

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) 2025, 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/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Jan 23, 2025
@coveralls
Copy link

coveralls commented Jan 23, 2025

Coverage Status

coverage: 88.37% (+0.02%) from 88.352%
when pulling 335ae24 on dart-api
into 7a72868 on main.

@dcharkes dcharkes marked this pull request as ready for review January 23, 2025 16:30
Copy link
Contributor

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no context for this but it seems reasonable.

/// Only `dart_api_dl.h` should be used. `dart_api_dl.c` shoud be compiled into
/// your `CodeAsset`.
///
/// Note: If you're precompiling code assets, you must check that [version] is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation might not be sufficient in the long wrong - how would I check that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One should check the major version is identical and the minor version is at least what you need.

@mkustermann
Copy link
Member

mkustermann commented Jan 24, 2025

Instead of baking this into the protocol and complicating dart & flutter bundling tools, have we considered simply making a package:dart_c_api package?

That would mean if an app depends on many packages, several of them use this package, we get the dart_api_impl.c compiled only once. With static linking possibly all into one binary or else as a shared library that all other C libraries can compile against.

Furthermore we'd have the ability to start versioning this API with SDK constraints of the package.

Basically the question is: What benefit does it have to bake it into the protocol?

@dcharkes
Copy link
Collaborator Author

dcharkes commented Jan 24, 2025

Instead of baking this into the protocol and complicating dart & flutter bundling tools, have we considered simply making a package:dart_c_api package?

That would mean if an app depends on many packages, several of them use this package, we get the dart_api_impl.c compiled only once. With static linking possibly all into one binary or else as a shared library that all other C libraries can compile against.

I like this.

Furthermore we'd have the ability to start versioning this API with SDK constraints of the package.

I don't believe this will work properly.

We don't have a way in pub to retroactively add an SDK upper bound to a package version.

Also it would mean that a Dart package has to chose a Dart C API version and will fail to resolve if it has chosen a different version.

Basically the question is: What benefit does it have to bake it into the protocol?

Being able to build against (or download) the exact Dart C API version.

This enables the C build of your package to have ifdefs against different versions of the Dart C API and make your package succeed building against different versions of the Dart C API.

Iif the dylibs are prebuilt, you can have multiple prebuilt binaries (os * arch * dart-c-api-version) similarly to how Python does it. #839 (comment)

@mkustermann
Copy link
Member

Furthermore we'd have the ability to start versioning this API with SDK constraints of the package.

I don't believe this will work properly.
We don't have a way in pub to retroactively add an SDK upper bound to a package version.

There's no reason for doing that retroactively. I've mentioned this in other places before, but cannot find where, so I've described it in a new issue here: #1941

This enables the C build of your package to have ifdefs against different versions of the Dart C API and make your package succeed building against different versions of the Dart C API.

That doesn't help. Imagine we break the API by removing a function. All the existing packages that use that function (and didn't if/def it because they don't know it's not available in newer versions) will break in obscure ways at hook build time or even at runtime.

Iif the dylibs are prebuilt, you can have multiple prebuilt binaries (os * arch * dart-c-api-version) similarly to how Python does it. #839 (comment)

I don't think one wants to have prebuilt binaries for huge cross products. Would a package author want to prebuilt their binaries for years old Dart SDKs?

We have a mechanism to deal breaking changes - semantic package versions - and I think we should use that mechanism instead of introducing X new versioning mechanisms that make things only fail at build or even runtime.

@dcharkes
Copy link
Collaborator Author

This enables the C build of your package to have ifdefs against different versions of the Dart C API and make your package succeed building against different versions of the Dart C API.

That doesn't help. Imagine we break the API by removing a function. All the existing packages that use that function (and didn't if/def it because they don't know it's not available in newer versions) will break in obscure ways at hook build time or even at runtime.

Right, if we do a breaking change we need to do a major version bump, and that's never forward compatible.

But a package can still decide to also support a previous major version.

Iif the dylibs are prebuilt, you can have multiple prebuilt binaries (os * arch * dart-c-api-version) similarly to how Python does it. #839 (comment)

I don't think one wants to have prebuilt binaries for huge cross products. Would a package author want to prebuilt their binaries for years old Dart SDKs?

Hopefully people move on quicker in Dart than in the Python ecosystem.

We have a mechanism to deal breaking changes - semantic package versions - and I think we should use that mechanism instead of introducing X new versioning mechanisms that make things only fail at build or even runtime.

Agreed. My suggestion in #93 (comment) with the SDK specifying constraints would have

# dart SDK
dependencies:
  dart_c_api: 2.5.0 # pinned

The package can then have both the C source and generated FFI bindings.

What this PR does has no solution for FFI bindings, because one would have to generate the bindings in their own package and once the API changes. (#839 (comment))

So publishing a package instead is a good idea.

It's kind of ugly we have to copy-paste the include dir, and we have to not forget to publish such package, but I believe that's acceptable.

(We can't really properly work with SDK constraints, because if we want to do a breaking change we'd need to postpone landing that breaking change in the SDK until the upper bound of the last published package runs out. That seems like considerable lag. And most likely we're going to forget about this constraint...)

@dcharkes
Copy link
Collaborator Author

Closing this PR for now in favor of adding a package in the future in some way. Let's continue the discussion on the tracking bug instead of PR. #839

@dcharkes dcharkes closed this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:native_assets_builder package:native_assets_cli type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants