-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[path_provider] Convert iOS/macOS to FFI package #9762
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
base: main
Are you sure you want to change the base?
[path_provider] Convert iOS/macOS to FFI package #9762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully converts path_provider_foundation
from a Flutter plugin using Pigeon to a Dart-only package leveraging FFI. This is a significant and well-executed architectural change that eliminates the native Swift codebase. The new implementation is clean, well-structured, and includes a comprehensive set of tests adapted for the FFI approach. The testing strategy, particularly moving unit tests that require an Objective-C runtime into the integration test suite, is a clever solution. My review only found a couple of minor typos in the new CONTRIBUTING.md
file.
packages/path_provider/path_provider_foundation/CONTRIBUTING.md
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_foundation/CONTRIBUTING.md
Outdated
Show resolved
Hide resolved
This is blocked on dart-lang/native#2477 (or a policy decision not to wait for that change), but I wanted to go ahead with a review and discussion in the meantime. Unlike many of our other plugins, adopting FFI here isn't blocked on non-optional thread merging, since per Apple docs, |
<?xml version="1.0" encoding="UTF-8"?> | ||
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
<plist version="1.0"> | ||
<dict> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know how Privacy Manifests are going to work for FFI? If I remember correctly on how the tooling works for this, I believe a dylib gets built into a .framework
. Technically .framework
isn't listed as being needed in Apple's documentation on Privacy Manifests, but .xcframework
is listed, and .xcframework
gets thinned to .framework
when put into the app bundle.
Basically, I think we likely still need a Privacy Manifest.
Looking at ffigen docs, perhaps an asset can be added to the config?
ffi-native:
asset-id: 'package:some_pkg/asset' # Optional, was assetId in previous versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know how Privacy Manifests are going to work for FFI?
Basically, I think we likely still need a Privacy Manifest.
I'll set up a meeting to discuss; I don't think this version of the package would still need a manifest.
@@ -0,0 +1,13 @@ | |||
targets: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file for? Is it specific to packages CI or is it needed for ffi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to make mockito generate mocks from annotations on files in integration_test
; by default it will ignore them (because normally unit-style tests, where you use mocks, don't go in integration_test
).
NSFileManager: | ||
include: | ||
- "containerURLForSecurityApplicationGroupIdentifier:" | ||
- "defaultManager" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't NSSearchPathDirectory
need to be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types that are needed by an API are included transitively, so NSSearchPathForDirectoriesInDomains
would be bringing it in.
@@ -1,3 +1,8 @@ | |||
## 2.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a breaking change? Developers who are importing the "path_provider_foundation" module directly will get a build failure since it's not longer available. Here's an example I found:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't generally consider native code changes to be breaking changes for the plugin unless the native code is explicitly intended for direct native use (e.g., this file).
I can't think of a good reason to directly import this native package. Notably, that code... doesn't appear to make sense? The import is there to create an instance of the plugin that is never used and never wired up to anything. Also, even if we considered this a breaking change for path_provider_foundation
, we wouldn't consider it a breaking change for path_provider
, and that app is not directly relying on path_provider_foundation
so would be broken anyway.
Ultimately, there's a large grey area of things that can break unexpected use cases, but that we don't consider breaking (e.g., adding a parameter to a method of a Dart class that's not intended to be subclassed). I would definitely include this change in that area. We have not considered it breaking in the past to change the name of the plugin class, which is very similar.
// platforms that don't support FFI (e.g., web) to avoid having transitive | ||
// dependencies break web compilation. | ||
export 'src/path_provider_foundation_stub.dart' | ||
if (dart.library.ffi) 'src/path_provider_foundation_real.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on remove _real
? Adding _stub
to the placeholder one seems sufficient to me
if (dart.library.ffi) 'src/path_provider_foundation_real.dart'; | |
if (dart.library.ffi) 'src/path_provider_foundation.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how path_provider_windows
is set up, so I mirrored it. IIRC we did this to avoid having two files with the same name in the same package? I don't have strong feelings about it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but CI is blocked on analyze issues from the objective_c
package
In file included from /Volumes/Work/s/w/ir/x/w/.pub-cache/hosted/pub.dev/objective_c-8.1.0/ios/Classes/../../src/include/dart_api_dl.h:10:
/Volumes/Work/s/w/ir/x/w/.pub-cache/hosted/pub.dev/objective_c-8.1.0/ios/Classes/../../src/include/dart_api.h:1794:11: error: parameter 'port_id' not found in the function declaration [-Werror,-Wdocumentation]
1794 | * \param port_id The port to be checked.
| ^~~~~~~
/Volumes/Work/s/w/ir/x/w/.pub-cache/hosted/pub.dev/objective_c-8.1.0/ios/Classes/../../src/include/dart_api.h:1794:11: note: did you mean 'port'?
1794 | * \param port_id The port to be checked.
| ^~~~~~~
| port
/Volumes/Work/s/w/ir/x/w/.pub-cache/hosted/pub.dev/objective_c-8.1.0/ios/Classes/../../src/include/dart_api.h:3354:11: error: parameter 'path' not found in the function declaration [-Werror,-Wdocumentation]
3354 | * \param path The asset id requested in the `@Native` external function.
| ^~~~
/Volumes/Work/s/w/ir/x/w/.pub-cache/hosted/pub.dev/objective_c-8.1.0/ios/Classes/../../src/include/dart_api.h:3354:11: note: did you mean 'asset_id'?
3354 | * \param path The asset id requested in the `@Native` external function.
| ^~~~
| asset_id
/Volumes/Work/s/w/ir/x/w/.pub-cache/hosted/pub.dev/objective_c-8.1.0/ios/Classes/../../src/include/dart_api.h:3372:50: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
3372 | typedef char* (*Dart_NativeAssetsAvailableAssets)();
| ^
| void
3 errors generated.
Yep, that's dart-lang/native#2482 |
Converts
path_provider_foundation
from a Flutter plugin to a Dart-only package usingffigen
to call into Foundation directly.Most of the Dart unit tests had to be converted to integration tests, although they are still conceptually unit tests (i.e., they still inject mocks for the platform calls). This is because unlike Pigeon, where the API layer being mocked uses Dart types, the FFI calls that we now mock use Obj-C types, and those don't exist outside of an actual app runtime, so the integration test driver is the only place they can be hosted. This is very similar to how most of our web plugin implementations are unit tested.
Fixes flutter/flutter#111033
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3