Skip to content

Conversation

@guzman-raphael
Copy link
Contributor

@guzman-raphael guzman-raphael commented Apr 1, 2025

Depends on #68

We discussed a need for a reorg and the goals of such a reorg.

The following is a summary:

Motivation

Research showed that we can introduce a Python API based on C libs that are compiled and facilitated by the UniFFI Rust package. Thinking ahead, UniFFI brings many great benefits in automation, exposing an interoperable client, and writing code that respects DRY principles with less boilerplate.

Restrictions to Consider

However, there are several design implications to be aware of. For instance, it was found during validation that anything exposed over the CFFI boundary with UniFFI needs to respect several rules (not exhaustive but relevant to us):

  1. No use of Rust generics
  2. No use of const values in traits
  3. Primitives must be owned by the client e.g. String, u8, bool, f32, etc.
  4. BtreeMap isn't supported but HashMap is.
  5. async functions must use the custom derive attribute from the async-trait crate
  6. Custom types that are to be owned by client:
    1. are passed by value i.e. as a move
    2. cannot export any methods associated with them i.e. can't be invoked on client side
  7. Custom types that are to be owned by Rust:
    1. are passed by reference using Arc<T>
    2. can have methods exported i.e. can be invoked on client side
    3. require exporting constructor methods
    4. require exporting getter methods (e.g. for each field on a struct) since the underlying values are owned by Rust

Limit Restrictions and Facilitate Maintenance

Given that there are a number of rules, the reorg's purpose is to confine them to be applicable only to a subset of the source. Confining them to a subset means it is easier to explain this (and write documentaton) for maintainers/contributors and grants us free reign of Rust's complete feature set in finding solutions outside of the uniffi context.

We decided to go with 2 top-level modules for the source: core and uniffi.

As a side-effect of splitting things in this way, it means some features may need to be applied on both sides to be complete (or just entirely in uniffi). For example in LocalFileStore, there is some pure Rust in core and some that is exposed over uniffi. As a general practice, we could always just instruct contributors to target core if they aren't comfortable working with UniFFI and treat the task of exposing it to uniffi separately; either by us or someone else.

Visibility

Also, I've done my best in keeping the contents of core private since uniffi is the exposed client for Rust, Python, and anything in the future. However, there were some methods that we made public that we did not intend to expose over uniffi e.g. to_yaml which is a generic and used in tests. To minimize the noise, I reorg'ed them as-is but we could always change that later e.g. gate-keep them behind a feature flag that can be enabled during test runs. Since this update will be chaotic enough, this reorg PR is meant to only apply the reorg and not introduce any other change or functionality†.

Footnote

† = The only exception to this was the last commit. The tests broke due to a new clippy lint added in a new version of Rust that had a bug in it. That commit bumped the Rust version and removed the manual getter method to avoid the bug since future PR's already had plans to remove it (see point 1. ii. in description of #71).

@guzman-raphael guzman-raphael mentioned this pull request Apr 3, 2025
Copy link
Contributor

@Synicix Synicix left a comment

Choose a reason for hiding this comment

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

Reviewed locally and everything looks good now!

@Synicix Synicix merged commit 5a4f641 into nauticalab:dev Apr 9, 2025
1 check passed
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