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

Rust SDK: group all codegen'd names exported from the SDK under pub mod __codegen #1900

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Oct 24, 2024

Description of Changes

Based on #1897 . While writing that PR, I moved a definition from db_connection.rs to client_cache.rs. This needlessly broke codegen.

Rather than having the codegen rely on a whole bunch of different #[doc(hidden)] pub mods in the SDK library by path, group every name the codegen refers to under spacetimedb_sdk::__codegen. This has several benefits:

  • It makes very clear that users should not be referring to those interfaces.
  • It allows us to move definitions around in the SDK library without changing the codegen, so long as we update the re-exports from the __codegen module.

In a similar vein, the unstable interface Cloud use(s|d) to set its client address is moved to spacetimedb_sdk::unstable. This may end up getting removed anyways; I'm not sure what the plan is going forward for how cloud nodes will identify themselves.

API and ABI breaking changes

Breaks codegen. Minor user-facing breakage: u256, i256 are now re-exported from the SDK root rather than via the spacetimedb_sdk::sats::{u256, i256}. The sats crate is no longer fully re-exported, so users who want it will need a separate dependency.

Expected complexity level and risk

1

Testing

  • SDK test suite.

This turns out to be effectively a prerequisite for client-side indices.
Storing indexes in a type-erased way
while making them `Clone` for storage in `im::HashMap`
is likely possible, but it's at least very challenging and complex.
Given that we no longer actually need the concurrent persistence offered by `im::HashMap`,
it seems easier to just revert to using `std::collections::HashMap`.
@gefjon gefjon added the abi-break A PR that makes an ABI breaking change label Oct 24, 2024
@gefjon gefjon force-pushed the phoebe/sdk-dunder-codegen-module branch from 758baa1 to f6a954b Compare October 24, 2024 16:11
As opposed to having a bunch of `#[doc(hidden)] pub mod`s
with a variety of different purposes.
This means we can reorganize the SDK internals without breaking codegen.
@gefjon gefjon force-pushed the phoebe/sdk-dunder-codegen-module branch from d529c12 to eca780e Compare October 24, 2024 17:29
@bfops bfops added release-1.0 release-any To be landed in any release window and removed release-1.0 labels Oct 28, 2024
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

Everything here makes sense to me, but again, I'm not the most familiar with the Rust SDK so I'd like it if this got more eyes on it.

///
/// `TableHandle`s don't actually hold a direct reference to the table they access,
/// as that would require both gnarly lifetimes and also a `MutexGuard` on the client cache.
/// Instead, they hold an `Arc<Mutex>` on the whole [`ClientCache`],
Copy link
Contributor

Choose a reason for hiding this comment

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

obligatory:

  • what if we used RwLock
  • nah we can do that later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants