feat: Make spider installable (resolves #138). #139
Conversation
|
""" WalkthroughThis update introduces CMake packaging support, generates and installs CMake package configuration files, and formalizes install/export rules for the Changes
Sequence Diagram(s)sequenceDiagram
participant DownstreamProject
participant CMake
participant SpiderPackage
DownstreamProject->>CMake: find_package(spider REQUIRED)
CMake->>SpiderPackage: Load Config.cmake
SpiderPackage->>CMake: find_dependency(Boost, fmt, spdlog, etc.)
SpiderPackage->>CMake: Check SPIDER_YSTDLIB_SOURCE_DIRECTORY
SpiderPackage->>CMake: add_subdirectory(ystdlib)
SpiderPackage->>CMake: include spider_core_export.cmake
SpiderPackage->>CMake: include spider_client_export.cmake
CMake->>DownstreamProject: Make spider::spider_client and spider::spider_core available
Possibly related PRs
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
cmake/Config.cmake.in (1)
14-16: Consider making YSTDLIB dependency optionalThe current check forces users to define
SPIDER_YSTDLIB_SOURCE_DIRECTORYto use the spider package. Consider making this dependency optional or providing an alternative way to configure it.if(NOT DEFINED SPIDER_YSTDLIB_SOURCE_DIRECTORY) - message(FATAL_ERROR "SPIDER_YSTDLIB_SOURCE_DIRECTORY must be set to use spider") + # Look for an installed ystdlib or provide a default path option + set(SPIDER_YSTDLIB_SOURCE_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}/../ystdlib + CACHE PATH "Path to ystdlib source directory") + message(STATUS "SPIDER_YSTDLIB_SOURCE_DIRECTORY not defined, using default: ${SPIDER_YSTDLIB_SOURCE_DIRECTORY}") endif()src/spider/CMakeLists.txt (3)
65-113: Consider simplifying header installation with GLOBThe current approach lists each header file individually, which is verbose and error-prone when adding new headers. Consider using GLOB to simplify maintenance.
-install( - FILES - ${CMAKE_CURRENT_SOURCE_DIR}/core/Error.hpp - ${CMAKE_CURRENT_SOURCE_DIR}/core/Data.hpp - ${CMAKE_CURRENT_SOURCE_DIR}/core/Driver.hpp - ${CMAKE_CURRENT_SOURCE_DIR}/core/KeyValueData.hpp - ${CMAKE_CURRENT_SOURCE_DIR}/core/Task.hpp - ${CMAKE_CURRENT_SOURCE_DIR}/core/TaskGraph.hpp - ${CMAKE_CURRENT_SOURCE_DIR}/core/JobMetadata.hpp - DESTINATION include/${PROJECT_NAME}/core -) +file(GLOB CORE_HEADERS "${CMAKE_CURRENT_SOURCE_DIR}/core/*.hpp") +install( + FILES ${CORE_HEADERS} + DESTINATION include/${PROJECT_NAME}/core +)Note: While this approach simplifies the code, be aware that CMake generally discourages using GLOB for build targets due to potential issues with newly added files not triggering rebuilds. For installation rules, this limitation is less important.
273-273: Consider renaming alias target for clarityThe current alias
${PROJECT_NAME}::spiderfor thespider_clienttarget might cause confusion. Consider using${PROJECT_NAME}::spider_clientfor consistency with the actual target name.-add_library(${PROJECT_NAME}::spider ALIAS spider_client) +add_library(${PROJECT_NAME}::spider_client ALIAS spider_client)
169-186: Consider adding installation rules for executablesThe current changes don't include installation rules for executables like
spider_task_executor,spider_worker, andspider_scheduler. Consider adding installation rules for these executables to make them available to users.target_link_libraries( spider_task_executor PRIVATE Boost::filesystem Boost::program_options Boost::system ${CMAKE_DL_LIBS} fmt::fmt spdlog::spdlog ) +install( + TARGETS spider_task_executor + RUNTIME DESTINATION bin +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(1 hunks)cmake/Config.cmake.in(1 hunks)examples/quick-start/CMakeLists.txt(3 hunks)src/spider/CMakeLists.txt(4 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
🔇 Additional comments (12)
CMakeLists.txt (1)
200-218: LGTM: CMake packaging support properly implementedThe changes correctly add CMake packaging support, enabling the project to be installed and used via CMake's
find_package()mechanism. The implementation follows best practices by usingCMakePackageConfigHelpersto generate and install the necessary configuration files.examples/quick-start/CMakeLists.txt (3)
4-7: LGTM: Proper package consumption setupThe example project correctly demonstrates how to use the spider package as an installable dependency. Setting the right paths and using
find_package(spider REQUIRED)showcases the intended use case.
18-18: LGTM: Updated target referenceCorrectly updated to use the namespaced target
spider::spider_clientfrom the installed package instead of direct linking.
27-27: LGTM: Consistent target namingTarget reference is consistent with the exported namespace and matches the one used for the tasks library.
cmake/Config.cmake.in (2)
1-12: LGTM: Well-structured package configurationThe configuration properly initializes the package and correctly handles dependency discovery with appropriate version requirements.
18-25: LGTM: Proper integration of required componentsThe configuration correctly adds the ystdlib subdirectory and includes the export files for the core and client components.
src/spider/CMakeLists.txt (6)
46-51: LGTM: Properly configured include directoriesThe changes correctly switch header files from PUBLIC to PRIVATE in target_sources while adding explicit PUBLIC include directories with proper build and install interfaces.
63-63: LGTM: Added proper alias target for spider_coreThe alias target follows CMake best practices by using the project namespace.
115-125: LGTM: Properly configured export targets for spider_coreThe export targets are correctly defined with namespace and installed to the appropriate location.
127-137: LGTM: Properly configured export targets for error_handlingThe export targets are correctly defined with namespace and installed to the appropriate location.
258-263: LGTM: Properly configured include directories for spider_clientSimilar to spider_core, the changes correctly switch header files from PUBLIC to PRIVATE in target_sources while adding explicit PUBLIC include directories with proper build and install interfaces.
275-306: LGTM: Properly configured installation and export for spider_clientThe installation rules for header files and export targets are correctly defined.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
taskfile.yaml (2)
45-47: Extract and reuse the install-test path as a variable & clean it upTo avoid hard-coded paths and ensure idempotent runs, pull
install-testinto a dedicatedvarsentry (e.g.G_INSTALL_TEST), replace inline literals, and update thecleantask to remove it:vars: G_DEPS_CMAKE_SETTINGS_FILE: "{{.G_DEPS_CMAKE_SETTINGS_DIR}}/settings.cmake" + G_INSTALL_TEST: "{{.ROOT_DIR}}/install-test" tasks: clean: cmds: - - "rm -rf '{{.G_BUILD_DIR}}'" + - "rm -rf '{{.G_BUILD_DIR}}' '{{.G_INSTALL_TEST}}'" config-cmake-project: cmds: - - "cmake -S '{{.ROOT_DIR}}' -B '{{.G_BUILD_SPIDER_DIR}}' -DCMAKE_INSTALL_PREFIX='{{.ROOT_DIR}}/install-test'" - - "cmake --build '{{.G_BUILD_SPIDER_DIR}}' --target install -j$(nproc)" + - "cmake -S '{{.ROOT_DIR}}' -B '{{.G_BUILD_SPIDER_DIR}}' -DCMAKE_INSTALL_PREFIX='{{.G_INSTALL_TEST}}'" + - "cmake --build '{{.G_BUILD_SPIDER_DIR}}' --target install -j$(nproc)"Happy to draft the full updated
taskfile.yamlif you’d like.
47-47: Remove trailing semicolon & add example build stepThe trailing semicolon in
CMAKE_PREFIX_PATHmay introduce an empty search entry, and currently the example is only configured, not built. Consider:config-cmake-project: cmds: - - "cmake -S '{{.ROOT_DIR}}/examples/quick-start' -B '{{.G_BUILD_EXAMPLES_DIR}}' -DCMAKE_PREFIX_PATH='{{.ROOT_DIR}}/install-test;'" + - "cmake -S '{{.ROOT_DIR}}/examples/quick-start' -B '{{.G_BUILD_EXAMPLES_DIR}}' -DCMAKE_PREFIX_PATH='{{.ROOT_DIR}}/install-test'" + - "cmake --build '{{.G_BUILD_EXAMPLES_DIR}}' --target all -j$(nproc)"Let me know if you’d like me to integrate these changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(1 hunks)examples/quick-start/CMakeLists.txt(3 hunks)src/spider/CMakeLists.txt(4 hunks)taskfile.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/quick-start/CMakeLists.txt
- CMakeLists.txt
- src/spider/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)
Description
#138
Checklist
breaking change.
Validation performed
Summary by CodeRabbit