build(deps): Migrate dependencies from script-based installations to task-based installations.#115
Conversation
WalkthroughThis change removes platform-specific library installation scripts and replaces them with a unified, task-based dependency installation framework. The CMake configuration is updated to adjust how dependencies like spdlog and MariaDBClientCpp are found and linked, including the removal of the custom Findspdlog.cmake module. The Dockerfile is updated to stop invoking the removed installation script. The dependency management is now handled through a new set of internal tasks defined in dep-tasks.yaml, which handle downloading, building, and installing libraries such as Boost, fmtlib, spdlog, mariadb-connector-cpp, and msgpack. Minor fixes and updates are also made to error messages and linking configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskRunner
participant DependencyInstaller
participant CMake
participant BuildSystem
User->>TaskRunner: Run install-all-run
TaskRunner->>DependencyInstaller: install-fmtlib
TaskRunner->>DependencyInstaller: install-boost
TaskRunner->>DependencyInstaller: install-spdlog
TaskRunner->>DependencyInstaller: install-mariadb-connector-cpp
TaskRunner->>DependencyInstaller: install-msgpack
DependencyInstaller->>BuildSystem: Download, build, and install dependencies
BuildSystem->>CMake: Configure with installed dependencies
CMake-->>User: Build ready with unified dependencies
Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cmake/Modules/FindMariaDBClientCpp.cmake (1)
72-72:⚠️ Potential issuePotentially undefined variable used for version detection
The version is set to
mariadbclientcpp_PKGCONF_VERSION, but this variable doesn't appear to be defined anywhere in the file since pkg-config usage was removed. This could result in an empty or incorrect version being reported.Replace with a version detection mechanism that doesn't rely on pkg-config, such as:
-set(MariaDBClientCpp_VERSION ${mariadbclientcpp_PKGCONF_VERSION}) +# Set version from a header file or fallback to a default version +set(MariaDBClientCpp_VERSION "1.1.5" CACHE STRING "MariaDB Connector C++ version")
🧹 Nitpick comments (1)
tools/scripts/lib_install/linux/install-dev.sh (1)
35-38: Quote paths and enrich TODO
Wrap thelib_install_scripts_dirassignment in quotes to guard against spaces, and consider expanding the TODO with context (e.g. what needs fixing or tracking).Apply:
- lib_install_scripts_dir=$script_dir/.. + lib_install_scripts_dir="${script_dir}/.."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.devcontainer/Dockerfile(0 hunks).github/workflows/code-linting-checks.yaml(1 hunks).github/workflows/unit-tests.yaml(1 hunks)CMakeLists.txt(1 hunks)cmake/Modules/FindMariaDBClientCpp.cmake(3 hunks)cmake/Modules/Findspdlog.cmake(0 hunks)dep-tasks.yaml(2 hunks)src/spider/CMakeLists.txt(1 hunks)tools/scripts/lib_install/boost.sh(0 hunks)tools/scripts/lib_install/fmtlib.sh(0 hunks)tools/scripts/lib_install/linux/install-dev.sh(1 hunks)tools/scripts/lib_install/linux/install-lib.sh(0 hunks)tools/scripts/lib_install/macOS/install-lib.sh(0 hunks)tools/scripts/lib_install/mariadb-connector-cpp.sh(0 hunks)tools/scripts/lib_install/msgpack.sh(0 hunks)tools/scripts/lib_install/spdlog.sh(0 hunks)
💤 Files with no reviewable changes (9)
- .devcontainer/Dockerfile
- tools/scripts/lib_install/macOS/install-lib.sh
- tools/scripts/lib_install/mariadb-connector-cpp.sh
- tools/scripts/lib_install/linux/install-lib.sh
- tools/scripts/lib_install/msgpack.sh
- tools/scripts/lib_install/fmtlib.sh
- tools/scripts/lib_install/boost.sh
- tools/scripts/lib_install/spdlog.sh
- cmake/Modules/Findspdlog.cmake
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (18)
.github/workflows/code-linting-checks.yaml (1)
49-49: Increase dependency install timeout
Extending thetimeout-minutesto 20 is appropriate given the longer task-based Boost installation. This aligns with similar changes in other workflows..github/workflows/unit-tests.yaml (1)
52-52: Align dependency install timeout for unit tests
Raising thetimeout-minutesto 20 ensures thedeps:lib_installtask completes under the new task-based installation process. This change mirrors the code-linting workflow update.CMakeLists.txt (1)
135-135: Fix typo in spdlog error message
Removing the extraneous backtick cleans up the fatal error text when static spdlog libraries aren’t found.tools/scripts/lib_install/linux/install-dev.sh (1)
27-27: Add MariaDB development headers
Includinglibmariadb-devis necessary for linking the MariaDB Connector C++ during build. Ensure this package exists on all supported distributions.src/spider/CMakeLists.txt (1)
182-182: Explicitly link fmt to spider_client
Addingfmt::fmtto the PUBLIC interface ensures formatting symbols are available to all consumers ofspider_client.cmake/Modules/FindMariaDBClientCpp.cmake (3)
21-25: Improved library detection by using standard header path and environment variable hintThe change simplifies header detection by using a standard path to
mariadb/conncpp.hppand leveraging themariadb-connector-cpp_ROOTenvironment variable instead of relying on pkg-config. This aligns with modern CMake practices and makes the detection more reliable across different platforms.
50-53: Enhanced library path detection with architecture-specific directoriesThe updated path detection uses the same environment variable hint approach and adds support for architecture-specific library directories. This ensures libraries can be found on various system configurations, particularly important for cross-compilation scenarios.
99-101: Simplified target include directory configurationThe interface include directory configuration has been simplified to use the variable directly without any additional manipulation. This makes the code cleaner and more maintainable.
dep-tasks.yaml (10)
22-24: Well-organized dependency structure for install-all-run taskThe new dependencies for spdlog, MariaDB Connector C++, and msgpack are correctly added to the install-all-run task, ensuring they're installed as part of the comprehensive dependency installation process.
106-119: Well-structured fmtlib installation taskThe installation task for fmtlib is properly configured with version, checksum, and compilation flags. The position-independent code flag is particularly important for libraries that might be linked to shared libraries.
120-139: Comprehensive spdlog installation with proper dependenciesThe spdlog installation task correctly depends on boost and fmtlib, ensuring dependencies are installed in the right order. The configuration properly sets up external fmt usage and loads the necessary CMake settings.
157-174: Well-configured msgpack installation with boost dependencyThe msgpack installation task properly depends on boost and uses the correct version, checksum, and compilation flags. The boost.cmake configuration is correctly loaded.
175-193: Complete boost installation task with comprehensive library selectionThe boost installation task correctly lists all required library components (filesystem, iostreams, process, program_options, regex, system) needed by the project. The task delegates to the specialized boost-download-and-install task for implementation.
194-213: Well-documented boost-generate taskThe boost-generate task is well-documented with clear parameter descriptions. The implementation correctly uses bootstrap.sh with appropriate prefix and library selection options.
214-234: Flexible boost-build task with concurrency controlThe boost-build task provides good flexibility by allowing specification of extra arguments and controlling build parallelism through the JOBS parameter. The code correctly handles conditional inclusion of the jobs parameter.
235-263: Well-implemented boost-install task with CMake integrationThe boost-install task correctly handles the installation step and generates the CMake settings file when CMAKE_SETTINGS_DIR is provided. The BOOST_ROOT variable is properly set to enable CMake to find Boost.
264-350: Comprehensive boost-download-and-install orchestration taskThis task effectively orchestrates the entire Boost installation process, from downloading to installation. The parameter handling with defaults is well-implemented, and the task dependencies are correctly set up to ensure proper execution order.
140-156:Details
❓ Verification inconclusive
Verify MariaDB installation layout compatibility
The MariaDB Connector C++ installation uses
-DINSTALL_LAYOUT=RPM, which is typically for RPM-based Linux distributions. This parameter might not be optimal for all platforms.
🏁 Script executed:
#!/bin/bash # Check if MariaDB Connector C++ CMake configuration accepts INSTALL_LAYOUT option # and what values are supported # Clone a shallow copy of the MariaDB Connector C++ repository to examine options git clone --depth 1 --branch 1.1.5 https://github.com/mariadb-corporation/mariadb-connector-cpp.git /tmp/mariadb-check cd /tmp/mariadb-check # Check CMake files for INSTALL_LAYOUT options and documentation echo "Checking INSTALL_LAYOUT options in CMake files:" grep -r "INSTALL_LAYOUT" --include="*.cmake" --include="CMakeLists.txt" . # Check if RPM layout is supported on all platforms echo "Checking platform-specific installation layouts:" find . -name "*cmake*" -type f -exec grep -l "RPM" {} \; | xargs grep -A 5 -B 5 "RPM"Length of output: 13134
Confirm MariaDB Connector C++ INSTALL_LAYOUT choice
CMake’s
install.cmakesupports four layouts—DEFAULT,RPM,DEBandPKG. While-DINSTALL_LAYOUT=RPMis valid, it configures install paths specifically for RPM packaging. If you’re installing from the tarball on non-RPM systems (e.g. Debian, macOS), you may prefer the standard layout or make this option platform-aware.• File:
dep-tasks.yaml(install-mariadb-connector-cpp, lines 140–156)
• Suggested update:- - "-DINSTALL_LAYOUT=RPM" + - "-DINSTALL_LAYOUT=DEFAULT"Please verify your target environments and adjust
INSTALL_LAYOUTaccordingly.
Co-authored-by: davidlion <davidlion2@protonmail.com>
Co-authored-by: davidlion <davidlion2@protonmail.com>
Co-authored-by: davidlion <davidlion2@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
dep-tasks.yaml (4)
117-118: Disable fmtlib tests and docs to optimize build time.
Consider extendinginstall-fmtlib'sGEN_ARGSto skip building tests and documentation:GEN_ARGS: - - "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" + - "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" + - "-DFMT_TEST=OFF" + - "-DFMT_DOC=OFF"
134-138: Disable spdlog tests to reduce install footprint.
Add a flag ininstall-spdlogto prevent building its tests:GEN_ARGS: - "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" - "-DUSE_EXTERNAL_FMT=ON" + - "-DSPDLOG_BUILD_TESTS=OFF" - "-C {{.G_DEPS_CMAKE_SETTINGS_DIR}}/boost.cmake" - "-C {{.G_DEPS_CMAKE_SETTINGS_DIR}}/fmt.cmake"
153-156: MakeINSTALL_LAYOUTparameter configurable.
Hardcoding-DINSTALL_LAYOUT=RPMmay not work on Debian/Ubuntu systems. Refactor to use a default variable:- - "-DINSTALL_LAYOUT=RPM" + - "-DINSTALL_LAYOUT={{default \"RPM\" .INSTALL_LAYOUT}}"This allows overriding
INSTALL_LAYOUTfor other packaging conventions (e.g.,DEB).
171-174: Skip msgpack examples and tests.
To streamlineinstall-msgpack, disable building examples and tests:GEN_ARGS: - "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" + - "-DMSGPACK_BUILD_EXAMPLES=OFF" + - "-DMSGPACK_BUILD_TESTS=OFF"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dep-tasks.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (5)
dep-tasks.yaml (5)
22-24: Extendinstall-all-runto include new tasks.
Theinstall-all-runpipeline now listsinstall-spdlog,install-mariadb-connector-cpp, andinstall-msgpackas dependencies (lines 22–24), integrating these into the unified installation flow.
205-213: Bootstrap step for Boost is correctly defined.
Theboost-generatetask invokes./bootstrap.shwith the proper prefixes and library list, and respects extra args.
306-313: Review templating for default list variables.
Theref: "default (list) .TARGETS"and similar var definitions are critical for passing lists into sub-tasks. Confirm that these render as proper YAML lists at runtime; otherwise, downstream tasks may fail:Add a debug command in
boost-download-and-installto echoTARGETSduring execution.
256-262:Details
❓ Verification inconclusive
Validate formatting of generated
boost.cmake.
The shell here-doc inboost-installappendsset(BOOST_ROOT ...). Ensure the output has correct quoting and no leading spaces that could break CMake parsing:
🏁 Script executed:
task install-boost sed -n '1,5p' "{{.G_DEPS_CMAKE_SETTINGS_DIR}}/boost.cmake"Length of output: 260
🏁 Script executed:
#!/usr/bin/env bash set -e # Locate dep-tasks.yaml TASK_FILE=$(fd dep-tasks.yaml -t f | head -n1) echo "Found dep-tasks.yaml at: $TASK_FILE" # Show where CMAKE_SETTINGS_DIR is defined rg -n 'CMAKE_SETTINGS_DIR' -B3 -A3 "$TASK_FILE" # Show lines 250–270 to inspect the boost.cmake snippet sed -n '250,270p' "$TASK_FILE"Length of output: 7454
Verify
boost.cmakeformatting
Please confirm that the generatedboost.cmakebegins exactly with:set(BOOST_ROOT "<INSTALL_PREFIX>" CACHE PATH "Package root for boost." )– with no extra leading spaces before each line. You can inspect the first few lines of the file to ensure correct quoting and indentation:
sed -n '1,5p' "${G_DEPS_CMAKE_SETTINGS_DIR}/boost.cmake"
180-186:Details
✅ Verification successful
Verify Boost installation parameters and CMake settings.
Theinstall-boosttask passes several vars intoboost-download-and-install. Please confirm that:
boost.cmakeis created under{{.G_DEPS_CMAKE_SETTINGS_DIR}}- Only the specified
TARGETS(filesystem,iostreams,process,program_options,regex,system) are built.
🏁 Script executed:
task install-boost grep -R "set(BOOST_ROOT" "{{.G_DEPS_CMAKE_SETTINGS_DIR}}"Length of output: 239
Let’s locate and inspect the
install-boostandboost-download-and-installtask definitions to verify where and how the CMake settings and targets are handled:
🏁 Script executed:
#!/bin/bash set -e # 1. Find the dep-tasks.yaml file(s) echo "dep-tasks.yaml locations:" find . -type f -name 'dep-tasks.yaml' echo "----------------------------------------" # 2. Show the install-boost task with context echo "install-boost task snippet:" rg -n 'install-boost' -C 5 echo "----------------------------------------" # 3. Show the boost-download-and-install task with context echo "boost-download-and-install task snippet:" rg -n 'boost-download-and-install' -C 5Length of output: 3133
Let’s inspect the full
boost-download-and-installdefinition to confirm whereboost.cmakeis generated and howTARGETSis applied:
🏁 Script executed:
#!/bin/bash set -e # Dump the boost-download-and-install task to see CMAKE_SETTINGS_DIR usage and TARGETS handling rg -n 'boost-download-and-install:' -A 80 dep-tasks.yamlLength of output: 2020
Let’s inspect how
boost-generate,boost-build, and especiallyboost-installuseCMAKE_SETTINGS_DIRand handleTARGETS:
🏁 Script executed:
#!/bin/bash set -e # Show boost-generate task definition echo "=== boost-generate ===" rg -n 'boost-generate:' -C 30 dep-tasks.yaml echo echo "=== boost-build ===" rg -n 'boost-build:' -C 30 dep-tasks.yaml echo echo "=== boost-install ===" rg -n 'boost-install:' -A 80 dep-tasks.yamlLength of output: 8379
Boost installation parameters and CMake settings have been verified.
- The
boost-installstep writes aboost.cmakefile to{{.G_DEPS_CMAKE_SETTINGS_DIR}}/boost.cmake.- The
bootstrap.shinvocation uses--with-libraries={{(join "," .TARGETS)}}, so only the specified targets (filesystem,iostreams,process,program_options,regex,system) are built and installed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dep-tasks.yaml (1)
319-321: Ensure parallel Boost builds by defaultAs noted in prior reviews,
JOBScurrently defaults to an empty string, so builds will be sequential unless a user manually setsJOBS. To guarantee parallel builds across all b2 versions, update the default to the number of CPU cores:- JOBS: >- - {{default "" .JOBS}} + JOBS: >- + {{default (printf "%d" (cpu.NumCPU)) .JOBS}}
🧹 Nitpick comments (1)
dep-tasks.yaml (1)
22-24: Consolidate installation tasks but clarify transitive depsYou’ve correctly added
install-spdlog,install-mariadb-connector-cpp, andinstall-msgpackto theinstall-all-runpipeline, and their owndepsclauses will pull ininstall-boostandinstall-fmtlibas needed. For readability and faster discovery, consider listinginstall-boostandinstall-fmtlibexplicitly here, so it’s immediately obvious which core components are being built.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dep-tasks.yaml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (7)
dep-tasks.yaml (7)
39-40: Disable Abseil testsGreat addition of
-DABSL_BUILD_TESTING=OFFto theGEN_ARGSfor Abseil—it speeds up the build and avoids unnecessary test binaries.
54-54: Disable Catch2 testsIncluding
-DCATCH_BUILD_TESTING=OFFis spot-on for streamlining the Catch2 installation and avoiding unwanted test builds.
107-107: Unmodified empty line inserted by the templating engine; no review needed.
108-123: Disable fmtlib tests and docsYou’ve correctly set
-DFMT_TEST=OFFand-DFMT_DOC=OFF, which keeps the fmtlib install lean. This aligns nicely with the rest of our “testing off” conventions.
124-144: Configure spdlog with external fmt and no testsThe
install-spdlogtask properly referencesinstall-boostandinstall-fmtlib, disables tests (-DSPDLOG_BUILD_TESTS=OFF), and enforces use of the external fmt library (-DUSE_EXTERNAL_FMT=ON). CMake settings files are chained correctly via-C.
162-180: Disable msgpack testsYou’ve correctly set
-DMSGPACK_BUILD_TESTS=OFFand chained the Boost CMake settings file. This keeps the msgpack install efficient and consistent with other dependencies.
232-239: Conditional parallel build flag for BoostThe
boost-buildtask now only emits-jNwhen.JOBSis set, which is exactly what we want to control concurrency. Well implemented!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dep-tasks.yaml (1)
321-322: 🛠️ Refactor suggestionEnable parallel Boost builds by default
CurrentlyJOBSdefaults to an empty string, so-jis only passed when explicitly set. To guarantee parallel builds across Boost versions (<1.76 defaulting to sequential), consider changing to:- JOBS: >- - {{default "" .JOBS}} + JOBS: >- + {{default (printf "%d" (cpu.NumCPU)) .JOBS}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dep-tasks.yaml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (7)
dep-tasks.yaml (7)
19-25: Add new install tasks to install-all-run
Theinstall-all-runtask now includes Boost, fmtlib, MariaDB Connector/C++, msgpack, and spdlog. This properly reflects the new task-based approach for dependency installations.
39-42: Disable Abseil tests to speed up CI
The-DABSL_BUILD_TESTING=OFFflag correctly disables Abseil’s test suite, reducing build time and resource usage.
54-57: Disable Catch2 tests to optimise build
Adding-DCATCH_BUILD_TESTING=OFFprevents Catch2’s internal tests from building, which is consistent with other dependencies.
109-125: Implement fmtlib installation with tests and docs disabled
The newinstall-fmtlibtask configures fmtlib correctly, usinginstall-remote-tar, disabling tests (-DFMT_TEST=OFF) and docs (-DFMT_DOC=OFF).
126-146: Configure spdlog installation with external fmt and disabled tests
Theinstall-spdlogtask depends on boost and fmtlib and uses-DSPDLOG_BUILD_TESTS=OFFand-DUSE_EXTERNAL_FMT=ONalongside CMake presets. This aligns with the unified installation approach.
164-182: Disable msgpack-cxx tests during installation
The-DMSGPACK_BUILD_TESTS=OFFflag correctly skips MessagePack tests, ensuring consistency with other dependencies.
183-201: Add boost as an installable dependency
Theinstall-boosttask leverages theboost-download-and-installchain with specific targets for filesystem, iostreams, process, program_options, regex, and system. This encapsulates Boost installation cleanly.
Description
This pr migrates all dependencies installed through scripts to install tasks.
Boosthas limited support for build and install throughcmake, so we create special tasks to installBoostusing the traditionalbootstrap.shandb2.Checklist
breaking change.
Validation performed
deps:lib_installinstalls the dependencies.Summary by CodeRabbit