build: Add mariadb-connector-cpp library #18
Conversation
Replace mariadb-connector-c library with mariadb-connector-cpp.
WalkthroughThe pull request introduces significant changes 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: 10
🧹 Outside diff range and nitpick comments (2)
tools/scripts/lib_install/mariadb-connector-cpp.sh (1)
1-72: Add logging and progress informationThe script would benefit from better progress information and logging capabilities.
Add a logging function at the beginning of the script:
+# Logging function +log() { + echo "[$(date +'%Y-%m-%d %H:%M:%S')] $*" +} + +log "Starting installation of ${package_name} version ${version}"Then replace echo statements throughout the script with the log function.
CMakeLists.txt (1)
131-131: Maintain consistent error message formattingConsider updating the error message to match the format used for other dependencies in this file. For example, fmt's error message is more concise.
- message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClientCpp") + message(FATAL_ERROR "Could not find libraries for MariaDBClientCpp")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
CMakeLists.txt(1 hunks)cmake/Modules/FindMariaDBClientCpp.cmake(1 hunks)src/spider/CMakeLists.txt(1 hunks)tools/scripts/lib_install/mariadb-connector-c.sh(0 hunks)tools/scripts/lib_install/mariadb-connector-cpp.sh(1 hunks)tools/scripts/linux/install-lib.sh(1 hunks)tools/scripts/macOS/install-lib.sh(1 hunks)
💤 Files with no reviewable changes (1)
- tools/scripts/lib_install/mariadb-connector-c.sh
🔇 Additional comments (8)
tools/scripts/macOS/install-lib.sh (3)
15-16: LGTM! Robust script directory resolution
The implementation correctly handles script path resolution, including cases with spaces in paths.
17-17: 🛠️ Refactor suggestion
Consider making the MariaDB connector version configurable
The MariaDB connector version (1.1.5) is hardcoded. Consider making it configurable through an environment variable or command-line argument for easier version management.
-"${lib_install_scripts_dir}"/lib_install/mariadb-connector-cpp.sh 1.1.5
+MARIADB_CPP_VERSION="${MARIADB_CPP_VERSION:-1.1.5}"
+"${lib_install_scripts_dir}"/lib_install/mariadb-connector-cpp.sh "${MARIADB_CPP_VERSION}"Also, verify that the referenced script exists:
#!/bin/bash
# Verify the existence of the MariaDB connector installation script
if ! [ -f "tools/scripts/lib_install/mariadb-connector-cpp.sh" ]; then
echo "Error: MariaDB connector installation script not found"
exit 1
fi
# Check if the script is executable
if ! [ -x "tools/scripts/lib_install/mariadb-connector-cpp.sh" ]; then
echo "Warning: MariaDB connector installation script is not executable"
fi13-13: Verify the removal of coreutils dependency
The removal of coreutils from the installation list might impact scripts that rely on GNU versions of core utilities. Please ensure that no scripts in the project depend on GNU-specific features not available in macOS's BSD utilities.
✅ Verification successful
Removal of coreutils is safe for macOS builds
The shell script analysis shows that the commands used across the build scripts are compatible with BSD utilities provided by macOS. The commands found (mkdir, rm, curl, tar, etc.) use standard options that work identically on both GNU and BSD implementations. The only GNU-specific flag found is --recursive in a git command, which is not part of coreutils.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for common GNU coreutils commands that might have BSD-incompatible usage
rg -g '*.sh' -i '\b(sed|awk|find|sort|grep|tr|cut|head|tail|wc|ls|cp|mv|rm|cat|chmod|chown|date|df|du|ln|mkdir|rmdir|touch|uniq|basename|dirname|readlink|realpath|stat|test|expr|seq|split|csplit|join|paste|expand|unexpand|pr|fold|column|nl|od|base64|sha[0-9]+sum|md5sum|sum|cksum|comm|ptx|tsort|shuf|timeout|truncate|stdbuf|tee|mktemp|sponge)\b.*-[a-zA-Z]' -A 2 -B 2
Length of output: 315
Script:
#!/bin/bash
# Let's try a simpler approach to find GNU-specific command usage
# Search for common GNU-specific options
rg -g '*.sh' '\s-[A-Za-z]+\s' --type sh
# Also check for specific GNU extensions
rg -g '*.sh' '(--version|--help|--verbose|--recursive|--preserve|--force|--interactive)' --type sh
Length of output: 4631
tools/scripts/linux/install-lib.sh (2)
Line range hint 10-15: Review dependency installation order
The current installation order places MariaDB Connector/C++ before Boost. If the connector requires Boost, we should install Boost first.
#!/bin/bash
# Description: Check for Boost dependencies in the MariaDB Connector/C++ build
# Test: Search for Boost requirements in the installation script
rg -B 5 -A 5 "find_package.*Boost" "$lib_install_scripts_dir/lib_install/mariadb-connector-cpp.sh"
# Test: Check CMake files for Boost dependencies
fd CMakeLists.txt -x rg -B 5 -A 5 "find_package.*Boost.*REQUIRED"Consider moving the Boost installation before MariaDB Connector/C++ if a dependency exists.
14-14: Verify MariaDB Connector/C++ version compatibility
Please ensure that version 1.1.5 of MariaDB Connector/C++ is compatible with the other libraries, particularly Boost 1.86.0.
✅ Verification successful
MariaDB Connector/C++ version 1.1.5 appears compatible with project requirements
Based on the codebase analysis:
- The project requires MariaDB Connector/C++ version 1.0 or higher (
find_package(MariaDBClientCpp 1.0 REQUIRED)) - Boost 1.74 is the minimum required version (
find_package(Boost 1.74 REQUIRED)) - Version 1.1.5 of MariaDB Connector/C++ is newer than the minimum requirement and is built from source with the latest Boost
The installation script properly handles:
- Dependencies through git submodules
- CMake-based build process
- System-level installation with proper privileges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version compatibility requirements in the MariaDB Connector/C++ installation script
# Test: Search for version requirements in the installation script
rg -A 5 "boost|version" "$lib_install_scripts_dir/lib_install/mariadb-connector-cpp.sh"
# Test: Check if there are any version-specific CMake configurations
fd -e cmake -x rg -A 5 "MariaDBClient(Cpp)?.*VERSION"
Length of output: 689
Script:
#!/bin/bash
# Let's try to find and examine the MariaDB Connector/C++ installation script
fd mariadb-connector-cpp.sh -x cat {}
# Also check for any CMake files that might specify version requirements
fd CMakeLists.txt -x rg -A 5 "MariaDB|Boost"
# Check if there are any dependency documentation files
fd -g "README*" -g "DEPENDENCIES*" -g "INSTALL*" -x rg -i "mariadb|boost"
Length of output: 3088
tools/scripts/lib_install/mariadb-connector-cpp.sh (1)
1-72: Verify system requirements
The script should verify system requirements before proceeding with the installation.
Let's check the system requirements:
CMakeLists.txt (2)
127-131: Verify MariaDB C++ connector version requirement
The version requirement has been reduced from 3.1.0 (C connector) to 1.0 (C++ connector). Please verify if this version provides all the required functionality for the project.
✅ Verification successful
Version requirement for MariaDB C++ connector is appropriate
The version requirement of 1.0 for MariaDBClientCpp is correct for the following reasons:
- The codebase shows a straightforward usage pattern through
spider_corelibrary - The CMake configuration properly handles the library dependency and licensing concerns
- The version numbering between C and C++ connectors follows different schemes, making direct version comparison inappropriate
- The warning about GPL licensing restrictions is already properly addressed in the CMake file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check MariaDB C++ connector version compatibility
# Look for any version-specific API usage in the codebase
# Search for MariaDB C++ connector API usage
rg -l "mariadb::"
Length of output: 17
Script:
#!/bin/bash
# Let's search for MariaDB-related code patterns more broadly
# Search for MariaDB includes
rg -l -g "*.{cpp,h,hpp}" "include.*mariadb"
# Search for SQL-related code that might use MariaDB
rg -l -g "*.{cpp,h,hpp}" "(?i)mysql|mariadb"
# Check if there are any example or documentation files
fd -g "README*" -X cat {}
Length of output: 2642
Script:
#!/bin/bash
# Let's examine the MariaDB-related code to understand the requirements
rg -A 10 -B 10 "MariaDB|mysql" src/spider/storage/MysqlStorage.hpp
# Also check the implementation file
rg -A 10 -B 10 "MariaDB|mysql" src/spider/storage/MysqlStorage.cpp
# Check CMake files for any additional context about MariaDB requirements
fd -g "CMakeLists.txt" -X cat {}
Length of output: 5314
127-131: Update static linking warning for MariaDB C++ connector
The existing GPL license warning for static linking (line 125) should be verified and potentially updated for the C++ connector, as licensing terms might differ between the C and C++ connectors.
✅ Verification successful
Let me gather more information about the MariaDB C++ connector's licensing.
Let me check one more specific location for licensing information.
No update needed for MariaDB C++ connector licensing warning
The existing warning about static linking is already correctly placed before finding the MariaDB C++ connector package. Both the C and C++ connectors are subject to the same GPL licensing restrictions that prevent static linking. The current implementation appropriately warns about this limitation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for licensing information in the repository
# Look for license files or documentation mentioning MariaDB C++ connector
# Search for license-related files
fd -g "*LICENSE*" -g "*COPYING*"
# Search for MariaDB licensing mentions in documentation
rg -i "mariadb.*licen[cs]"
Length of output: 4127
Script:
#!/bin/bash
# Check for licensing information specifically about the C++ connector
rg -i "mariadb.*c\+\+.*licen[cs]|mariadb.*connector.*licen[cs]"
# Check contents of install scripts for any licensing information
cat tools/scripts/lib_install/mariadb-connector-cpp.sh
# Look for any README files that might contain licensing info
fd -g "*README*" -x cat {}
Length of output: 4199
Script:
#!/bin/bash
# Check CMakeLists.txt around the mentioned line for context about the licensing warning
rg -B5 -A5 "MariaDBClient cannot be statically linked" CMakeLists.txt
# Check if there's any distinction between C and C++ connector licensing warnings
rg -i "static.*link.*mariadb" CMakeLists.txt
Length of output: 582
| target_link_libraries( | ||
| spider_core | ||
| Boost::boost | ||
| absl::flat_hash_map | ||
| MariaDBClientCpp::MariaDBClientCpp | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add visibility specifiers to target_link_libraries.
The target_link_libraries command should explicitly specify the visibility (PUBLIC/PRIVATE) for each dependency to prevent unintended exposure of internal dependencies to consumers of spider_core.
Consider applying this change:
target_link_libraries(
spider_core
- Boost::boost
- absl::flat_hash_map
- MariaDBClientCpp::MariaDBClientCpp
+ PUBLIC
+ Boost::boost
+ PRIVATE
+ absl::flat_hash_map
+ MariaDBClientCpp::MariaDBClientCpp
)Note: MariaDBClientCpp is listed last as it likely depends on Boost. Consider adding a comment to document this dependency order requirement if it's critical for static linking.
📝 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.
| target_link_libraries( | |
| spider_core | |
| Boost::boost | |
| absl::flat_hash_map | |
| MariaDBClientCpp::MariaDBClientCpp | |
| ) | |
| target_link_libraries( | |
| spider_core | |
| PUBLIC | |
| Boost::boost | |
| PRIVATE | |
| absl::flat_hash_map | |
| MariaDBClientCpp::MariaDBClientCpp | |
| ) |
| # Clean up | ||
| rm -rf $temp_dir |
There was a problem hiding this comment.
Add error handling for cleanup
The cleanup process should handle errors gracefully and ensure the temporary directory is removed even if the script fails.
Replace the cleanup with:
-# Clean up
-rm -rf $temp_dir
+# Clean up
+cleanup() {
+ if [ -d "$temp_dir" ]; then
+ echo "Cleaning up temporary files..."
+ rm -rf "$temp_dir"
+ 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.
| # Clean up | |
| rm -rf $temp_dir | |
| # Clean up | |
| cleanup() { | |
| if [ -d "$temp_dir" ]; then | |
| echo "Cleaning up temporary files..." | |
| rm -rf "$temp_dir" | |
| fi | |
| } | |
| trap cleanup EXIT |
| #!/bin/bash | ||
|
|
||
| # Dependencies: | ||
| # - curl | ||
| # - rsync | ||
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | ||
|
|
There was a problem hiding this comment.
Add version check for dependencies
While the script lists curl and rsync as dependencies, it doesn't verify their presence before execution. This could lead to cryptic failures later in the script.
Add these checks after line 7:
+# Check dependencies
+for cmd in curl rsync; do
+ if ! command -v "$cmd" >/dev/null 2>&1; then
+ echo "Error: $cmd is required but not installed."
+ 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.
| #!/bin/bash | |
| # Dependencies: | |
| # - curl | |
| # - rsync | |
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | |
| #!/bin/bash | |
| # Dependencies: | |
| # - curl | |
| # - rsync | |
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | |
| # Check dependencies | |
| for cmd in curl rsync; do | |
| if ! command -v "$cmd" >/dev/null 2>&1; then | |
| echo "Error: $cmd is required but not installed." | |
| exit 1 | |
| fi | |
| done |
| mkdir build | ||
| cd build | ||
| cmake .. | ||
| make -j${num_cpus} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add build configuration options
The cmake configuration lacks important build options that might be needed for different use cases.
Enhance the build configuration:
-cmake ..
+cmake .. \
+ -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+ -DCONC_WITH_UNIT_TESTS=Off \
+ -DINSTALL_LAYOUT=RPM \
+ -DCMAKE_INSTALL_PREFIX=/usr📝 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.
| mkdir build | |
| cd build | |
| cmake .. | |
| make -j${num_cpus} | |
| mkdir build | |
| cd build | |
| cmake .. \ | |
| -DCMAKE_BUILD_TYPE=RelWithDebInfo \ | |
| -DCONC_WITH_UNIT_TESTS=Off \ | |
| -DINSTALL_LAYOUT=RPM \ | |
| -DCMAKE_INSTALL_PREFIX=/usr | |
| make -j${num_cpus} |
| mkdir -p $temp_dir | ||
| cd $temp_dir | ||
| git clone https://github.com/mariadb-corporation/mariadb-connector-cpp.git "mariadb-connector-cpp-${version}" | ||
| cd "mariadb-connector-cpp-${version}" | ||
| git checkout "${version}" | ||
| git submodule update --init --recursive |
There was a problem hiding this comment.
Add integrity verification for downloaded code
The script downloads and builds from source without verifying the integrity of the downloaded code.
Add checksum verification after the clone:
git clone https://github.com/mariadb-corporation/mariadb-connector-cpp.git "mariadb-connector-cpp-${version}"
cd "mariadb-connector-cpp-${version}"
git checkout "${version}"
+# Verify the tag
+if ! git verify-tag "${version}" 2>/dev/null; then
+ echo "Warning: Unable to verify signature of version ${version}"
+ read -p "Continue anyway? (y/N) " -n 1 -r
+ echo
+ if [[ ! $REPLY =~ ^[Yy]$ ]]; then
+ exit 1
+ fi
+fi
git submodule update --init --recursive📝 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.
| mkdir -p $temp_dir | |
| cd $temp_dir | |
| git clone https://github.com/mariadb-corporation/mariadb-connector-cpp.git "mariadb-connector-cpp-${version}" | |
| cd "mariadb-connector-cpp-${version}" | |
| git checkout "${version}" | |
| git submodule update --init --recursive | |
| mkdir -p $temp_dir | |
| cd $temp_dir | |
| git clone https://github.com/mariadb-corporation/mariadb-connector-cpp.git "mariadb-connector-cpp-${version}" | |
| cd "mariadb-connector-cpp-${version}" | |
| git checkout "${version}" | |
| # Verify the tag | |
| if ! git verify-tag "${version}" 2>/dev/null; then | |
| echo "Warning: Unable to verify signature of version ${version}" | |
| read -p "Continue anyway? (y/N) " -n 1 -r | |
| echo | |
| if [[ ! $REPLY =~ ^[Yy]$ ]]; then | |
| exit 1 | |
| fi | |
| fi | |
| git submodule update --init --recursive |
| cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]" | ||
| if [ "$#" -lt 1 ] ; then | ||
| echo $cUsage | ||
| exit | ||
| fi | ||
| version=$1 | ||
|
|
There was a problem hiding this comment.
Improve argument validation
The script could benefit from more robust argument validation.
Replace the current validation with:
-cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]"
-if [ "$#" -lt 1 ] ; then
- echo $cUsage
- exit
-fi
-version=$1
+cUsage="Usage: ${BASH_SOURCE[0]} <version> [<.deb output directory>]"
+if [ "$#" -lt 1 ] || [ "$#" -gt 2 ]; then
+ echo "$cUsage"
+ exit 1
+fi
+
+version="$1"
+if [[ ! "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
+ echo "Error: Version must be in format X.Y.Z"
+ exit 1
+fi📝 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.
| cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]" | |
| if [ "$#" -lt 1 ] ; then | |
| echo $cUsage | |
| exit | |
| fi | |
| version=$1 | |
| cUsage="Usage: ${BASH_SOURCE[0]} <version> [<.deb output directory>]" | |
| if [ "$#" -lt 1 ] || [ "$#" -gt 2 ]; then | |
| echo "$cUsage" | |
| exit 1 | |
| fi | |
| version="$1" | |
| if [[ ! "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | |
| echo "Error: Version must be in format X.Y.Z" | |
| exit 1 | |
| fi |
| # Check if already installed | ||
| set +e | ||
| dpkg -l ${package_name} | grep ${version} | ||
| installed=$? | ||
| set -e | ||
| if [ $installed -eq 0 ] ; then | ||
| # Nothing to do | ||
| exit | ||
| fi |
There was a problem hiding this comment.
Enhance package version check logic
The current dpkg check might miss some edge cases and the error handling could be improved.
Replace the current check with:
-# Check if already installed
-set +e
-dpkg -l ${package_name} | grep ${version}
-installed=$?
-set -e
-if [ $installed -eq 0 ] ; then
- # Nothing to do
- exit
-fi
+# Check if already installed
+if command -v dpkg >/dev/null 2>&1; then
+ if dpkg -l "${package_name}" 2>/dev/null | grep -q "^ii.*${version}"; then
+ echo "${package_name} version ${version} is already installed"
+ exit 0
+ fi
+elif command -v brew >/dev/null 2>&1; then
+ if brew list "${package_name}" &>/dev/null; then
+ installed_version=$(brew list "${package_name}" --versions | awk '{print $2}')
+ if [ "${installed_version}" = "${version}" ]; then
+ echo "${package_name} version ${version} is already installed"
+ exit 0
+ fi
+ fi
+fi📝 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.
| # Check if already installed | |
| set +e | |
| dpkg -l ${package_name} | grep ${version} | |
| installed=$? | |
| set -e | |
| if [ $installed -eq 0 ] ; then | |
| # Nothing to do | |
| exit | |
| fi | |
| # Check if already installed | |
| if command -v dpkg >/dev/null 2>&1; then | |
| if dpkg -l "${package_name}" 2>/dev/null | grep -q "^ii.*${version}"; then | |
| echo "${package_name} version ${version} is already installed" | |
| exit 0 | |
| fi | |
| elif command -v brew >/dev/null 2>&1; then | |
| if brew list "${package_name}" &>/dev/null; then | |
| installed_version=$(brew list "${package_name}" --versions | awk '{print $2}') | |
| if [ "${installed_version}" = "${version}" ]; then | |
| echo "${package_name} version ${version} is already installed" | |
| exit 0 | |
| fi | |
| fi | |
| fi |
| find_package(MariaDBClientCpp 1.0 REQUIRED) | ||
| if(MariaDBClientCpp_FOUND) | ||
| message(STATUS "Found MariaDBClientCpp ${MariaDBClientCpp_VERSION}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClient") | ||
| message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClientCpp") |
There was a problem hiding this comment.
💡 Codebase verification
Document MariaDB C++ connector build dependencies
The MariaDB C++ connector installation script (tools/scripts/lib_install/mariadb-connector-cpp.sh) only lists curl and rsync as dependencies, but misses critical build dependencies required for compilation. The script clones and builds from source, which requires:
- CMake
- C++ compiler toolchain
- Git (for cloning)
- OpenSSL development libraries
Suggested locations to document these requirements:
- Add build dependencies to the comment section in
tools/scripts/lib_install/mariadb-connector-cpp.sh - Update the main README.md's Requirements section to include system-level dependencies
🔗 Analysis chain
Verify C++ connector dependencies
The MariaDB C++ connector might have additional system dependencies compared to the C connector. Consider documenting these requirements in the installation scripts.
Let me check the installation scripts specifically for MariaDB C++ connector dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dependency documentation
# Look for installation scripts and documentation
# Search for installation scripts
fd -g "*install*.sh"
# Search for dependency documentation
rg -i "prerequisites|dependencies|requirements"
Length of output: 4127
Script:
#!/bin/bash
# Check MariaDB C++ connector installation script content
cat tools/scripts/lib_install/mariadb-connector-cpp.sh
# Check main installation scripts that might reference MariaDB dependencies
cat tools/scripts/linux/install-lib.sh
cat tools/scripts/macOS/install-lib.sh
# Check README for any MariaDB-specific requirements
cat README.md
Length of output: 4968
| @@ -0,0 +1,133 @@ | |||
| # Try to find MariaDBClient | |||
| # NOTE: The FindMariaDBClientCpp.cmake included with CMake has no support for static libraries, so we use our own. | |||
There was a problem hiding this comment.
Correct the module name in the comment
The comment on line 2 refers to FindMariaDBClientCpp.cmake included with CMake. However, CMake does not include a FindMariaDBClientCpp.cmake module by default. The standard module is FindMariaDB.cmake or FindMariaDBClient.cmake. Please update the comment to reflect the correct module name.
Apply this diff to fix the comment:
-# NOTE: The FindMariaDBClientCpp.cmake included with CMake has no support for static libraries, so we use our own.
+# NOTE: The default FindMariaDB.cmake included with CMake has no support for static libraries, so we use our own FindMariaDBClientCpp.cmake module.📝 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.
| # NOTE: The FindMariaDBClientCpp.cmake included with CMake has no support for static libraries, so we use our own. | |
| # NOTE: The default FindMariaDB.cmake included with CMake has no support for static libraries, so we use our own FindMariaDBClientCpp.cmake module. |
| if(MariaDBClientCpp_USE_STATIC_LIBS) | ||
| # Save current value of CMAKE_FIND_LIBRARY_SUFFIXES | ||
| set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES}) | ||
|
|
||
| # Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix | ||
| set(CMAKE_FIND_LIBRARY_SUFFIXES .a) | ||
| else() | ||
| # mariadb-connector-cpp uses .dylib for dynamic library, at least on macOS | ||
| set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES}) | ||
| set(CMAKE_FIND_LIBRARY_SUFFIXES | ||
| .so | ||
| .dylib | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
Fix the incorrect saving of CMAKE_FIND_LIBRARY_SUFFIXES
You're overwriting mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES in both the if and else blocks, causing the original value to be lost. This may lead to incorrect restoration of CMAKE_FIND_LIBRARY_SUFFIXES later. To resolve this, save the original value before the if statement.
Apply this diff to fix the issue:
+# Save current value of CMAKE_FIND_LIBRARY_SUFFIXES
+set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
if(MariaDBClientCpp_USE_STATIC_LIBS)
- # Save current value of CMAKE_FIND_LIBRARY_SUFFIXES
- set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
# Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix
set(CMAKE_FIND_LIBRARY_SUFFIXES .a)
else()
- # Save current value of CMAKE_FIND_LIBRARY_SUFFIXES
- set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
# `mariadb-connector-cpp` uses `.dylib` for dynamic libraries, at least on macOS
set(CMAKE_FIND_LIBRARY_SUFFIXES
.so
.dylib
)
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(MariaDBClientCpp_USE_STATIC_LIBS) | |
| # Save current value of CMAKE_FIND_LIBRARY_SUFFIXES | |
| set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES}) | |
| # Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix | |
| set(CMAKE_FIND_LIBRARY_SUFFIXES .a) | |
| else() | |
| # mariadb-connector-cpp uses .dylib for dynamic library, at least on macOS | |
| set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES}) | |
| set(CMAKE_FIND_LIBRARY_SUFFIXES | |
| .so | |
| .dylib | |
| ) | |
| endif() | |
| # Save current value of CMAKE_FIND_LIBRARY_SUFFIXES | |
| set(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES}) | |
| if(MariaDBClientCpp_USE_STATIC_LIBS) | |
| # Temporarily change CMAKE_FIND_LIBRARY_SUFFIXES to static library suffix | |
| set(CMAKE_FIND_LIBRARY_SUFFIXES .a) | |
| else() | |
| # mariadb-connector-cpp uses .dylib for dynamic library, at least on macOS | |
| set(CMAKE_FIND_LIBRARY_SUFFIXES | |
| .so | |
| .dylib | |
| ) | |
| endif() |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmake/Modules/FindMariaDBClientCpp.cmake(1 hunks)tools/scripts/lib_install/mariadb-connector-cpp.sh(1 hunks)
🔇 Additional comments (1)
cmake/Modules/FindMariaDBClientCpp.cmake (1)
33-46: Duplicate issue: Incorrect saving of CMAKE_FIND_LIBRARY_SUFFIXES
As previously mentioned, you are overwriting mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES in both the if and else blocks, which may lead to incorrect restoration later. Consider saving the original value before the if statement to ensure proper restoration.
| echo "Checking for elevated privileges..." | ||
| privileged_command_prefix="" | ||
| if [ ${EUID:-$(id -u)} -ne 0 ] ; then | ||
| sudo echo "Script can elevate privileges." | ||
| privileged_command_prefix="${privileged_command_prefix} sudo" | ||
| fi |
There was a problem hiding this comment.
Enhance privilege escalation handling
The current sudo check lacks proper error handling and could fail silently.
Apply this diff to improve the privilege handling:
echo "Checking for elevated privileges..."
privileged_command_prefix=""
if [ ${EUID:-$(id -u)} -ne 0 ] ; then
- sudo echo "Script can elevate privileges."
+ if ! sudo -v; then
+ echo "Error: This script requires sudo privileges to install packages"
+ exit 1
+ fi
+ echo "Script can elevate privileges."
privileged_command_prefix="${privileged_command_prefix} sudo"
fi📝 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.
| echo "Checking for elevated privileges..." | |
| privileged_command_prefix="" | |
| if [ ${EUID:-$(id -u)} -ne 0 ] ; then | |
| sudo echo "Script can elevate privileges." | |
| privileged_command_prefix="${privileged_command_prefix} sudo" | |
| fi | |
| echo "Checking for elevated privileges..." | |
| privileged_command_prefix="" | |
| if [ ${EUID:-$(id -u)} -ne 0 ] ; then | |
| if ! sudo -v; then | |
| echo "Error: This script requires sudo privileges to install packages" | |
| exit 1 | |
| fi | |
| echo "Script can elevate privileges." | |
| privileged_command_prefix="${privileged_command_prefix} sudo" | |
| fi |
| package_name=mariadb-connector-cpp | ||
| temp_dir=/tmp/${package_name}-installation | ||
| deb_output_dir=${temp_dir} |
There was a problem hiding this comment.
Secure temporary directory creation
The temporary directory is created without proper permissions, which could lead to security issues.
Apply this diff to improve security:
package_name=mariadb-connector-cpp
-temp_dir=/tmp/${package_name}-installation
+temp_dir=$(mktemp -d -t "${package_name}-XXXXXX")
+# Ensure proper permissions
+chmod 700 "${temp_dir}"
deb_output_dir=${temp_dir}📝 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.
| package_name=mariadb-connector-cpp | |
| temp_dir=/tmp/${package_name}-installation | |
| deb_output_dir=${temp_dir} | |
| package_name=mariadb-connector-cpp | |
| temp_dir=$(mktemp -d -t "${package_name}-XXXXXX") | |
| # Ensure proper permissions | |
| chmod 700 "${temp_dir}" | |
| deb_output_dir=${temp_dir} |
| # Build | ||
| mkdir build | ||
| cd build | ||
| cmake .. | ||
| make -j${num_cpus} |
There was a problem hiding this comment.
Add error handling for build failures
The build commands lack proper error handling and logging.
Apply this diff to improve build reliability:
mkdir build
cd build
+
+echo "Configuring build..."
cmake ..
-make -j${num_cpus}
+
+echo "Building with ${num_cpus} CPU cores..."
+if ! make -j${num_cpus}; then
+ echo "Build failed. Check the error messages above."
+ echo "Trying again with a single core..."
+ if ! make -j1; then
+ echo "Build failed again. Aborting."
+ exit 1
+ fi
+fi📝 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.
| # Build | |
| mkdir build | |
| cd build | |
| cmake .. | |
| make -j${num_cpus} | |
| # Build | |
| mkdir build | |
| cd build | |
| echo "Configuring build..." | |
| cmake .. | |
| echo "Building with ${num_cpus} CPU cores..." | |
| if ! make -j${num_cpus}; then | |
| echo "Build failed. Check the error messages above." | |
| echo "Trying again with a single core..." | |
| if ! make -j1; then | |
| echo "Build failed again. Aborting." | |
| exit 1 | |
| fi | |
| fi |
Description
Add mariadb-connector-cpp library as dependency. Add scripts to install mariadb-connector-cpp library.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Chores