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

Make uniffi_build optional in uniffi_macros #2019

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

mgeisler
Copy link
Contributor

@mgeisler mgeisler commented Mar 8, 2024

As far as I can see, the dependency on uniffi_build is not needed for the macros used by regular clients of UniFFI — it is only used to create a convenience macro for the UI tests.

I named the new feature “trybuild” after the description of the macro. Please let me know if you have preferences for a better name.

The overall goal here is to minimize the number of crates I need to vendor to use UniFFI — I’m hoping to get away with checking in the generated target language sources, so I’m currently focusing on the dependencies in the rest of the code base.

@mgeisler mgeisler requested a review from a team as a code owner March 8, 2024 16:02
@mgeisler mgeisler requested review from mhammond and removed request for a team March 8, 2024 16:02
@mgeisler mgeisler force-pushed the remove-uniffi-build-dep branch from 03acd43 to 5b9956b Compare March 8, 2024 16:16
@mhammond
Copy link
Member

mhammond commented Mar 8, 2024

This seems fine to me once the conflicts and CI failures are dealt with.

@mgeisler mgeisler force-pushed the remove-uniffi-build-dep branch from 5b9956b to a7ace94 Compare March 11, 2024 12:57
@mgeisler
Copy link
Contributor Author

This seems fine to me once the conflicts and CI failures are dealt with.

Thanks a lot!

I've actually been struggling to get cargo test to run cleanly locally (in general, not just for this PR), so I've been relying on the CI tests to catch mistakes.

Here, the problem seems to be a Serde dependency which doens't enable the derive feature (despite deriving Serialize and Deserialize a lot). I've fixed it now in the commit and updated the commit message — please let me know if you would prefer me to split the commit into two.

@mgeisler mgeisler force-pushed the remove-uniffi-build-dep branch from a7ace94 to 7ba0ad3 Compare March 11, 2024 13:10
The code built without this, but that seems to because of an accident
since the derive feature is not a default serde feature.

I noticed this when trying to make uniffi_build an optional
dependency of uniffi_macros — for some reason I don’t understand, this
would make `cargo build -p uniffi_macros` fail wtih:

    error: cannot find derive macro `Deserialize` in this scope
      --> uniffi_macros/src/util.rs:29:14
       |
    29 |     #[derive(Deserialize)]
       |              ^^^^^^^^^^^
       |

The error makes sense since the derive feature wasn’t enabled. What
does not make sense to me is that the error wasn’t triggered before.
As far as I can see, the dependency on uniffi_build is not needed for
the macros used by regular clients of UniFFI — it is only used to
create a convenience macro for the UI tests.

I named the new feature “trybuild” after the description of the macro.
Please let me know if you have preferences for a better name.

The overall goal here is to minimize the number of crates I need to
vendor to use UniFFI — I’m hoping to get away with checking in the
generated target language sources, so I’m currently focusing on the
dependencies in the rest of the code base.
@mgeisler mgeisler force-pushed the remove-uniffi-build-dep branch from 7ba0ad3 to 0a5a868 Compare March 11, 2024 13:14
@mhammond mhammond merged commit 075efd9 into mozilla:main Mar 11, 2024
5 checks passed
@mgeisler
Copy link
Contributor Author

Thanks for the quick review!

I've actually been struggling to get cargo test to run cleanly locally (in general, not just for this PR), so I've been relying on the CI tests to catch mistakes.

To follow up on this, I asked in the chat room and got a reply super fast — basically, I have a too old Kotlin compiler installed (version 1.3 from Debian testing). The current version is 1.9 and Kotlin 1.3 lacks (at least!) support for trailing commas in several places.

So I now simply ignore the Kotlin tests with

UNIFFI_TESTS_DISABLE_EXTENSIONS=kts cargo test

and this works!

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.

2 participants