Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Add mariadb-connector-cpp library #18

Merged
merged 3 commits into from
Nov 1, 2024
Merged

build: Add mariadb-connector-cpp library #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a4ee5f
storage: Add mariadb connector cpp as dependency
sitaowang1998 Oct 27, 2024
0ca3944
build: Add script to build mariadb-connector-cpp from source
sitaowang1998 Nov 1, 2024
019239a
style: Improve comments for install script and cmake module
sitaowang1998 Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,11 @@ if(SPIDER_USE_STATIC_LIBS)
# NOTE: We can't statically link to MariaDBClient since it's GPL
message(AUTHOR_WARNING "MariaDBClient cannot be statically linked due to its license.")
endif()
find_package(MariaDBClient 3.1.0 REQUIRED)
if(MariaDBClient_FOUND)
message(STATUS "Found MariaDBClient ${MariaDBClient_VERSION}")
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")
Comment on lines +127 to +131
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

endif()

# Add abseil-cpp
Expand Down
132 changes: 132 additions & 0 deletions cmake/Modules/FindMariaDBClientCpp.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
# Try to find MariaDBClientCpp
#
# Set MariaDBClientCpp_USE_STATIC_LIBS=ON to look for static libraries.
#
# Once done this will define:
# MariaDBClientCpp_FOUND - Whether MariaDBClient was found on the system
# MariaDBClientCpp_INCLUDE_DIR - The MariaDBClient include directories
# MariaDBClientCpp_VERSION - The version of MariaDBClient installed on the system
#
# Conventions:
# - Variables only for use within the script are prefixed with "mariadbclientcpp_"
# - Variables that should be externally visible are prefixed with "MariaDBClientCpp_"

set(mariadbclientcpp_LIBNAME "mariadbcpp")

include(cmake/Modules/FindLibraryDependencies.cmake)

# Run pkg-config
find_package(PkgConfig)
pkg_check_modules(mariadbclientcpp_PKGCONF QUIET "lib${mariadbclientcpp_LIBNAME}")

# Set include directory
find_path(
MariaDBClientCpp_INCLUDE_DIR
conncpp.hpp
HINTS
${mariadbclientcpp_PKGCONF_INCLUDEDIR}
PATH_SUFFIXES
mariadb
)

# Handle static libraries
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()
Comment on lines +33 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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()


# Find library
find_library(
MariaDBClientCpp_LIBRARY
NAMES
${mariadbclientcpp_LIBNAME}
HINTS
${mariadbclientcpp_PKGCONF_LIBDIR}
PATH_SUFFIXES
mariadb
)
if(MariaDBClientCpp_LIBRARY)
# NOTE: This must be set for find_package_handle_standard_args to work
set(MariaDBClientCpp_FOUND ON)
endif()

if(MariaDBClientCpp_USE_STATIC_LIBS)
findstaticlibrarydependencies(${mariadbclientcpp_LIBNAME} mariadbclientcpp
"${mariadbclientcpp_PKGCONF_STATIC_LIBRARIES}"
)

# Restore original value of CMAKE_FIND_LIBRARY_SUFFIXES
set(CMAKE_FIND_LIBRARY_SUFFIXES ${mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES})
unset(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES)
else()
set(CMAKE_FIND_LIBRARY_SUFFIXES ${mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES})
unset(mariadbclientcpp_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES)
endif()

finddynamiclibrarydependencies(mariadbclientcpp "${mariadbclientcpp_DYNAMIC_LIBS}")

# Set version
set(MariaDBClientCpp_VERSION ${mariadbclientcpp_PKGCONF_VERSION})

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(
MariaDBClientCpp
REQUIRED_VARS
MariaDBClientCpp_INCLUDE_DIR
VERSION_VAR MariaDBClientCpp_VERSION
)

if(NOT TARGET MariaDBClientCpp::MariaDBClientCpp)
# Add library to build
if(MariaDBClientCpp_FOUND)
if(MariaDBClientCpp_USE_STATIC_LIBS)
add_library(MariaDBClientCpp::MariaDBClientCpp 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(MariaDBClientCpp::MariaDBClientCpp UNKNOWN IMPORTED)
endif()
endif()

# Set include directories for library
if(MariaDBClientCpp_INCLUDE_DIR)
set_target_properties(
MariaDBClientCpp::MariaDBClientCpp
PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES
"${MariaDBClientCpp_INCLUDE_DIR}"
)
endif()

# Set location of library
if(EXISTS "${MariaDBClientCpp_LIBRARY}")
set_target_properties(
MariaDBClientCpp::MariaDBClientCpp
PROPERTIES
IMPORTED_LINK_INTERFACE_LANGUAGES
"CXX"
IMPORTED_LOCATION
"${MariaDBClientCpp_LIBRARY}"
)

# Add component's dependencies for linking
if(mariadbclientcpp_LIBRARY_DEPENDENCIES)
set_target_properties(
MariaDBClientCpp::MariaDBClientCpp
PROPERTIES
INTERFACE_LINK_LIBRARIES
"${mariadbclientcpp_LIBRARY_DEPENDENCIES}"
)
endif()
endif()
endif()
7 changes: 6 additions & 1 deletion src/spider/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ else()
add_library(spider_core SHARED)
endif()
target_sources(spider_core PRIVATE ${SPIDER_CORE_SOURCES})
target_link_libraries(spider_core PUBLIC Boost::boost PRIVATE absl::flat_hash_map)
target_link_libraries(
spider_core
Boost::boost
absl::flat_hash_map
MariaDBClientCpp::MariaDBClientCpp
)
Comment on lines +21 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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
)


set(SPIDER_WORKER_SOURCES worker/worker.cpp CACHE INTERNAL "spider worker source files")

Expand Down
90 changes: 0 additions & 90 deletions tools/scripts/lib_install/mariadb-connector-c.sh
View file
Open in desktop

This file was deleted.

73 changes: 73 additions & 0 deletions tools/scripts/lib_install/mariadb-connector-cpp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
#!/bin/bash

# Dependencies:
# - cmake
# - g++
# - git
# NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic

# Exit on any error
set -e

cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]"
if [ "$#" -lt 1 ] ; then
echo $cUsage
exit
fi
version=$1

Comment on lines +12 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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

package_name=mariadb-connector-cpp
temp_dir=/tmp/${package_name}-installation
deb_output_dir=${temp_dir}
Comment on lines +19 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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}

if [[ "$#" -gt 1 ]] ; then
deb_output_dir="$(readlink -f "$2")"
if [ ! -d ${deb_output_dir} ] ; then
echo "${deb_output_dir} does not exist or is not a directory"
exit
fi
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
Comment on lines +30 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
# 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


# Get number of cpu cores
if [ "$(uname -s)" == "Darwin" ]; then
num_cpus=$(sysctl -n hw.ncpu)
else
num_cpus=$(grep -c ^processor /proc/cpuinfo)
fi

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
Comment on lines +47 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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


# Download
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
Comment on lines +55 to +60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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


# Build
mkdir build
cd build
cmake ..
make -j${num_cpus}
Comment on lines +63 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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}

Comment on lines +62 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
# 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


# Install
install_command_prefix="${privileged_command_prefix}"
${install_command_prefix} make install

# Clean up
rm -rf $temp_dir
Comment on lines +72 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
# 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

2 changes: 1 addition & 1 deletion tools/scripts/linux/install-lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ lib_install_scripts_dir=$script_dir/..

"$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
"$lib_install_scripts_dir"/lib_install/mariadb-connector-cpp.sh 1.1.5
"$lib_install_scripts_dir"/lib_install/boost.sh 1.86.0
9 changes: 5 additions & 4 deletions tools/scripts/macOS/install-lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ set -u
brew update
brew install \
boost \
coreutils \
fmt \
mariadb-connector-c \
spdlog \
pkg-config
spdlog

script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
lib_install_scripts_dir=$script_dir/..
"${lib_install_scripts_dir}"/lib_install/mariadb-connector-cpp.sh 1.1.5