build: Split core into spearate library #15
Conversation
|
Warning Rate limit exceeded@sitaowang1998 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 20 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 pull request modifies two 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/spider/CMakeLists.txt (1)
Line range hint
1-30: Consider adding installation and package configuration.To make this library properly reusable, consider:
- Adding installation rules for headers and binaries
- Generating and installing package configuration files
- Adding version information
This will allow other projects to properly find and use the spider_core library through CMake's find_package mechanism.
Would you like me to provide an example of these configurations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)src/spider/CMakeLists.txt(1 hunks)
🔇 Additional comments (1)
CMakeLists.txt (1)
12-13: Verify all dependencies support C++20.
While the specified minimum versions of Boost, fmt, and spdlog support C++20, let's verify there are no compatibility issues.
✅ Verification successful
C++20 compatibility is verified with all dependencies
The CMakeLists.txt shows that all dependencies have compatible minimum versions that support C++20:
- Boost 1.74+ (supports C++20)
- fmt 8.0.1+ (supports C++20)
- spdlog 1.9.2+ (supports C++20)
- Abseil-cpp (explicitly propagates C++ standard via ABSL_PROPAGATE_CXX_STD)
- MariaDB Client 3.1.0+ (C++20 compatible)
Additionally, no C++20-specific features that might cause compatibility issues were found in the codebase, except for standard build configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential C++20 compatibility issues in dependencies
# Test 1: Check if any C++20 features are used in the codebase
echo "Checking for C++20 feature usage..."
rg -l "concept|requires|constinit|consteval|co_await|co_yield|co_return|ranges::|views::|<span>|<concepts>|<ranges>|<coroutine>"
# Test 2: Look for any compiler compatibility checks
echo "Checking for compiler version checks..."
rg -l "CMAKE_CXX_COMPILER_VERSION|__cplusplus.*202"
# Test 3: Check for C++20-specific CMake configurations in dependencies
echo "Checking dependency CMake configurations..."
fd -e cmake -e txt CMakeLists --exec rg -l "CMAKE_CXX_STANDARD.*(20|2a)"
Length of output: 559
Script:
#!/bin/bash
# Let's check the specific C++20 features used in lint-tasks.yaml and examine CMakeLists.txt more closely
echo "Checking C++20 features in lint-tasks.yaml:"
cat lint-tasks.yaml
echo -e "\nChecking complete CMakeLists.txt content:"
cat CMakeLists.txt
echo -e "\nChecking for third-party dependency requirements:"
fd -t f "CMakeLists.txt|*.cmake|*.pc|*.pri|requirements" -X grep -l "REQUIRED|minimum|VERSION"
Length of output: 9616
src/spider/CMakeLists.txt
Outdated
| spider_core | ||
| Boost::boost | ||
| absl::flat_hash_map | ||
| ) | ||
|
|
There was a problem hiding this comment.
Specify dependency visibility for linked libraries.
Each dependency should explicitly declare its visibility scope to prevent unnecessary transitive dependencies.
target_link_libraries(
spider_core
- Boost::boost
- absl::flat_hash_map
+ PUBLIC
+ Boost::boost
+ PRIVATE
+ absl::flat_hash_map
)📝 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.
| spider_core | |
| Boost::boost | |
| absl::flat_hash_map | |
| ) | |
| target_link_libraries( | |
| spider_core | |
| PUBLIC | |
| Boost::boost | |
| PRIVATE | |
| absl::flat_hash_map | |
| ) | |
src/spider/CMakeLists.txt
Outdated
| set(SPIDER_WORKER_SOURCES worker/worker.cpp CACHE INTERNAL "spider worker source files") | ||
|
|
||
| add_executable(spider_worker) | ||
| target_sources(spider_worker PRIVATE ${SPIDER_WORKER_SOURCES}) |
There was a problem hiding this comment.
Add missing dependency on spider_core library.
The worker executable needs to link against the core library it depends on.
add_executable(spider_worker)
target_sources(spider_worker PRIVATE ${SPIDER_WORKER_SOURCES})
+target_link_libraries(spider_worker PRIVATE spider_core)
add_executable(spider::worker ALIAS spider_worker)Committable suggestion was skipped due to low confidence.
| if(SPIDER_USE_STATIC_LIBS) | ||
| add_library(spider_core STATIC) | ||
| else() | ||
| add_library(spider_core SHARED) | ||
| endif() | ||
| target_sources(spider_core PRIVATE ${SPIDER_CORE_SOURCES}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding modern CMake target properties and export controls.
The core library definition could benefit from additional configuration:
if(SPIDER_USE_STATIC_LIBS)
add_library(spider_core STATIC)
else()
add_library(spider_core SHARED)
endif()
target_sources(spider_core PRIVATE ${SPIDER_CORE_SOURCES})
+target_include_directories(spider_core
+ PUBLIC
+ $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
+ $<INSTALL_INTERFACE:include>
+)
+set_target_properties(spider_core PROPERTIES
+ CXX_VISIBILITY_PRESET hidden
+ VISIBILITY_INLINES_HIDDEN YES
+ POSITION_INDEPENDENT_CODE ON
+)📝 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(SPIDER_USE_STATIC_LIBS) | |
| add_library(spider_core STATIC) | |
| else() | |
| add_library(spider_core SHARED) | |
| endif() | |
| target_sources(spider_core PRIVATE ${SPIDER_CORE_SOURCES}) | |
| if(SPIDER_USE_STATIC_LIBS) | |
| add_library(spider_core STATIC) | |
| else() | |
| add_library(spider_core SHARED) | |
| endif() | |
| target_sources(spider_core PRIVATE ${SPIDER_CORE_SOURCES}) | |
| target_include_directories(spider_core | |
| PUBLIC | |
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> | |
| $<INSTALL_INTERFACE:include> | |
| ) | |
| set_target_properties(spider_core PROPERTIES | |
| CXX_VISIBILITY_PRESET hidden | |
| VISIBILITY_INLINES_HIDDEN YES | |
| POSITION_INDEPENDENT_CODE ON | |
| ) |
CMakeLists.txt
Outdated
| set(CMAKE_CXX_STANDARD 20) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding additional C++ standard configuration flags.
The C++20 standard setting looks good, but consider adding these flags for more precise control:
set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_CXX_EXTENSIONS OFF)This ensures:
- Compilation fails if C++20 is not available (STANDARD_REQUIRED)
- Only ISO C++20 features are used, without compiler-specific extensions (EXTENSIONS OFF)
📝 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(CMAKE_CXX_STANDARD 20) | |
| set(CMAKE_CXX_STANDARD 20) | |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |
| set(CMAKE_CXX_EXTENSIONS OFF) | |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/spider/CMakeLists.txt (1)
Line range hint
1-13: Consider separating interface and implementation files.The source list combines header and implementation files. For better maintainability and to clearly distinguish the public interface, consider organizing them into separate lists.
-set(SPIDER_CORE_SOURCES +set(SPIDER_CORE_PUBLIC_HEADERS core/Error.hpp core/Data.hpp core/Task.hpp core/TaskGraph.hpp storage/MetadataStorage.hpp storage/DataStorage.hpp + storage/MysqlStorage.hpp + CACHE INTERNAL + "spider core public headers" +) + +set(SPIDER_CORE_SOURCES storage/MysqlStorage.cpp - storage/MysqlStorage.hpp CACHE INTERNAL "spider core source files" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)src/spider/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
🔇 Additional comments (3)
src/spider/CMakeLists.txt (3)
15-19: Previous suggestion about modern CMake properties is still applicable.
The library definition would benefit from additional target properties as suggested in the previous review.
26-26: LGTM! Worker executable configuration is well-structured.
The worker executable configuration follows CMake best practices with proper source management and aliasing.
Line range hint 1-28: Verify complete separation of core components.
The changes successfully implement the core library split. Let's verify there are no remaining direct inclusions of core files in the worker component.
✅ Verification successful
Core separation successfully verified
The worker component shows proper separation from core components with:
- No direct includes of core headers found
- No relative path includes to core directory
- Clean worker implementation without any dependencies on internal core headers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct includes of core headers in worker files
# that should now come through the library interface
# Test: Search for direct includes of core headers in worker files
# Expect: No direct includes of core headers in worker sources
rg -t cpp -t hpp '#include.*"core/' src/spider/worker/
Length of output: 84
Script:
#!/bin/bash
# Search for any includes of core headers in worker files, fixing the file type flags
rg --type cpp '#include.*"core/' src/spider/worker/
# Also check for potential indirect includes through relative paths
rg --type cpp '#include.*"\.\./core/' src/spider/worker/
# List all files in worker directory to ensure we're searching the right place
fd . src/spider/worker/
# Check the actual content of worker.cpp to analyze its dependencies
cat src/spider/worker/worker.cpp
Length of output: 269
src/spider/CMakeLists.txt
Outdated
| absl::flat_hash_map | ||
| ) | ||
| target_sources(spider_worker PRIVATE ${SPIDER_WORKER_SOURCES}) | ||
| target_link_libraries(spider_worker PRIVATE ${spider_core}) |
There was a problem hiding this comment.
Fix incorrect library reference.
The ${spider_core} variable reference is incorrect. The library target name should be used directly.
-target_link_libraries(spider_worker PRIVATE ${spider_core})
+target_link_libraries(spider_worker PRIVATE spider_core)📝 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.
| target_link_libraries(spider_worker PRIVATE ${spider_core}) | |
| target_link_libraries(spider_worker PRIVATE spider_core) |
Description
Split core into a separate library for faster compile time and better dependency management.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes