[Client] Add support for zig client library
#160
Conversation
|
@NishantJoshi00 can we remove the merge commit? It breaks semantic guidelines here. Please rebase if possible |
I have done that 👍 . |
|
rebase and merge/squash and merge, depending on the use-case |
In this case, my commit messages shouldn't matter, only the PR title would be of significance, right? |
|
@NishantJoshi00 yup |
|
@juspay/sdk-backend can we start reviewing this? |
clients/zig/build.zig.zon
Outdated
| // | ||
| // It is redundant to include "zig" in this name because it is already | ||
| // within the Zig package namespace. | ||
| .name = "pauli", |
There was a problem hiding this comment.
@NishantJoshi00 , what is pauli here, the package/lib name?
There was a problem hiding this comment.
Can we have a generic name zig-cac-client? We don't want to get into naming clients for other languages too.
There was a problem hiding this comment.
sure, I started of with this, will change that
There was a problem hiding this comment.
@NishantJoshi00 few changes:
- We have to avoid the clone of superposition into the client
- link to the binaries in the parent folder
- binaries should be compiled for the target platform
- can you specify the min version of zig
- can you add a zig and zls package in flake.nix
- Avoid relative paths
Let us know if we need to make any changes to make this work.
| test: test-cac test-expt | ||
|
|
||
| test-cac: | ||
| zig test -I../../headers -lc src/cac.zig ../../target/debug/libcac_client.so |
There was a problem hiding this comment.
@NishantJoshi00 few things here:
- Can the linking binary be an absolute path
- This should be different based on the OS compiled for
If any support is needed from Superposition, we can add this
There was a problem hiding this comment.
This is no longer required, I was using this initially to test it. Will remove this
There was a problem hiding this comment.
the libs and headers are figured out in build by build.zig
|
|
||
| ### Pre-requisites | ||
|
|
||
| - As this client leverages `build.zig.zon` file for managing dependencies. Your zig version should atleast satisfy the minimum version mentioned in the `build.zig.zon` file. |
There was a problem hiding this comment.
@NishantJoshi00 can we include some usage examples too?
There was a problem hiding this comment.
This is part of the example/ folder. I will add a README in that folder too
| const repository = b.addSystemCommand(&.{ | ||
| "sh", "-c", std.fmt.comptimePrint( | ||
| \\if [ ! -d superposition ]; then | ||
| \\ git clone https://github.com/juspay/superposition.git |
There was a problem hiding this comment.
How do we avoid this clone? Can you link to the libraries in target directory in the parent folder?
There was a problem hiding this comment.
I think when using this client as a dependency, we need some mechanism to be able to link the library, header files. By, doing this I don't need to worry about if you have the .so files present in any of your lib paths (same goes to header files). But, it does incur the cost of compilation.
I think one possible fix might be to only build the client crates, while not touching the others.
What do you think?
There was a problem hiding this comment.
Yeah you can only build the clients rather than the whole project
| /// This is a blocking call, make sure to run this in a separate thread. The polling frequency is set in the [`StartupConfig`] struct using `update_frequency`. | ||
| /// | ||
| pub fn poll(self: *const Client) void { | ||
| expt_bindings.expt_start_polling_update(self.config.tenant.ptr); |
There was a problem hiding this comment.
@NishantJoshi00 Can we provide an example in docs of spawning a thread?
There was a problem hiding this comment.
So here, I faced an issue. This FFI function didn't block. I wasn't able to debug it. But, even without using this I was still able to get the default configs
| // BUG: The current implementation of `cac_free_string` accepts `char *` but a significant other functions defined return `const char *`. This makes it impossible to free the memory allocated by the CAC client. | ||
| // cac_bindings.cac_free_string(s_ptr.ptr); |
There was a problem hiding this comment.
Why is it impossible? Should we change something?
There was a problem hiding this comment.
Let's translate it to rust concepts and understand this.
most of the functions imagine are returning &String while this function is accepting &mut String. Will this work in rust?
I am looking at this with a similar lens
Datron
force-pushed
the
main
branch
7 times, most recently
from
2a6bf86
to
6ca353c
Compare
Datron
force-pushed
the
main
branch
4 times, most recently
from
02832c3
to
a9afdb9
Compare
Description
Adding support for
zigclient for superposition. The steps to work with the client are mentioned in theREADME.mdfile in the client.