feat: separate embedded logos client#166
Conversation
jazzz
left a comment
There was a problem hiding this comment.
In #163 I asked for strong justifications as to:
- why this approach makes LogosChat the easiest to use for new developers when compared to other options.
- How we intend to minimize breaking changes in Apps during development.
Unfortunately this PR does not address my questions, and I still don't understand the reasoning behind these changes.
As outlined in #159 my primary design goals are :
- Making the public entry point easier and simpler to use for developers, rather than more flexible options.
- Eliminating footguns that lead to Fragmentation, and interop failures.
- Making the entrypoint's imports, examples and documentation easier, by isolating to a separate crate. Reducing noise, and encouraging developers to become contributors.
I currently don't see how these PR's #159, #163, #166 contribute towards these goals.
| [package] | ||
| name = "embedded-logos-delivery" | ||
| version = "0.1.0" | ||
| edition = "2024" | ||
| links = "logosdelivery" |
There was a problem hiding this comment.
[Pebble] I need to be convinced this is better than keeping components namespaced to he components crate.
I'd agree that moving to its own crate has the benefit that the build infrastructure is easy to manage by default. However this doesn't appear to be a blocker.
The developer experience however appears significantly worse. When installing logos-components cargo lists available features (aka components) which are available for the advanced developer to pass to ChatClientBuilder. This allows developers to discover what is available and install their desired components to change behavior.
e.g. Current
❯ cargo add components
Adding components to dependencies
- embedded_p2p_delivery
In the future:
❯ cargo add logos-chat-components
Adding logos-chat-components to dependencies
+ logos_delivery
+ sqlite_message_store
- embedded_p2p_delivery
- ephemeral_message_store
- keypackage_cache_client
It appears this change sacrifices developer experience for maintainer ease - What am I missing?
There was a problem hiding this comment.
I have experienced a few options,
- Separate crate for the whole Logos client, main...logos-separate-crate
- Narrow the feature gate, native code stays in components, main...logos-narrow-gate
- this PR, split only the embedded logos-delivery into its own crate.
I proposed this solution instead of the others because:
- It isolates the embedded delivery properly. It has very different logic from the other components — a self-contained FFI wrapper (node thread, C callbacks, dylib install-name stamping) with no coupling to the pure-Rust registries and stores. Native-linking code getting its own crate is the standard -sys-crate convention.
- The feature-discovery DX you describe is preserved — it just lives on the entrypoint crate
logos-chat. This is the same facade pattern as tokio's macros: features for discovery on the crate you install, crate boundaries for cost isolation underneath. Your future catalog is fully compatible — pure-Rust components can stay ordinary features; only native-linking ones become optional dependencies surfaced as features. - The crate boundary is about scoping the native cost, not maintainer ease. Two things a feature inside components can't fix: the links = "logosdelivery" key and build.rs apply to the whole crate even with the feature off, so every components consumer (someone who only wants EphemeralRegistry) carries that machinery, and anything with --all-features (cargo test, docs tooling) hits the native link requirement. And cargo features are unified across the build graph — any crate anywhere enabling embedded_p2p_delivery imposes the link requirement on every build of components in that graph. A dependency edge scopes the cost to exactly the dependents who opted in.
- Why not the other two branches: option 1 splits the entrypoint, so developers need to know two crates — strictly worse DX. Option 2 fixes the client-side gating but leaves all consumers of components carrying the native build infrastructure, plus a two-hop feature chain (logos-chat/embedded-p2p-delivery → components/embedded_p2p_delivery).
So, discoverability is unchanged at the crate developers actually install, and the boundary moves the native-link cost from "everyone who touches components" to "exactly who wants the embedded node." Isn't this a better code architecture?
64976cb to
ef58a1a
Compare
ef58a1a to
5c84930
Compare
jazzz
left a comment
There was a problem hiding this comment.
I've been requested offline to allow this work to be merged as is.
My reservations as described, still mostly stand.
However given priorities lie elsewhere currently and this change set does not make irrevocable commitments, I'm inclined to merge and close this work stream. Re-visiting this work should only occur once there has been a chance for alignment and planning about the best direction for the future.
Change
Split the client from the transport and draw the native-link boundary at a
crate, keeping
logos-chatas the single entrypoint:extensions/embedded-logos-delivery— the embeddedlogos-delivery transport service, moved out of
componentstogether withits FFI shims,
build.rs, andlinkskey. The service is renamedEmbeddedP2pDeliveryService→EmbeddedLogosDelivery, and the nodedefaults (network preset, TCP port) now live here with
P2pConfig. Thecrate is excluded from the workspace's default members, so default
builds/tests never need the native library.
componentsno longer has anynative dependency or feature.
logos-chatgains a transport-generic Logos stack, compiledunconditionally:
LogosConfig(db path/key + registry endpoint only — notransport settings), the
LogosChatClient<T>alias, andopen_with_transport(config, transport)for any injectedTransport.embeddedmodule behind theembedded-logos-deliveryfeature, which justswitches on the optional dependency: the
Transportimpl forEmbeddedLogosDelivery, theEmbeddedLogosClientalias, and itsopen(config, p2p_config). The feature also re-exports the transport typesso consumers only ever depend on
logos-chat.Future transports (e.g. an FFI client) follow the same pattern: another named
alias over
LogosChatClient<T>, plus an optional dependency only if itcarries a native cost.
Consumers
chat-clienables the feature and opens the p2p path viaEmbeddedLogosClient::open; its file-transport path reusesopen_with_transportinstead of hand-rolling the account/registry/storagewiring.
embedded-logos-delivery; thenix smoketest job still builds and runs the fully linked binary.