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

[CP] [dart2wasm] Make dart compile wasm compiled apps disable dart.library.ffi #55979

Closed
mkustermann opened this issue Jun 11, 2024 · 5 comments
Assignees
Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve

Comments

@mkustermann
Copy link
Member

Commit(s) to merge

847c356

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/370880

Issue Description

Packages on pub use conditional imports that branch on dart.library.ffi. The ffi-specific code is meant to be run on the VM. Unfortunately currently this also evaluates to true in dart2wasm, which makes apps that work with dart2js not work with dart2wasm due to different evaluation of the conditional import.

What is the fix

Change dart2wasm to make this conditional import evaluate to false.

Why cherry-pick

Users are hitting this when using package:flutter_svg which uses package:vector_graphics_compiler which uses `dart:ffi.

Risk

As always: If CI passes the risk is low.

Issue link(s)

#55948

Extra Info

No response

@mkustermann
Copy link
Member Author

/cc @athomas for awareness

If flutter also releases a new stable with this CP in, it may need to cherry-pick:

@mit-mit
Copy link
Member

mit-mit commented Jun 11, 2024

sgtm

@athomas
Copy link
Member

athomas commented Jun 11, 2024

LGTM

@athomas athomas added the cherry-pick-approved Label for approved cherrypick request label Jun 11, 2024
@mkustermann
Copy link
Member Author

The CL landed.

copybara-service bot pushed a commit that referenced this issue Jun 11, 2024
…dart.library.ffi`

This is a follow-up to [0]. That CL changed dart2wasm's modular
transformer to issue an error if `dart:ffi` is imported.

Users of packages that have specialized code for the VM (which supports
FFI) use conditional imports based on `dart.library.ffi`. We don't want
the VM-specific code to be used for web in dart2wasm (as dart2wasm
doesn't support the entirety of `dart:ffi`).

As a result we're going to make `dart.library.ffi` be false in
coditional imports (as well as in
`const bool.fromEnvironment('dart.library.ffi')`).

[0] https://dart-review.googlesource.com/c/sdk/+/368568

Bug: #55948
Bug: flutter/flutter#149984

Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/370580
Cherry-pick-request: #55979
Change-Id: Ia968bacf92566d421606fc026c7f016fe5abff01
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/370880
Reviewed-by: Slava Egorov <vegorov@google.com>
@devoncarew devoncarew added the area-dart2wasm Issues for the dart2wasm compiler. label Jun 12, 2024
@mkustermann
Copy link
Member Author

I believe this was released yesterday with Dart SDK 3.4.4 . So we can close this.

christopherfujino added a commit to flutter/engine that referenced this issue Jul 12, 2024
# Flutter stable 3.22.3 Engine

## Scheduled Cherrypicks

- Roll dart revision: dart-lang/sdk@604651494
### Dart
- dart-lang/sdk#55979
- dart-lang/sdk#55943
### Engine
- flutter/flutter#149700
- flutter/flutter#149701
- flutter/flutter#149702
- flutter/flutter#149704
- flutter/flutter#149745
- flutter/flutter#149771
- #53183

---------

Co-authored-by: Martin Kustermann <kustermann@google.com>
Co-authored-by: Jackson Gardner <eyebrowsoffire@gmail.com>
Co-authored-by: Christopher Fujino <christopherfujino@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve
Projects
None yet
Development

No branches or pull requests

6 participants