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

[WIP] Using UniFFi bindgen as a cargo subcommand #1002

Closed
wants to merge 3 commits into from

Conversation

skhamis
Copy link
Contributor

@skhamis skhamis commented Aug 5, 2021

Discussion #472

Taking a crack at making a more robust UniFFi CLI that allows for more flexibility as well as potential new tooling for checking environment to ensure success when building using UniFFi.

Suggestions welcome!

To install: In the bindgen dir -> cargo install --path . will install two binaries uniffi-bindgen and cargo-uniffi

To use:

  • (used today) uniffi-bindgen <commands>
  • (new subcommand) cargo uniffi <commands>

New args:
cargo uniffi check -l [language] will ensure that you have things properly installed
(in progress) cargo uniffi check ? will check scaffolding and udl files are properly setup

Planned changes:

  • Ability to have uniffi-bindgen as a subcommand ex cargo uniffi scaffolding <udl>
  • Adding a check command to check if local environment can succeed in building the foreign language bindings
  • Adding checks to the build process to ensure the udl is correct
  • Check to see if there are not conflicting uniffi-bindgens installed(?)
  • Attempt to integrate with the work in [WIP] split backends into their own crate #997

    The possibility of split back-ends will mean we potentially should look into allowing "plugin-like" format for different bindgen into the core and defer to those bindgens via external subcommands to allow language-specific crates to have their own set of commands

@skhamis skhamis marked this pull request as draft August 5, 2021 02:35
@@ -11,7 +11,7 @@ edition = "2018"
keywords = ["ffi", "bindgen"]

[[bin]]
name = "uniffi-bindgen"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not relevant for prototyping, but longer-term, I expect we may want to keep the existing uniffi-bindgen command in addition to the new one for b/w compat at least for a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually a little more tricky than appears on the surface. The three ways it seems we can get this done is:
(1) in the cargo.toml have

[[bin]]
name = "cargo-uniffi"
path = "src/main.rs"

[[bin]]
name = "uniffi-bindgen"
path = "src/main.rs"

which will produce an warning warning: .../uniffi-rs/uniffi_bindgen/Cargo.toml: file found to be present in multiple build targets: .../uniffi-rs/uniffi_bindgen/src/main.rs

which may not be an issue as we are intentionally doing this...

(2) Have a second main.rs in a separate directory that just calls the mod uniffi_bindgen

(3) Create a shared lib and then have two mains use the same shared library rust-lang/cargo#5930

It seems 3 is the "recommended" solution but open to any ideas!

Copy link
Collaborator

Choose a reason for hiding this comment

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

(3) Create a shared lib and then have two mains use the same shared library rust-lang/cargo#5930

Yeah, I think if I were a new developer coming into the project, this is what I'd expect to see - a shared lib with the functionality and then several small "wrapper" crates that use it to implement a specific command-line tool.

_ => bail!("No command specified; try `--help` for some help."),
}
Ok(())
}

fn gen_comands<'a>() -> ArgMatches<'a> {
const POSSIBLE_LANGUAGES: &[&str] = &["kotlin", "python", "swift", "gecko_js", "ruby"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something to think about is, how might this inter-operate with the work @tarikeshaq is doing to split out the languages in separate crates? Would we end up shelling out to cargo-uniffi-kotlin which then implements its own suite of sub-sub-sub-commands?

(I think it would be fine for us to do that, just thinkin' it through out loud)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm yes that's definitely something that's been in the back of my mind during these meetings -- I think that might be the most straightforward way to implement it. One thought I had is the idea of "plugins" to the uniffi bindgen that allows the commands from other bindgens to be "imported" to the main bindgen. I got inspired from the bevy game engine:
https://bevyengine.org/learn/book/getting-started/plugins/

Of course it wouldn't look like how their doing it but throwing some some wild ideas out there.

@skhamis skhamis force-pushed the uniffi-cargo-subcommands branch from 72cf9b2 to 3ecbc89 Compare August 5, 2021 23:47
@skhamis
Copy link
Contributor Author

skhamis commented Aug 6, 2021

Trying to write some "production-ready" code and thinking about this just a tad bit more. I wonder if it makes sense instead of having a cargo uniffi check command with all different args underneath it. We actually include a different arg called --dry-run that runs within the generate, scaffolding, etc commands which will then check the environment and that everything compiles fine with helpful statements instead of a side check....thoughts @rfk (and of course anyone else)?

@skhamis skhamis requested a review from a team August 6, 2021 21:43
@rfk
Copy link
Collaborator

rfk commented Aug 9, 2021

We actually include a different arg called --dry-run that runs within the generate, scaffolding, etc commands
which will then check the environment and that everything compiles fine with helpful statements instead of a side check

It sounds worth a try! One thing about a separate check command could be that it's in a position to do more expensive checks than you might want in your build process by default (for example, it could shell out to cargo metadata and look at all the crates in your dependency tree in order to check that things look OK, while you might prefer that your day-to-day build process avoid this overhead by assuming everything is properly configured). But, I guess the --dry-run flag could also enable such checks!

@rfk
Copy link
Collaborator

rfk commented Aug 9, 2021

Ability to have uniffi-bindgen as a subcommand ex cargo uniffi scaffolding

So here's an example of something that I think would be a really interesting direction for UniFFI - what if I didn't need to specify the <udl> path explicitly here? That is, what if cargo uniffi scaffolding did the right thing without having to specify any options?

There are a number of ways we could achieve that:

  • Configure the path to the .udl file in Cargo.toml and/or uniffi.toml.
  • Presume some default filename like ./src/interface.udl if one isn't specified.
  • (...eventually get rid of UDL files altogether in favour of macros 😁)

What I'm interested in is the big-picture shift from pointing at an interface file, to pointing at the crate itself.

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Not sure if we will pick this back up or not, but requesting changes to not have it show up as awaiting review.

@jplatte
Copy link
Collaborator

jplatte commented May 31, 2022

Given the progressing work on #1257, it might not sure a whole lot of sense to continue work on improving UDL-related workflows, which are likely going to be replaced entirely by that in a few months.

@badboy
Copy link
Member

badboy commented Apr 4, 2023

the uniffi-bindgen binary is since gone, so this would need some rethinking. I think the best for now is to close it.

@badboy badboy closed this Apr 4, 2023
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.

5 participants