build: Add CMake config and spdlog and mariadb-connector-c dependencies #12
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.
|
Warning Rate limit exceeded@sitaowang1998 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduced in this pull request include the addition of a new submodule for the Catch2 testing framework, updates to the CMake configuration for the Spider project, and the introduction of new CMake modules for managing library dependencies. 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 (
|
sitaowang1998
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (7)
.gitmodules (1)
4-6: Consider pinning the specific commit in .gitmodulesTo ensure build reproducibility across all environments, it would be beneficial to pin the specific commit (a6ee7e20cd4011129816df7992c1a9db2ef4b58f) in the .gitmodules file.
Apply this diff:
[submodule "submodules/Catch2"] path = submodules/Catch2 url = https://github.com/catchorg/Catch2.git + commit = a6ee7e20cd4011129816df7992c1a9db2ef4b58fsrc/spider/CMakeLists.txt (1)
1-14: Consider enhancing the CMake configuration with additional featuresTo improve the robustness and maintainability of the build system, consider adding:
- Install rules for proper deployment
- Version information for the worker component
- Configuration options (e.g., build type, warning levels)
- Compiler warnings and options
Here's a suggested structure:
# Version information set(SPIDER_WORKER_VERSION_MAJOR 0) set(SPIDER_WORKER_VERSION_MINOR 1) set(SPIDER_WORKER_VERSION_PATCH 0) # Configuration options option(SPIDER_WORKER_ENABLE_WARNINGS "Enable compiler warnings" ON) # Compiler options if(SPIDER_WORKER_ENABLE_WARNINGS) target_compile_options(spider_worker PRIVATE $<$<CXX_COMPILER_ID:GNU>:-Wall -Wextra -Wpedantic> $<$<CXX_COMPILER_ID:Clang>:-Wall -Wextra -Wpedantic> ) endif() # Install rules install(TARGETS spider_worker EXPORT spider-worker-targets RUNTIME DESTINATION bin )Would you like me to create a GitHub issue to track these enhancements?
cmake/Modules/FindLibraryDependencies.cmake (4)
1-9: Documentation could be enhanced for better clarity.Consider adding:
- Format specification for return values (e.g., semicolon-separated list)
- Usage examples
- Note about how dynamic libraries are handled differently
# 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 - Semicolon-separated list of libraries that should be dynamically linked +# ${fld_PREFIX}_LIBRARY_DEPENDENCIES - Semicolon-separated list of absolute paths to found libraries +# +# Example: +# set(MY_STATIC_LIBS "ssl;crypto;z") +# FindStaticLibraryDependencies("mylib" "MYPREFIX" "${MY_STATIC_LIBS}") +# # Results in: +# # MYPREFIX_LIBRARY_DEPENDENCIES = "/usr/lib/libssl.a;/usr/lib/libcrypto.a;/usr/lib/libz.a" +# # mylib_DYNAMIC_LIBS = "c;m;pthread;rt"
10-38: Consider architectural improvements for better flexibility and robustness.
- The hardcoded list of dynamic libraries should be configurable to support different platforms and requirements.
- Add validation for empty or invalid input parameters.
- Expand PATH_SUFFIXES to include more common library paths.
+# Allow override of default dynamic libraries +if(NOT DEFINED DEFAULT_DYNAMIC_LIBS) + set(DEFAULT_DYNAMIC_LIBS "c;m;pthread;rt") +endif() - set(fld_DYNAMIC_LIBS "c;m;pthread;rt") + set(fld_DYNAMIC_LIBS "${DEFAULT_DYNAMIC_LIBS}") + # Validate input parameters + if("${fld_LIBNAME}" STREQUAL "") + message(FATAL_ERROR "fld_LIBNAME cannot be empty") + endif() + if("${fld_PREFIX}" STREQUAL "") + message(FATAL_ERROR "fld_PREFIX cannot be empty") + endif() find_library(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY NAMES ${fld_DEP_LIB} - PATH_SUFFIXES lib + PATH_SUFFIXES lib lib64 lib/x86_64-linux-gnu )
40-46: Documentation could be enhanced with examples.Similar to the previous macro, consider adding usage examples and more specific format descriptions.
# Find the given libraries # Params: # fld_PREFIX - Prefix for created variables # fld_DYNAMIC_LIBS - List of libraries to find # Returns: -# ${fld_PREFIX}_LIBRARY_DEPENDENCIES - Found libraries +# ${fld_PREFIX}_LIBRARY_DEPENDENCIES - Semicolon-separated list of absolute paths to found libraries +# +# Example: +# set(MY_DYNAMIC_LIBS "dl;rt") +# FindDynamicLibraryDependencies("MYPREFIX" "${MY_DYNAMIC_LIBS}") +# # Results in: +# # MYPREFIX_LIBRARY_DEPENDENCIES = "/usr/lib/libdl.so;/usr/lib/librt.so"
1-58: Consider adding advanced CMake features.The implementation could benefit from:
- Using CACHE variables for found libraries to improve subsequent CMake runs
- Adding version checking capabilities for libraries that support it
- Adding REQUIRED and QUIET options similar to standard Find modules
These improvements would make the module more consistent with standard CMake Find modules.
Would you like me to provide an example implementation of these features?
cmake/Modules/FindMariaDBClient.cmake (1)
17-17: Consider renaming the custom module for consistency.To maintain consistency with the naming convention used for the
FindMariaDBClient.cmakemodule, consider renamingFindLibraryDependencies.cmaketoFindLibraryDeps.cmakeor similar. This would align with the concise naming style employed in the CMake ecosystem.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- .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)
- src/spider/CMakeLists.txt (1 hunks)
- src/spider/storage/MetadataStorage.hpp (1 hunks)
- src/spider/worker/worker.cpp (1 hunks)
- submodules/Catch2 (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/spider/storage/MetadataStorage.hpp
- src/spider/worker/worker.cpp
- submodules/Catch2
🔇 Additional comments (12)
.gitmodules (1)
4-6: Good choice adding Catch2 as a testing framework!The addition of Catch2 as a submodule is a solid choice for C++ testing infrastructure. The path organization under
submodules/follows good practices for third-party dependencies.CMakeLists.txt (3)
32-36: LGTM! Module path setup follows CMake best practices.The module path configuration correctly preserves existing paths while adding the local module directory.
122-124: Verify CMakeLists.txt exists in subdirectories.The structure looks good, separating source and test files. However, let's verify the required CMakeLists.txt files exist.
✅ Verification successful
CMakeLists.txt files are present in both subdirectories ✓
The verification confirms that CMakeLists.txt files exist in both required locations:
- src/spider/CMakeLists.txt
- tests/CMakeLists.txt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for CMakeLists.txt in subdirectories echo "Checking for CMakeLists.txt in subdirectories..." fd --type f "CMakeLists.txt" "src/spider" "tests"Length of output: 199
55-78:⚠️ Potential issueFix inconsistent variable naming convention.
The code uses
SPIDER_USE_STATIC_LIBSbut later refers toCLP_USE_STATIC_LIBS. This inconsistency could cause issues.-if(CLP_USE_STATIC_LIBS) +if(SPIDER_USE_STATIC_LIBS)Likely invalid or redundant comment.
cmake/Modules/FindMariaDBClient.cmake (5)
1-14: Excellent documentation and conventions!The file header provides a clear overview of the purpose and usage of the
FindMariaDBClient.cmakemodule. The documentation explains the key variables defined by the module and how to control its behaviour using theMariaDBClient_USE_STATIC_LIBSvariable.The conventions section establishes a consistent naming scheme for variables, distinguishing between internal and externally visible ones. This enhances the readability and maintainability of the code.
30-36: Robust handling of static libraries!The script demonstrates a well-structured approach to handling static libraries. It temporarily modifies
CMAKE_FIND_LIBRARY_SUFFIXESto search for.afiles whenMariaDBClient_USE_STATIC_LIBSis set toON. After processing static libraries, it restores the original value ofCMAKE_FIND_LIBRARY_SUFFIXES.Additionally, the script calls
FindStaticLibraryDependenciesto manage dependencies for static libraries, ensuring a comprehensive integration into the build process.Also applies to: 49-56
69-105: Robust creation of the MariaDBClient target!The script takes great care to create the
MariaDBClient::MariaDBClienttarget only if it has not been defined previously. This prevents potential conflicts and ensures a clean integration into the build process.The target is created as either a
STATIC IMPORTEDorUNKNOWN IMPORTEDlibrary, depending on the value ofMariaDBClient_USE_STATIC_LIBS. TheUNKNOWNdesignation allows fallback to static libraries if shared libraries are not available.The script meticulously sets the include directories and library location for the target, ensuring proper configuration. It also handles the addition of the component's dependencies for linking, providing a comprehensive setup.
1-105: Thorough and well-structured FindMariaDBClient module!Overall, the
FindMariaDBClient.cmakemodule is well-designed and provides a robust solution for integrating the MariaDB client library into a CMake project. It handles both static and dynamic libraries, manages dependencies, and sets up the necessary variables and targets.The script is well-documented, follows consistent naming conventions, and includes appropriate checks and error handling. It demonstrates a deep understanding of CMake best practices and provides a solid foundation for managing the MariaDB client library dependency.
Great job on this module! It will undoubtedly streamline the build process and make it easier for other developers to work with the MariaDB client library in their CMake projects.
58-58: Verify the necessity of finding dynamic library dependencies.Consider whether calling
FindDynamicLibraryDependenciesis necessary whenMariaDBClient_USE_STATIC_LIBSis set toON. If dynamic dependencies are not required in this case, you may want to move this line inside anelseblock to avoid unnecessary processing.cmake/Modules/Findspdlog.cmake (3)
65-70: Ensure consistent use offind_package_handle_standard_args.The
find_package_handle_standard_argsfunction should receive the correct package name and variables. Verify that${SPDLOG_LIBNAME}correctly represents the package name and that all required variables are included.
95-97: Check for typos in conditionals affectingspdlog_FOUND.The condition:
if (NOT EXISTS "${spdlog_INCLUDE_DIR}") set(spdlog_FOUND OFF) else() # ... endif()Ensure that
spdlog_INCLUDE_DIRis correctly set and that the conditional logic accurately reflects the availability of the include directory.
106-108: Ensure the library file exists before proceeding.The check for
SPDLOG_LIBRARYuses:if(NOT EXISTS "${SPDLOG_LIBRARY}") set(spdlog_FOUND OFF) else() # ... endif()Make sure that
SPDLOG_LIBRARYcontains a valid path and that theNOT EXISTScondition correctly detects missing libraries.
| # set variable as CACHE INTERNAL to access it from other scope | ||
| set(SPIDER_CORE_SOURCES storage/MetadataStorage.hpp CACHE INTERNAL "spider core source files") | ||
|
|
||
| set(SPIDER_WORKER_SOURCES worker/worker.cpp CACHE INTERNAL "spider worker source files") |
There was a problem hiding this comment.
Review source file organization and documentation
The current setup has several potential issues:
SPIDER_CORE_SOURCESonly includes a header file. Typically, source lists should contain implementation files (.cpp).- There appears to be no implementation file for
MetadataStorage.
Consider reorganizing as follows:
-set(SPIDER_CORE_SOURCES storage/MetadataStorage.hpp CACHE INTERNAL "spider core source files")
+# Core source files and headers
+set(SPIDER_CORE_HEADERS
+ storage/MetadataStorage.hpp
+ CACHE INTERNAL "Spider core header files"
+)
+
+set(SPIDER_CORE_SOURCES
+ storage/MetadataStorage.cpp
+ CACHE INTERNAL "Spider core implementation files"
+)Committable suggestion was skipped due to low confidence.
| add_executable(spider_worker) | ||
| target_compile_features(spider_worker PRIVATE cxx_std_20) | ||
| target_sources( | ||
| spider_worker | ||
| PRIVATE | ||
| ${SPIDER_CORE_SOURCES} | ||
| ${SPIDER_WORKER_SOURCES} | ||
| ) | ||
| add_executable(spider::worker ALIAS spider_worker) |
There was a problem hiding this comment.
💡 Codebase verification
Add required dependency links and adjust include paths
The review was partially correct. While the dependencies (spdlog and MariaDB) are properly configured in the root CMakeLists.txt, they need to be linked to the spider_worker target. However, the suggested include directories were incorrect as there's no include directory in the project structure.
add_executable(spider_worker)
target_compile_features(spider_worker PRIVATE cxx_std_20)
+
+# Add include directories
+target_include_directories(spider_worker
+ PRIVATE
+ ${CMAKE_CURRENT_SOURCE_DIR}
+)
+
target_sources(
spider_worker
PRIVATE
${SPIDER_CORE_SOURCES}
${SPIDER_WORKER_SOURCES}
)
+
+# Link required dependencies
+target_link_libraries(spider_worker
+ PRIVATE
+ spdlog::spdlog
+ MariaDB::MariaDB
+ Threads::Threads
+)
+
add_executable(spider::worker ALIAS spider_worker)🔗 Analysis chain
Add missing target configurations
The executable setup is missing several important configurations:
- Include directories are not specified
- Required dependencies (spdlog, mariadb-connector-c) mentioned in the PR are not linked
- No visibility controls are set
Add the following configurations:
add_executable(spider_worker)
target_compile_features(spider_worker PRIVATE cxx_std_20)
+
+# Add include directories
+target_include_directories(spider_worker
+ PUBLIC
+ $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
+ $<INSTALL_INTERFACE:include>
+ PRIVATE
+ ${CMAKE_CURRENT_SOURCE_DIR}/src
+)
+
target_sources(
spider_worker
PRIVATE
${SPIDER_CORE_SOURCES}
${SPIDER_WORKER_SOURCES}
)
+
+# Link required dependencies
+target_link_libraries(spider_worker
+ PRIVATE
+ spdlog::spdlog
+ MariaDB::MariaDB
+)
+
add_executable(spider::worker ALIAS spider_worker)Let's verify if the dependencies are properly configured in the parent CMakeLists.txt:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if find_package commands exist for required dependencies
rg -l "find_package.*spdlog|find_package.*MariaDB" --type cmake
Length of output: 118
Script:
#!/bin/bash
# Check the content of root CMakeLists.txt for dependency configuration
cat CMakeLists.txt
# Check if there are any existing target_include_directories
rg "target_include_directories" --type cmake
# Check if there are any existing target_link_libraries
rg "target_link_libraries" --type cmake
Length of output: 3834
Script:
#!/bin/bash
# Check the content of src/spider/CMakeLists.txt to see the full context
cat src/spider/CMakeLists.txt
# Check if there are any header files in include directory
fd . include/ -e hpp -e h
# Check source directory structure
fd . src/spider/ -t d
Length of output: 718
| foreach(fld_DEP_LIB ${fld_DYNAMIC_LIBS}) | ||
| find_library(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY | ||
| NAMES ${fld_DEP_LIB} | ||
| PATH_SUFFIXES lib | ||
| ) | ||
| if(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY) | ||
| list(APPEND ${fld_PREFIX}_LIBRARY_DEPENDENCIES "${${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY}") | ||
| else() | ||
| message(SEND_ERROR "${fld_DEP_LIB} library not found") | ||
| endif() | ||
| endforeach() | ||
| endmacro() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and library search paths.
The dynamic library handling could be improved with:
- Input parameter validation
- More descriptive error messages
- Consistent library search paths with the static library macro
+ # Validate input parameters
+ if("${fld_PREFIX}" STREQUAL "")
+ message(FATAL_ERROR "fld_PREFIX cannot be empty")
+ endif()
+ if("${fld_DYNAMIC_LIBS}" STREQUAL "")
+ message(FATAL_ERROR "fld_DYNAMIC_LIBS cannot be empty")
+ endif()
find_library(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY
NAMES ${fld_DEP_LIB}
- PATH_SUFFIXES lib
+ PATH_SUFFIXES lib lib64 lib/x86_64-linux-gnu
)
if(${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY)
list(APPEND ${fld_PREFIX}_LIBRARY_DEPENDENCIES "${${fld_PREFIX}_${fld_DEP_LIB}_LIBRARY}")
else()
- message(SEND_ERROR "${fld_DEP_LIB} library not found")
+ message(SEND_ERROR "Dynamic library '${fld_DEP_LIB}' not found. Please ensure it is installed in your system.")
endif()Committable suggestion was skipped due to low confidence.
| # Find and setup MariaDBClient library | ||
| if(CLP_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}") | ||
| else() | ||
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient") | ||
| endif() |
There was a problem hiding this comment.
Fix variable inconsistencies in MariaDBClient setup.
There are two issues in the MariaDBClient configuration:
- Using
CLP_USE_STATIC_LIBSinstead ofSPIDER_USE_STATIC_LIBS - Error message uses undefined
CLP_LIBS_STRINGinstead ofSPIDER_LIBS_STRING
-if(CLP_USE_STATIC_LIBS)
+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}")
else()
- message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient")
+ message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClient")
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.
| # Find and setup MariaDBClient library | |
| if(CLP_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}") | |
| else() | |
| message(FATAL_ERROR "Could not find ${CLP_LIBS_STRING} libraries for MariaDBClient") | |
| endif() | |
| # Find and setup MariaDBClient library | |
| 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}") | |
| else() | |
| message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClient") | |
| endif() |
| # Macro providing the length of the absolute source directory path so we can | ||
| # create a relative (rather than absolute) __FILE__ macro | ||
| string(LENGTH "${CMAKE_SOURCE_DIR}/" SOURCE_PATH_SIZE) | ||
| add_definitions("-DSOURCE_PATH_SIZE=${SOURCE_PATH_SIZE}") | ||
|
|
||
| # Profiling options | ||
| add_definitions(-DPROF_ENABLED=0) | ||
|
|
||
| # Compile-in debug logging statements | ||
| #add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG) | ||
|
|
||
| # Flush to disk switch | ||
| add_definitions(-DFLUSH_TO_DISK_ENABLED=1) | ||
|
|
||
| # Make off_t 64-bit | ||
| add_definitions(-D_FILE_OFFSET_BITS=64) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making FLUSH_TO_DISK_ENABLED configurable.
Forcing disk flushing (FLUSH_TO_DISK_ENABLED=1) could impact performance. Consider making this configurable via CMake option.
-add_definitions(-DFLUSH_TO_DISK_ENABLED=1)
+option(SPIDER_FLUSH_TO_DISK "Enable immediate disk flushing" OFF)
+add_definitions(-DFLUSH_TO_DISK_ENABLED=$<BOOL:${SPIDER_FLUSH_TO_DISK}>)Also, consider adding documentation for these compile definitions in a README or CMake documentation file.
📝 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.
| # Macro providing the length of the absolute source directory path so we can | |
| # create a relative (rather than absolute) __FILE__ macro | |
| string(LENGTH "${CMAKE_SOURCE_DIR}/" SOURCE_PATH_SIZE) | |
| add_definitions("-DSOURCE_PATH_SIZE=${SOURCE_PATH_SIZE}") | |
| # Profiling options | |
| add_definitions(-DPROF_ENABLED=0) | |
| # Compile-in debug logging statements | |
| #add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG) | |
| # Flush to disk switch | |
| add_definitions(-DFLUSH_TO_DISK_ENABLED=1) | |
| # Make off_t 64-bit | |
| add_definitions(-D_FILE_OFFSET_BITS=64) | |
| # Macro providing the length of the absolute source directory path so we can | |
| # create a relative (rather than absolute) __FILE__ macro | |
| string(LENGTH "${CMAKE_SOURCE_DIR}/" SOURCE_PATH_SIZE) | |
| add_definitions("-DSOURCE_PATH_SIZE=${SOURCE_PATH_SIZE}") | |
| # Profiling options | |
| add_definitions(-DPROF_ENABLED=0) | |
| # Compile-in debug logging statements | |
| #add_definitions(-DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG) | |
| # Flush to disk switch | |
| option(SPIDER_FLUSH_TO_DISK "Enable immediate disk flushing" OFF) | |
| add_definitions(-DFLUSH_TO_DISK_ENABLED=$<BOOL:${SPIDER_FLUSH_TO_DISK}>) | |
| # Make off_t 64-bit | |
| add_definitions(-D_FILE_OFFSET_BITS=64) |
| find_package(PkgConfig) | ||
| pkg_check_modules(SPDLOG_PKGCONF QUIET ${SPDLOG_LIBNAME}) | ||
|
|
There was a problem hiding this comment.
Check for the availability of PkgConfig before using it.
You are using pkg_check_modules without verifying that PkgConfig was found. This could lead to build errors if PkgConfig is not installed. Consider adding a conditional check:
if (PkgConfig_FOUND)
pkg_check_modules(SPDLOG_PKGCONF QUIET ${SPDLOG_LIBNAME})
endif()Apply this diff to add the conditional check:
-find_package(PkgConfig)
-pkg_check_modules(SPDLOG_PKGCONF QUIET ${SPDLOG_LIBNAME})
+find_package(PkgConfig QUIET)
+if (PkgConfig_FOUND)
+ pkg_check_modules(SPDLOG_PKGCONF QUIET ${SPDLOG_LIBNAME})
+endif()| # Handle static libraries | ||
| if(spdlog_USE_STATIC_LIBS) | ||
| # Save current value of CMAKE_FIND_LIBRARY_SUFFIXES | ||
| set(SPDLOG_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) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Restore CMAKE_FIND_LIBRARY_SUFFIXES even if spdlog_USE_STATIC_LIBS is false.
Currently, CMAKE_FIND_LIBRARY_SUFFIXES is only restored if spdlog_USE_STATIC_LIBS is ON. If an error occurs before the restoration, or if multiple options are toggled, this could lead to unexpected behaviour. Ensure that CMAKE_FIND_LIBRARY_SUFFIXES is restored to its original value in all cases.
Consider encapsulating the modification and restoration of CMAKE_FIND_LIBRARY_SUFFIXES to prevent side effects.
| find_library( | ||
| SPDLOG_LIBRARY | ||
| NAMES ${SPDLOG_LIBNAME} | ||
| HINTS ${SPDLOG_PKGCONF_LIBDIR} | ||
| PATH_SUFFIXES lib | ||
| ) | ||
| if (SPDLOG_LIBRARY) | ||
| # NOTE: This must be set for find_package_handle_standard_args to work | ||
| set(spdlog_FOUND ON) | ||
| endif() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify that the library search is platform-independent.
The find_library command uses PATH_SUFFIXES lib, which assumes a certain directory structure. On some platforms, libraries might be located in different directories (e.g., lib64). Consider adding lib64 to the PATH_SUFFIXES for better compatibility:
PATH_SUFFIXES lib lib64Apply this diff to include lib64 in the search:
HINTS ${SPDLOG_PKGCONF_LIBDIR}
- PATH_SUFFIXES lib
+ PATH_SUFFIXES lib lib64
)| if(NOT TARGET ${SPDLOG_TARGET_NAME}) | ||
| # Add library to build | ||
| if (spdlog_FOUND) | ||
| if (spdlog_USE_STATIC_LIBS) | ||
| add_library(${SPDLOG_TARGET_NAME} STATIC IMPORTED) | ||
| set_target_properties( | ||
| ${SPDLOG_TARGET_NAME} | ||
| PROPERTIES | ||
| COMPILE_FLAGS "${SPDLOG_PKGCONF_STATIC_CFLAGS}" | ||
| ) | ||
| 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(${SPDLOG_TARGET_NAME} UNKNOWN IMPORTED) | ||
| set_target_properties( | ||
| ${SPDLOG_TARGET_NAME} | ||
| PROPERTIES | ||
| COMPILE_FLAGS "${SPDLOG_PKGCONF_CFLAGS}" | ||
| ) | ||
| endif() | ||
| endif() | ||
|
|
||
| # Set include directories for library | ||
| if (NOT EXISTS "${spdlog_INCLUDE_DIR}") | ||
| set(spdlog_FOUND OFF) | ||
| else() | ||
| set_target_properties( | ||
| ${SPDLOG_TARGET_NAME} | ||
| PROPERTIES | ||
| INTERFACE_INCLUDE_DIRECTORIES "${spdlog_INCLUDE_DIR}" | ||
| ) | ||
| endif() | ||
|
|
||
| # Set location of library | ||
| if(NOT EXISTS "${SPDLOG_LIBRARY}") | ||
| set(spdlog_FOUND OFF) | ||
| else() | ||
| set_target_properties( | ||
| ${SPDLOG_TARGET_NAME} | ||
| PROPERTIES | ||
| IMPORTED_LINK_INTERFACE_LANGUAGES "C" | ||
| IMPORTED_LOCATION "${SPDLOG_LIBRARY}" | ||
| ) | ||
|
|
||
| # Add component's dependencies for linking | ||
| if(SPDLOG_LIBRARY_DEPENDENCIES) | ||
| set_target_properties( | ||
| ${SPDLOG_TARGET_NAME} | ||
| PROPERTIES | ||
| INTERFACE_LINK_LIBRARIES "${SPDLOG_LIBRARY_DEPENDENCIES}" | ||
| ) | ||
| endif() | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify target creation and property settings.
The logic for creating the target and setting properties is complex and could be simplified. Consider the following:
- Use
IMPORTEDtarget properties more effectively. - Reduce duplication between static and dynamic library handling.
- Ensure that all necessary properties are set for the target, including usage requirements.
Would you like assistance in refactoring this section for clarity and maintainability?
| if(SPDLOG_LIBRARY_DEPENDENCIES) | ||
| set_target_properties( | ||
| ${SPDLOG_TARGET_NAME} | ||
| PROPERTIES | ||
| INTERFACE_LINK_LIBRARIES "${SPDLOG_LIBRARY_DEPENDENCIES}" | ||
| ) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Handle possible empty SPDLOG_LIBRARY_DEPENDENCIES.
When setting INTERFACE_LINK_LIBRARIES, if SPDLOG_LIBRARY_DEPENDENCIES is empty or unset, it might cause issues. Consider adding a conditional check:
if (SPDLOG_LIBRARY_DEPENDENCIES)
set_target_properties(
${SPDLOG_TARGET_NAME}
PROPERTIES
INTERFACE_LINK_LIBRARIES "${SPDLOG_LIBRARY_DEPENDENCIES}"
)
endif()Apply this diff to add the conditional check:
-if(SPDLOG_LIBRARY_DEPENDENCIES)
+if (SPDLOG_LIBRARY_DEPENDENCIES)
set_target_properties(
${SPDLOG_TARGET_NAME}
PROPERTIES
INTERFACE_LINK_LIBRARIES "${SPDLOG_LIBRARY_DEPENDENCIES}"
)
+endif()
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (4)
tools/scripts/lib_install/fmtlib.sh (1)
12-16: Improve argument validation
The usage message could be more descriptive, and the argument validation could be more robust.
-cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]"
+cUsage="Usage: ${BASH_SOURCE[0]} <version> [<.deb output directory>]
+
+Arguments:
+ version The fmtlib version to install (e.g., 11.0.2)
+ .deb output directory Optional: Directory to store the generated .deb package"tools/scripts/lib_install/mariadb-connector-c.sh (2)
23-26: Enhance error message for directory check
The current error message could be more informative.
- echo "${deb_output_dir} does not exist or is not a directory"
+ echo "Error: Specified .deb output directory '${deb_output_dir}' does not exist or is not a directory"
exit46-53: Enhance OS detection and support
The script only supports Ubuntu, which limits its usability.
Consider adding support for other major distributions:
source /etc/os-release
-if [ $ID = "ubuntu" ] ; then
+case $ID in
+ ubuntu)
os_version=ubuntu-$UBUNTU_CODENAME
-else
+ ;;
+ debian)
+ os_version=debian-$VERSION_CODENAME
+ ;;
+ *)
echo "Error: Unsupported OS ID: $ID"
+ echo "Currently supported: Ubuntu, Debian"
exit 1
-fi
+ ;;
+esactools/scripts/lib_install/spdlog.sh (1)
3-7: Add version requirements for dependencies
Consider adding minimum version requirements for the dependencies to ensure compatibility.
# Dependencies:
-# - cmake
-# - curl
-# - g++
+# - cmake (>= 3.10)
+# - curl (>= 7.0)
+# - g++ (>= 7.0)📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- dep-tasks.yaml (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)
🧰 Additional context used
🪛 yamllint
dep-tasks.yaml
[error] 36-36: no new line character at the end of file
(new-line-at-end-of-file)
🪛 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 (5)
dep-tasks.yaml (3)
1-4: LGTM! File structure and variable definition are well-organized.
The use of .ROOT_DIR reference ensures portable path construction across different environments.
8-10: LGTM! Well-structured dependency orchestration.
The task effectively coordinates the installation of all required libraries.
33-36: LGTM! Proper submodule initialization configuration.
The recursive update ensures all nested submodules are properly initialized.
🧰 Tools
🪛 yamllint
[error] 36-36: no new line character at the end of file
(new-line-at-end-of-file)
taskfile.yaml (2)
5-5: LGTM! Good separation of dependency management tasks.
The inclusion of dep-tasks.yaml follows the existing pattern and maintains a clean separation of concerns for dependency management.
28-28: LGTM! Verify the dependency task exists.
Adding the deps:lib_install dependency ensures libraries are installed before CMake configuration.
Let's verify the dependency task exists:
✅ Verification successful
Task dependency deps:lib_install exists and is properly configured
The verification confirms that the lib_install task exists in dep-tasks.yaml with its own dependencies on fmtlib, spdlog, and mariadb-connector-c. The task is marked as internal and properly configured to handle library installation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the lib_install task exists in dep-tasks.yaml
# Expected: Should find the task definition
if [ -f "dep-tasks.yaml" ]; then
echo "Checking dep-tasks.yaml for lib_install task..."
rg "lib_install:" dep-tasks.yaml -A 5
else
echo "Warning: dep-tasks.yaml not found"
fi
Length of output: 288
| submodules: | ||
| internal: true | ||
| cmds: | ||
| - "git submodule update --init --recursive" |
There was a problem hiding this comment.
Add newline at end of file.
Add a newline character at the end of the file to comply with POSIX standards.
submodules:
internal: true
cmds:
- - "git submodule update --init --recursive"
+ - "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.
| - "git submodule update --init --recursive" | |
| - "git submodule update --init --recursive" | |
🧰 Tools
🪛 yamllint
[error] 36-36: no new line character at the end of file
(new-line-at-end-of-file)
dep-tasks.yaml
Outdated
| fmtlib: | ||
| internal: true | ||
| platforms: ["linux"] | ||
| dir: "{{.G_LIB_INSTALL_DIR}}" | ||
| cmds: | ||
| - "./fmtlib.sh 11.0.2" | ||
|
|
||
| spdlog: | ||
| internal: true | ||
| platforms: ["linux"] | ||
| dir: "{{.G_LIB_INSTALL_DIR}}" | ||
| cmds: | ||
| - "./spdlog.sh 1.14.1" | ||
|
|
||
| mariadb-connector-c: | ||
| internal: true | ||
| platforms: ["linux"] | ||
| dir: "{{.G_LIB_INSTALL_DIR}}" | ||
| cmds: | ||
| - "./mariadb-connector-c.sh 3.4.1" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using variables for library versions.
While the current implementation works, managing versions in shell commands makes updates more error-prone. Consider defining versions as variables at the top level.
Here's a suggested improvement:
vars:
G_LIB_INSTALL_DIR: "{{.ROOT_DIR}}/tools/scripts/lib_install/"
+ FMTLIB_VERSION: "11.0.2"
+ SPDLOG_VERSION: "1.14.1"
+ MARIADB_CONNECTOR_C_VERSION: "3.4.1"
# ... later in the file ...
fmtlib:
cmds:
- - "./fmtlib.sh 11.0.2"
+ - "./fmtlib.sh {{.FMTLIB_VERSION}}"
spdlog:
cmds:
- - "./spdlog.sh 1.14.1"
+ - "./spdlog.sh {{.SPDLOG_VERSION}}"
mariadb-connector-c:
cmds:
- - "./mariadb-connector-c.sh 3.4.1"
+ - "./mariadb-connector-c.sh {{.MARIADB_CONNECTOR_C_VERSION}}"Committable suggestion was skipped due to low confidence.
| #!/bin/bash | ||
|
|
||
| # Dependencies: | ||
| # - cmake | ||
| # - curl | ||
| # - g++ | ||
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add shebang options for improved script robustness
Consider enhancing the shebang line with options for safer execution.
-#!/bin/bash
+#!/usr/bin/env bashAlso, consider adding these options after the shebang for improved safety:
set -u # Exit on undefined variable
set -o pipefail # Exit on pipe failures| # Clean up | ||
| rm -rf $temp_dir |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove explicit cleanup as it's handled by trap
The explicit cleanup is redundant if using the trap command suggested earlier.
-# Clean up
-rm -rf $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.
| # Clean up | |
| rm -rf $temp_dir |
| 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.
🛠️ Refactor suggestion
Improve package version check logic
The current package version check might miss some edge cases.
-set +e
-dpkg -l ${package_name} | grep ${version}
-installed=$?
-set -e
+if dpkg-query -W -f='${Status} ${Version}\n' "${package_name}" 2>/dev/null | grep -q "^install ok installed ${version}"; then
+ echo "${package_name} version ${version} is already installed"
+ exit 0
+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.
| set +e | |
| dpkg -l ${package_name} | grep ${version} | |
| installed=$? | |
| set -e | |
| if [ $installed -eq 0 ] ; then | |
| # Nothing to do | |
| exit | |
| fi | |
| if dpkg-query -W -f='${Status} ${Version}\n' "${package_name}" 2>/dev/null | grep -q "^install ok installed ${version}"; then | |
| echo "${package_name} version ${version} is already installed" | |
| exit 0 | |
| fi |
| # Clean up | ||
| rm -rf $temp_dir |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cleanup handling
Add trap for cleanup and better error handling.
+cleanup() {
+ if [ -d "${temp_dir}" ]; then
+ echo "Cleaning up temporary files..."
+ rm -rf "${temp_dir}"
+ fi
+}
+
+# Set up trap for cleanup on script exit
+trap cleanup EXIT
+
-# Clean up
-rm -rf $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.
| # Clean up | |
| rm -rf $temp_dir | |
| cleanup() { | |
| if [ -d "${temp_dir}" ]; then | |
| echo "Cleaning up temporary files..." | |
| rm -rf "${temp_dir}" | |
| fi | |
| } | |
| # Set up trap for cleanup on script exit | |
| trap cleanup EXIT |
| # Check if already installed | ||
| set +e | ||
| pkg-config --exact-version="${version}" "${lib_name}" | ||
| pkg_found=$? | ||
| if [ $pkg_found -eq 0 ] ; then | ||
| find /usr/lib/ /usr/local/lib/ -name "libspdlog.a" | grep -q "." | ||
| static_lib_found=$? | ||
| fi | ||
| installed=$((pkg_found | static_lib_found)) | ||
| set -e | ||
| if [ $installed -eq 0 ] ; then | ||
| echo "Found ${lib_name}=${version}." | ||
| # Nothing to do | ||
| exit | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve installation check reliability
The current installation check has several potential issues:
- The static library search is too broad and might find libraries in non-standard locations
- The error handling could be simplified using command substitution
-# Check if already installed
-set +e
-pkg-config --exact-version="${version}" "${lib_name}"
-pkg_found=$?
-if [ $pkg_found -eq 0 ] ; then
- find /usr/lib/ /usr/local/lib/ -name "libspdlog.a" | grep -q "."
- static_lib_found=$?
-fi
-installed=$((pkg_found | static_lib_found))
-set -e
-if [ $installed -eq 0 ] ; then
+# Check if already installed with correct version
+if pkg-config --exact-version="${version}" "${lib_name}" && \
+ [ -f "$(pkg-config --variable=libdir ${lib_name})/libspdlog.a" ]; then
echo "Found ${lib_name}=${version}."
# Nothing to do
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.
| # Check if already installed | |
| set +e | |
| pkg-config --exact-version="${version}" "${lib_name}" | |
| pkg_found=$? | |
| if [ $pkg_found -eq 0 ] ; then | |
| find /usr/lib/ /usr/local/lib/ -name "libspdlog.a" | grep -q "." | |
| static_lib_found=$? | |
| fi | |
| installed=$((pkg_found | static_lib_found)) | |
| set -e | |
| if [ $installed -eq 0 ] ; then | |
| echo "Found ${lib_name}=${version}." | |
| # Nothing to do | |
| exit | |
| fi | |
| # Check if already installed with correct version | |
| if pkg-config --exact-version="${version}" "${lib_name}" && \ | |
| [ -f "$(pkg-config --variable=libdir ${lib_name})/libspdlog.a" ]; then | |
| echo "Found ${lib_name}=${version}." | |
| # Nothing to do | |
| exit | |
| fi |
| tar_filename=v${version}.tar.gz | ||
| if [ ! -e ${tar_filename} ] ; then | ||
| curl -fsSL https://github.com/gabime/spdlog/archive/${tar_filename} -o ${tar_filename} | ||
| fi |
There was a problem hiding this comment.
Add security measures for download process
The current download process lacks checksum verification and explicit HTTPS certificate validation.
tar_filename=v${version}.tar.gz
if [ ! -e ${tar_filename} ] ; then
- curl -fsSL https://github.com/gabime/spdlog/archive/${tar_filename} -o ${tar_filename}
+ # Verify HTTPS certificate
+ curl --proto '=https' --tlsv1.2 -fsSL \
+ https://github.com/gabime/spdlog/archive/${tar_filename} -o ${tar_filename}
+
+ # Verify checksum (replace with actual checksum for each version)
+ expected_sha256="REPLACE_WITH_ACTUAL_CHECKSUM"
+ if ! echo "${expected_sha256} ${tar_filename}" | sha256sum --check --status; then
+ echo "Error: Checksum verification failed"
+ rm -f "${tar_filename}"
+ exit 1
+ fi
fiCommittable suggestion was skipped due to low confidence.
| 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.
🛠️ Refactor suggestion
Enhance privilege escalation handling
Add checks for sudo availability and implement a timeout to prevent script from hanging.
echo "Checking for elevated privileges..."
privileged_command_prefix=""
if [ ${EUID:-$(id -u)} -ne 0 ] ; then
+ if ! command -v sudo >/dev/null 2>&1; then
+ echo "Error: This script requires root privileges. Please install sudo or run as root."
+ exit 1
+ fi
- sudo echo "Script can elevate privileges."
+ if ! sudo -n true 2>/dev/null; then
+ echo "Please enter your password for sudo access:"
+ sudo -v
+ fi
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 ! command -v sudo >/dev/null 2>&1; then | |
| echo "Error: This script requires root privileges. Please install sudo or run as root." | |
| exit 1 | |
| fi | |
| if ! sudo -n true 2>/dev/null; then | |
| echo "Please enter your password for sudo access:" | |
| sudo -v | |
| fi | |
| privileged_command_prefix="${privileged_command_prefix} sudo" | |
| fi |
| if [ $checkinstall_installed -eq 0 ] ; then | ||
| # NOTE: We use "1:${version}" to override the version installed by Ubuntu 18.04 | ||
| install_command_prefix="${install_command_prefix} checkinstall --pkgname '${package_name}' --pkgversion '1:${version}' --provides '${package_name}' --nodoc -y --pakdir \"${deb_output_dir}\"" | ||
| fi | ||
| ${install_command_prefix} make install |
There was a problem hiding this comment.
Fix command construction security issues
The current command construction is unsafe and may not handle special characters correctly.
- # NOTE: We use "1:${version}" to override the version installed by Ubuntu 18.04
- install_command_prefix="${install_command_prefix} checkinstall --pkgname '${package_name}' --pkgversion '1:${version}' --provides '${package_name}' --nodoc -y --pakdir \"${deb_output_dir}\""
-fi
-${install_command_prefix} make install
+ # Use array to properly handle arguments
+ if [ $checkinstall_installed -eq 0 ]; then
+ # Split privileged_command_prefix into array
+ read -ra cmd <<< "${privileged_command_prefix}"
+ # Build complete command array
+ cmd+=(checkinstall --pkgname "${package_name}" \
+ --pkgversion "1:${version}" \
+ --provides "${package_name}" \
+ --nodoc -y \
+ --pakdir "${deb_output_dir}" \
+ make install)
+ "${cmd[@]}"
+ else
+ # Split privileged_command_prefix into array
+ read -ra cmd <<< "${privileged_command_prefix}"
+ cmd+=(make install)
+ "${cmd[@]}"
+ 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.
| if [ $checkinstall_installed -eq 0 ] ; then | |
| # NOTE: We use "1:${version}" to override the version installed by Ubuntu 18.04 | |
| install_command_prefix="${install_command_prefix} checkinstall --pkgname '${package_name}' --pkgversion '1:${version}' --provides '${package_name}' --nodoc -y --pakdir \"${deb_output_dir}\"" | |
| fi | |
| ${install_command_prefix} make install | |
| if [ $checkinstall_installed -eq 0 ]; then | |
| # Split privileged_command_prefix into array | |
| read -ra cmd <<< "${privileged_command_prefix}" | |
| # Build complete command array | |
| cmd+=(checkinstall --pkgname "${package_name}" \ | |
| --pkgversion "1:${version}" \ | |
| --provides "${package_name}" \ | |
| --nodoc -y \ | |
| --pakdir "${deb_output_dir}" \ | |
| make install) | |
| "${cmd[@]}" | |
| else | |
| # Split privileged_command_prefix into array | |
| read -ra cmd <<< "${privileged_command_prefix}" | |
| cmd+=(make install) | |
| "${cmd[@]}" | |
| fi |
🧰 Tools
🪛 Shellcheck
[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)
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (1)
dep-tasks.yaml (1)
1-8: LGTM! Consider documenting version pinning strategy.
The variables section is well-structured with centralized version management. However, it would be beneficial to document the version pinning strategy for future maintainers.
Consider adding a comment block explaining:
- How often versions should be updated
- Any compatibility requirements between these dependencies
- The process for testing new versions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- dep-tasks.yaml (1 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)
🧰 Additional context used
🪛 yamllint
dep-tasks.yaml
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🪛 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] 73-73: privileged_command_prefix is referenced but not assigned.
(SC2154)
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 (2)
dep-tasks.yaml (1)
11-14: LGTM! Well-structured task hierarchy.
The lib_install task properly orchestrates the dependency installation process with clear dependencies.
tools/scripts/lib_install/mariadb-connector-c.sh (1)
48-59:
Add checksum verification for downloaded files
The script downloads files without verifying their integrity, which is a security risk.
The previous review comment about adding SHA256 checksum verification is still valid and should be implemented.
| submodules: | ||
| internal: true | ||
| cmds: | ||
| - "git submodule update --init --recursive" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing submodule update command.
The submodule update command could benefit from additional options for better error handling and quieter operation.
Consider this enhanced version:
submodules:
internal: true
cmds:
- - "git submodule update --init --recursive"
+ - "git submodule update --init --recursive --quiet || (echo 'Failed to update submodules' && exit 1)"📝 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.
| submodules: | |
| internal: true | |
| cmds: | |
| - "git submodule update --init --recursive" | |
| submodules: | |
| internal: true | |
| cmds: | |
| - "git submodule update --init --recursive --quiet || (echo 'Failed to update submodules' && exit 1)" |
🧰 Tools
🪛 yamllint
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
| fmtlib: | ||
| internal: true | ||
| platforms: ["linux"] | ||
| dir: "{{.G_LIB_INSTALL_DIR}}" | ||
| cmds: | ||
| - "./fmtlib.sh {{.FMTLIB_VERSION}}" | ||
|
|
||
| spdlog: | ||
| internal: true | ||
| platforms: ["linux"] | ||
| dir: "{{.G_LIB_INSTALL_DIR}}" | ||
| cmds: | ||
| - "./spdlog.sh {{.SPDLOG_VERSION}}" | ||
|
|
||
| mariadb-connector-c: | ||
| internal: true | ||
| platforms: ["linux"] | ||
| dir: "{{.G_LIB_INSTALL_DIR}}" | ||
| cmds: | ||
| - "./mariadb-connector-c.sh {{.MARIADB_CONNECTOR_C_VERSION}}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and installation verification.
While the installation tasks are well-structured, they could benefit from additional robustness:
- Error handling for failed installations
- Verification of successful installation
Here's a suggested improvement pattern for each installation task:
fmtlib:
internal: true
platforms: ["linux"]
dir: "{{.G_LIB_INSTALL_DIR}}"
cmds:
- - "./fmtlib.sh {{.FMTLIB_VERSION}}"
+ - |
+ if ! ./fmtlib.sh {{.FMTLIB_VERSION}}; then
+ echo "Failed to install fmtlib"
+ exit 1
+ fi
+ # Verify installation
+ if [ ! -f "{{.ROOT_DIR}}/lib/libfmt.a" ]; then
+ echo "fmtlib installation verification failed"
+ exit 1
+ fiApply similar patterns to spdlog and mariadb-connector-c tasks.
Committable suggestion was skipped due to low confidence.
| # Clean up | ||
| rm -rf $temp_dir |
There was a problem hiding this comment.
Remove explicit cleanup
The cleanup is redundant since we're using trap with mktemp -d.
Remove these lines as the cleanup is now handled by the trap:
-# Clean up
-rm -rf $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.
| # Clean up | |
| rm -rf $temp_dir |
| #!/bin/bash | ||
|
|
||
| # Dependencies: | ||
| # - curl | ||
| # - rsync | ||
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | ||
|
|
||
| # Exit on any error | ||
| set -e | ||
|
|
There was a problem hiding this comment.
Add privilege and dependency validation
The script requires elevated privileges but doesn't properly validate them. Additionally, while dependencies are listed, they aren't verified.
Apply this diff to improve initialization:
#!/bin/bash
+# Exit on any error
+set -e
+
+# Check for root/sudo privileges
+if [ ${EUID:-$(id -u)} -ne 0 ]; then
+ if ! sudo -v; then
+ echo "Error: This script requires sudo privileges"
+ exit 1
+ fi
+ privileged_command_prefix="sudo"
+fi
+
# Dependencies:
# - curl
# - rsync
# NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic
-# Exit on any error
-set -e
+# Verify dependencies
+for cmd in curl rsync; do
+ if ! command -v "$cmd" >/dev/null 2>&1; then
+ echo "Error: Required command '$cmd' not found"
+ 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 | |
| # Exit on any error | |
| set -e | |
| #!/bin/bash | |
| # Exit on any error | |
| set -e | |
| # Check for root/sudo privileges | |
| if [ ${EUID:-$(id -u)} -ne 0 ]; then | |
| if ! sudo -v; then | |
| echo "Error: This script requires sudo privileges" | |
| exit 1 | |
| fi | |
| privileged_command_prefix="sudo" | |
| fi | |
| # Dependencies: | |
| # - curl | |
| # - rsync | |
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | |
| # Verify dependencies | |
| for cmd in curl rsync; do | |
| if ! command -v "$cmd" >/dev/null 2>&1; then | |
| echo "Error: Required command '$cmd' not found" | |
| exit 1 | |
| fi | |
| done |
| cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]" | ||
| if [ "$#" -lt 1 ] ; then | ||
| echo $cUsage | ||
| exit | ||
| fi | ||
| version=$1 | ||
|
|
||
| package_name=libmariadb-dev | ||
| temp_dir=/tmp/${package_name}-installation | ||
| 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 |
There was a problem hiding this comment.
Improve argument validation and path handling
The script should validate the version format and use more robust path handling.
Apply this diff to enhance argument validation:
-cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]"
+readonly USAGE="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]"
+
if [ "$#" -lt 1 ] ; then
- echo $cUsage
- exit
+ echo "$USAGE"
+ exit 1
fi
version=$1
+# Validate version format (e.g., X.Y.Z)
+if ! [[ $version =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
+ echo "Error: Invalid version format. Expected X.Y.Z (e.g., 3.4.1)"
+ exit 1
+fi
+
package_name=libmariadb-dev
-temp_dir=/tmp/${package_name}-installation
+temp_dir="$(mktemp -d)"
+trap 'rm -rf "${temp_dir}"' EXIT
+
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
+ 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.
| cUsage="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]" | |
| if [ "$#" -lt 1 ] ; then | |
| echo $cUsage | |
| exit | |
| fi | |
| version=$1 | |
| package_name=libmariadb-dev | |
| temp_dir=/tmp/${package_name}-installation | |
| 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 | |
| readonly USAGE="Usage: ${BASH_SOURCE[0]} <version>[ <.deb output directory>]" | |
| if [ "$#" -lt 1 ] ; then | |
| echo "$USAGE" | |
| exit 1 | |
| fi | |
| version=$1 | |
| # Validate version format (e.g., X.Y.Z) | |
| if ! [[ $version =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | |
| echo "Error: Invalid version format. Expected X.Y.Z (e.g., 3.4.1)" | |
| exit 1 | |
| fi | |
| package_name=libmariadb-dev | |
| temp_dir="$(mktemp -d)" | |
| trap 'rm -rf "${temp_dir}"' EXIT | |
| 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 1 | |
| fi | |
| 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 |
There was a problem hiding this comment.
Improve sudo privilege handling
The current sudo check lacks timeout and error handling, which could cause the script to hang or fail silently.
Enhance the sudo check:
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"
+ if ! sudo -n true 2>/dev/null; then
+ echo "This script requires sudo privileges. Please enter your password."
+ if ! sudo -v -p "Password for sudo: "; then
+ echo "Failed to obtain sudo privileges. Exiting."
+ exit 1
+ fi
+ fi
+ 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 -n true 2>/dev/null; then | |
| echo "This script requires sudo privileges. Please enter your password." | |
| if ! sudo -v -p "Password for sudo: "; then | |
| echo "Failed to obtain sudo privileges. Exiting." | |
| exit 1 | |
| fi | |
| fi | |
| privileged_command_prefix="sudo" | |
| fi |
| if [ ! -e ${tar_filename} ] ; then | ||
| curl -fsSL https://github.com/fmtlib/fmt/archive/refs/tags/${tar_filename} -o ${tar_filename} | ||
| fi |
There was a problem hiding this comment.
Enhance download reliability
The current download mechanism lacks timeout and retry logic, which could be problematic with unreliable connections.
Add timeout and retry logic:
if [ ! -e ${tar_filename} ] ; then
- curl -fsSL https://github.com/fmtlib/fmt/archive/refs/tags/${tar_filename} -o ${tar_filename}
+ max_attempts=3
+ attempt=1
+ while [ $attempt -le $max_attempts ]; do
+ if curl -fsSL --connect-timeout 10 --max-time 300 \
+ https://github.com/fmtlib/fmt/archive/refs/tags/${tar_filename} -o ${tar_filename}; then
+ break
+ fi
+ echo "Download attempt $attempt failed. Retrying..."
+ attempt=$((attempt + 1))
+ sleep 5
+ done
+ if [ $attempt -gt $max_attempts ]; then
+ echo "Failed to download after $max_attempts attempts. Exiting."
+ 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.
| if [ ! -e ${tar_filename} ] ; then | |
| curl -fsSL https://github.com/fmtlib/fmt/archive/refs/tags/${tar_filename} -o ${tar_filename} | |
| fi | |
| if [ ! -e ${tar_filename} ] ; then | |
| max_attempts=3 | |
| attempt=1 | |
| while [ $attempt -le $max_attempts ]; do | |
| if curl -fsSL --connect-timeout 10 --max-time 300 \ | |
| https://github.com/fmtlib/fmt/archive/refs/tags/${tar_filename} -o ${tar_filename}; then | |
| break | |
| fi | |
| echo "Download attempt $attempt failed. Retrying..." | |
| attempt=$((attempt + 1)) | |
| sleep 5 | |
| done | |
| if [ $attempt -gt $max_attempts ]; then | |
| echo "Failed to download after $max_attempts attempts. Exiting." | |
| exit 1 | |
| fi | |
| fi |
| # Dependencies: | ||
| # - cmake | ||
| # - curl | ||
| # - g++ | ||
| # 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 dependency verification
The script lists required dependencies but doesn't verify their presence. This could lead to confusing errors later in the installation process.
Add this verification code after the dependencies list:
# NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic
+# Verify dependencies
+for cmd in cmake curl g++; do
+ if ! command -v "$cmd" >/dev/null 2>&1; then
+ echo "Error: Required dependency '$cmd' is 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.
| # Dependencies: | |
| # - cmake | |
| # - curl | |
| # - g++ | |
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | |
| # Dependencies: | |
| # - cmake | |
| # - curl | |
| # - g++ | |
| # NOTE: Dependencies should be installed outside the script to allow the script to be largely distro-agnostic | |
| # Verify dependencies | |
| for cmd in cmake curl g++; do | |
| if ! command -v "$cmd" >/dev/null 2>&1; then | |
| echo "Error: Required dependency '$cmd' is not installed" | |
| exit 1 | |
| fi | |
| done |
| cd ${extracted_dir} | ||
| mkdir -p build | ||
| cd build | ||
| cmake ../ | ||
| make -j${num_cpus} |
There was a problem hiding this comment.
Improve build process error handling
The build process lacks proper error handling and workspace cleanup on failure.
Add error handling and workspace management:
cd ${extracted_dir}
+
+# Clean up any previous failed builds
+rm -rf build
mkdir -p build
cd build
-cmake ../
-make -j${num_cpus}
+
+echo "Configuring build..."
+if ! cmake ../; then
+ echo "Error: CMake configuration failed"
+ exit 1
+fi
+
+echo "Building with ${num_cpus} CPU cores..."
+if ! make -j${num_cpus}; then
+ echo "Error: Build failed"
+ 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.
| cd ${extracted_dir} | |
| mkdir -p build | |
| cd build | |
| cmake ../ | |
| make -j${num_cpus} | |
| cd ${extracted_dir} | |
| # Clean up any previous failed builds | |
| rm -rf build | |
| mkdir -p build | |
| cd build | |
| echo "Configuring build..." | |
| if ! cmake ../; then | |
| echo "Error: CMake configuration failed" | |
| exit 1 | |
| fi | |
| echo "Building with ${num_cpus} CPU cores..." | |
| if ! make -j${num_cpus}; then | |
| echo "Error: Build failed" | |
| exit 1 | |
| fi |
| #!/bin/bash | ||
|
|
||
| # Dependencies: | ||
| # - cmake | ||
| # - curl | ||
| # - g++ | ||
| # 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 | ||
|
|
||
| lib_name=spdlog | ||
| package_name=libspdlog-dev | ||
| temp_dir=/tmp/${package_name}-installation | ||
| 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 | ||
| pkg-config --exact-version="${version}" "${lib_name}" | ||
| pkg_found=$? | ||
| if [ $pkg_found -eq 0 ] ; then | ||
| find /usr/lib/ /usr/local/lib/ -name "libspdlog.a" | grep -q "." | ||
| static_lib_found=$? | ||
| fi | ||
| installed=$((pkg_found | static_lib_found)) | ||
| set -e | ||
| if [ $installed -eq 0 ] ; then | ||
| echo "Found ${lib_name}=${version}." | ||
| # Nothing to do | ||
| exit | ||
| 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 | ||
|
|
||
| # Get number of cpu cores | ||
| num_cpus=$(grep -c ^processor /proc/cpuinfo) | ||
|
|
||
| # Download | ||
| mkdir -p $temp_dir | ||
| cd $temp_dir | ||
| extracted_dir=${temp_dir}/spdlog-${version} | ||
| if [ ! -e ${extracted_dir} ] ; then | ||
| tar_filename=v${version}.tar.gz | ||
| if [ ! -e ${tar_filename} ] ; then | ||
| curl -fsSL https://github.com/gabime/spdlog/archive/${tar_filename} -o ${tar_filename} | ||
| fi | ||
|
|
||
| tar -xf ${tar_filename} | ||
| fi | ||
|
|
||
| # Build | ||
| cd ${extracted_dir} | ||
| mkdir -p build | ||
| cd build | ||
| cmake ../ | ||
| make -j${num_cpus} | ||
|
|
||
| # Check if checkinstall is installed | ||
| set +e | ||
| command -v checkinstall | ||
| checkinstall_installed=$? | ||
| set -e | ||
|
|
||
| # Install | ||
| install_command_prefix="${privileged_command_prefix}" | ||
| if [ $checkinstall_installed -eq 0 ] ; then | ||
| # NOTE: We use "1:${version}" to override the version installed by Ubuntu 18.04 | ||
| install_command_prefix="${install_command_prefix} checkinstall --pkgname '${package_name}' --pkgversion '1:${version}' --provides '${package_name}' --nodoc -y --pakdir \"${deb_output_dir}\"" | ||
| fi | ||
| ${install_command_prefix} make install | ||
|
|
||
| # Clean up | ||
| rm -rf $temp_dir |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper logging and progress indication
The script would benefit from clear progress indicators and logging to help users understand the installation status and troubleshoot issues.
Add a logging function and progress indicators:
#!/bin/bash
+# Logging function
+log() {
+ local level="$1"
+ shift
+ echo "[$(date +'%Y-%m-%d %H:%M:%S')] ${level}: $*"
+}
+
+# Progress indicator
+show_progress() {
+ log "INFO" "==> $*"
+}
# Dependencies:
+show_progress "Checking dependencies..."Use these functions throughout the script:
-echo "Found ${lib_name}=${version}."
+log "INFO" "Found ${lib_name}=${version}"
-echo "Checking for elevated privileges..."
+show_progress "Checking for elevated privileges..."Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Shellcheck
[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)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
dep-tasks.yaml (1)
17-18: Consider platform-specific path handling.
While the platform restriction is set to "linux", the path separator might need adjustment for cross-platform development.
Consider using OS-specific path separators:
dir: '{{if eq OS "windows"}}{{.ROOT_DIR}}\tools\scripts\lib_install\{{else}}{{.G_LIB_INSTALL_DIR}}{{end}}'📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- dep-tasks.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
dep-tasks.yaml
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
dep-tasks.yaml (2)
1-8: LGTM! Well-structured variable declarations.
The version specification and variable organization demonstrate good practices for maintainability.
39-39:
Add newline at end of file.
To comply with POSIX standards and improve git diff behaviour, add a newline character at the end of the file.
- "git submodule update --init --recursive"
+Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
| dir: "{{.G_LIB_INSTALL_DIR}}" | ||
| cmds: | ||
| - "./fmtlib.sh {{.FMTLIB_VERSION}}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding script existence verification.
Add a check to verify the installation script exists before execution to prevent cryptic errors.
dir: "{{.G_LIB_INSTALL_DIR}}"
cmds:
- - "./fmtlib.sh {{.FMTLIB_VERSION}}"
+ - |
+ if [ ! -f "./fmtlib.sh" ]; then
+ echo "Error: fmtlib.sh not found in {{.G_LIB_INSTALL_DIR}}"
+ exit 1
+ fi
+ ./fmtlib.sh {{.FMTLIB_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.
| dir: "{{.G_LIB_INSTALL_DIR}}" | |
| cmds: | |
| - "./fmtlib.sh {{.FMTLIB_VERSION}}" | |
| dir: "{{.G_LIB_INSTALL_DIR}}" | |
| cmds: | |
| - | | |
| if [ ! -f "./fmtlib.sh" ]; then | |
| echo "Error: fmtlib.sh not found in {{.G_LIB_INSTALL_DIR}}" | |
| exit 1 | |
| fi | |
| ./fmtlib.sh {{.FMTLIB_VERSION}} |
Description
Add basic structure to separate core files and worker files. Add CMake files that finds spdlog, fmt and mariadb-connector-c library.
Validation performed
task:checkSummary by CodeRabbit
Release Notes
New Features
Improvements
New Files
MetadataStorage.hppfor future development needs.fmtlib,spdlog, andMariaDB Connector/C.Updates