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

External types #20

Merged
merged 5 commits into from
Jul 23, 2024
Merged

External types #20

merged 5 commits into from
Jul 23, 2024

Conversation

skeet70
Copy link
Member

@skeet70 skeet70 commented Jul 19, 2024

This required small changes to uniffi to thread all the external values we needed for a fully qualified RustBuffer through. Also some big changes to our code to thread config through basically all over the place to support fully qualified external types.

This also prompted that for subdependencies we can't do the uniffi.toml extension trick we've been doing in the tests (see custom_types usage in TestImportedTypes. I wrote up a failed attempt at doing something similar in the branch failed-rewrite-uniffi-toml-dep, but the overwritten values aren't used in the build and we don't have a way to manually pass the overrides down to subdependencies like we are for the top level fixtures with our generate_bindings call. For now I just changed the tests to not expect the nice URL swapping to work, but long term hopefully there's a better way.

skeet70 added 4 commits July 17, 2024 13:35
Will need to PR breaking changes to uniffi-rs if this works, which
likely means running off this fork for a while
Need a test extension to be able to add to the uniffi-toml of an
external crate.
@skeet70 skeet70 requested a review from a team as a code owner July 19, 2024 22:25
@skeet70 skeet70 requested review from coltfred and removed request for a team July 19, 2024 22:25
Copy link

File Coverage Lines
All files 79% 79%
src/gen_java/enum_.rs 50% 50%
src/gen_java/primitives.rs 20% 20%
src/gen_java/variant.rs 50% 50%
src/gen_java/compounds.rs 78% 78%
src/gen_java/mod.rs 89% 89%

Minimum allowed coverage is 0%

Generated by 🐒 cobertura-action against 50a1534

Copy link
Member

@coltfred coltfred left a comment

Choose a reason for hiding this comment

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

Looks like the test failure is the flaky multi-threading test from CI?

I think this all looks good. Threading the config everywhere is kind of 😢, but I don't see a better way.

@skeet70 skeet70 merged commit 95c9560 into main Jul 23, 2024
6 of 7 checks passed
@skeet70 skeet70 deleted the external-types branch July 23, 2024 20:26
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