build: Add CMake config and libraries #13
Conversation
Add catch2 as submodules and include fmt and spdlog as dependencies in cmake. Add basic project structure like separation of core files with components.
WalkthroughThe pull request introduces significant changes to the project, including the addition of a new submodule, Catch2, for unit testing. Modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
🧹 Outside diff range and nitpick comments (10)
tools/scripts/macOS/install-lib.sh (1)
16-16: Add newline at end of file
POSIX standards require text files to end with a newline character.
pkg-config
+dep-tasks.yaml (1)
1-24: Consider modern dependency management alternatives
While shell scripts provide flexibility, consider these architectural improvements:
- Use CMake FetchContent for C++ dependencies where possible
- Implement version pinning for dependencies
- Consider integrating with platform package managers (Homebrew/apt) for system libraries
- Add dependency verification steps (checksums, signatures)
This would provide better reproducibility and security.
tools/scripts/lib_install/fmtlib.sh (1)
3-7: Add checkinstall to dependencies list
The script uses checkinstall for package creation but it's not listed in the dependencies section.
Apply this diff to update the dependencies:
# Dependencies:
# - cmake
# - curl
# - g++
+# - checkinstall (optional, for creating .deb packages)cmake/Modules/FindLibraryDependencies.cmake (2)
1-9: Documentation could be enhanced with examples and scope clarification
Consider enhancing the documentation by:
- Adding an example usage section
- Clarifying that return variables are set in the parent scope (CMake macro behaviour)
- Documenting the expected format of input libraries (e.g., whether to include 'lib' prefix)
# Find the given libraries ignoring libraries that should be dynamically linked
# Params:
# fld_LIBNAME - Name of the library (without the 'lib' prefix)
# fld_PREFIX - Prefix for created variables
# fld_STATIC_LIBS - List of libraries to find
# Returns:
-# ${fld_LIBNAME}_DYNAMIC_LIBS - List of libraries that should be dynamically linked
-# ${fld_PREFIX}_LIBRARY_DEPENDENCIES - Found libraries
+# ${fld_LIBNAME}_DYNAMIC_LIBS - List of libraries that should be dynamically linked (set in parent scope)
+# ${fld_PREFIX}_LIBRARY_DEPENDENCIES - List of absolute paths to found libraries (set in parent scope)
+# Example:
+# FindStaticLibraryDependencies(mariadb MariaDB "z;crypto;ssl")10-12: Consider making the dynamic libraries list configurable
The hardcoded list of dynamic libraries (libc, libm, libpthread, librt) should be configurable to support different platforms and use cases.
+# Allow override of default dynamic libraries through cache variable
+if(NOT DEFINED SPIDER_DEFAULT_DYNAMIC_LIBS)
+ set(SPIDER_DEFAULT_DYNAMIC_LIBS "c;m;pthread;rt" CACHE STRING
+ "List of libraries that should always be linked dynamically")
+endif()
- set(fld_DYNAMIC_LIBS "c;m;pthread;rt")
+ set(fld_DYNAMIC_LIBS "${SPIDER_DEFAULT_DYNAMIC_LIBS}")tools/scripts/lib_install/mariadb-connector-c.sh (1)
47-53: Enhance OS version detection
The script only supports Ubuntu, but the comment suggests it should be "largely distro-agnostic".
Consider:
- Supporting other major distributions (Debian, CentOS, etc.)
- Using package manager detection instead of distribution checks
- Documenting supported distributions in the script header
tools/scripts/lib_install/spdlog.sh (1)
19-29: Enhance path handling safety
The path handling could be more robust by using quotes around variables and ensuring clean paths.
- if [ ! -d ${deb_output_dir} ] ; then
+ if [ ! -d "${deb_output_dir}" ] ; thenCMakeLists.txt (2)
38-41: Add documentation for SOURCE_PATH_SIZE usage
The macro's purpose isn't immediately clear. Consider adding a comment explaining how this definition is used to create relative paths from __FILE__ macros.
# Macro providing the length of the absolute source directory path so we can
# create a relative (rather than absolute) __FILE__ macro
+# This is used to strip the build path prefix from __FILE__ at compile time,
+# making debug output and logs more readable and portable
string(LENGTH "${CMAKE_SOURCE_DIR}/" SOURCE_PATH_SIZE)
add_definitions("-DSOURCE_PATH_SIZE=${SOURCE_PATH_SIZE}")80-106: Enhance error messages with installation instructions
The error messages could be more helpful by including specific installation instructions.
if(fmt_FOUND)
message(STATUS "Found fmt ${fmt_VERSION}")
else()
- message(FATAL_ERROR "Could not find static libraries for fmt")
+ message(FATAL_ERROR "Could not find fmt ${fmt_VERSION}. Please run tools/scripts/lib_install/<dist_name>/install-packages-from-source.sh or install fmt-devel package.")
endif()
# Find and setup spdlog
if(SPIDER_USE_STATIC_LIBS)
# NOTE: On some Linux distributions (e.g. Ubuntu), the spdlog package only contains a dynamic
# library. If the `find_package(spdlog)` call below fails, re-run
# `tools/scripts/lib_install/<dist_name>/install-packages-from-source.sh` to build spdlog from
# source.
set(spdlog_USE_STATIC_LIBS ON)
endif()cmake/Modules/FindMariaDBClient.cmake (1)
20-20: Consider adding 'REQUIRED' to 'find_package(PkgConfig)'
Adding the REQUIRED keyword ensures that CMake will terminate with an error message if PkgConfig is not found, preventing subsequent commands from failing unexpectedly.
Apply this diff to make PkgConfig a required dependency:
-find_package(PkgConfig)
+find_package(PkgConfig REQUIRED)📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- .gitmodules (1 hunks)
- CMakeLists.txt (1 hunks)
- cmake/Modules/FindLibraryDependencies.cmake (1 hunks)
- cmake/Modules/FindMariaDBClient.cmake (1 hunks)
- cmake/Modules/Findspdlog.cmake (1 hunks)
- dep-tasks.yaml (1 hunks)
- src/spider/CMakeLists.txt (1 hunks)
- src/spider/storage/MetadataStorage.hpp (1 hunks)
- src/spider/worker/worker.cpp (1 hunks)
- submodules/Catch2 (1 hunks)
- taskfile.yaml (2 hunks)
- tools/scripts/lib_install/fmtlib.sh (1 hunks)
- tools/scripts/lib_install/mariadb-connector-c.sh (1 hunks)
- tools/scripts/lib_install/spdlog.sh (1 hunks)
- tools/scripts/linux/install-lib.sh (1 hunks)
- tools/scripts/macOS/install-lib.sh (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/spider/storage/MetadataStorage.hpp
- src/spider/worker/worker.cpp
- submodules/Catch2
🧰 Additional context used
🪛 Shellcheck
tools/scripts/lib_install/fmtlib.sh
[warning] 79-79: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
[warning] 81-81: Quotes/backslashes in this variable will not be respected.
(SC2090)
tools/scripts/lib_install/mariadb-connector-c.sh
[warning] 82-82: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
[warning] 84-84: Quotes/backslashes in this variable will not be respected.
(SC2090)
tools/scripts/lib_install/spdlog.sh
[warning] 87-87: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
[warning] 89-89: Quotes/backslashes in this variable will not be respected.
(SC2090)
🔇 Additional comments (9)
tools/scripts/linux/install-lib.sh (2)
1-7: LGTM! Proper error handling setup.
The script correctly uses both set -e and set -u for robust error handling, which is essential for installation scripts.
1-14: Consider adding installation verification.
The script should verify that installations were successful by checking for the presence of installed libraries.
dep-tasks.yaml (2)
8-10: Well-structured task orchestration
The main installation task effectively coordinates platform-specific installations through dependencies while maintaining encapsulation with the internal flag.
3-4: Verify the .ROOT_DIR variable definition
The G_SCRIPT_DIR variable depends on .ROOT_DIR, which should be defined in the parent Taskfile or environment.
✅ Verification successful
Let me check the content of these files to verify the ROOT_DIR definition.
Let me check if ROOT_DIR is defined in the utils.yml file since it's imported in taskfile.yaml.
ROOT_DIR variable is used but not explicitly defined
The .ROOT_DIR variable is used in multiple taskfiles (taskfile.yaml, dep-tasks.yaml, lint-tasks.yaml) but its definition is not found in any of these files or the referenced utils.yml. This variable is likely:
- Set by the Task CLI tool itself as a built-in variable
- Defined in the environment
- Set when invoking the task command
Since Task has built-in variables and ROOT_DIR is a common convention that typically points to the repository root, this usage is acceptable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for ROOT_DIR definition in Taskfiles
# Expected: Find ROOT_DIR definition in parent Taskfile
echo "Searching for ROOT_DIR definition in Taskfiles..."
rg -l "ROOT_DIR" --type yaml
Length of output: 182
Script:
#!/bin/bash
# Check the content of taskfile.yaml for ROOT_DIR definition
echo "Content of taskfile.yaml:"
rg "ROOT_DIR" -A 3 -B 3 taskfile.yaml
echo -e "\nContent of lint-tasks.yaml:"
rg "ROOT_DIR" -A 3 -B 3 lint-tasks.yaml
Length of output: 1011
Script:
#!/bin/bash
# Check if utils.yml exists and contains ROOT_DIR definition
echo "Checking utils.yml content:"
fd utils.yml
cat tools/yscope-dev-utils/taskfiles/utils.yml 2>/dev/null || echo "File not found"
Length of output: 167
src/spider/CMakeLists.txt (1)
1-14: Verify CMake configuration and required files
Let's verify the presence of all required files and CMake modules.
✅ Verification successful
CMake configuration and required files are present and correctly structured
All required files specified in the CMake configuration exist in their expected locations:
src/spider/storage/MetadataStorage.hppsrc/spider/worker/worker.cpp
The project also has a proper CMake structure with:
- Root
CMakeLists.txt - CMake modules in
cmake/Modules/directory - Properly organized source directories under
src/spider/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CMake configuration and required files
echo "Checking source files..."
for file in "src/spider/storage/MetadataStorage.hpp" "src/spider/worker/worker.cpp"; do
if [ ! -f "$file" ]; then
echo "ERROR: Missing source file: $file"
exit 1
fi
done
echo "Checking CMake modules..."
fd --type f --glob "Find*.cmake" cmake/modules/ || {
echo "ERROR: No CMake Find modules found in cmake/modules/"
exit 1
}
echo "Checking for main CMakeLists.txt..."
if [ ! -f "CMakeLists.txt" ]; then
echo "ERROR: Missing root CMakeLists.txt"
exit 1
fi
Length of output: 669
Script:
#!/bin/bash
# Let's verify the existence of files and CMake structure more thoroughly
# Check if the spider directory exists and list its contents
echo "=== Spider Directory Structure ==="
fd . src/spider -t d
# Check for specific source files
echo -e "\n=== Source Files Check ==="
fd . src/spider -t f -e hpp -e cpp
# Check for CMake files
echo -e "\n=== CMake Files Check ==="
fd . -t f -e cmake -e txt | grep -i cmake
Length of output: 622
taskfile.yaml (2)
5-5: LGTM! Good separation of dependency management.
The inclusion of a separate dependency task file aligns well with the modular organization of build configurations.
28-28: LGTM! Proper dependency ordering.
Making the CMake configuration dependent on library installation ensures all required dependencies are available before project configuration begins.
Let's verify the dependency task file exists and contains the expected lib_install task:
✅ Verification successful
Dependency chain verification successful
The dependency ordering is correctly implemented:
config-cmake-projecttask depends ondeps:lib_installdeps:lib_installtask depends on bothlib_install_macandlib_install_linux- The tasks are properly defined in their respective files (taskfile.yaml and dep-tasks.yaml)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dep-tasks.yaml exists and contains lib_install task
# Expected: File exists and contains lib_install task definition
if [ -f "dep-tasks.yaml" ]; then
echo "dep-tasks.yaml exists"
# Search for lib_install task definition
rg "lib_install:" dep-tasks.yaml -A 5
else
echo "Error: dep-tasks.yaml not found"
exit 1
fi
Length of output: 242
Script:
#!/bin/bash
# Let's verify the full dependency chain by checking deps:lib_install and its sub-tasks
cat taskfile.yaml
Length of output: 895
CMakeLists.txt (1)
122-124: Verify directory structure existence
Ensure that the referenced directories exist and contain their own CMakeLists.txt files.
✅ Verification successful
Directory structure and CMake files are properly set up
The verification script confirmed that both src/spider and tests directories exist and contain their respective CMakeLists.txt files, validating the project structure referenced in the root CMakeLists.txt.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the required directories and their CMake files exist
echo "Checking directory structure..."
for dir in "src/spider" "tests"; do
if [ ! -d "$dir" ]; then
echo "ERROR: Directory $dir does not exist"
fi
if [ ! -f "$dir/CMakeLists.txt" ]; then
echo "ERROR: $dir/CMakeLists.txt is missing"
fi
done
Length of output: 272
cmake/Modules/FindMariaDBClient.cmake (1)
17-17: Verify inclusion of 'FindLibraryDependencies.cmake'
Please ensure that FindLibraryDependencies.cmake exists in the specified path cmake/Modules/ and is correctly included. Missing this file could lead to build errors.
You can run the following script to confirm the presence of the file:
✅ Verification successful
File exists at the specified path - no issues found
The verification confirms that FindLibraryDependencies.cmake exists at the correct path cmake/Modules/FindLibraryDependencies.cmake, ensuring proper inclusion in the build system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'FindLibraryDependencies.cmake' exists in 'cmake/Modules/'
# Test: Verify the file exists. Expect: Path to the file is displayed.
fd 'FindLibraryDependencies.cmake' cmake/Modules/
Length of output: 93
| [submodule "submodules/Catch2"] | ||
| path = submodules/Catch2 | ||
| url = https://github.com/catchorg/Catch2.git |
There was a problem hiding this comment.
💡 Codebase verification
Submodule initialization needs attention
The Catch2 submodule is present but appears to be missing its integration:
- The submodule is initialized but not referenced in CMake files
- No test files currently use the Catch2 framework
- The submodule commit hash differs from the initialized state
Please ensure:
- Add proper CMake integration for Catch2
- Create test files that utilize the framework
- Run
git submodule update --initto sync the submodule
🔗 Analysis chain
LGTM! The Catch2 submodule is properly configured.
The addition follows Git submodule best practices with proper path organization and secure URL protocol.
Let's verify the submodule initialization:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Catch2 submodule initialization and version
# Test: Check if submodule is properly initialized
git submodule status submodules/Catch2
# Test: Verify Catch2 version matches the one mentioned in AI summary
cd submodules/Catch2 2>/dev/null && git rev-parse HEAD
Length of output: 185
Script:
#!/bin/bash
# Let's verify if the submodule is properly referenced in CMake files
# since the PR objectives mention adding CMake config
# Search for Catch2 references in CMake files
fd -e cmake -e txt --exec rg -l "Catch2" {}
# Check if there's a proper add_subdirectory for Catch2
rg "add_subdirectory.*Catch2" -A 2 -B 2
# Look for test files that might use Catch2
fd -e cpp -e h --exec rg -l "catch2|CATCH2" {}
Length of output: 138
| #!/usr/bin/env bash | ||
|
|
||
| # Exit on any error | ||
| set -e | ||
|
|
||
| # Error on undefined variable | ||
| set -u | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance script robustness with prerequisite checks
The script should verify prerequisites before proceeding with installations:
#!/usr/bin/env bash
# Exit on any error
set -e
# Error on undefined variable
set -u
+# Verify running on macOS
+if [[ "$(uname)" != "Darwin" ]]; then
+ echo "Error: This script is for macOS only" >&2
+ exit 1
+fi
+
+# Check if Homebrew is installed
+if ! command -v brew >/dev/null 2>&1; then
+ echo "Error: Homebrew is required but not installed" >&2
+ echo "Install from https://brew.sh" >&2
+ exit 1
+fi
+
+# Cleanup function
+cleanup() {
+ if [ $? -ne 0 ]; then
+ echo "Installation failed" >&2
+ fi
+}
+trap cleanup EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/usr/bin/env bash | |
| # Exit on any error | |
| set -e | |
| # Error on undefined variable | |
| set -u | |
| #!/usr/bin/env bash | |
| # Exit on any error | |
| set -e | |
| # Error on undefined variable | |
| set -u | |
| # Verify running on macOS | |
| if [[ "$(uname)" != "Darwin" ]]; then | |
| echo "Error: This script is for macOS only" >&2 | |
| exit 1 | |
| fi | |
| # Check if Homebrew is installed | |
| if ! command -v brew >/dev/null 2>&1; then | |
| echo "Error: Homebrew is required but not installed" >&2 | |
| echo "Install from https://brew.sh" >&2 | |
| exit 1 | |
| fi | |
| # Cleanup function | |
| cleanup() { | |
| if [ $? -ne 0 ]; then | |
| echo "Installation failed" >&2 | |
| fi | |
| } | |
| trap cleanup EXIT |
| brew update | ||
| brew install \ | ||
| boost \ | ||
| coreutils \ | ||
| fmt \ | ||
| mariadb-connector-c \ | ||
| spdlog \ | ||
| pkg-config |
There was a problem hiding this comment.
Improve package management reliability
The current implementation could lead to inconsistent builds across different environments:
- Add version pinning for reproducible builds
- Add installation verification
- Provide progress feedback
-brew update
-brew install \
- boost \
- coreutils \
- fmt \
- mariadb-connector-c \
- spdlog \
- pkg-config
+echo "Updating Homebrew..."
+brew update
+
+# Define package versions
+declare -A packages=(
+ ["boost"]="1.82.0"
+ ["coreutils"]="9.3"
+ ["fmt"]="10.1.1"
+ ["mariadb-connector-c"]="3.3.5"
+ ["spdlog"]="1.12.0"
+ ["pkg-config"]="0.29.2"
+)
+
+# Install packages with version pinning
+for pkg in "${!packages[@]}"; do
+ version="${packages[$pkg]}"
+ echo "Installing $pkg version $version..."
+
+ if brew list "$pkg" &>/dev/null; then
+ echo "$pkg is already installed, upgrading if needed..."
+ brew upgrade "$pkg@$version" || brew upgrade "$pkg"
+ else
+ brew install "$pkg@$version" || brew install "$pkg"
+ fi
+
+ # Verify installation
+ if ! brew list "$pkg" &>/dev/null; then
+ echo "Error: Failed to install $pkg" >&2
+ exit 1
+ fi
+done
+
+echo "All packages installed successfully"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| brew update | |
| brew install \ | |
| boost \ | |
| coreutils \ | |
| fmt \ | |
| mariadb-connector-c \ | |
| spdlog \ | |
| pkg-config | |
| echo "Updating Homebrew..." | |
| brew update | |
| # Define package versions | |
| declare -A packages=( | |
| ["boost"]="1.82.0" | |
| ["coreutils"]="9.3" | |
| ["fmt"]="10.1.1" | |
| ["mariadb-connector-c"]="3.3.5" | |
| ["spdlog"]="1.12.0" | |
| ["pkg-config"]="0.29.2" | |
| ) | |
| # Install packages with version pinning | |
| for pkg in "${!packages[@]}"; do | |
| version="${packages[$pkg]}" | |
| echo "Installing $pkg version $version..." | |
| if brew list "$pkg" &>/dev/null; then | |
| echo "$pkg is already installed, upgrading if needed..." | |
| brew upgrade "$pkg@$version" || brew upgrade "$pkg" | |
| else | |
| brew install "$pkg@$version" || brew install "$pkg" | |
| fi | |
| # Verify installation | |
| if ! brew list "$pkg" &>/dev/null; then | |
| echo "Error: Failed to install $pkg" >&2 | |
| exit 1 | |
| fi | |
| done | |
| echo "All packages installed successfully" |
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| lib_install_scripts_dir=$script_dir/.. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding path validation.
While the directory resolution is robust, it would be beneficial to verify that the target installation scripts exist before proceeding.
script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
lib_install_scripts_dir=$script_dir/..
+
+# Verify required scripts exist
+for script in "lib_install/fmtlib.sh" "lib_install/spdlog.sh" "lib_install/mariadb-connector-c.sh"; do
+ if [[ ! -f "${lib_install_scripts_dir}/${script}" ]]; then
+ echo "Error: Required script not found: ${script}" >&2
+ exit 1
+ fi
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | |
| lib_install_scripts_dir=$script_dir/.. | |
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | |
| lib_install_scripts_dir=$script_dir/.. | |
| # Verify required scripts exist | |
| for script in "lib_install/fmtlib.sh" "lib_install/spdlog.sh" "lib_install/mariadb-connector-c.sh"; do | |
| if [[ ! -f "${lib_install_scripts_dir}/${script}" ]]; then | |
| echo "Error: Required script not found: ${script}" >&2 | |
| exit 1 | |
| fi | |
| done |
| "$lib_install_scripts_dir"/lib_install/fmtlib.sh 11.0.2 | ||
| "$lib_install_scripts_dir"/lib_install/spdlog.sh 1.14.1 | ||
| "$lib_install_scripts_dir"/lib_install/mariadb-connector-c.sh 3.4.1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the installation process.
The current implementation has several areas for improvement:
- Hardcoded versions make updates difficult
- No installation status logging
- Sequential installation could be slow for multiple libraries
Consider these improvements:
-"$lib_install_scripts_dir"/lib_install/fmtlib.sh 11.0.2
-"$lib_install_scripts_dir"/lib_install/spdlog.sh 1.14.1
-"$lib_install_scripts_dir"/lib_install/mariadb-connector-c.sh 3.4.1
+# Define versions in a central location
+declare -A LIB_VERSIONS=(
+ ["fmtlib"]="11.0.2"
+ ["spdlog"]="1.14.1"
+ ["mariadb-connector-c"]="3.4.1"
+)
+
+# Function to install a library
+install_library() {
+ local lib=$1
+ local version=${LIB_VERSIONS[$lib]}
+ echo "Installing $lib version $version..."
+ if "$lib_install_scripts_dir/lib_install/${lib}.sh" "$version"; then
+ echo "$lib installation completed successfully"
+ else
+ echo "Error: Failed to install $lib" >&2
+ return 1
+ fi
+}
+
+# Install libraries with basic logging
+for lib in "${!LIB_VERSIONS[@]}"; do
+ install_library "$lib" || exit 1
+doneCommittable suggestion was skipped due to low confidence.
| include(FindPackageHandleStandardArgs) | ||
| find_package_handle_standard_args(MariaDBClient | ||
| REQUIRED_VARS MariaDBClient_INCLUDE_DIR | ||
| VERSION_VAR MariaDBClient_VERSION | ||
| ) |
There was a problem hiding this comment.
Add 'MariaDBClient_LIBRARY' to 'REQUIRED_VARS' in 'find_package_handle_standard_args'
Including MariaDBClient_LIBRARY in the REQUIRED_VARS ensures that both the include directory and library are validated during the configuration process. This prevents potential issues if the library is not found.
Apply this diff to include MariaDBClient_LIBRARY as a required variable:
find_package_handle_standard_args(MariaDBClient
- REQUIRED_VARS MariaDBClient_INCLUDE_DIR
+ REQUIRED_VARS MariaDBClient_INCLUDE_DIR MariaDBClient_LIBRARY
VERSION_VAR MariaDBClient_VERSION
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| include(FindPackageHandleStandardArgs) | |
| find_package_handle_standard_args(MariaDBClient | |
| REQUIRED_VARS MariaDBClient_INCLUDE_DIR | |
| VERSION_VAR MariaDBClient_VERSION | |
| ) | |
| include(FindPackageHandleStandardArgs) | |
| find_package_handle_standard_args(MariaDBClient | |
| REQUIRED_VARS MariaDBClient_INCLUDE_DIR MariaDBClient_LIBRARY | |
| VERSION_VAR MariaDBClient_VERSION | |
| ) |
| FindDynamicLibraryDependencies(mariadbclient "${mariadbclient_DYNAMIC_LIBS}") | ||
|
|
There was a problem hiding this comment.
Ensure 'mariadbclient_DYNAMIC_LIBS' is defined before use
The variable mariadbclient_DYNAMIC_LIBS may not be set before being used in FindDynamicLibraryDependencies. Please verify that this variable is correctly defined prior to this call to avoid potential errors.
If mariadbclient_DYNAMIC_LIBS is intended to be populated from pkg-config, ensure it is assigned appropriately. For example:
+set(mariadbclient_DYNAMIC_LIBS ${mariadbclient_PKGCONF_LIBRARIES})
FindDynamicLibraryDependencies(mariadbclient "${mariadbclient_DYNAMIC_LIBS}")Committable suggestion was skipped due to low confidence.
cmake/Modules/Findspdlog.cmake
Outdated
| # Find library | ||
| find_library( | ||
| SPDLOG_LIBRARY | ||
| NAMES ${SPDLOG_LIBNAME} | ||
| HINTS ${SPDLOG_PKGCONF_LIBDIR} | ||
| PATH_SUFFIXES lib | ||
| ) | ||
| if (SPDLOG_LIBRARY) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider treating spdlog as a header-only library
spdlog is primarily a header-only library and often doesn't require linking against a separate library file. Searching for the library might cause issues if only the headers are installed. Consider adjusting the script to handle spdlog as a header-only library by skipping the library search and focusing on finding the headers.
cmake/Modules/Findspdlog.cmake
Outdated
| set_target_properties( | ||
| ${SPDLOG_TARGET_NAME} | ||
| PROPERTIES | ||
| IMPORTED_LINK_INTERFACE_LANGUAGES "C" |
There was a problem hiding this comment.
Correct IMPORTED_LINK_INTERFACE_LANGUAGES to 'CXX' for C++ library
The property IMPORTED_LINK_INTERFACE_LANGUAGES is set to "C", but spdlog is a C++ library. It should be set to "CXX" to ensure proper linkage in CMake.
Apply this diff to correct the language setting:
- IMPORTED_LINK_INTERFACE_LANGUAGES "C"
+ IMPORTED_LINK_INTERFACE_LANGUAGES "CXX"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IMPORTED_LINK_INTERFACE_LANGUAGES "C" | |
| IMPORTED_LINK_INTERFACE_LANGUAGES "CXX" |
cmake/Modules/Findspdlog.cmake
Outdated
| ${SPDLOG_LIBNAME} | ||
| REQUIRED_VARS spdlog_INCLUDE_DIR | ||
| VERSION_VAR spdlog_VERSION | ||
| ) |
There was a problem hiding this comment.
Include spdlog_LIBRARY in REQUIRED_VARS for validity check
Currently, find_package_handle_standard_args is called with REQUIRED_VARS spdlog_INCLUDE_DIR only. To ensure that both the include directory and the library are found, consider adding spdlog_LIBRARY to the REQUIRED_VARS list.
Apply this diff to include spdlog_LIBRARY:
find_package_handle_standard_args(
${SPDLOG_LIBNAME}
REQUIRED_VARS spdlog_INCLUDE_DIR
+ spdlog_LIBRARY
VERSION_VAR spdlog_VERSION
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ${SPDLOG_LIBNAME} | |
| REQUIRED_VARS spdlog_INCLUDE_DIR | |
| VERSION_VAR spdlog_VERSION | |
| ) | |
| ${SPDLOG_LIBNAME} | |
| REQUIRED_VARS spdlog_INCLUDE_DIR | |
| spdlog_LIBRARY | |
| VERSION_VAR spdlog_VERSION | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/code-linting-checks.yaml (1)
48-49: Add a descriptive name for the task step
The current task name " " is not descriptive. Consider using a more meaningful name that describes what the step does.
- - name: "task "
+ - name: "Install project dependencies"📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/code-linting-checks.yaml (1 hunks)
- dep-tasks.yaml (1 hunks)
- taskfile.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dep-tasks.yaml
- taskfile.yaml
| - name: "task " | ||
| run: "task deps:lib_install" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding timeout and error handling
For robustness, consider adding timeout and error handling settings to the dependency installation step. Also, implementing dependency caching could improve workflow execution time.
- name: "Install project dependencies"
+ timeout-minutes: 10
+ continue-on-error: false
run: "task deps:lib_install"
+
+ - name: "Cache dependencies"
+ uses: actions/cache@v3
+ with:
+ path: |
+ ~/.cache/spider-deps
+ key: ${{ runner.os }}-deps-${{ hashFiles('**/CMakeLists.txt') }}
+ restore-keys: |
+ ${{ runner.os }}-deps-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: "task " | |
| run: "task deps:lib_install" | |
| - name: "Install project dependencies" | |
| timeout-minutes: 10 | |
| continue-on-error: false | |
| run: "task deps:lib_install" | |
| - name: "Cache dependencies" | |
| uses: actions/cache@v3 | |
| with: | |
| path: | | |
| ~/.cache/spider-deps | |
| key: ${{ runner.os }}-deps-${{ hashFiles('**/CMakeLists.txt') }} | |
| restore-keys: | | |
| ${{ runner.os }}-deps- |
There was a problem hiding this comment.
Don't forget to add new files to the linting rules as described in the README.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
cmake/Modules/FindMariaDBClient.cmake (1)
69-70: Add version constraints
Consider adding minimum version requirements to ensure compatibility with the project's needs.
# Set version
set(MariaDBClient_VERSION ${mariadbclient_PKGCONF_VERSION})
+
+# Verify minimum required version
+if(MariaDBClient_VERSION VERSION_LESS "10.5.0")
+ message(WARNING "MariaDB Client version ${MariaDBClient_VERSION} might be too old. Recommended version is 10.5.0 or newer.")
+endif()📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- cmake/Modules/FindLibraryDependencies.cmake (1 hunks)
- cmake/Modules/FindMariaDBClient.cmake (1 hunks)
- cmake/Modules/Findspdlog.cmake (1 hunks)
- dep-tasks.yaml (1 hunks)
- lint-tasks.yaml (2 hunks)
- src/spider/spider.cpp (0 hunks)
- tests/CMakeLists.txt (1 hunks)
💤 Files with no reviewable changes (1)
- src/spider/spider.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- cmake/Modules/FindLibraryDependencies.cmake
- cmake/Modules/Findspdlog.cmake
- dep-tasks.yaml
🔇 Additional comments (2)
lint-tasks.yaml (2)
108-108: LGTM: Appropriate inclusion of dependency configuration for linting
The addition of dep-tasks.yaml to the yamllint command list ensures proper validation of the new dependency management configurations.
139-148: LGTM: Well-structured CMake linting configuration
The restructuring provides a more maintainable and explicit approach to CMake file linting. Let's verify the completeness of the CMake file coverage.
✅ Verification successful
CMake linting configuration is complete and accurate
The verification confirms that all CMake files in the repository are properly covered by the linting configuration:
- All
CMakeLists.txtfiles are explicitly listed in the sources - All
.cmakefiles incmake/Modulesare covered by the wildcard patterncmake/Modules/*.cmake
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all CMake files are included in the linting configuration
# Expected: No CMake files outside the specified patterns
# Find all CMake files in the repository
echo "=== CMake files in repository ==="
fd -e cmake -e txt --type f CMakeLists
# Find all files in cmake/Modules
echo -e "\n=== Files in cmake/Modules ==="
fd . "cmake/Modules" -e cmake
Length of output: 399
| # Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix | ||
| set(CMAKE_FIND_LIBRARY_SUFFIXES .a) | ||
| endif() |
There was a problem hiding this comment.
Consider platform-specific static library suffixes
The current implementation only handles .a files, which might not work on all platforms. Consider using CMAKE_STATIC_LIBRARY_SUFFIX instead.
- set(CMAKE_FIND_LIBRARY_SUFFIXES .a)
+ set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_STATIC_LIBRARY_SUFFIX})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix | |
| set(CMAKE_FIND_LIBRARY_SUFFIXES .a) | |
| endif() | |
| # Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix | |
| set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_STATIC_LIBRARY_SUFFIX}) | |
| endif() |
| find_path( | ||
| MariaDBClient_INCLUDE_DIR | ||
| mysql.h | ||
| HINTS | ||
| ${mariadbclient_PKGCONF_INCLUDEDIR} | ||
| PATH_SUFFIXES | ||
| mariadb | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for pkg-config results
Consider validating the pkg-config results before proceeding with the path search. This would provide clearer error messages when the library isn't properly configured in the system.
# Set include directory
+if(NOT mariadbclient_PKGCONF_FOUND)
+ message(DEBUG "pkg-config could not find lib${mariadbclient_LIBNAME}")
+endif()
+
find_path(
MariaDBClient_INCLUDE_DIR
mysql.h📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find_path( | |
| MariaDBClient_INCLUDE_DIR | |
| mysql.h | |
| HINTS | |
| ${mariadbclient_PKGCONF_INCLUDEDIR} | |
| PATH_SUFFIXES | |
| mariadb | |
| ) | |
| if(NOT mariadbclient_PKGCONF_FOUND) | |
| message(DEBUG "pkg-config could not find lib${mariadbclient_LIBNAME}") | |
| endif() | |
| find_path( | |
| MariaDBClient_INCLUDE_DIR | |
| mysql.h | |
| HINTS | |
| ${mariadbclient_PKGCONF_INCLUDEDIR} | |
| PATH_SUFFIXES | |
| mariadb | |
| ) |
| if(MariaDBClient_FOUND) | ||
| if(MariaDBClient_USE_STATIC_LIBS) | ||
| add_library(MariaDBClient::MariaDBClient STATIC IMPORTED) | ||
| else() | ||
| # NOTE: We use UNKNOWN so that if the user doesn't have the SHARED | ||
| # libraries installed, we can still use the STATIC libraries | ||
| add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add IMPORTED_SONAME property for shared libraries
When using shared libraries, it's important to set the IMPORTED_SONAME property to ensure proper runtime linking.
else()
# NOTE: We use UNKNOWN so that if the user doesn't have the SHARED
# libraries installed, we can still use the STATIC libraries
add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED)
+ if(NOT MariaDBClient_USE_STATIC_LIBS)
+ get_filename_component(_soname "${MariaDBClient_LIBRARY}" NAME)
+ set_target_properties(MariaDBClient::MariaDBClient
+ PROPERTIES
+ IMPORTED_SONAME "${_soname}")
+ endif()
endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(MariaDBClient_FOUND) | |
| if(MariaDBClient_USE_STATIC_LIBS) | |
| add_library(MariaDBClient::MariaDBClient STATIC IMPORTED) | |
| else() | |
| # NOTE: We use UNKNOWN so that if the user doesn't have the SHARED | |
| # libraries installed, we can still use the STATIC libraries | |
| add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED) | |
| endif() | |
| endif() | |
| if(MariaDBClient_FOUND) | |
| if(MariaDBClient_USE_STATIC_LIBS) | |
| add_library(MariaDBClient::MariaDBClient STATIC IMPORTED) | |
| else() | |
| # NOTE: We use UNKNOWN so that if the user doesn't have the SHARED | |
| # libraries installed, we can still use the STATIC libraries | |
| add_library(MariaDBClient::MariaDBClient UNKNOWN IMPORTED) | |
| if(NOT MariaDBClient_USE_STATIC_LIBS) | |
| get_filename_component(_soname "${MariaDBClient_LIBRARY}" NAME) | |
| set_target_properties(MariaDBClient::MariaDBClient | |
| PROPERTIES | |
| IMPORTED_SONAME "${_soname}") | |
| endif() | |
| endif() | |
| endif() |
Description
Add basic code structure and cmake build files. Add
spdlog,fmtandmariadb-connector-cas dependencies.Validation performed
lint:checkSummary by CodeRabbit
Release Notes
New Features
fmtlib,spdlog, andMariaDB Connector/Cfor both Linux and macOS environments.Improvements
spdlogandMariaDBClient.