fzyzcjy / flutter_rust_bridge

Flutter/Dart <-> Rust binding generator, feature-rich, but seamless and simple.
https://fzyzcjy.github.io/flutter_rust_bridge/
MIT License
3.64k stars 256 forks source link

Add a "fat" option for codegen #947

Closed GregoryConrad closed 6 months ago

GregoryConrad commented 1 year ago

The problem

The issue presented here is still only theoretical with no actual issues caused by it (AFAIK), but I imagine it could present substantial issues in the near future.

At the moment, projects/libraries using FRB need to use the same FRB version amongst Dart and Rust, or else the support libraries might not match up properly. While this makes sense, this can present issues.

Take the following configuration:

# A's pubspec.yaml
dependencies:
  b: ^1.0.0
  c: ^1.0.0

# B's pubspec.yaml
dependencies:
  flutter_rust_bridge: "1.58.0"

# C's pubspec.yaml
dependencies:
  flutter_rust_bridge: "1.59.0"

Note how the Dart/Rust libraries (B/C) need an exact version of FRB; this is so the support library versions will match up correctly. Otherwise, the project may fail to work correctly, or not even compile.

While using an exact version is the correct fix from the library's perspective, it will create a problem for the end user that relies upon 2+ FRB libraries. Obviously, 1.58.0 and 1.59.0 are not compatible, and Dart will fail to resolve dependencies, or perhaps may force one library to be constrained to an old version (if it can even find one).

Thus, I am proposing a new feature: a "fat" option in FRB itself.

Proposed solution

Add a new option, I will call it "fat", to codegen that builds "fat" Dart files. These "fat" Dart files bundle the necessary FRB support code alongside the generated FFI interop code in the same Dart files. This would remove the Dart FRB package dependency, fixing the problem altogether. In the future, once/if this solution is implemented, it also might be a good idea to discontinue the Dart package altogether and simply force the use of this "fat" option.

fzyzcjy commented 1 year ago

Good idea! Just one nit: We should not lose the advantage of a normal package (the frb_dart for example) - For example, frb_dart as a package can be linted, easily unit tested, etc. Thus, maybe we can still make frb_dart a package (just like currently), and copy the content of the package into the correct location during codegen (when enabling fat).

GregoryConrad commented 1 year ago

Was looking into this a bit more (briefly), because this issue should be addressed sooner rather than later with the amount of people that have been following the "make a package/library" part of the guide. If they publish to pub.dev, chances are some user somewhere will run into this issue eventually.

Looks like frb_dart is a bit heavy-weight at the moment. Thinking the best approach may be to split up frb_dart into a (super) simple folder frb_dart_ffi (Dart package name perhaps flutter_rust_bridge_ffi) that just contains the support functions necessary for ffi interop that will be needed by libraries using FRB. Then, we could keep all the other stuff in the current frb_dart folder for current application users. frb_dart would just export the frb_dart_ffi stuff.

I suggest we implement frb_dart_ffi in just a single file under lib/ so that code gen can just copy the file contents over directly without any other worries. Yes, this is bad practice in general, but here it makes sense because otherwise we would need to parse through import/export statements in code gen and that is highly error prone (with all the variability in the statements that Dart allows and may allow in the future).

fzyzcjy commented 1 year ago

that just contains the support functions necessary for ffi interop that will be needed by libraries using FRB

So, will it be small enough? Not checked into details but I am afraid a big portion of code is mandatory...

Another question is, shall we consider the same style on Rust? i.e. someone may use a older version of Rust frb while another one uses a new version

GregoryConrad commented 1 year ago

So, will it be small enough?

Yes, because I worry about maintainability if we move everything to one source file. Thus, the less we move, the better.

I am afraid a big portion of code is mandatory…

Ahh that is a bit unfortunate 😅😅

I did notice some code that was for dart examples and some dependencies in the pubspec that were only for integration tests. Perhaps some of those could remain separate.

Another question is, shall we consider the same style on Rust? i.e. someone may use a older version of Rust frb while another one uses a new version

Had thought about that too when I opened the issue, but actually a non issue as far as I can tell. The rust binaries are compiled before ever touching dart, so it’s easy to ship 2+ frb libraries in the same app from the rust perspective, since each will have its own frb support library compiled in.

Only issue I could possibly see is duplicate symbols amongst the two compiled library files (like .so or .a). However, not an issue in Dart today due to Dynamic Loading. Maybe it could be an issue if static linking when Native Assets come around, but honestly I think Dart should be in charge of ensuring that doesn’t happen. In any case, I don’t know enough about how those compiled binary files work. Maybe when rust compiles them down, rust only keeps a certain set of global symbols? I’ve got no clue. (I.e. is every function from source code present and named globally in the file, or only the ones declared pub extern “C”, or something else entirely?)

fzyzcjy commented 1 year ago

Only issue I could possibly see is duplicate symbols amongst the two compiled library files (like .so or .a). However, not an issue in Dart today due to Dynamic Loading.

Well, iOS uses static linking IIRC (at least for apps)

GregoryConrad commented 1 year ago

Well, iOS uses static linking IIRC (at least for apps)

You’re right; it does. And so does macOS, at least for the package/library setup steps in the docs

I also don’t think the symbol names would be an issue, but just were something that crossed my mind. I really should learn how those library files work though regardless…

fzyzcjy commented 1 year ago

I also don’t think the symbol names would be an issue, but just were something that crossed my mind

IIRC this seems to have been discussed before (maybe search through issue section), though I am not very sure. Symbols can be a problem indeed.

maybe related: https://github.com/fzyzcjy/flutter_rust_bridge/pull/1024#discussion_r1102723983

cc @dbsxdbsx - who created the multi-file feature (thus also faces the symbol conflict problem)

dbsxdbsx commented 1 year ago

Actually, it occurs to me that sometimes there are different versions of frbs in dart(pubspec.yaml) and rust(cargo.toml) and the frb generation tool. Though, the whole project would be compilable. I do hope there is some mechanism that could inform me to make frb consistent.

fzyzcjy commented 1 year ago

I do hope there is some mechanism that could inform me to make frb consistent.

That looks like another issue/PR. Sure! I guess one way is: When we use frb_codegen version A to generate code, the generated_bridge.rs should has a compile-time assertion saying frb_rust's version should be A, and similar for the generated_bridge.dart. As for how to know the version, maybe we can put it as a constant inside frb_rust/frb_dart, and use CI/justfile to verify/update it.

GregoryConrad commented 1 year ago

I'm going to throw together a quick example that tests using two statically linked libraries under iOS, but I am guessing it will fail since the generated C output from FRB would have duplicate symbols.

If that is the case, I'll make a quick comment in the Native Asset issue to ask for support for Dynamic Linking on all platforms for when Native Assets come out since that could be a pretty common issue. iOS8+ support dynamic linking, so there is no issue with supported iOS versions in Flutter.

Related:

GregoryConrad commented 1 year ago

Somewhat surprisingly, the static linking in iOS worked. Something must be done somewhere in the build process to prevent name collisions, perhaps related to the XCFramework. I duplicated my library, changing the Dart name (but all Rust functions remained the same) so that the Dart tool would be happy, and tried running an example app. Both instances of the library worked together at the same time despite using the same symbols in the library files.

GregoryConrad commented 1 year ago

~So at least for the case of depending on different packages using FRB, name collisions aren't an issue.~ Edit: this is incorrect, see https://github.com/fzyzcjy/flutter_rust_bridge/issues/947#issuecomment-1437697037

fzyzcjy commented 1 year ago

Somewhat surprisingly, the static linking in iOS worked. Something must be done somewhere in the build process to prevent name collisions, perhaps related to the XCFramework.

I am worried about an even more terrible hidden reason: iOS just picks one and throw away the other when the name conflicts. IIRC I have read that behavior somewhere. You know, this is really unacceptable and can ruin whole code.

I guess we can check this by the following: Create two functions of the same symbol name and different content (say they print "1" and "2" respectively). Then we call them, and see what happens.

Similarly, consider this:

Then we call fn_1 and fn_2 respectively and see what happens. If the terrible case above happens, we will see both function result in common in 1 (or the mirroring case)

GregoryConrad commented 1 year ago

iOS just picks one and throw away the other when the name conflicts.

Good catch! I'll run a quick experiment and see what happens.

GregoryConrad commented 1 year ago

@fzyzcjy You are in right, iOS discards duplicate symbols! I'll go make a comment over on the Native Assets issue to ask if they can support Dynamic Loading on all platforms to circumvent this issue.

GregoryConrad commented 1 year ago

For future reference, here is the (hackily thrown together) test I used: https://drive.google.com/file/d/10dNYNiFYddMb6dNrWjRosmCv0JTqitEI/view?usp=sharing No guarantees that link will work in the future, as I may delete it some months down the road. Figured it would be good to share now though for reference.

fzyzcjy commented 1 year ago

You are in right, iOS discards duplicate symbols!

Ah... Then that's pretty bad news, and we should be very careful

No guarantees that link will work in the future, as I may delete it some months down the road. Figured it would be good to share now though for reference.

Then what about making it in a github public repository ;)

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

GregoryConrad commented 1 year ago

Bump

stale[bot] commented 10 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

GregoryConrad commented 10 months ago

bump

stale[bot] commented 8 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 6 months ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new issue.