chinedufn / swift-bridge

swift-bridge facilitates Rust and Swift interop.
https://chinedufn.github.io/swift-bridge
Apache License 2.0
842 stars 62 forks source link

Add parse-bridges CLI subcommand #274

Closed integer-overflown closed 5 months ago

integer-overflown commented 6 months ago

What's this?

A new parse-bridges subcommand. Also, an update to the documentation in the example showing this feature's preview.

This is handy for usage in higher-level build systems. For example, in my case, I build both Rust and Swift with CMake, and having this CLI command implemented would allow generating the glue code from CMake as well, as a dependency step before building Rust/Swift targets (via add_custom_target API).

Notes

One caveat of the current implementation is that one would have to duplicate the crate name in the invocation line (the first argument).

This is fine for cases like mine, where this would be taken from the build system anyway, but it may not be handy for other cases.

The package name can be automatically deduced if one's running in the correct directory (the package root). In this case, we can parse the cargo read-manifest output and get the name from there.

This would require a new dependency, though (serde_json), so I decided not to do this just yet, but if this sounds okay to you, I'll go ahead and implement this as well.

Examples

Single file:

swift-bridge-cli parse-bridges --crate-name my-awesome-crate -f src/lib.rs -o generated

Multiple files:

swift-bridge-cli parse-bridges --crate-name my-superb-crate \
-f src/lib.rs \
-f src/some_other_file.rs \
-o generated
integer-overflown commented 5 months ago

Thanks for reviewing.

I'm not understanding what you mean here.

Inferring crate name from the manifest if a user is located in the package root.

Currently no known use cases where people would be repeatedly typing out this command by hand.

So, let's hold off on simplifying it and prefer explicit/obvious behavior for now.

I agree; this would be less error-prone and more explicit. As you also mentioned, this command will likely be written once, and then integrated into a script or build-system rules.

Please update your PR body with an example of how the command looks.

Good point; done, see the examples section.

chinedufn commented 5 months ago

Thanks for putting this together. Can land once you:

integer-overflown commented 5 months ago

@chinedufn done, pushed the changes :)

integer-overflown commented 5 months ago

@chinedufn I've pushed another commit that fixes the tests.

At least, on my side, cargo test --all passes successfully.

I'm not sure how my changes broke the UI tests. There was probably some update on the CI runner side, which made the compiler more verbose and caused the test to fail.

I've put an explainer in the commit message.

Hopefully, it passes now 😅

NiwakaDev commented 5 months ago

I'm not sure how my changes broke the UI tests.

@integer-overflown

You need to address https://github.com/chinedufn/swift-bridge/pull/276#discussion_r1637913279.

chinedufn commented 5 months ago

Congrats on landing your first contribution. Thank you for pushing this to completion. Good stuff.