Add ruby adapter #436

saks · 2021-05-06T05:56:26Z

Hey!

First of all, thanks for your hard work on this project. I really like the idea and the code quality that it produces for mobile platforms.

I was wondering if I can use uniffi-rs to generate ruby library in addition to swift and kotlin. While experimenting I have reached the point where the generated ruby code is good enough as a direct replacement to my FFI layer that was written by hand.

This adapter is by no mean on par with all features but it does provide basic integration that can be useful for starters. I happy to continue improving and maintaining this code.

FYI: this PR is heavily influenced and based on python integration.

Any feedback is very much appreciated!

saks · 2021-05-06T05:58:22Z

.circleci/config.yml

@@ -47,6 +47,12 @@ jobs:
          name: "Set RUSTFLAGS to fail the build if there are warnings"
          command: echo 'export RUSTFLAGS="-D warnings"' >> $BASH_ENV
      - checkout
+      - run:
+          name: "Install ruby"


This step can certainly be added to the docker image if necessary

Yep, I'd be happy to add it to the docker image, and that would make each individual test run go a little faster.

Would you mind preparing a separate PR that adds this to the docker image? I can land and publish that change first and then we can use it for CI on the rest of this PR.

Sure thing! (#461)

Great; I've pushed the updated docker image, so in theory we can remove this from the CircleCI config and everything should still run and pass 🤞🏻

Done 👍

FYI: I've noticed that docker image is built with rust 1.50.0 but then we install a different version based on rust-toolchain.toml (1.47.0 at the moment). I've been following why this repo uses a specific rust version, but I guess we can cut a bit of time from each test if we build docker image with rust 1.50.0. I can prepare a PR if this is useful :)

FYI: I've noticed that docker image is built with rust 1.50.0 but then we install a different version based on
rust-toolchain.toml (1.47.0 at the moment).

Huh, so we do! I guess the scripting for the docker image there may pre-date our decision to pin to an earlier Rust. If you have a moment to prep a PR, that would be helpful, please do.

hey @rfk, #463

mhammond

This looks good to me with a quick glance by someone who has never written Ruby :) It does raise a good question re CI and the level of support - I'm not sure that it should be a hard requirement that we never break the Ruby bindings. Or if we consider a future where more bindings are supported, there certainly will be some point where we need to declare that some bindings are "tier 1" and others are lower.

I'd welcome any thoughts on that.

Note that for various reasons, there might not be much movement on this PR for a week or few, but don't take that as a lack of interest.

mhammond · 2021-05-07T05:04:22Z

uniffi_bindgen/src/bindings/ruby/gen_ruby.rs

+
+// Some config options for it the caller wants to customize the generated ruby.
+// Note that this can only be used to control details of the ruby *that do not affect the underlying component*,
+// sine the details of the underlying component are entirely determined by the `ComponentInterface`.


typo - since

mhammond · 2021-05-08T13:12:53Z

Might also be worth adding support for the new "coverall" fixture (#431) - there are significant changes coming (see the other various open PRs) which will cause some churn here, so getting base-level support in early might help you later :)

saks · 2021-05-08T19:13:06Z

Thanks for pointing that out @mhammond! Done 👍
I'm totally fine further changes based on upcoming PRs, no worries.

I'm not sure that it should be a hard requirement that we never break the Ruby bindings

I certainly agree, please let me know if you have an idea on how to organize CI so that failed ruby tests will be easy to detect but these won't fail entire CI run.

rfk · 2021-05-18T07:02:04Z

.circleci/config.yml

@@ -47,6 +47,12 @@ jobs:
          name: "Set RUSTFLAGS to fail the build if there are warnings"
          command: echo 'export RUSTFLAGS="-D warnings"' >> $BASH_ENV
      - checkout
+      - run:
+          name: "Install ruby"


Yep, I'd be happy to add it to the docker image, and that would make each individual test run go a little faster.

rfk · 2021-05-18T07:04:25Z

uniffi_bindgen/src/bindings/ruby/gen_ruby.rs

+
+fn is_reserved_word(word: &str) -> bool {
+    RESERVED_WORDS.contains(&word)
+}


Ah yes, we've put off dealing with reserved words for far too long. We might be able to find a way to push some of the logic for handling this down into shared code.

We might be able to find a way to push some of the logic for handling this down into shared code.

@rfk, sounds good. Do you think we should look into that as part of this PR?

Do you think we should look into that as part of this PR?

No, unless you have any good ideas you'd like to explore. I think it's fine to start with figuring out how to handle it here in a specific language backend, and evolve the approach as we understand it better.

Sounds good @rfk, I'd prefer to avoid scope creep and deal with one issue at the time. I'm open to look at it later as a separate PR.

saks · 2021-05-21T22:25:52Z

hey @rfk! Is there anything that I can do to facilitate code review of this PR?

rfk · 2021-05-24T00:15:02Z

Is there anything that I can do to facilitate code review of this PR?

@saks hey, thanks for asking - nothing specific yet, I have it on my stack to look at in detail this week.

One thing that's on my mind is how this will interact with some changes to the way we handle object references, which I'm hoping to also start landing this week (ref ADR-0004 and the proposed ADR-0005). These will mean that the handlemap-related parts of the Ruby backend will need to change, and I'm thinking it may be worth holding off on merging this until that work is complete, and asking you to rebase this PR on top of those changes. (I'll cross-reference the active PRs for that work from here when they're ready to be consumed).

Of course, I can still review the PR in its current state in order to give broader feedback, pending those changes.

Does that sound OK?

saks · 2021-05-24T17:37:51Z

hey @rfk, thanks for getting back to me. I'm glad to see how UniFFI is evolving and I'm looking forward to new opportunities that both ADR-0004 and ADR-0005 would enable.

As for the ruby adapter, the code was prepared and tested with the current UniFFI architecture. Landing this PR before implementing upcoming changes can be beneficial since I can start migrating my projects from hand-written FFI layers into UniFFI and unblock implementation of new functionality.

At the same time I'm sympathetic to development plans for UniFFI and respect decisions made according to it. I'm happy to see what changes are going to be necessary to make ruby adapter work with new pointers handling and thread safety rules.

Thanks!

rfk

With the very large caveat that I don't really know much Ruby, let alone idiomatic Ruby, this looks solid to me 👍🏻. I'm really happy to see that you went through and made Ruby versions of all the tests. Thanks again for your interest and for diving in here!

I think we can merge this, but I want to reinforce again that we'll have to treat it as a "best effort" maintenance situation. We'll try to keep it running while we work on new features for UniFFI, but if we need to get a new feature released and we can't figure out how to make it work for the Ruby backend, then we won't be able to block the release on getting it working in Ruby. (This is basically how we treat the current gecko_js backend as well, FWIW).

That said, I hope you'll stick around to help keep the Ruby backend running, and feel welcome to dive in to any discussions about new features! Please don't hesitate to open an issue or email me if there are things I can do to help you stay involved on an ongoing basis.

Longer term, I hope that we can find a way to decouple the development of specific language backends from all being in the same repo. Assuming that we do, I will probably advocate for the Ruby backend to move into its own separate repository (and the Python backend as well).

What I think we should do from here:

If you're happy to, please make a separate PR that just adds Ruby to the Docker image. I'll get that pushed and then we can update this PR to use it.
Please rebase this PR on top of current main, which I think will involve some minor conflicts in the test setup code.
I will finish up my current batch of work on replacing handlemaps, and see how much that's likely to affect the code here. Having had a closer look, I'm actually fairly hopeful that it won't conflict too badly.
With luck, I'll be able to apply my changes cleanly on top of this PR, in which case I'll go ahead and merge it while we keep working on those handlemap changes.

rfk · 2021-05-25T03:36:32Z

.circleci/config.yml

@@ -47,6 +47,12 @@ jobs:
          name: "Set RUSTFLAGS to fail the build if there are warnings"
          command: echo 'export RUSTFLAGS="-D warnings"' >> $BASH_ENV
      - checkout
+      - run:
+          name: "Install ruby"


Would you mind preparing a separate PR that adds this to the docker image? I can land and publish that change first and then we can use it for CI on the rest of this PR.

rfk · 2021-05-25T03:38:11Z

examples/arithmetic/uniffi.toml

@@ -5,5 +5,8 @@ cdylib_name = "arithmetical"
 [bindings.python]
 cdylib_name = "arithmetical"

+[bindings.ruby]
+cdylib_name = "arithmetical"
+


Note to self: we shouldn't need to repeat this individually for each binding backend in the config file 😬

(That's something we can improve in a future PR though)

rfk · 2021-05-25T03:41:07Z

fixtures/coverall/tests/bindings/test_coverall.rb

+    assert_raise_message /expected panic: oh no/ do
+      coveralls.panic 'expected panic: oh no'
+    end
+  end


Latest main has a new test here called test_self_by_arc, could you please rebase this on top of latest main and add an equivalent test here for Ruby?

Done! Thanks for pointing that out :)

rfk · 2021-05-25T03:42:18Z

uniffi_bindgen/src/bindings/ruby/gen_ruby.rs

+                Type::Enum(name) => format!("{}::{}", class_name_rb(name)?, enum_name_rb(v)?),
+                _ => panic!("Unexpected type in enum literal: {:?}", type_),
+            },
+            // https://docs.ruby-lang.org/en/2.0.0/syntax/literals_rdoc.html


Thanks for this link for context 👍🏻

rfk · 2021-05-25T03:47:33Z

uniffi_bindgen/src/bindings/ruby/templates/RustBufferStream.rb

+
+  {% when Type::CallbackInterface with (object_name) -%}
+  # The Callback Interface type {{ object_name }}.
+  # Objects cannot currently be serialized, but we can produce a helpful error.


"Objects" should be "Callback Interfaces" here (this might be my own typo from the Python backend, but at least we can fix it up while we're in here 😄)

rfk · 2021-05-26T01:30:44Z

This branch needs a couple of tweaks before final landing, for compatibility with other things that have merged to main. I'm going to go ahead and so those myself in a fresh PR over in #464 so that we can get this into a release for you to try out. Thanks again!

rfk · 2021-05-26T02:03:26Z

Landed in 962320b

rfk · 2021-05-27T03:07:42Z

The ruby adapter is now availalbe in the v0.10.0 release of UniFFI 🎉

saks mentioned this pull request May 6, 2021

Question: are there any plans to add more languages? #435

Open

saks commented May 6, 2021

View reviewed changes

mhammond reviewed May 7, 2021

View reviewed changes

saks marked this pull request as ready for review May 11, 2021 14:10

rfk reviewed May 18, 2021

View reviewed changes

saks force-pushed the rb branch from 95c7643 to 44b4c44 Compare May 20, 2021 06:47

rfk approved these changes May 25, 2021

View reviewed changes

saks mentioned this pull request May 25, 2021

Add ruby dependencies into CI docker image #461

Merged

saks added 8 commits May 24, 2021 21:44

Add ruby adapter

3fecda3

use test/unit

78b804e

fix typo and formatting

14a1566

add support for "coverall" fixture

3d18954

fix f32 pack/unpack format

1e6243d

fix round-trip for f32

7bf750b

typo fix

911abe9

Add test_self_by_arc ruby test for coverall

689874e

saks force-pushed the rb branch from 44b4c44 to 689874e Compare May 25, 2021 04:52

saks and others added 2 commits May 24, 2021 22:46

Cleanup (rely on updated docker image)

6f64a29

Merge branch 'mozilla:main' into rb

a655591

rfk mentioned this pull request May 26, 2021

Add codegen backend for Ruby. #464

Merged

rfk closed this May 26, 2021

data-sync-user mentioned this pull request Jun 23, 2021

Question: are there any plans to add more languages? mozilla/application-services#4245

Closed

stan-irl mentioned this pull request Dec 6, 2022

Typescript support #1361

Closed

saks deleted the rb branch October 19, 2024 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ruby adapter #436

Add ruby adapter #436

saks commented May 6, 2021 •

edited

Loading

saks May 6, 2021

rfk May 18, 2021

saks May 18, 2021

rfk May 25, 2021

saks May 25, 2021

rfk May 25, 2021

saks May 25, 2021

rfk May 25, 2021

saks May 25, 2021

mhammond left a comment

mhammond May 7, 2021

saks May 8, 2021

mhammond commented May 8, 2021

saks commented May 8, 2021

rfk May 18, 2021

rfk May 18, 2021

saks May 18, 2021

rfk May 19, 2021

saks May 19, 2021

saks commented May 21, 2021

rfk commented May 24, 2021

saks commented May 24, 2021

rfk left a comment •

edited

Loading

rfk May 25, 2021

rfk May 25, 2021

rfk May 25, 2021

saks May 25, 2021

rfk May 25, 2021

rfk May 25, 2021

rfk commented May 26, 2021

rfk commented May 26, 2021

rfk commented May 27, 2021

Add ruby adapter #436

Add ruby adapter #436

Conversation

saks commented May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond commented May 8, 2021

saks commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saks commented May 21, 2021

rfk commented May 24, 2021

saks commented May 24, 2021

rfk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented May 26, 2021

rfk commented May 26, 2021

rfk commented May 27, 2021

saks commented May 6, 2021 •

edited

Loading

rfk left a comment •

edited

Loading