build(deps): Update yscope-dev-utils and migrate dependencies from submodules to task-based installations. - #108
Conversation
## Walkthrough
This change removes the Catch2 and abseil-cpp submodules from the project and updates the dependency management workflow. Instead of using submodules, dependencies are now installed and configured through a new automated task-based system. The build configuration is updated to use `find_package` for locating dependencies, and dependency paths are managed via generated CMake settings. The README is updated to instruct users to run a task for dependency installation rather than initializing submodules. Additionally, a new dependency on `ystdlib::error_handling` is added to the `spider_core` target.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------|------------------------------------------------------------------------------------------------|
| .gitmodules, submodules/Catch2, submodules/abseil-cpp | Removed Catch2 and abseil-cpp submodules and their references. |
| CMakeLists.txt, src/spider/CMakeLists.txt | Replaced submodule-based dependency inclusion with `find_package` calls for dependencies; added `ystdlib::error_handling` as a public dependency to `spider_core`. |
| dep-tasks.yaml | Introduced a structured, automated workflow for installing and configuring third-party dependencies, including new tasks and variables for dependency handling. |
| taskfile.yaml | Added variables for dependency and CMake settings directories/files to support the new installation workflow. |
| README.md | Updated setup instructions to use the new dependency installation task instead of submodule commands. |
| src/spider/.clang-format | Added `ystdlib` to the regex pattern for library header grouping in formatting rules. |
| lint-tasks.yaml | Split clang-tidy linting into two subtasks to separately lint example code and main/test code. |
| src/spider/worker/DllLoader.cpp, src/spider/worker/FunctionManager.cpp, src/spider/worker/FunctionNameManager.cpp | Renamed the exported aliases for `FunctionManager::get_instance` and `FunctionNameManager::get_instance` by prefixing with `g_`. |
| tools/yscope-dev-utils | Updated subproject commit reference for `tools/yscope-dev-utils`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant TaskRunner
participant DependencyInstaller
participant CMake
participant BuildSystem
User->>TaskRunner: Run deps:dep_install
TaskRunner->>DependencyInstaller: Install Abseil, Catch2, Outcome, QuickCPPLib, ystdlib
DependencyInstaller->>DependencyInstaller: Download & extract archives, configure CMake settings
DependencyInstaller->>CMake: Write settings.cmake with dependency paths
User->>CMake: Configure project
CMake->>BuildSystem: Locate dependencies via find_package using settings.cmake
BuildSystem->>User: Build with external dependenciesPossibly related PRs
Suggested reviewers
|
davidlion
left a comment
davidlion
left a comment
There was a problem hiding this comment.
Update building/setup parts of docs/README.
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
13-16: Clarify dependency installation context
The new instructions correctly guide contributors to install dependencies via the Taskfile. For improved clarity, consider noting thattask deps:dep_installmust be run from the project’s root directory and relies on Task CLI v3.40.0 or newer (as specified in the requirements).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
davidlion
left a comment
davidlion
left a comment
There was a problem hiding this comment.
It's my fault this PR dragged on, but can we update it to use the new helper from y-scope/yscope-dev-utils#41? The change should mostly just be removing some of the new tasks as they are now in dev-utils.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
dep-tasks.yaml (3)
6-15: Evaluate and centralize dependency variables
The repeated pattern ofG_<LIB>_LIB_NAMEandG_<LIB>_WORK_DIRcould be abstracted (e.g., loop over a list of libs) to reduce duplication and simplify future additions. Also verify thatG_DEPS_DIRandG_DEPS_CMAKE_SETTINGS_DIRare defined elsewhere to avoid task failures.
44-49: Ensure safe directory cleanup and creation
Usingrm -rfandmkdir -pis standard, but consider adding a guard to validateG_DEPS_CMAKE_SETTINGS_DIRisn’t empty or/to prevent accidental deletions.
159-171: Reuse add-package-root-to-cmake-settings for ystdlib
Instead of a manualechoblock, you can invoke the existing helper to keep code DRY and ensure consistent quoting:download-ystdlib: internal: true run: "once" cmds: - task: ":utils:remote:download-and-extract-tar" vars: FILE_SHA256: "d3fc9804eacb3ee4f156ae0ca37151cb04847580" OUTPUT_DIR: "{{.G_YSTDLIB_WORK_DIR}}/{{.G_YSTDLIB_LIB_NAME}}-src" URL: "https://github.com/y-scope/ystdlib-cpp/archive/d3fc980.tar.gz" - - >- - echo "set( - SPIDER_YSTDLIB_SOURCE_DIRECTORY \"{{.G_YSTDLIB_WORK_DIR}}/{{.G_YSTDLIB_LIB_NAME}}-src\" - )" >> "{{.G_DEPS_CMAKE_SETTINGS_DIR}}/{{.G_YSTDLIB_LIB_NAME}}.cmake" + - task: "add-package-root-to-cmake-settings" + vars: + NAME: "SPIDER_YSTDLIB_SOURCE_DIRECTORY" + INSTALL_PREFIX: "{{.G_YSTDLIB_WORK_DIR}}/{{.G_YSTDLIB_LIB_NAME}}-src"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(1 hunks)dep-tasks.yaml(2 hunks)src/spider/.clang-format(1 hunks)src/spider/CMakeLists.txt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/spider/.clang-format
- src/spider/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
⏰ 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 (8)
dep-tasks.yaml (8)
28-29: Confirm proper sequencing of dep_install invocation
Adding- task: "dep_install"to bothlib_install_macandlib_install_linuxensures the new workflow runs after platform‐specific scripts. Verify on each OS that only onedep_installexecutes and there are no unintended repeats.Also applies to: 36-37
38-42: Encapsulate dependency installation steps
Thedep_installtask cleanly sequencesinstall-all-init,install-all-run, andinstall-all-finish. This modular breakdown is clear and maintainable.
50-57: Verify installation task dependencies order
install-all-runproperly declares its sub‐tasks (install-abseil,install-Catch2,install-outcome,download-ystdlib). Withinstall-outcomedepending oninstall-quickcpplib, the overall order is correct.
83-99: Use consistent task naming conventions
Theinstall-abseiltask leverages:utils:cmake:install-remote-tarcorrectly. Ensure the lowercaseabslvalue forG_ABSEIL_LIB_NAMEmatches your CMakefind_package(absl ...)calls for consistency.
100-116: Validate Catch2 version and naming
The Catch2 URL, checksum, and CMake options look correct. Note thatG_CATCH2_LIB_NAMEisCatch2(capitalized)—confirm this aligns with thefind_package(Catch2 ...)usage in your CMake scripts.
117-139: Ensure correct CMake policy and ordering
Including-DCMAKE_POLICY_DEFAULT_CMP0074=NEWallows<Package>_ROOTsupport. Theinstall-outcomedependency oninstall-quickcpplibis explicit and ideal.
140-158: Confirm QuickCPPLib installation parameters
The QuickCPPLib task uses the sameinstall-remote-tarhelper with appropriate flags. It aligns well with the otherinstall-xxxpatterns.
58-67:⚠️ Potential issueEscape quotes in the shell loop
The current block injects template paths inside double‐quoted strings, which can break shell parsing. Use single quotes around the YAML block or escape inner quotes to ensure the loop runs reliably.Apply this diff to fix quoting:
install-all-finish: internal: true cmds: - - >- - for file in {{.G_DEPS_CMAKE_SETTINGS_DIR}}/*.cmake; do - if [ "$file" != "{{.G_DEPS_CMAKE_SETTINGS_FILE}}" ]; then - echo "include(\"$file\")" >> "{{.G_DEPS_CMAKE_SETTINGS_FILE}}"; - fi - done + - >- + for file in '{{.G_DEPS_CMAKE_SETTINGS_DIR}}'/*.cmake; do + if [ "$file" != '{{.G_DEPS_CMAKE_SETTINGS_FILE}}' ]; then + echo 'include("'$file'")' >> '{{.G_DEPS_CMAKE_SETTINGS_FILE}}'; + fi + doneLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lint-tasks.yaml (1)
96-105: Separate report outputs for main vs. examples
Bothclang-tidy-findcalls write to the sameOUTPUT_DIR({{.G_LINT_CLANG_TIDY_DIR}}), which will merge diagnostics fromsrc/testsandexamplesinto one place. Consider directing the examples scan into its own subdirectory (e.g.,{{.G_LINT_CLANG_TIDY_DIR}}/examples) or use a distinct variable so you can review findings for production code and examples independently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lint-tasks.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (1)
lint-tasks.yaml (1)
96-105:Details
✅ Verification successful
Verify definition of G_EXAMPLES_COMPILE_COMMANDS_DB
The newclang-tidy-findinvocation references{{.G_EXAMPLES_COMPILE_COMMANDS_DB}}, but I don’t see it declared in this YAML. Ensure that this variable is defined (for example in your top‑leveltaskfile.yamlor a sharedvarsblock) so the examples compile‑commands database can be located.
🏁 Script executed:
#!/bin/bash # Check that G_EXAMPLES_COMPILE_COMMANDS_DB is actually defined in the repo rg -n 'G_EXAMPLES_COMPILE_COMMANDS_DB' .Length of output: 287
No action required: G_EXAMPLES_COMPILE_COMMANDS_DB is defined
Confirmed thatG_EXAMPLES_COMPILE_COMMANDS_DBis declared intaskfile.yaml(line 18), so theclang-tidy-findinvocation will resolve the compile‑commands database as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dep-tasks.yaml (1)
53-67: Refactor multi-line echo to a here‑doc for clarity
The complex quoting inadd-package-root-to-cmake-settingscan be simplified using a here‑doc, reducing the risk of syntax errors and improving readability.add-package-root-to-cmake-settings: internal: true requires: vars: - "NAME" - "INSTALL_PREFIX" cmds: - - >- - echo "set( - {{.NAME}}_ROOT - \"{{.INSTALL_PREFIX}}\" - CACHE PATH - \"Path to {{.NAME}} settings\" - )" >> "{{.G_DEPS_CMAKE_SETTINGS_DIR}}/{{.NAME}}.cmake" + - | + cat <<EOF >> "{{.G_DEPS_CMAKE_SETTINGS_DIR}}/{{.NAME}}.cmake" + set({{.NAME}}_ROOT "{{.INSTALL_PREFIX}}" CACHE PATH "Path to {{.NAME}} settings") + EOF
🧹 Nitpick comments (2)
dep-tasks.yaml (2)
68-81: Consolidate remote‑tar tasks with a YAML anchor for DRYness
Theinstall-abseiltask is nearly identical to otherinstall-*-tartasks. Consider defining a reusable anchor:x-install-remote-tar: &install-remote-tar internal: true run: "once" cmds: - task: ":utils:cmake:install-remote-tar" vars: {} ... install-abseil: <<: *install-remote-tar vars: NAME: "{{.G_ABSEIL_LIB_NAME}}" WORK_DIR: "{{.G_ABSEIL_WORK_DIR}}" ...This will reduce repetition across
install-Catch2,install-outcome, etc.
132-145: Unifydownload-ystdlibecho with helper or here‑doc
Instead of a rawecho, consider reusingadd-package-root-to-cmake-settingsor switching to a here‑doc for theSPIDER_YSTDLIB_SOURCE_DIRECTORYentry:cat <<EOF >> "{{.G_DEPS_CMAKE_SETTINGS_DIR}}/{{.G_YSTDLIB_LIB_NAME}}.cmake" set( SPIDER_YSTDLIB_SOURCE_DIRECTORY "{{.G_YSTDLIB_WORK_DIR}}/{{.G_YSTDLIB_LIB_NAME}}-src" ) EOFThis keeps formatting consistent and reduces risk of quoting mistakes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txt(1 hunks)dep-tasks.yaml(2 hunks)tools/yscope-dev-utils(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/yscope-dev-utils
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
⏰ 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 (8)
dep-tasks.yaml (8)
6-15: Variables for dependency names and directories are well-defined
TheG_<LIB>_LIB_NAMEand correspondingG_<LIB>_WORK_DIRvariables are consistently named and correctly referenceG_DEPS_DIR.
28-29: Invokedep_installafter submodule scripts
Runningdep_installimmediately afterinstall-lib.shkeeps platform-specific installs and dependency tasks in sequence as intended.
36-37: Platform-specific Linux install also triggersdep_install
The mirrored change for Linux ensures the dependency task runs on both macOS and Linux builds.
38-44: Consider markingdep_installas internal or adding a description
Most tasks in this file are markedinternal: trueand include a briefdesc:. Was omittinginternalfordep_installintentional? Addinginternal: trueor adesc:field could improve consistency.
45-52:install-all-runorchestrates all installs appropriately
The task dependencies correctly sequenceinstall-abseil,install-Catch2,install-outcome, anddownload-ystdlib.
82-95:install-Catch2follows the same pattern
The task correctly fetches Catch2 and generates its CMake settings.
96-115: Dependency on QuickCPPLib is configured for Outcome
Specifyinginstall-quickcpplibininstall-outcomeensures the correct build order before generating CMake settings.
116-131: QuickCPPLib installation task is correctly set up
Theinstall-quickcpplibtask mirrors other remote installs with testing disabled and proper build type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/spider/worker/DllLoader.cpp (1)
28-28: Ensure symbol name change is consistent with the compiled DLLs.The symbol name change from
"function_manager_get_instance"to"g_function_manager_get_instance"aligns with the corresponding change inFunctionManager.cpp. However, this change will cause runtime failures if any existing compiled DLLs still export the old symbol name.For naming consistency, consider also updating the function name manager symbol at line 42 to follow the same convention (
"g_function_name_manager_get_instance").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/spider/worker/DllLoader.cpp(1 hunks)src/spider/worker/FunctionManager.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/spider/worker/FunctionManager.cpp
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
dep-tasks.yaml (4)
28-37: Usedepsfield instead of in-band task invocation
Embedding- task: "dep_install"in thecmdsmay interleave shell commands and task invocations unpredictably. It would be clearer to declare the dependency explicitly:lib_install_mac: deps: ["dep_install"] dir: "{{.G_SCRIPT_DIR}}/lib_install/macOS" cmds: - "./install-lib.sh"This ensures
dep_installruns before the shell script and improves readability.
38-44: Markdep_installas internal
Sincedep_installorchestrates lower‑level tasks and isn’t intended for direct user invocation, consider addinginternal: trueto its definition. This hides the task from the public API and aligns with other internal tasks.
45-52: Unifydepslist syntax
Theinstall-all-runtask currently uses map‑style dependencies (- task: "install-abseil") whereaslib_installuses simple string lists. For consistency and readability, prefer the simpler string list syntax:deps: - install-abseil - install-Catch2 - install-outcome - download-ystdlib
53-116: Reduce duplication across library installation tasks
Theinstall-abseil,install-Catch2,install-outcome, andinstall-quickcpplibtasks all follow the same pattern invoking:utils:cmake:install-remote-tarwith similar variables. Consider templating this pattern with YAML anchors or a parameterised task to adhere to DRY principles and simplify future dependency additions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dep-tasks.yaml(2 hunks)src/spider/worker/DllLoader.cpp(2 hunks)src/spider/worker/FunctionNameManager.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/spider/worker/DllLoader.cpp
- src/spider/worker/FunctionNameManager.cpp
⏰ 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)
davidlion
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(2 hunks)examples/quick-start/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
⏰ 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 (1)
examples/quick-start/CMakeLists.txt (1)
4-4: VerifyPROJECT_IS_TOP_LEVELguard is reliably defined
Before relying onPROJECT_IS_TOP_LEVEL, ensure it’s always set in the parent CMake context to avoid unexpected behaviour when this project is included elsewhere. If it isn’t guaranteed, consider guarding with:if(DEFINED PROJECT_IS_TOP_LEVEL AND PROJECT_IS_TOP_LEVEL) # … endif()
2 participants
Description
Spidernow uses git submodule to include dependencies, i.e. abseil and catch2. Adding dependencies usesadd_subdirectoryis not ideal, and it make build longer.This pr uses tasks in
yscope-dev-utilsto install these dependencies.Checklist
breaking change.
Validation performed
task:dep_installinstalls the dependencies.Spiderbuilds and pass all unit tests and integration tests with new dependencies.Summary by CodeRabbit