application-services icon indicating copy to clipboard operation
application-services copied to clipboard

Uniffi library mode [ci full]

Open bendk opened this issue 2 years ago • 8 comments

This branch is testing out the new UniFFI library mode system and the results seem pretty good:

  • The uniffi.toml files get simpler (although I'm not totally sure about the swift removal, since there's still some more work to do)
  • It facilitates the transition from UDL -> proc-macros
  • I think it makes it easier to use different megazords for focus vs firefox

There's still some work TODO:

  • [x] The swift code needs some more work. I ran the parts of taskcluster/scripts/build-and-test-swift.py that I could and the results were identical. But there are some parts that need an Mac box, for example megazords/ios-rust/build-xcframework.sh). I'm running full CI now, it'll be interesting to see what errors come back.
  • [x] We need to land a few PRs on UniFFI and maybe do a release.
  • [x] The gradle dependencies aren't totally right. I was trying to run test and it failed because it depends on the release build of the megazord.

This part is speculative, but I think this could be taken further in the future. We could remove the wrapper code from each component making them closer to vanilla Rust crates, no need for the android and swift directories. The android/swift parts could be all in a single place that ran UniFFI library mode for a megazord and generated bindings for all crates that it depended on. Removing the wrapper code means refactoring the Glean metrics, removing a backward compatibility layer, and adding UniFFI features like async callbacks so that we can express the API we want to export without needing an extra wrapper. That's a lot of work, but many of those things come with their own motivation. I know @tarikeshaq has a plan for reworking the Glean code, maybe it could be compatible with this dream. The other changes would be nice for our documentation story -- as long as there's a wrapper layer, our Rust documentation won't accurately describe the consumer API.

Pull Request checklist

  • [ ] Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • [ ] Tests: This PR includes thorough tests or an explanation of why it does not
  • [ ] Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • [ ] Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

bendk avatar May 20 '23 14:05 bendk

Codecov Report

Patch and project coverage have no change.

Comparison is base (fad43b7) 89.86% compared to head (2603ef6) 89.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5592   +/-   ##
=======================================
  Coverage   89.86%   89.86%           
=======================================
  Files           1        1           
  Lines         148      148           
=======================================
  Hits          133      133           
  Misses         15       15           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar May 20 '23 14:05 codecov-commenter

For interest I had a poke at seeing if this works with the current uniffi version and it does :) There's a trivial fix to sync-manager importing DeviceType twice, and running the tests locally on a Mac failed due to publish.gradle failing a find libmegazord.so (it's .dylib) - and I failed to quickly google the best way to handle that elegantly, but hard-coding .dylib gets local tests passing.

mhammond avatar Jul 14 '23 19:07 mhammond

I'm pushing an updated version of this that:

  • Fixes the merge conflicts
  • Removes the commit that was testing if we can use proc-macros. I'd rather switch over fully to proc-macros than just do one function. Maybe we can test that out after merging this
  • Has a blind fix for the libmegazord filename issue, let's see if it passes CI.

bendk avatar Jul 21 '23 14:07 bendk

I think this is getting pretty close to ready. @jeddai Do the changes to run_python_tests.sh look good to you? I'm actually a bit jealous of the cirrus situation, you're the first megazord where we can generate all the bindings with 1 command.

bendk avatar Jul 24 '23 17:07 bendk

Do the changes to run_python_tests.sh look good to you?

Yep! I checked the python test circleci job logs and they look good.

I'm actually a bit jealous of the cirrus situation, you're the first megazord where we can generate all the bindings with 1 command.

Oh I didn't realize that, that's awesome

jeddai avatar Jul 26 '23 14:07 jeddai

@mhammond what do you think about merging this one to main, maybe after 117 starts?

bendk avatar Jul 26 '23 15:07 bendk

Happy for this to land whenever you feel it's ready!

mhammond avatar Jul 26 '23 18:07 mhammond

Maybe we can try to land https://github.com/mozilla/application-services/pull/5704 and use that as a confidence builder as well?

+1 to this, I think that will go a long way.

bendk avatar Aug 04 '23 16:08 bendk