CMake - addressing comments

- Changed clone URL to https version - Updated BUILD_cmake.md - Fixed LTO build error for UT - build.sh script: added --run-test option - Added missing build flags: -DBAZEL_BUILD, -Wno-unknown-pragmas - Ensured that all UT are passing - Do not rebuild on consecutive calls for cmake when nothing was changed - Added missing tests: vmsdk/testing - Reduced CMake requirement to 3.16 Signed-off-by: Eran Ifrah <[email protected]>
valkey-io · Feb 17, 2025 · 88cfc96 · 88cfc96
1 parent f8389e2
commit 88cfc96
Show file tree

Hide file tree

Showing 16 changed files with 241 additions and 164 deletions.
diff --git a/BUILD_cmake.md b/BUILD_cmake.md
@@ -2,7 +2,7 @@
 
 If you wish to work with `Bazel` instead of `CMake`, follow this [guide instead][1]
 
-Note: the below was tested on Ubuntu Linux, it might not work on other Linux distros
+Note: the below was tested on `Ubuntu Linux`, it might not work on other Linux distros
 
 
 ## Install basic tools
@@ -20,45 +20,30 @@ sudo apt install -y clangd          \
 
 ## Build the module
 
+For your convenience, we provide a `build.sh` script. First, clone the code:
+
 ```bash
-git clone git@github.com:valkey-io/valkey-search.git
+git clone https://github.com/valkey-io/valkey-search.git
 cd valkey-search
-git submodule update --remote --init --recursive --depth 1
-mkdir build-debug
-cd $_
-cmake ..
-make -j$(nproc)
 ```
 
-## Integration with IDE
-
-During the `CMake` stage of the build, `CMake` generates a JSON file named `compile_commands.json` and places it under the
-build folder. This file is used by many IDEs and text editors for providing code completion (via `clangd`).
-
-A small caveat is that these tools will look for `compile_commands.json` under the workspace root folder.
-A common workaround is to create a symbolic link to it:
+Next, build the module for the `release` configuration by typing this into your terminal:
 
 ```bash
-cd /path/to/valkey/
-# We assume here that your build folder is `build-debug`
-ln -sf $(pwd)/build-debug/compile_commands.json $(pwd)/compile_commands.json
+./build.sh --run-tests=all
 ```
 
-Another option, is to instruct `clangd` where to search for the `compile_commands.json` by using the `--compile-commands-dir` option:
-
-```bash
-clangd --compile-commands-dir=/path/to/compile_commands.json
-```
+Once the build the completed, all build generated output can be found in `.build-release/` folder.
 
-Restart your IDE and voila
+Tip: use `./build.sh --help` to see more build options
 
 ## Loading the module
 
-After a successful build, the module is placed under `build-debug/lib/valkeysearch.so` (assuming your build folder is `build-debug`),
-to load it into `valkey-server`, you can use this command:
+After a successful build, the module is placed under `.build-release/valkeysearch.so`.
+to load it into `valkey-server`, use the following command:
 
 ```bash
-valkey-server --loadmodule build-debug/lib/valkeysearch.so
+valkey-server --loadmodule .build-release/valkeysearch.so
 ```
 
 

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.24)
+cmake_minimum_required(VERSION 3.16)
 project(VSS)
 
 # Options
@@ -10,10 +10,6 @@ set(CMAKE_CXX_STANDARD 20 REQUIRED)
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 
-set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib")
-set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin")
-set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib")
-
 include_directories(${CMAKE_SOURCE_DIR})
 include_directories(${CMAKE_BINARY_DIR}/.deps/install/include)
 

diff --git a/build.sh b/build.sh
@@ -1,10 +1,11 @@
 #!/bin/bash -e
 
-BUILD_CONFIG=debug
+BUILD_CONFIG=release
 RUN_CMAKE="no"
 ROOT_DIR=$(readlink -f $(dirname $0))
 VERBOSE_ARGS=""
 CMAKE_TARGET=""
+RUN_TEST=""
 
 echo "Root directory: ${ROOT_DIR}"
 
@@ -13,34 +14,37 @@ cat<<EOF
 Usage: build.sh [options...]
 
     --help | -h         Print this help message and exit
-    --cmake             Run cmake stage
+    --configure         Run cmake stage (aka configure stage)
     --verbose | -v      Run verbose build
-    --release           Build for release
+    --debug             Build for debug version
     --clean             Clean the current build configuration (debug or release)
+    --run-tests         Run all tests. Optionally, pass a test name to run: "--run-tests=<test-name>"
 
 Example usage:
-    # run CMake for debug build & build
-    build.sh --configure
 
-    # run CMake for release build & build
-    build.sh --release --configure
+    # Build the release configuration, run cmake if needed
+    build.sh
+
+    # Force run cmake and build the debug configuration
+    build.sh --configure --debug
+
 EOF
 }
 
 function configure() {
-    mkdir -p ${ROOT_DIR}/build-${BUILD_CONFIG}
+    mkdir -p ${ROOT_DIR}/.build-${BUILD_CONFIG}
     cd $_
     local BUILD_TYPE=$(echo ${BUILD_CONFIG^})
-    cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DBUILD_TESTS=ON
+    cmake .. -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DBUILD_TESTS=ON -Wno-dev
     cd ${ROOT_DIR}
 }
 
 function build() {
     # If the build folder does not exist, run cmake
-    if [ ! -d "${ROOT_DIR}/build-${BUILD_CONFIG}" ]; then
+    if [ ! -d "${ROOT_DIR}/.build-${BUILD_CONFIG}" ]; then
         configure
     fi
-    cd ${ROOT_DIR}/build-${BUILD_CONFIG}
+    cd ${ROOT_DIR}/.build-${BUILD_CONFIG}
     make -j$(nproc) ${VERBOSE_ARGS} ${CMAKE_TARGET}
     cd ${ROOT_DIR}
 }
@@ -55,16 +59,26 @@ do
         CMAKE_TARGET="clean"
         echo "Will run 'make clean'"
         ;;
-    --release)
+    --debug)
         shift || true
-        BUILD_CONFIG="release"
-        echo "Building in Release mode"
+        BUILD_CONFIG="debug"
+        echo "Building in Debug mode"
         ;;
-    --cmake)
+    --configure)
         shift || true
         RUN_CMAKE="yes"
         echo "Running cmake: true"
         ;;
+    --run-tests)
+        RUN_TEST="all"
+        shift || true
+        echo "Running all tests"
+        ;;
+    --run-tests=*)
+        RUN_TEST=${1#*=}
+        shift || true
+        echo "Running test ${RUN_TEST}"
+        ;;
     --verbose|-v)
         shift || true
         VERBOSE_ARGS="VERBOSE=1"
@@ -81,7 +95,23 @@ do
     esac
 done
 
+function PRINT_TEST_NAME() {
+    echo -e "\e[35;1m-- Running: $1 \e[0m"
+}
+
 if [[ "${RUN_CMAKE}" == "yes" ]]; then
     configure
 fi
 build
+
+TESTS_DIR=${ROOT_DIR}/.build-${BUILD_CONFIG}/tests
+if [[ "${RUN_TEST}" == "all" ]]; then
+    TESTS=$(ls ${TESTS_DIR}/*_test)
+    for test in $TESTS; do
+        PRINT_TEST_NAME "${test}"
+        ${test} --gtest_color=yes
+    done
+elif [ ! -z "${RUN_TEST}" ]; then
+    PRINT_TEST_NAME "${TESTS_DIR}/${RUN_TEST}"
+    ${TESTS_DIR}/${RUN_TEST} --gtest_color=yes
+fi
diff --git a/cmake/Modules/linux_utils.cmake b/cmake/Modules/linux_utils.cmake
@@ -2,17 +2,9 @@ set(MODULE_BASE_DIR "${CMAKE_BINARY_DIR}/.deps")
 set(MODULES_BIN_DIR "${MODULE_BASE_DIR}/install/bin")
 set(MODULES_LIB_DIR "${CMAKE_BINARY_DIR}/.deps/install/lib")
 
+# Protobuf C++ plugin (needed by coordinator)
 set(grpc_cpp_plugin_location ${MODULES_BIN_DIR}/grpc_cpp_plugin)
-set(Protobuf_PROTOC_EXECUTABLE ${MODULES_BIN_DIR}/protoc)
-
 message(STATUS "gRPC C++ plugin: ${grpc_cpp_plugin_location}")
-message(STATUS "protoc: ${Protobuf_PROTOC_EXECUTABLE}")
-
-# Protobuf compiler and library
-set(protobuf_generate_PROTOC_EXE ${Protobuf_PROTOC_EXECUTABLE})
-message(
-  STATUS
-    "protobuf_generate_PROTOC_EXE is set to ${protobuf_generate_PROTOC_EXE}")
 
 find_library(
   PROTOBUF_LIBS protobuf
@@ -207,7 +199,6 @@ foreach(LIBNAME ${GTEST_LIBS_NAME})
   unset(__LIB CACHE)
 endforeach()
 
-
 find_library(
   LIBZ z REQUIRED
   PATHS ${MODULES_LIB_DIR}

diff --git a/cmake/Modules/protobuf_generate.cmake b/cmake/Modules/protobuf_generate.cmake
@@ -1,3 +1,6 @@
+set(MODULE_BASE_DIR "${CMAKE_BINARY_DIR}/.deps")
+set(MODULES_BIN_DIR "${MODULE_BASE_DIR}/install/bin")
+
 function(protobuf_generate)
   include(CMakeParseArguments)
 
@@ -100,14 +103,8 @@ function(protobuf_generate)
     endforeach()
   endif()
 
-  find_program(protobuf_generate_PROTOC_EXE protoc HINTS /usr/local/bin
-                                                         /usr/bin)
-  if(NOT protobuf_generate_PROTOC_EXE)
-    # Default to using the CMake executable
-    message(WARNING "Using default protobuf::protoc")
-    set(protobuf_generate_PROTOC_EXE protobuf::protoc)
-  endif()
-
+  set(protobuf_generate_PROTOC_EXE ${MODULES_BIN_DIR}/protoc)
+  message(STATUS "Using protoc => ${protobuf_generate_PROTOC_EXE}")
   foreach(DIR ${protobuf_generate_IMPORT_DIRS})
     get_filename_component(ABS_PATH ${DIR} ABSOLUTE)
     list(FIND _protobuf_include_path ${ABS_PATH} _contains_already)
@@ -205,34 +202,18 @@ endfunction()
 # extension). "OUT_LIBNAME" is the user provided target name to create
 function(create_proto_library PROTO_FILE_SRC_DIR PROTO_BASE_FILE_NAME
          OUT_LIBNAME)
-  set(PROTO_OUT_DIR ${CMAKE_BINARY_DIR}/${PROTO_FILE_SRC_DIR})
   set(PROTO_IN_DIR ${CMAKE_SOURCE_DIR}/${PROTO_FILE_SRC_DIR})
-  file(MAKE_DIRECTORY ${PROTO_OUT_DIR})
-  if(NOT EXISTS ${PROTO_OUT}/${PROTO_BASE_FILE_NAME}.pb.h
-     OR NOT EXISTS ${PROTO_OUT_DIR}/${PROTO_BASE_FILE_NAME}.pb.cc)
-    execute_process(
-      COMMAND
-        ${Protobuf_PROTOC_EXECUTABLE} --cpp_out=${PROTO_OUT_DIR}
-        --proto_path=${PROTO_IN_DIR}
-        ${PROTO_IN_DIR}/${PROTO_BASE_FILE_NAME}.proto
-      OUTPUT_VARIABLE PROTOC_OUTPUT
-      OUTPUT_STRIP_TRAILING_WHITESPACE)
-  endif()
+  set(PROTO_OUT_DIR ${CMAKE_BINARY_DIR}/${PROTO_FILE_SRC_DIR})
+  add_library(${OUT_LIBNAME} STATIC
+              "${PROTO_IN_DIR}/${PROTO_BASE_FILE_NAME}.proto")
+  protobuf_generate(TARGET ${OUT_LIBNAME} IMPORT_DIRS "${PROTO_IN_DIR}"
+                    PROTOC_OUT_DIR "${PROTO_OUT_DIR}")
   message(STATUS "Creating target ${OUT_LIBNAME}")
-  set(__LIB_SRCS "${PROTO_OUT_DIR}/${PROTO_BASE_FILE_NAME}.pb.h"
-                 "${PROTO_OUT_DIR}/${PROTO_BASE_FILE_NAME}.pb.cc")
-  add_library(${OUT_LIBNAME} STATIC ${__LIB_SRCS})
-  if(VALKEY_SEARCH_RELEASE_BUILD)
-    message(STATUS "Enabling LTO for target ${OUT_LIBNAME}")
-    set_property(TARGET ${OUT_LIBNAME} PROPERTY INTERPROCEDURAL_OPTIMIZATION
-                                                TRUE)
-  endif()
   valkey_search_target_update_compile_flags(${OUT_LIBNAME})
   target_link_libraries(${OUT_LIBNAME} PUBLIC ${Protobuf_LIBRARIES})
   target_include_directories(${OUT_LIBNAME} PUBLIC ${CMAKE_BINARY_DIR})
   target_include_directories(${OUT_LIBNAME} PUBLIC ${PROTO_OUT_DIR})
   target_include_directories(${OUT_LIBNAME} PUBLIC ${Protobuf_INCLUDE_DIRS})
   unset(PROTO_OUT_DIR CACHE)
   unset(PROTO_IN_DIR CACHE)
-  unset(__LIB_SRCS CACHE)
 endfunction()
diff --git a/cmake/Modules/valkey_search.cmake b/cmake/Modules/valkey_search.cmake
@@ -9,33 +9,27 @@ else()
   message(STATUS "Building in debug mode")
 endif()
 
-# A wrapper around "add_library" (STATIC) that enables the
-# INTERPROCEDURAL_OPTIMIZATION property for release builds ("lto")
+# A wrapper around "add_library" (STATIC) that enables the various build flags
 function(valkey_search_add_static_library name sources)
+  message(STATUS "Adding static library ${name}")
   add_library(${name} STATIC ${sources})
-  if(VALKEY_SEARCH_RELEASE_BUILD)
-    set_property(TARGET ${name} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
-    message(STATUS "Adding static library ${name} - LTO enabled")
-  else()
-    message(STATUS "Adding static library ${name}")
-  endif()
   valkey_search_target_update_compile_flags(${name})
 endfunction()
 
-# A wrapper around "add_library" (SHARED) that enables the
-# INTERPROCEDURAL_OPTIMIZATION property for release builds ("lto")
+# A wrapper around "add_library" (SHARED) that enables the various build flags
 function(valkey_search_add_shared_library name sources)
+  message(STATUS "Adding shared library ${name}")
   add_library(${name} SHARED ${sources})
   if(VALKEY_SEARCH_RELEASE_BUILD)
-    set_property(TARGET ${name} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
-    message(STATUS "Adding shared library ${name} - LTO enabled")
-  else()
-    message(STATUS "Adding shared library ${name}")
+    # Enable full LTO (takes longer to link, but produces faster code)
+    target_link_options(${name} PRIVATE -flto)
   endif()
   valkey_search_target_update_compile_flags(${name})
+  set_target_properties(${name} PROPERTIES LIBRARY_OUTPUT_DIRECTORY
+                                           "${CMAKE_BINARY_DIR}")
 endfunction()
 
-# Setup global compile flags to match BAZEL build flags
+# Setup global compile flags
 function(_add_global_build_flag _FLAG)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${_FLAG}")
 endfunction()
@@ -62,14 +56,43 @@ function(valkey_search_target_update_compile_flags TARGET)
   target_compile_options(${TARGET} PRIVATE -mfma)
   target_compile_options(${TARGET} PRIVATE -ffp-contract=off)
   target_compile_options(${TARGET} PRIVATE -flax-vector-conversions)
+  target_compile_options(${TARGET} PRIVATE -Wno-unknown-pragmas)
+  target_compile_options(${TARGET} PRIVATE -fPIC)
+  target_compile_definitions(${TARGET} PRIVATE BAZEL_BUILD)
   if(VALKEY_SEARCH_DEBUG_BUILD)
     target_compile_options(${TARGET} PRIVATE -O0)
     target_compile_options(${TARGET} PRIVATE -fno-omit-frame-pointer)
     target_compile_definitions(${TARGET} PRIVATE NDEBUG)
+    target_compile_options(${TARGET} PRIVATE -fno-lto)
+  else()
+    # Release build, dump both IR & obj so we can defer the lto to the link time
+    target_compile_options(${TARGET} PRIVATE -ffat-lto-objects)
   endif()
+  target_compile_options(${TARGET} PRIVATE -Wno-sign-compare)
+  target_compile_options(${TARGET} PRIVATE -Wno-uninitialized)
+#  target_compile_options(${TARGET} PRIVATE -Wno-nan-infinity-disabled)
+#  target_compile_options(${TARGET} PRIVATE -Wno-unknown-warning-option)
+#  target_compile_options(${TARGET} PRIVATE -Wno-unused-command-line-argument)
 endfunction()
 
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-missing-requires")
 
 include(protobuf_generate)
 include(linux_utils)
+
+set(TESTING_LIBS_EXTRA ${THIRD_PARTY_LIBS})
+list(APPEND TESTING_LIBS_EXTRA ${GTEST_LIBS})
+
+macro(finalize_test_flags __TARGET)
+  # Note that we only add "--start-group" and not "--end-group". "ld" will add
+  # it as last parameter in the command line. If we add it explicitly, it will
+  # be added somewhere in the middle
+  target_link_options(${__TARGET} PRIVATE "-Wl,--start-group"
+                      "${TESTING_LIBS_EXTRA}")
+  target_link_options(${__TARGET} PRIVATE "-Wl,--allow-multiple-definition")
+  target_link_options(${__TARGET} PRIVATE "-Wl,-S")
+  target_compile_options(${__TARGET} PRIVATE -O1)
+  valkey_search_target_update_compile_flags(${__TARGET})
+  set_target_properties(${__TARGET} PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+                                               "${CMAKE_BINARY_DIR}/tests")
+endmacro()
diff --git a/src/indexes/CMakeLists.txt b/src/indexes/CMakeLists.txt
@@ -1,4 +1,3 @@
-# We need protobuf
 create_proto_library("src" "index_schema" "index_schema_cc_proto")
 message(STATUS "Created proto library index_schema_cc_proto")