Copilot commented on code in PR #107:
URL: https://github.com/apache/fluss-rust/pull/107#discussion_r2637656839
##########
bindings/cpp/MODULE.bazel:
##########
@@ -0,0 +1,7 @@
+module(
Review Comment:
Missing Apache License header. All source files in Apache projects should
include the standard Apache License header. Add the header at the beginning of
the file using '#' comment syntax, similar to CMakeLists.txt or Cargo.toml.
##########
bindings/cpp/ci.sh:
##########
@@ -0,0 +1,85 @@
+#!/bin/bash
+
+set -xe
Review Comment:
Trailing whitespace after 'set -xe'. This should be 'set -xe' without the
trailing space for consistency.
##########
bindings/cpp/BUILD.bazel:
##########
@@ -0,0 +1,142 @@
+licenses(["notice"])
+
+load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary")
+
+genrule(
+ name = "cargo_build_debug",
+ srcs = glob([
+ "src/**/*.rs",
+ "Cargo.toml",
+ ]),
+ outs = [
+ "rust_lib_debug.a",
+ "rust_bridge_cc.cc",
+ "rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ ],
+ cmd = """
+ set -e
+ EXECROOT=$$(pwd)
+ OUTPUT_LIB=$(location rust_lib_debug.a)
+ OUTPUT_CC=$(location rust_bridge_cc.cc)
+ OUTPUT_H=$(location rust_bridge_h.h)
+ OUTPUT_SRC_H=$(location src/lib.rs.h)
+ OUTPUT_CXX_H=$(location cxxbridge/rust/cxx.h)
+ # Resolve real source path from sandbox symlink
+ SANDBOX_CARGO=$(location Cargo.toml)
+ REAL_CARGO=$$(readlink -f $$SANDBOX_CARGO 2>/dev/null || python3 -c
"import os; print(os.path.realpath('$$SANDBOX_CARGO'))")
+ CARGO_DIR=$$(dirname $$REAL_CARGO)
+ # Find Cargo workspace root (fluss-rust directory, 2 levels up from
bindings/cpp)
+ WORKSPACE_ROOT=$$(cd $$CARGO_DIR/../.. && pwd)
+ if [ ! -f $$WORKSPACE_ROOT/Cargo.toml ]; then
+ echo "Error: Cannot find workspace root Cargo.toml at
$$WORKSPACE_ROOT" >&2
+ exit 1
+ fi
+ cd $$WORKSPACE_ROOT
+ cargo build --manifest-path $$CARGO_DIR/Cargo.toml
+ CARGO_TARGET_DIR=$$WORKSPACE_ROOT/target
+ RUST_BRIDGE_DIR=$$CARGO_TARGET_DIR/cxxbridge/fluss-cpp/src
+ RUST_LIB=$$CARGO_TARGET_DIR/debug/libfluss_cpp.a
+ if [ ! -f $$RUST_LIB ]; then
+ echo "Error: Rust library not found at $$RUST_LIB" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.cc ]; then
+ echo "Error: cxxbridge CC file not found at
$$RUST_BRIDGE_DIR/lib.rs.cc" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.h ]; then
+ echo "Error: cxxbridge header file not found at
$$RUST_BRIDGE_DIR/lib.rs.h" >&2
+ exit 1
+ fi
+ cd $$EXECROOT
+ mkdir -p $$(dirname $$OUTPUT_SRC_H) $$(dirname $$OUTPUT_CXX_H)
+ cp $$RUST_LIB $$OUTPUT_LIB || (echo "Failed to copy $$RUST_LIB to
$$OUTPUT_LIB" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.cc $$OUTPUT_CC || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.cc to $$OUTPUT_CC" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_H" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_SRC_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_SRC_H" >&2; exit 1)
+ CXX_H_SOURCE=$$CARGO_TARGET_DIR/cxxbridge/rust/cxx.h
+ if [ ! -f $$CXX_H_SOURCE ] && [ ! -L $$CXX_H_SOURCE ]; then
+ echo "Error: cxx.h not found at $$CXX_H_SOURCE" >&2
+ exit 1
+ fi
+ cp -L $$CXX_H_SOURCE $$OUTPUT_CXX_H || (echo "Failed to copy
$$CXX_H_SOURCE to $$OUTPUT_CXX_H" >&2; exit 1)
+ """,
+ message = "Building Rust library with cargo...",
+ local = 1,
+)
+
+cc_import(
+ name = "rust_lib",
+ static_library = ":rust_lib_debug.a",
+ alwayslink = True,
+)
+
+cc_library(
+ name = "fluss_cpp",
+ srcs = [
+ "src/admin.cpp",
+ "src/connection.cpp",
+ "src/table.cpp",
+ ":rust_bridge_cc.cc",
+ ],
+ hdrs = [
+ "include/fluss.hpp",
+ ],
+ textual_hdrs = [
+ ":rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ "src/ffi_converter.hpp",
+ ],
+ strip_include_prefix = "include",
+ copts = [
+ "-std=c++17",
+ "-g3",
+ "-O0",
+ "-ggdb",
+ "-fno-omit-frame-pointer",
+ "-DDEBUG",
+ ],
+ includes = [
+ "src",
+ "cxxbridge",
+ ],
+ linkopts = [
+ "-g",
+ "-ldl",
+ "-lpthread",
+ ] + select({
+ "@platforms//os:macos": [
+ "-framework", "CoreFoundation",
+ "-framework", "Security",
+ ],
+ "//conditions:default": [],
+ }),
+ deps = [
+ ":rust_lib",
+ ],
+ visibility = ["//visibility:public"],
+)
+
+cc_binary(
+ name = "fluss_cpp_example",
+ srcs = [
+ "examples/example.cpp",
+ ],
+ deps = [":fluss_cpp"],
+ copts = [
+ "-std=c++17",
+ "-g3",
+ "-O0",
+ "-ggdb",
+ "-fno-omit-frame-pointer",
+ "-DDEBUG",
+ ],
+ linkopts = [
+ "-g",
+ ],
+ visibility = ["//visibility:public"],
+)
+
Review Comment:
Two trailing blank lines at the end of the file. Consider removing these
extra blank lines for consistency with common style guidelines.
```suggestion
```
##########
bindings/cpp/BUILD.bazel:
##########
@@ -0,0 +1,142 @@
+licenses(["notice"])
Review Comment:
Missing Apache License header. All source files in Apache projects should
include the standard Apache License header. Add the header at the beginning of
the file using '#' comment syntax, similar to CMakeLists.txt or Cargo.toml.
##########
bindings/cpp/.bazelrc:
##########
@@ -0,0 +1,15 @@
+# Bazel configuration for fluss-rust C++ bindings
Review Comment:
Missing Apache License header. All source files in Apache projects should
include the standard Apache License header. Add the header at the beginning of
the file using '#' comment syntax, similar to CMakeLists.txt or Cargo.toml.
##########
bindings/cpp/ci.sh:
##########
@@ -0,0 +1,85 @@
+#!/bin/bash
+
+set -xe
+
+DIR="$(cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd)"
+
+# Set Bazel output base to bazel-build directory
+# This ensures all Bazel outputs are in bazel-build/.bazel-output-base
+BAZEL_OUTPUT_BASE="$DIR/bazel-build/.bazel-output-base"
+
+# Create output base directory if it doesn't exist
+mkdir -p "$BAZEL_OUTPUT_BASE"
+
+# Wrapper function to run bazel with --output_base
+bazel() {
+ command bazel --output_base="$BAZEL_OUTPUT_BASE" "$@"
+}
+
+compile() {
+ bazel build //:fluss_cpp
+}
+
+build_example() {
+ bazel build //:fluss_cpp_example
+}
+
+run_example() {
+ build_example
+ bazel run //:fluss_cpp_example
+}
+
+clean() {
+ bazel clean
+ # Remove bazel-* symlinks (Bazel automatically creates these)
+ rm -f "$DIR"/bazel-*
+ # Also remove the bazel-build directory if it exists
+ if [ -d "$DIR/bazel-build" ]; then
+ rm -rf "$DIR/bazel-build"
+ fi
+ echo "Cleaned all Bazel outputs and symlinks"
+}
+
+show_outputs() {
+ echo "=== Library outputs ==="
+ bazel cquery //:fluss_cpp --output=files 2>/dev/null || echo "Run 'bazel
build //:fluss_cpp' first"
+ echo ""
+ echo "=== Example binary outputs ==="
+ bazel cquery //:fluss_cpp_example --output=files 2>/dev/null || echo "Run
'bazel build //:fluss_cpp_example' first"
+ echo ""
+ echo "=== To run the example ==="
+ echo " bazel run //:fluss_cpp_example"
+ echo ""
+ echo "=== To find outputs manually ==="
+ echo " bazel info bazel-bin"
+}
+
+case $1 in
+ compile )
+ compile
+ ;;
+ example )
+ build_example
+ ;;
+ run )
+ run_example
+ ;;
+ outputs )
+ show_outputs
+ ;;
+ clean )
+ clean
+ ;;
+ * )
+ echo "Usage: $0 {compile|example|run|outputs|clean}"
+ echo ""
+ echo "Commands:"
+ echo " compile - Build the fluss_cpp library"
+ echo " example - Build the example binary"
+ echo " run - Build and run the example binary"
+ echo " outputs - Show the location of build outputs"
+ echo " clean - Clean all Bazel outputs"
+ exit 1
+ ;;
+esac
+
Review Comment:
Blank line at the end of the file. While this is generally a minor style
issue, it's inconsistent with typical bash script conventions. Consider
removing this trailing blank line for consistency.
```suggestion
```
##########
bindings/cpp/BUILD.bazel:
##########
@@ -0,0 +1,142 @@
+licenses(["notice"])
+
+load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary")
+
+genrule(
+ name = "cargo_build_debug",
+ srcs = glob([
+ "src/**/*.rs",
+ "Cargo.toml",
+ ]),
+ outs = [
+ "rust_lib_debug.a",
+ "rust_bridge_cc.cc",
+ "rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ ],
+ cmd = """
+ set -e
+ EXECROOT=$$(pwd)
+ OUTPUT_LIB=$(location rust_lib_debug.a)
+ OUTPUT_CC=$(location rust_bridge_cc.cc)
+ OUTPUT_H=$(location rust_bridge_h.h)
+ OUTPUT_SRC_H=$(location src/lib.rs.h)
+ OUTPUT_CXX_H=$(location cxxbridge/rust/cxx.h)
+ # Resolve real source path from sandbox symlink
+ SANDBOX_CARGO=$(location Cargo.toml)
+ REAL_CARGO=$$(readlink -f $$SANDBOX_CARGO 2>/dev/null || python3 -c
"import os; print(os.path.realpath('$$SANDBOX_CARGO'))")
+ CARGO_DIR=$$(dirname $$REAL_CARGO)
+ # Find Cargo workspace root (fluss-rust directory, 2 levels up from
bindings/cpp)
+ WORKSPACE_ROOT=$$(cd $$CARGO_DIR/../.. && pwd)
+ if [ ! -f $$WORKSPACE_ROOT/Cargo.toml ]; then
+ echo "Error: Cannot find workspace root Cargo.toml at
$$WORKSPACE_ROOT" >&2
+ exit 1
+ fi
+ cd $$WORKSPACE_ROOT
+ cargo build --manifest-path $$CARGO_DIR/Cargo.toml
+ CARGO_TARGET_DIR=$$WORKSPACE_ROOT/target
+ RUST_BRIDGE_DIR=$$CARGO_TARGET_DIR/cxxbridge/fluss-cpp/src
+ RUST_LIB=$$CARGO_TARGET_DIR/debug/libfluss_cpp.a
Review Comment:
The hardcoded library name 'libfluss_cpp.a' doesn't match the expected
output based on the Cargo package name 'fluss-cpp' (with hyphen). Cargo
converts hyphens to underscores in library names, so this is correct. However,
the cxxbridge path uses 'fluss-cpp' with a hyphen. This inconsistency between
line 39 (using 'fluss-cpp') and line 40 (using 'fluss_cpp') is potentially
confusing and error-prone. Verify that these paths are correct given Cargo's
naming conventions.
##########
bindings/cpp/MODULE.bazel:
##########
@@ -0,0 +1,7 @@
+module(
+ name = "fluss_cpp",
+)
+
+bazel_dep(name = "rules_cc", version = "0.0.17")
+bazel_dep(name = "platforms", version = "0.0.10")
+
Review Comment:
Trailing blank line at the end of the file. Consider removing this for
consistency with Bazel style guidelines.
```suggestion
```
##########
bindings/cpp/ci.sh:
##########
@@ -0,0 +1,85 @@
+#!/bin/bash
Review Comment:
Missing Apache License header. All source files in Apache projects should
include the standard Apache License header. Add the header at the beginning of
the file, similar to other files in the project (e.g., CMakeLists.txt,
Cargo.toml, build.rs).
```suggestion
#!/bin/bash
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
```
##########
bindings/cpp/ci.sh:
##########
@@ -0,0 +1,85 @@
+#!/bin/bash
+
+set -xe
+
+DIR="$(cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd)"
+
+# Set Bazel output base to bazel-build directory
+# This ensures all Bazel outputs are in bazel-build/.bazel-output-base
+BAZEL_OUTPUT_BASE="$DIR/bazel-build/.bazel-output-base"
+
+# Create output base directory if it doesn't exist
+mkdir -p "$BAZEL_OUTPUT_BASE"
+
+# Wrapper function to run bazel with --output_base
+bazel() {
+ command bazel --output_base="$BAZEL_OUTPUT_BASE" "$@"
+}
+
+compile() {
+ bazel build //:fluss_cpp
+}
+
+build_example() {
+ bazel build //:fluss_cpp_example
+}
+
+run_example() {
+ build_example
+ bazel run //:fluss_cpp_example
+}
+
+clean() {
+ bazel clean
+ # Remove bazel-* symlinks (Bazel automatically creates these)
+ rm -f "$DIR"/bazel-*
+ # Also remove the bazel-build directory if it exists
+ if [ -d "$DIR/bazel-build" ]; then
+ rm -rf "$DIR/bazel-build"
+ fi
+ echo "Cleaned all Bazel outputs and symlinks"
+}
+
+show_outputs() {
+ echo "=== Library outputs ==="
+ bazel cquery //:fluss_cpp --output=files 2>/dev/null || echo "Run 'bazel
build //:fluss_cpp' first"
+ echo ""
+ echo "=== Example binary outputs ==="
+ bazel cquery //:fluss_cpp_example --output=files 2>/dev/null || echo "Run
'bazel build //:fluss_cpp_example' first"
+ echo ""
+ echo "=== To run the example ==="
+ echo " bazel run //:fluss_cpp_example"
+ echo ""
+ echo "=== To find outputs manually ==="
+ echo " bazel info bazel-bin"
+}
+
+case $1 in
Review Comment:
Trailing whitespace after the case pattern. This should be 'case $1 in'
without the trailing space for consistency.
```suggestion
case $1 in
```
##########
bindings/cpp/BUILD.bazel:
##########
@@ -0,0 +1,142 @@
+licenses(["notice"])
+
+load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary")
+
+genrule(
+ name = "cargo_build_debug",
+ srcs = glob([
+ "src/**/*.rs",
+ "Cargo.toml",
+ ]),
+ outs = [
+ "rust_lib_debug.a",
+ "rust_bridge_cc.cc",
+ "rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ ],
+ cmd = """
+ set -e
+ EXECROOT=$$(pwd)
+ OUTPUT_LIB=$(location rust_lib_debug.a)
+ OUTPUT_CC=$(location rust_bridge_cc.cc)
+ OUTPUT_H=$(location rust_bridge_h.h)
+ OUTPUT_SRC_H=$(location src/lib.rs.h)
+ OUTPUT_CXX_H=$(location cxxbridge/rust/cxx.h)
+ # Resolve real source path from sandbox symlink
+ SANDBOX_CARGO=$(location Cargo.toml)
+ REAL_CARGO=$$(readlink -f $$SANDBOX_CARGO 2>/dev/null || python3 -c
"import os; print(os.path.realpath('$$SANDBOX_CARGO'))")
+ CARGO_DIR=$$(dirname $$REAL_CARGO)
+ # Find Cargo workspace root (fluss-rust directory, 2 levels up from
bindings/cpp)
+ WORKSPACE_ROOT=$$(cd $$CARGO_DIR/../.. && pwd)
+ if [ ! -f $$WORKSPACE_ROOT/Cargo.toml ]; then
+ echo "Error: Cannot find workspace root Cargo.toml at
$$WORKSPACE_ROOT" >&2
+ exit 1
+ fi
+ cd $$WORKSPACE_ROOT
+ cargo build --manifest-path $$CARGO_DIR/Cargo.toml
+ CARGO_TARGET_DIR=$$WORKSPACE_ROOT/target
+ RUST_BRIDGE_DIR=$$CARGO_TARGET_DIR/cxxbridge/fluss-cpp/src
+ RUST_LIB=$$CARGO_TARGET_DIR/debug/libfluss_cpp.a
+ if [ ! -f $$RUST_LIB ]; then
+ echo "Error: Rust library not found at $$RUST_LIB" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.cc ]; then
+ echo "Error: cxxbridge CC file not found at
$$RUST_BRIDGE_DIR/lib.rs.cc" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.h ]; then
+ echo "Error: cxxbridge header file not found at
$$RUST_BRIDGE_DIR/lib.rs.h" >&2
+ exit 1
+ fi
+ cd $$EXECROOT
+ mkdir -p $$(dirname $$OUTPUT_SRC_H) $$(dirname $$OUTPUT_CXX_H)
+ cp $$RUST_LIB $$OUTPUT_LIB || (echo "Failed to copy $$RUST_LIB to
$$OUTPUT_LIB" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.cc $$OUTPUT_CC || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.cc to $$OUTPUT_CC" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_H" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_SRC_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_SRC_H" >&2; exit 1)
+ CXX_H_SOURCE=$$CARGO_TARGET_DIR/cxxbridge/rust/cxx.h
+ if [ ! -f $$CXX_H_SOURCE ] && [ ! -L $$CXX_H_SOURCE ]; then
+ echo "Error: cxx.h not found at $$CXX_H_SOURCE" >&2
+ exit 1
+ fi
+ cp -L $$CXX_H_SOURCE $$OUTPUT_CXX_H || (echo "Failed to copy
$$CXX_H_SOURCE to $$OUTPUT_CXX_H" >&2; exit 1)
+ """,
+ message = "Building Rust library with cargo...",
+ local = 1,
+)
+
+cc_import(
+ name = "rust_lib",
+ static_library = ":rust_lib_debug.a",
+ alwayslink = True,
+)
+
+cc_library(
+ name = "fluss_cpp",
+ srcs = [
+ "src/admin.cpp",
+ "src/connection.cpp",
+ "src/table.cpp",
+ ":rust_bridge_cc.cc",
+ ],
+ hdrs = [
+ "include/fluss.hpp",
+ ],
+ textual_hdrs = [
+ ":rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ "src/ffi_converter.hpp",
+ ],
+ strip_include_prefix = "include",
+ copts = [
+ "-std=c++17",
+ "-g3",
+ "-O0",
+ "-ggdb",
+ "-fno-omit-frame-pointer",
+ "-DDEBUG",
+ ],
+ includes = [
+ "src",
+ "cxxbridge",
+ ],
+ linkopts = [
+ "-g",
+ "-ldl",
+ "-lpthread",
+ ] + select({
+ "@platforms//os:macos": [
+ "-framework", "CoreFoundation",
+ "-framework", "Security",
+ ],
+ "//conditions:default": [],
+ }),
+ deps = [
+ ":rust_lib",
+ ],
+ visibility = ["//visibility:public"],
+)
+
+cc_binary(
+ name = "fluss_cpp_example",
+ srcs = [
+ "examples/example.cpp",
+ ],
+ deps = [":fluss_cpp"],
+ copts = [
+ "-std=c++17",
+ "-g3",
+ "-O0",
+ "-ggdb",
+ "-fno-omit-frame-pointer",
+ "-DDEBUG",
+ ],
Review Comment:
Similar to the cc_library rule, debug compilation flags are hardcoded in the
cc_binary rule. This prevents building optimized example binaries. Consider
making these flags configurable to match the library's build configuration.
##########
bindings/cpp/BUILD.bazel:
##########
@@ -0,0 +1,142 @@
+licenses(["notice"])
+
+load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary")
+
+genrule(
+ name = "cargo_build_debug",
+ srcs = glob([
+ "src/**/*.rs",
+ "Cargo.toml",
+ ]),
+ outs = [
+ "rust_lib_debug.a",
+ "rust_bridge_cc.cc",
+ "rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ ],
+ cmd = """
+ set -e
+ EXECROOT=$$(pwd)
+ OUTPUT_LIB=$(location rust_lib_debug.a)
+ OUTPUT_CC=$(location rust_bridge_cc.cc)
+ OUTPUT_H=$(location rust_bridge_h.h)
+ OUTPUT_SRC_H=$(location src/lib.rs.h)
+ OUTPUT_CXX_H=$(location cxxbridge/rust/cxx.h)
+ # Resolve real source path from sandbox symlink
+ SANDBOX_CARGO=$(location Cargo.toml)
+ REAL_CARGO=$$(readlink -f $$SANDBOX_CARGO 2>/dev/null || python3 -c
"import os; print(os.path.realpath('$$SANDBOX_CARGO'))")
+ CARGO_DIR=$$(dirname $$REAL_CARGO)
+ # Find Cargo workspace root (fluss-rust directory, 2 levels up from
bindings/cpp)
+ WORKSPACE_ROOT=$$(cd $$CARGO_DIR/../.. && pwd)
+ if [ ! -f $$WORKSPACE_ROOT/Cargo.toml ]; then
+ echo "Error: Cannot find workspace root Cargo.toml at
$$WORKSPACE_ROOT" >&2
+ exit 1
+ fi
+ cd $$WORKSPACE_ROOT
+ cargo build --manifest-path $$CARGO_DIR/Cargo.toml
+ CARGO_TARGET_DIR=$$WORKSPACE_ROOT/target
+ RUST_BRIDGE_DIR=$$CARGO_TARGET_DIR/cxxbridge/fluss-cpp/src
+ RUST_LIB=$$CARGO_TARGET_DIR/debug/libfluss_cpp.a
+ if [ ! -f $$RUST_LIB ]; then
+ echo "Error: Rust library not found at $$RUST_LIB" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.cc ]; then
+ echo "Error: cxxbridge CC file not found at
$$RUST_BRIDGE_DIR/lib.rs.cc" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.h ]; then
+ echo "Error: cxxbridge header file not found at
$$RUST_BRIDGE_DIR/lib.rs.h" >&2
+ exit 1
+ fi
+ cd $$EXECROOT
+ mkdir -p $$(dirname $$OUTPUT_SRC_H) $$(dirname $$OUTPUT_CXX_H)
+ cp $$RUST_LIB $$OUTPUT_LIB || (echo "Failed to copy $$RUST_LIB to
$$OUTPUT_LIB" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.cc $$OUTPUT_CC || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.cc to $$OUTPUT_CC" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_H" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_SRC_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_SRC_H" >&2; exit 1)
+ CXX_H_SOURCE=$$CARGO_TARGET_DIR/cxxbridge/rust/cxx.h
+ if [ ! -f $$CXX_H_SOURCE ] && [ ! -L $$CXX_H_SOURCE ]; then
+ echo "Error: cxx.h not found at $$CXX_H_SOURCE" >&2
+ exit 1
+ fi
+ cp -L $$CXX_H_SOURCE $$OUTPUT_CXX_H || (echo "Failed to copy
$$CXX_H_SOURCE to $$OUTPUT_CXX_H" >&2; exit 1)
+ """,
+ message = "Building Rust library with cargo...",
+ local = 1,
+)
Review Comment:
The genrule name 'cargo_build_debug' suggests this only builds debug
variants, but there's no corresponding rule for release builds. This means
users can only build debug binaries with Bazel, which limits flexibility
compared to the CMake build that supports both debug and release
configurations. Consider adding a configuration or additional genrule to
support release builds.
##########
bindings/cpp/.bazelrc:
##########
@@ -0,0 +1,15 @@
+# Bazel configuration for fluss-rust C++ bindings
+
+# Enable BzlMod
+common --enable_bzlmod
+
+# Debug configuration (matches BUILD.bazel settings)
+build:debug --compilation_mode=dbg
+build:debug --copt=-g3
+build:debug --copt=-ggdb
+build:debug --copt=-O0
+build:debug --copt=-fno-omit-frame-pointer
+build:debug --copt=-DDEBUG
+build:debug --strip=never
+build:debug --linkopt=-g
+
Review Comment:
Trailing blank line at the end of the file. Consider removing this for
consistency.
```suggestion
```
##########
bindings/cpp/BUILD.bazel:
##########
@@ -0,0 +1,142 @@
+licenses(["notice"])
+
+load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary")
+
+genrule(
+ name = "cargo_build_debug",
+ srcs = glob([
+ "src/**/*.rs",
+ "Cargo.toml",
+ ]),
+ outs = [
+ "rust_lib_debug.a",
+ "rust_bridge_cc.cc",
+ "rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ ],
+ cmd = """
+ set -e
+ EXECROOT=$$(pwd)
+ OUTPUT_LIB=$(location rust_lib_debug.a)
+ OUTPUT_CC=$(location rust_bridge_cc.cc)
+ OUTPUT_H=$(location rust_bridge_h.h)
+ OUTPUT_SRC_H=$(location src/lib.rs.h)
+ OUTPUT_CXX_H=$(location cxxbridge/rust/cxx.h)
+ # Resolve real source path from sandbox symlink
+ SANDBOX_CARGO=$(location Cargo.toml)
+ REAL_CARGO=$$(readlink -f $$SANDBOX_CARGO 2>/dev/null || python3 -c
"import os; print(os.path.realpath('$$SANDBOX_CARGO'))")
+ CARGO_DIR=$$(dirname $$REAL_CARGO)
+ # Find Cargo workspace root (fluss-rust directory, 2 levels up from
bindings/cpp)
+ WORKSPACE_ROOT=$$(cd $$CARGO_DIR/../.. && pwd)
+ if [ ! -f $$WORKSPACE_ROOT/Cargo.toml ]; then
+ echo "Error: Cannot find workspace root Cargo.toml at
$$WORKSPACE_ROOT" >&2
+ exit 1
+ fi
+ cd $$WORKSPACE_ROOT
+ cargo build --manifest-path $$CARGO_DIR/Cargo.toml
+ CARGO_TARGET_DIR=$$WORKSPACE_ROOT/target
+ RUST_BRIDGE_DIR=$$CARGO_TARGET_DIR/cxxbridge/fluss-cpp/src
+ RUST_LIB=$$CARGO_TARGET_DIR/debug/libfluss_cpp.a
+ if [ ! -f $$RUST_LIB ]; then
+ echo "Error: Rust library not found at $$RUST_LIB" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.cc ]; then
+ echo "Error: cxxbridge CC file not found at
$$RUST_BRIDGE_DIR/lib.rs.cc" >&2
+ exit 1
+ fi
+ if [ ! -f $$RUST_BRIDGE_DIR/lib.rs.h ]; then
+ echo "Error: cxxbridge header file not found at
$$RUST_BRIDGE_DIR/lib.rs.h" >&2
+ exit 1
+ fi
+ cd $$EXECROOT
+ mkdir -p $$(dirname $$OUTPUT_SRC_H) $$(dirname $$OUTPUT_CXX_H)
+ cp $$RUST_LIB $$OUTPUT_LIB || (echo "Failed to copy $$RUST_LIB to
$$OUTPUT_LIB" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.cc $$OUTPUT_CC || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.cc to $$OUTPUT_CC" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_H" >&2; exit 1)
+ cp $$RUST_BRIDGE_DIR/lib.rs.h $$OUTPUT_SRC_H || (echo "Failed to copy
$$RUST_BRIDGE_DIR/lib.rs.h to $$OUTPUT_SRC_H" >&2; exit 1)
+ CXX_H_SOURCE=$$CARGO_TARGET_DIR/cxxbridge/rust/cxx.h
+ if [ ! -f $$CXX_H_SOURCE ] && [ ! -L $$CXX_H_SOURCE ]; then
+ echo "Error: cxx.h not found at $$CXX_H_SOURCE" >&2
+ exit 1
+ fi
+ cp -L $$CXX_H_SOURCE $$OUTPUT_CXX_H || (echo "Failed to copy
$$CXX_H_SOURCE to $$OUTPUT_CXX_H" >&2; exit 1)
+ """,
+ message = "Building Rust library with cargo...",
+ local = 1,
+)
+
+cc_import(
+ name = "rust_lib",
+ static_library = ":rust_lib_debug.a",
+ alwayslink = True,
+)
+
+cc_library(
+ name = "fluss_cpp",
+ srcs = [
+ "src/admin.cpp",
+ "src/connection.cpp",
+ "src/table.cpp",
+ ":rust_bridge_cc.cc",
+ ],
+ hdrs = [
+ "include/fluss.hpp",
+ ],
+ textual_hdrs = [
+ ":rust_bridge_h.h",
+ "src/lib.rs.h",
+ "cxxbridge/rust/cxx.h",
+ "src/ffi_converter.hpp",
+ ],
+ strip_include_prefix = "include",
+ copts = [
+ "-std=c++17",
+ "-g3",
+ "-O0",
+ "-ggdb",
+ "-fno-omit-frame-pointer",
+ "-DDEBUG",
+ ],
Review Comment:
Debug compilation flags are hardcoded in the cc_library rule. The
BUILD.bazel file always compiles with debug flags (-g3, -O0, -ggdb, -DDEBUG)
even though a .bazelrc file with a 'debug' configuration exists. This prevents
users from building optimized release binaries. Consider using select()
statements or configuration flags to allow switching between debug and release
builds, similar to how the CMake build supports CMAKE_BUILD_TYPE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
