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

Go/Core Refactor: Move FFI to a Dedicated Folder for Reusability #3372

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions .github/workflows/go-cd.yml
Original file line number Diff line number Diff line change
Expand Up @@ -119,16 +119,14 @@ jobs:
working-directory: go
run: |
make install-build-tools
make build-glide-client
# TODO: move generation of protobuf and header file to a non-matrix step
make generate-protobuf
make build
- name: Upload artifacts
continue-on-error: true
uses: actions/upload-artifact@v4
with:
name: ${{ matrix.host.TARGET }}
path: |
go/target/release/libglide_rs.a
ffi/target/release/libglide_ffi.a
go/lib.h
go/protobuf/

Expand All @@ -149,7 +147,7 @@ jobs:
for dir in */; do
target_name=${dir%/}
mkdir -p $GITHUB_WORKSPACE/go/rustbin/${target_name}
cp ${target_name}/target/release/libglide_rs.a $GITHUB_WORKSPACE/go/rustbin/${target_name}/
cp ${target_name}/target/release/libglide_ffi.a $GITHUB_WORKSPACE/go/rustbin/${target_name}/
done
# TODO: move generation of protobuf and header file to a non-matrix step
cd x86_64-unknown-linux-gnu
Expand Down
7 changes: 2 additions & 5 deletions .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ on:
paths:
- glide-core/src/**
- glide-core/redis-rs/redis/src/**
- ffi/src/**
- utils/cluster_manager.py
- go/**
- .github/workflows/go.yml
Expand All @@ -22,6 +23,7 @@ on:
paths:
- glide-core/src/**
- glide-core/redis-rs/redis/src/**
- ffi/src/**
- utils/cluster_manager.py
- go/**
- .github/workflows/go.yml
Expand Down Expand Up @@ -133,11 +135,6 @@ jobs:
steps:
- uses: actions/checkout@v4

- uses: ./.github/workflows/lint-rust
with:
cargo-toml-folder: go
github-token: ${{ secrets.GITHUB_TOKEN }}

- name: Set up Go ${{ matrix.go }}
uses: actions/setup-go@v5
with:
Expand Down
18 changes: 15 additions & 3 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ on:
- logger_core/**
- glide-core/**
- glide-core/redis-rs/redis/src/**
- ffi/**
- utils/cluster_manager.py
- .github/workflows/rust.yml
- .github/workflows/install-shared-dependencies/action.yml
Expand All @@ -23,6 +24,7 @@ on:
- logger_core/**
- glide-core/**
- glide-core/redis-rs/redis/src/**
- ffi/**
- utils/cluster_manager.py
- .github/workflows/rust.yml
- .github/workflows/install-shared-dependencies/action.yml
Expand Down Expand Up @@ -97,13 +99,17 @@ jobs:

- uses: Swatinem/rust-cache@v2

- name: Run tests
- name: Run glide-core tests
working-directory: ./glide-core
run: cargo test --all-features --release -- --test-threads=1 # TODO remove the concurrency limit after we fix test flakyness.
run: cargo test --all-features --release

- name: Run glide-ffi tests
working-directory: ./ffi
run: cargo test

- name: Run logger tests
working-directory: ./logger_core
run: cargo test --all-features -- --test-threads=1
run: cargo test --all-features

- name: Check features
working-directory: ./glide-core
Expand All @@ -127,6 +133,12 @@ jobs:
github-token: ${{ secrets.GITHUB_TOKEN }}
name: lint glide-core

- uses: ./.github/workflows/lint-rust
with:
cargo-toml-folder: ./ffi
github-token: ${{ secrets.GITHUB_TOKEN }}
name: lint glide-ffi

- uses: ./.github/workflows/lint-rust
with:
cargo-toml-folder: ./logger_core
Expand Down
2 changes: 1 addition & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"glide-core/redis-rs/Cargo.toml",
"benchmarks/rust/Cargo.toml",
"java/Cargo.toml",
"go/Cargo.toml",
"ffi/Cargo.toml",
],
"rust-analyzer.runnableEnv": {
"REDISRS_SERVER_TYPE": "tcp"
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* Go: Add Cluster Scan support ([#3295](https://github.com/valkey-io/valkey-glide/pull/3295))
* Go: Fix unsafe precondition violation for the slice::from_raw_parts ([#3350](https://github.com/valkey-io/valkey-glide/issues/3350))
* Go: Add `GeoAdd` and the Geospatial interface ([#3366](https://github.com/valkey-io/valkey-glide/pull/3366))
* Go/Core: Move FFI to a Dedicated Folder for Reusability ([#3372](https://github.com/valkey-io/valkey-glide/pull/3372))

#### Breaking Changes

Expand Down
2 changes: 1 addition & 1 deletion go/.cargo/config.toml → ffi/.cargo/config.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[env]
GLIDE_NAME = { value = "GlideGo", force = true }
GLIDE_NAME = { value = "GlideFFI", force = true }
GLIDE_VERSION = "0.1.0"
# Suppress error
# > ... was built for newer 'macOS' version (14.5) than being linked (14.0)
Expand Down
2 changes: 2 additions & 0 deletions ffi/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# cbindgen generated header file
lib.h
9 changes: 6 additions & 3 deletions go/Cargo.toml → ffi/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[package]
name = "glide-rs"
name = "glide-ffi"
version = "0.1.0"
edition = "2021"
license = "Apache-2.0"
Expand All @@ -9,10 +9,13 @@ authors = ["Valkey GLIDE Maintainers"]
crate-type = ["staticlib"]

[dependencies]
redis = { path = "../glide-core/redis-rs/redis", features = ["aio", "tokio-comp", "connection-manager", "tokio-rustls-comp"] }
protobuf = { version = "3", features = [] }
redis = { path = "../glide-core/redis-rs/redis", features = ["aio", "tokio-comp", "tokio-rustls-comp"] }
glide-core = { path = "../glide-core", features = ["proto"] }
tokio = { version = "^1", features = ["rt", "macros", "rt-multi-thread", "time"] }
protobuf = { version = "3.3.0", features = [] }

[dev-dependencies]
glide-ffi = { path = "." }

[profile.release]
lto = true
Expand Down
36 changes: 36 additions & 0 deletions ffi/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# glide_ffi

## Overview

The glide_ffi crate provides a C-compatible Foreign Function Interface (FFI) for interacting with `glide-core`. It serves as a bridge for internal wrappers written in other languages, such as Go, to integrate seamlessly with `glide-core`.

## Structure
• src/lib.rs: Defines the FFI interface.
• cbindgen.toml: Configuration for generating the C header file (lib.h).
• Cargo.toml: Manages dependencies, including glide-core.

## Building the Library

To build the FFI library:
```bash
cargo build --release
```
To run tests:
```bash
cargo test
```

## Generating the C Header File

```bash
cargo install cbindgen
cbindgen --config cbindgen.toml --crate glide-ffi --output lib.h --lang c
```

## Running Linters and Formatting

```bash
rustup component add clippy rustfmt
cargo clippy --all-features --all-targets -- -D warnings
cargo fmt --manifest-path ./Cargo.toml --all
```
File renamed without changes.
32 changes: 16 additions & 16 deletions go/src/lib.rs → ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,36 +38,36 @@ use tokio::runtime::Runtime;
/// The struct is freed by the external caller by using `free_command_response` to avoid memory leaks.
/// TODO: Add a type enum to validate what type of response is being sent in the CommandResponse.
#[repr(C)]
#[derive(Debug)]
#[derive(Debug, Clone)]
pub struct CommandResponse {
response_type: ResponseType,
int_value: i64,
float_value: c_double,
bool_value: bool,
pub response_type: ResponseType,
pub int_value: i64,
pub float_value: c_double,
pub bool_value: bool,

/// Below two values are related to each other.
/// `string_value` represents the string.
/// `string_value_len` represents the length of the string.
string_value: *mut c_char,
string_value_len: c_long,
pub string_value: *mut c_char,
pub string_value_len: c_long,

/// Below two values are related to each other.
/// `array_value` represents the array of CommandResponse.
/// `array_value_len` represents the length of the array.
array_value: *mut CommandResponse,
array_value_len: c_long,
pub array_value: *mut CommandResponse,
pub array_value_len: c_long,

/// Below two values represent the Map structure inside CommandResponse.
/// The map is transformed into an array of (map_key: CommandResponse, map_value: CommandResponse) and passed to Go.
/// These are represented as pointers as the map can be null (optionally present).
map_key: *mut CommandResponse,
map_value: *mut CommandResponse,
pub map_key: *mut CommandResponse,
pub map_value: *mut CommandResponse,

/// Below two values are related to each other.
/// `sets_value` represents the set of CommandResponse.
/// `sets_value_len` represents the length of the set.
sets_value: *mut CommandResponse,
sets_value_len: c_long,
pub sets_value: *mut CommandResponse,
pub sets_value_len: c_long,
}

impl Default for CommandResponse {
Expand All @@ -90,7 +90,7 @@ impl Default for CommandResponse {
}

#[repr(C)]
#[derive(Debug, Default)]
#[derive(Debug, Default, Clone)]
pub enum ResponseType {
#[default]
Null = 0,
Expand Down Expand Up @@ -132,8 +132,8 @@ pub type FailureCallback = unsafe extern "C" fn(
/// The struct is freed by the external caller by using `free_connection_response` to avoid memory leaks.
#[repr(C)]
pub struct ConnectionResponse {
conn_ptr: *const c_void,
connection_error_message: *const c_char,
pub conn_ptr: *const c_void,
pub connection_error_message: *const c_char,
}

/// A `GlideClient` adapter.
Expand Down
Loading
Loading