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

How is runtime/include versioned? #51459

Open
blaugold opened this issue Feb 18, 2023 · 7 comments
Open

How is runtime/include versioned? #51459

blaugold opened this issue Feb 18, 2023 · 7 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.

Comments

@blaugold
Copy link
Contributor

blaugold commented Feb 18, 2023

While verifying that my package still works with the current stable channel, I ran into an issue where my code broke because of a backwards incompatible change of Dart_CObject_Type in dart_native_api.h. Between Dart 2.18 and 2.19 two new enum values were added in a way that changed the order of other values. The code in question is part of a precompiled native library that is shipped with the package.

Currently, I'm not sure what the ABI versioning policy of the APIs in runtime/include is.

Dart_InitializeApiDL implements an API versioning check, but my understanding is that this is limited to the symbols available through dart_api_dl.h. Dart_InitializeApiDL also does not document the semantics of its return value.

@lrhn lrhn added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Feb 18, 2023
@a-siva
Copy link
Contributor

a-siva commented Feb 20, 2023

//cc @dcharkes

@dcharkes
Copy link
Contributor

dcharkes commented Feb 20, 2023

Thanks for reporting @blaugold !

Between Dart 2.18 and 2.19 two new enum values were added in a way that changed the order of other values.

That should have been a major version bump in https://github.com/dart-lang/sdk/blob/main/runtime/include/dart_version.h
Or, maybe better would have been to add the new values at the end of the enum, so it would have only been a minor version bump.

two new enum values were added

I only see Dart_CObject_kUnmodifiableExternalTypedData, which other one was added as well @blaugold ?

8c577c4 added Dart_CObject_kUnmodifiableExternalTypedData.
cc @rmacnak-google @mkustermann

I believe we should reorder the enum values (they are not alphabetically sorted anyway) so that the new ones are at the end, and file a cherry pick for the 2.19 stable and 3.0.0 beta.
cc @athomas

We've already run into this before:

  • Can we get a guarantee for ABI stability? We would break all flutter FFI plugins on bumping the major version (like we have to do now).
  • Can we make dart_api_dl.h standalone to limit the API surface? This requires copying some definitions and macros from dart_api.h and dart_native_api.h (gist). It is probably cleaner to move them to a separate standalone file. In any case we need to add tests that the version number is bumped when these are changed to prevent future breakage.

Issue for tracking standalone file:

(A standalone dart_api_dl.h might have caught this, if we had code that casts the API types blindly and invokes things. That would have led to errors if we had a test exercising this.)

@blaugold
Copy link
Contributor Author

blaugold commented Feb 20, 2023

I only see Dart_CObject_kUnmodifiableExternalTypedData, which other one was added as well @blaugold ?

Ah, sorry. I had an older copy of the files in runtime/include checked into my repo and when comparing it with the current version it looked like Dart_CObject_kNativePointer had also been added, but that was in Dart 2.15.

@dcharkes
Copy link
Contributor

I only see Dart_CObject_kUnmodifiableExternalTypedData, which other one was added as well @blaugold ?

Ah, sorry. I had an older copy of the files in runtime/include checked into my repo and when comparing it with the current version I looked like Dart_CObject_kNativePointer had also been added, but that was in Dart 2.15.

Hm, I don't think we'd want to cherry pick for 2.15, 2.16, 2.17, and 2.18 as well. The only enum value after Dart_CObject_kNativePointer is Dart_CObject_kUnsupported. I do you rely on that?

@blaugold
Copy link
Contributor Author

No, Dart_CObject_kSendPort is what caused my issue.

copybara-service bot pushed a commit that referenced this issue Feb 20, 2023
https://dart-review.googlesource.com/c/sdk/+/257925 added a new entry
in the middle of the `Dart_CObject_Type` enum, which changed the
value of the entries below. However, this enum is part of
`dart_api_dl.h` and versioned by `dart_version.h`.

New entries to `Dart_CObject_Type` should be added at the end of the
enum to avoid making breaking changes to the type.

TEST=tests/ffi/vmspecific_handle_dynamically_linked_test.dart

Bug: #51459

Change-Id: I367b54f62e59ddf925e255bb56c0f8660be7c227
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/284161
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
@blaugold
Copy link
Contributor Author

blaugold commented Mar 2, 2023

@dcharkes Is a CP planned for the fix? I don't need one, since I had already released a fix for my package by recompiling with a recent Dart SDK when I opened this issue. When the fix lands, I'll have to recompile and release a new version, so I just wanted to know when I should expect the fix to land.

@dcharkes
Copy link
Contributor

dcharkes commented Mar 3, 2023

Thanks for the ping @blaugold, this had slipped my mind!

copybara-service bot pushed a commit that referenced this issue Mar 16, 2023
https://dart-review.googlesource.com/c/sdk/+/257925 added a new entry
in the middle of the `Dart_CObject_Type` enum, which changed the
value of the entries below. However, this enum is part of
`dart_api_dl.h` and versioned by `dart_version.h`.

New entries to `Dart_CObject_Type` should be added at the end of the
enum to avoid making breaking changes to the type.

TEST=tests/ffi/vmspecific_handle_dynamically_linked_test.dart

Bug: #51459
Bug: #51615
Cherry-Pick: https://dart-review.googlesource.com/c/sdk/+/284161
Change-Id: Ic3e37255bdbc2d75af7b9894a61293c84428e042
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/286741
Auto-Submit: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
copybara-service bot pushed a commit that referenced this issue Mar 17, 2023
https://dart-review.googlesource.com/c/sdk/+/257925 added a new entry
in the middle of the `Dart_CObject_Type` enum, which changed the
value of the entries below. However, this enum is part of
`dart_api_dl.h` and versioned by `dart_version.h`.

New entries to `Dart_CObject_Type` should be added at the end of the
enum to avoid making breaking changes to the type.

TEST=tests/ffi/vmspecific_handle_dynamically_linked_test.dart

Bug: #51459
Bug: #51615
Cherry-Pick: https://dart-review.googlesource.com/c/sdk/+/284161
Change-Id: I0afa4128a17e7ef473ae3bbdcef511847fcafcd7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/286780
Reviewed-by: Siva Annamalai <asiva@google.com>
Auto-Submit: Daco Harkes <dacoharkes@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

4 participants