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

To replace *.udl with use of Uniffi proc macro initially #261

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Binlogo
Copy link

@Binlogo Binlogo commented Jul 18, 2024

Try to fix #131.

Progress of samples.

  • hello_world
  • simple_counter

Known issue tracking:

simple_counter

  • uniffi-bindgen using a library file failed.
    • uniffi-bindgen generate --library target/release/libshared.dylib --language swift --out-dir generate/swift/SharedFF
      • Error: cargo metadata returned 2 packages for crate name shared
      • Potential reason: conflict because following crate conflict.
        • "crate_types":["lib","staticlib","cdylib"]
        • "crate_types":["bin"]
      • Potential solution:
  • Still need a build phrase for Xcode project or just generate bindings in advance like SharedTypes package?.
    • Include SharedFFI as sources

@Binlogo Binlogo force-pushed the uniffi-proc-macro branch 4 times, most recently from 110c049 to ac61a56 Compare July 18, 2024 14:51
@Binlogo Binlogo marked this pull request as ready for review July 18, 2024 14:55
@Binlogo Binlogo changed the title To replace *.udl with use of Uniffi proc macro To replace *.udl with use of Uniffi proc macro initially Jul 18, 2024
@Binlogo
Copy link
Author

Binlogo commented Jul 18, 2024

@StuartHarris Hi, if you're interested, please review this PR. Any concerns or suggestion, just comment. If this solution works for us, we can provide or update more examples using this procedural macro approach.

@StuartHarris
Copy link
Member

Hey @Binlogo — thank you for this! I love the direction it's heading in. I looked through the diff and got quite excited. 🎉 I will dig in deeper when I get a chance over the next couple of days. 🤗

Comment on lines 5 to 7
#[cfg(feature = "typegen")]
#[test]
fn typegen() -> anyhow::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this change - is the foreign code type generation now done in a test? What's the thinking behind this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. I learned this pattern from tokio-console.

The motivation for this change in this PR was to address the following issue as mentioned in description:

  • uniffi-bindgen using a library file failed.
    • uniffi-bindgen generate --library target/release/libshared.dylib --language swift --out-dir generate/swift/SharedFF
      • Error: cargo metadata returned 2 packages for crate name shared
      • Potential reason: conflict because following crate conflict.
        • "crate_types":["lib","staticlib","cdylib"]
        • "crate_types":["bin"]
      • Potential solution:

I thought the issue with "2 packages for crate name shared" is stemmed from shared_types -> shared, so I tried this approach, and removed "shared_types" crate and the dependent relationship.

This method seems simpler for type generation than relying on another crate's build.rs, IMO. So I retained this change there. If there are any concerns, I can revert to previous method, as this is not the main issue in this PR. We can discuss further in a new thread then.

Copy link
Collaborator

@obmarg obmarg Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used this pattern before but everywhere else I've seen it has a couple of differences from this implementation:

  1. They commit the generated files. This means you don't need to run some unit tests before parts of the repository will ever build. I forget where we landed on that in crux, but a quick look at the examples suggests we don't do this. (Someone correct me if I'm wrong)
  2. They cause the test to fail if the generated files needed an update - this helps prevent the committed version of the generated files from getting out of date.

When you do it that way I've found it works quite well. I think it'd be quite awkward otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the direction of eliminating the shared_types crate if we can, even though I'm not sure why it's necessary in order to eliminat the udl files - the uniffi interface and the generated types are quite independent.

I'm not convinced test runner is the way to do it though. It's not clear to me whether tokio-console does the bootstraping in tests to verify the files it needs (so cargo test becomes a kind of readiness self-test), or whether it is genuinely just how you set it up, but for the crux type generation, which is a mandatory step in building the core for use from a "foreign" language, this feels really counter intuitive, however...

What we could do to a similar effect is add a bin/generate_types.rs to the shared create, which does the job that this test does? I think that bin target could then be configured to enable the typegen feature...?

Copy link
Author

@Binlogo Binlogo Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's not necessary related to eliminate the 'udl' actually. The true cause is related with:

It seems that this mozilla/uniffi-rs#2175 solved this issue.

Because it simplify the 'shared_types' crate, so I kept the change. As you suggest, it's a better way to add a bin/generate_types to do it, I'll update to this direction.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charypar I've migrated the typegen tests into 'bin/generate-types'. Please take a look.

@charypar
Copy link
Member

Thanks a lot for having a go at this, it looks like it will simplify the UniFFI setup a fair bit!

@Binlogo Binlogo requested a review from charypar July 24, 2024 11:22
@Binlogo Binlogo requested a review from obmarg July 24, 2024 12:54
@obmarg obmarg removed their request for review July 24, 2024 13:03
Copy link
Member

@charypar charypar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the direction this is going in - will need thorough testing and documentation, but let's get these basic couple examples to a good state and worry about that then.

Comment on lines 2 to 3
uniffi-gen-swift = "run --bin uniffi-bindgen generate --library target/release/libshared.dylib --language swift --out-dir shared/generated/swift/SharedFFI"
uniffi-gen-java = "run --bin uniffi-bindgen generate --library target/release/libshared.dylib --language kotlin --out-dir shared/generated/java"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an equivalent for the wasm version? Mostly a question for @StuartHarris I suppose, I don't 100% remember how that stage of the build process works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced either of these 2 lines is relevant now, as there is no .udl file anymore

workingDir "../../"
if (System.getProperty('os.name').toLowerCase().contains('windows')) {
commandLine("cmd", "/c",
"cargo build -p shared && " + "target\\debug\\uniffi-bindgen generate shared\\src\\shared.udl " + "--language kotlin " + "--out-dir " + outDir.replace('/', '\\'))
"cargo build -p shared && " + "target\\debug\\uniffi-bindgen generate --library target\\debug\\libshared.dylib " + "--language kotlin " + "--out-dir " + outDir.replace('/', '\\'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have misunderstood what the additional lines in cargo config do - I thought they add helper commands for this step, which we could then use in build scripts like this? Is that not the case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nice catch. We could utilize the alias config here too.

ENABLE_USER_SCRIPT_SANDBOXING: NO
buildRules:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I removed buildRules for ".udl" since the ".udl" file gone. It dependent on runining generate-swift first. I'm not familiar with xcodegen now. I'll investigate how to add a build phase.

@Binlogo Binlogo force-pushed the uniffi-proc-macro branch 2 times, most recently from cf4025a to b055ea3 Compare July 25, 2024 05:30
@Binlogo
Copy link
Author

Binlogo commented Jul 26, 2024

Additional notes: To replace *.udl with the Uniffi proc macro for generating bindings, we still rely on an intermediate product, '--library target/release/libshared.{dylib, so}'. This process is not as straightforward as I initially thought. We may need to reconsider the project structure.

  • Should the generation of all bindings be driven by the iOS/Android side?
  • Should the binary and bindings be combined into a single task?
    A deeper investigation is required.

@charypar
Copy link
Member

Indeed, there are fundamentally three parts we need to produce from the shared crate:

  • The library target built by cargo which the shell links to (.dylib/.so/.wasm)
  • The uniffi bindings generated by the uniffi-bindgen command for each language to surface the base core interface
  • The generated types from serde-generate to get equivalents of Rust types which cross the FFI boundary, which can serialize and deserialize across the languages

I think the latter two steps can probably be combined into a single step. It may even be possible to make them a single tool - we may be able to import and call some of the code the uniffi-bindgen tool runs in the same bin target as the one running the serde type generation and run both in one go. That may even allow us to target a specific language.

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.

Investigate replacing *.udl with use of Uniffi proc macro
4 participants