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

Feat/uniffi implementation #12

Merged
merged 21 commits into from
Aug 17, 2024
Merged

Feat/uniffi implementation #12

merged 21 commits into from
Aug 17, 2024

Conversation

Mydayyy
Copy link
Owner

@Mydayyy Mydayyy commented Apr 24, 2024

ToDo:

  • Implement basic tests for uniffi (Here)
  • Make uniffi in crate optional via a feature flag
  • Update readme explanation + credits
  • Update CI to generate and test bindings

@Mydayyy Mydayyy force-pushed the feat/uniffi-implementation branch 2 times, most recently from a4685f3 to 4da2f6c Compare April 27, 2024 22:41
@Mydayyy
Copy link
Owner Author

Mydayyy commented Apr 30, 2024

Appears to be a little more complicated than I initially figured due to mozilla/uniffi-rs#2000

@rafaelbeckel
Copy link

@Mydayyy I created a PR to support conditionally using uniffi mozilla/uniffi-rs#2086

Feel free to temporarily import it until they merge it upstream.

@Mydayyy
Copy link
Owner Author

Mydayyy commented May 1, 2024

Cheers, I'll check it out! Thanks

@Mydayyy Mydayyy force-pushed the feat/uniffi-implementation branch from c3a9b35 to a5f082b Compare July 24, 2024 22:02
@Mydayyy
Copy link
Owner Author

Mydayyy commented Jul 24, 2024

Thanks to the awesome work of the uniffi people, conditional constructors are now merged to main and released in 0.28, so this should be unblocked and I will continue to iterate on it

@Mydayyy Mydayyy force-pushed the feat/uniffi-implementation branch from a5f082b to 5303e93 Compare July 25, 2024 19:29
@Mydayyy
Copy link
Owner Author

Mydayyy commented Aug 9, 2024

Greetings @nain-F49FF806,

I hope you've been well. I did some testing with the implementation of the uniffi bindings in this branch and I managed to compile the sharepaste app with a small change. Since the uniffi-bindings would now be opt-in, one has to enable a feature flag for the compilation:

  • extraCargoBuildArguments = ["--features", "uniffi"]
    

(As seen in the fork here: Mydayyy/sharepaste.oo-uniffi-test@f8a5841)

I am not sure if there is a better way, I did not find any inside the documentation of cargondk. Because with that, the feature flag is technically enabled for all libraries that cargondk builds (in this case libfoobar as well). Very interested to get feedback on this from a more seasoned android dev.

Regarding the tests:
Due to a limitation I discovered, the tests are not possible right now, so I would move them a PR in the future. In order to find the cdylib and name of the crate, uniffi rebuilds the library during the testing process which omits the feature flag I pass for the test. Therefore the test will fail since the build does not have uniffi enabled anymore. (Seen here: https://github.com/mozilla/uniffi-rs/blob/b97973dbbaa628db763dfb016bea5a1273b7f371/uniffi_testing/src/lib.rs#L160)

@nain-F49FF806
Copy link
Contributor

Greetings,

Thanks for adding conditional activation for the uniffi bindings code. And for trying out the changes made here with sharepaste app! I have tried your fork locally, and it is indeed working. The app does seem to run properly on my device.

I am not sure if there is a better way, I did not find any inside the documentation of cargondk. Because with that, the feature flag is technically enabled for all libraries that cargondk builds (in this case libfoobar as well). Very interested to get feedback on this from a more seasoned android dev.

Yes. That is currently a limitation of cargo-ndk-android-gradle. I do not see a likelihood of more fine-grained options being implemented anytime soon, as it seems to not be actively maintained now.

There is a workaround / better way (not sure if it's the best way), but it's a bit more involved. I am considering moving to a one rust library per android module structure [1]. With each module having a separate build.gradle script, each library can have a separate cargoNDK config. With that, there shouldn't be a problem going forward. We could customize and fine-tune options for each library separately.

For now, the solution you have used I feel is apt. Also, the foobar lib doesn't really do anything in the app, so it certainly won't cause any issues for now.

@nain-F49FF806
Copy link
Contributor

nain-F49FF806 commented Aug 12, 2024

Regarding running the tests, I did a search, and it looks someone else have also tripped on a similar looking issue. Do you think it's the same issue? They don't seem to have taken the deep dive and discovered the root cause like you have here (nice finding :), but the linked discussion seems to have a workaround.

In any case, having the uniffi feature in the main branch (behind feature flag) would be useful.
It can be noted in the feature flag documentation that it's currently lacking thorough tests.

@Mydayyy
Copy link
Owner Author

Mydayyy commented Aug 13, 2024

Thank you for the feedback! I will proceed to merge this during the weekend!

Do you think it's the same issue?

It very much appears so, even though the initial issue is a bit light on information. But the workaround they ended up with is actually something I tried as well to trace the issue down and it would fix it. I did decide against that though because it would be an annoyance to keep editing the Cargo.toml and modify the default features every time.

@Mydayyy Mydayyy merged commit d4822d3 into master Aug 17, 2024
1 check passed
@Mydayyy Mydayyy deleted the feat/uniffi-implementation branch August 17, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants