feat: support ninja build,reduced dependency #178
Conversation
WalkthroughThis pull request introduces comprehensive changes to the CMake build system configuration across multiple files. The modifications primarily focus on standardizing external library management, updating source directory paths, and refining library linking strategies. Key changes include renaming external project targets, updating build commands to use flexible variables, and modifying library dependency management. The changes aim to improve the build system's flexibility, maintainability, and clarity without altering the core functionality of the project. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 5
🧹 Nitpick comments (7)
cmake/brpc.cmake (1)
57-58: Consider adding error handling for build command.While the use of
${EXTERNAL_BUILD}variable improves flexibility, consider adding error checking to handle build failures gracefully.- BUILD_COMMAND ${EXTERNAL_BUILD} -j${CPU_CORE} + BUILD_COMMAND bash -c "set -e; ${EXTERNAL_BUILD} -j${CPU_CORE} || (echo 'Build failed'; exit 1)"tests/assets/default.conf (2)
44-47: Document redis_compatible_mode implications.While the setting is added, it lacks documentation about what features or behaviors are affected when enabled.
Add comments explaining the implications of redis_compatible_mode, similar to other configuration parameters.
209-209: Consider using environment variable for password.Storing passwords directly in configuration files is not recommended for production environments.
Consider using an environment variable:
-# requirepass 123456 +# requirepass ${KIWI_PASSWORD}src/storage/tests/CMakeLists.txt (1)
27-36: Consider modularizing tests to reduce dependency complexity.While the changes to use variables for libraries are good, the large number of dependencies might indicate that these tests are doing too much. Consider splitting them into smaller, more focused test files.
src/storage/CMakeLists.txt (1)
Line range hint
14-23: Consider consistent visibility for include directories.The mix of PUBLIC and PRIVATE visibility for include directories seems inconsistent. Consider:
- Making implementation directories (src/) PRIVATE
- Keeping interface directories (include/) PUBLIC
- Making third-party includes PRIVATE
CMakeLists.txt (1)
119-129: LGTM! Consider adding documentation for the generator-specific variables.The Ninja build support implementation is well-structured and provides clear error handling.
Consider adding comments to document how
EXTERNAL_GENERATORandEXTERNAL_BUILDare used throughout the build system.src/raft/CMakeLists.txt (1)
54-71: LGTM! Consider standardizing the semicolon usage.The library linking changes follow modern CMake practices by using the PRIVATE keyword consistently.
For consistency, standardize the semicolon usage in TARGET_LINK_LIBRARIES:
- PRIVATE net; - PRIVATE dl; + PRIVATE net + PRIVATE dl
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
CMakeLists.txt(1 hunks)cmake/braft.cmake(3 hunks)cmake/brpc.cmake(3 hunks)cmake/fmt.cmake(2 hunks)cmake/gflags.cmake(2 hunks)cmake/gtest.cmake(1 hunks)cmake/leveldb.cmake(2 hunks)cmake/lz4.cmake(2 hunks)cmake/openssl.cmake(2 hunks)cmake/protobuf.cmake(2 hunks)cmake/rocksdb.cmake(2 hunks)cmake/snappy.cmake(2 hunks)cmake/spdlog.cmake(2 hunks)cmake/zlib.cmake(2 hunks)cmake/zstd.cmake(2 hunks)src/CMakeLists.txt(2 hunks)src/net/CMakeLists.txt(1 hunks)src/raft/CMakeLists.txt(2 hunks)src/resp/CMakeLists.txt(1 hunks)src/resp/tests/CMakeLists.txt(1 hunks)src/std/CMakeLists.txt(1 hunks)src/std/tests/CMakeLists.txt(1 hunks)src/storage/CMakeLists.txt(1 hunks)src/storage/tests/CMakeLists.txt(1 hunks)tests/assets/default.conf(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/resp/tests/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (30)
cmake/zlib.cmake (1)
26-30: LGTM! Build system changes look good.The changes to support Ninja build system are correct:
- Added
${EXTERNAL_GENERATOR}for Ninja support- Using flexible
${EXTERNAL_BUILD}instead of hardcoded make- Added
BUILD_BYPRODUCTSfor Ninja dependency trackingcmake/snappy.cmake (1)
29-31: LGTM! Build system changes look good.The changes to support Ninja build system are correct and the build options are well configured:
- Added
${EXTERNAL_GENERATOR}for Ninja support- Using flexible
${EXTERNAL_BUILD}- Added
BUILD_BYPRODUCTSfor Ninja dependency tracking- Appropriate build options (static libs, no tests/benchmarks)
cmake/gtest.cmake (1)
19-27: LGTM! Build system changes look good.The changes to support Ninja build system are correct and using the latest stable version of GoogleTest:
- Added
${EXTERNAL_GENERATOR}for Ninja support- Using flexible
${EXTERNAL_BUILD}- Using latest stable version v1.14.0
cmake/lz4.cmake (1)
31-34: LGTM! Build system changes look good.The changes to support Ninja build system are correct and well-configured:
- Added
${EXTERNAL_GENERATOR}for Ninja support- Using flexible
${EXTERNAL_BUILD}- Added
BUILD_BYPRODUCTSfor Ninja dependency tracking- Appropriate build options (static libs, no tests)
- Secure download with URL_HASH
cmake/fmt.cmake (2)
13-13: LGTM: Source directory path simplified.The simplified path structure aligns with the project's standardization efforts.
18-18: LGTM: Build system improvements for Ninja compatibility.Good improvements:
- Project name simplified to
fmt- Added Ninja support via
${EXTERNAL_GENERATOR}- Explicit
BUILD_BYPRODUCTSfor Ninja- Flexible build command using
${EXTERNAL_BUILD}Also applies to: 36-38
cmake/zstd.cmake (2)
6-6: LGTM: Source directory path simplified.The simplified path structure aligns with the project's standardization efforts.
11-11: LGTM: Build system improvements for Ninja compatibility.Good improvements:
- Project name simplified to
zstd- Added Ninja support via
${EXTERNAL_GENERATOR}- Explicit
BUILD_BYPRODUCTSfor Ninja- Flexible build command using
${EXTERNAL_BUILD}Also applies to: 31-34
cmake/spdlog.cmake (3)
12-12: LGTM: Source directory path simplified.The simplified path structure aligns with the project's standardization efforts.
19-19: LGTM: Build system improvements for Ninja compatibility.Good improvements:
- Project name simplified to
spdlog- Added Ninja support via
${EXTERNAL_GENERATOR}- Explicit
BUILD_BYPRODUCTSfor Ninja- Flexible build command using
${EXTERNAL_BUILD}Also applies to: 38-40
16-16: Verify the fmt external setting.There seems to be an inconsistency:
- Line 16 adds
-DSPDLOG_FMT_EXTERNAL- Line 36 sets
-DSPDLOG_FMT_EXTERNAL=OFFThis could lead to unexpected behavior. Please verify which setting is intended.
Also applies to: 36-36
cmake/leveldb.cmake (2)
6-6: LGTM: Source directory path simplified.The simplified path structure aligns with the project's standardization efforts.
12-12: LGTM: Build system improvements for Ninja compatibility.Good improvements:
- Project name simplified to
leveldb- Added Ninja support via
${EXTERNAL_GENERATOR}- Explicit
BUILD_BYPRODUCTSfor Ninja- Flexible build command using
${EXTERNAL_BUILD}Also applies to: 34-37
cmake/rocksdb.cmake (1)
6-6: LGTM! Build system improvements look good.The changes enhance build system flexibility and dependency tracking:
- Simplified directory structure with updated
ROCKSDB_SOURCES_DIR- Support for Ninja build through
${EXTERNAL_GENERATOR}and${EXTERNAL_BUILD}- Proper output tracking with
BUILD_BYPRODUCTSAlso applies to: 11-11, 44-45, 47-47
cmake/gflags.cmake (1)
12-12: LGTM! Build configuration simplified and improved.The changes streamline the build system:
- Simplified directory structure
- Enhanced build flexibility with
${EXTERNAL_GENERATOR}and${EXTERNAL_BUILD}- Proper dependency tracking with
BUILD_BYPRODUCTS- Removed redundant library declarations
Also applies to: 18-18, 26-26, 41-41, 43-43
cmake/braft.cmake (2)
8-8: LGTM! Build system improvements look good.The changes enhance build system flexibility and dependency tracking.
Also applies to: 20-20, 54-56
Line range hint
24-25: Verify the git tag version.The tag
v1.1.2-beta20250101appears to reference January 2025, which is a future date. Please verify if this is the correct version tag.✅ Verification successful
Git tag
v1.1.2-beta20250101is valid and exists in the repository🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the specified tag exists in the braft repository gh api repos/arana-db/braft/git/refs/tags/v1.1.2-beta20250101 || echo "Tag does not exist"Length of output: 431
cmake/openssl.cmake (2)
6-11: LGTM! Improved modularity in OpenSSL configuration.The centralization of library paths into variables and the simplified directory structure enhance maintainability.
46-46: Essential addition for Ninja build support.The BUILD_BYPRODUCTS specification is crucial for Ninja build system compatibility, as Ninja requires explicit declaration of build outputs.
cmake/brpc.cmake (1)
59-59: LGTM! Proper Ninja build support.The addition of BUILD_BYPRODUCTS is essential for Ninja build compatibility.
tests/assets/default.conf (1)
19-21: LGTM! Well-documented TCP keepalive configuration.The tcp-keepalive documentation clearly explains the purpose and behavior of the setting.
src/net/CMakeLists.txt (1)
19-19: LGTM! Proper include directory configuration.The addition of FMT_INCLUDE_DIR as a private include follows CMake best practices.
src/resp/CMakeLists.txt (1)
18-21: LGTM! Good improvements to CMake visibility.The changes correctly specify PRIVATE visibility for libraries and include directories, following modern CMake best practices. This helps prevent unnecessary transitive dependencies.
Also applies to: 28-28
src/std/CMakeLists.txt (1)
19-22: LGTM! Good use of variables for library dependencies.The changes correctly use CMake variables for library dependencies and explicitly mark them as PRIVATE since they're implementation details.
src/std/tests/CMakeLists.txt (1)
22-23: LGTM! Appropriate visibility for test dependencies.The changes correctly:
- Use PRIVATE visibility for test-specific dependencies
- Use CMake variables for GTest libraries
Also applies to: 28-29
src/storage/tests/CMakeLists.txt (1)
17-23: LGTM! Appropriate PRIVATE visibility for test includes.The changes correctly specify PRIVATE visibility for test-specific include directories.
src/storage/CMakeLists.txt (1)
23-23: LGTM! Added PSTD include directory.The addition of PSTD_INCLUDE_DIR as PRIVATE is appropriate for implementation-specific headers.
src/CMakeLists.txt (2)
38-38: LGTM! OpenSSL integration looks good.The OpenSSL include directory and dependency are properly configured.
Also applies to: 58-58
70-81: LGTM! Verify library variable definitions.The transition to variable-based library linking improves maintainability and follows modern CMake practices.
✅ Verification successful
All library variables are properly defined
All CMake library variables used in
src/CMakeLists.txtare correctly defined in their respective cmake files under thecmake/directory, with proper cache directives and build byproducts configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all library variables are properly defined for var in FMT_LIBRARIES ROCKSDB_LIBRARIES SPDLOG_LIBRARIES BRAFT_LIBRARIES PROTOBUF_LIBRARY LZ4_LIBRARIES ZLIB_LIBRARIES OPENSSL_SSL_LIBRARY LEVELDB_LIBRARIES Snappy_LIBRARIES zstd_LIBRARIES OPENSSL_CRYPTO_LIBRARY; do echo "Checking $var definition..." rg -l "\b${var}\b.*=.*" cmake/ doneLength of output: 4187
Script:
#!/bin/bash # Search for find_package calls and similar patterns echo "=== Searching for find_package calls ===" rg -i "find_package.*(?:fmt|rocksdb|spdlog|braft|protobuf|lz4|zlib|openssl|leveldb|snappy|zstd)" --type cmake echo -e "\n=== Checking root CMakeLists.txt ===" cat CMakeLists.txt echo -e "\n=== Looking for similar variable patterns ===" rg "(?:FMT|ROCKSDB|SPDLOG|BRAFT|PROTOBUF|LZ4|ZLIB|OPENSSL|LEVELDB|SNAPPY|ZSTD)_(?:LIBRARY|LIBRARIES|INCLUDE)" --type cmakeLength of output: 16498
src/raft/CMakeLists.txt (1)
45-45: LGTM! Include directory addition is consistent.The PSTD include directory is properly configured following the existing pattern.
| GIT_REPOSITORY "https://github.com/madler/zlib.git" | ||
| GIT_TAG "v1.2.8" |
There was a problem hiding this comment.
💡 Codebase verification
Update recommended: zlib v1.2.8 is significantly outdated
The current version (v1.2.8) is five releases behind the latest version (v1.3.1). Consider updating to either:
- v1.2.13 for minor version compatibility
- v1.3.1 for latest features and fixes (requires testing for breaking changes)
🔗 Analysis chain
Consider updating zlib version.
The current version v1.2.8 is from 2013. Consider updating to a newer version (e.g., v1.3) for security fixes and improvements.
Run this script to check the latest version and any security advisories:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest zlib version and security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "zlib") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 337
Script:
#!/bin/bash
# Check latest zlib tags and security advisories
echo "Latest tags from zlib repository:"
git ls-remote --tags https://github.com/madler/zlib.git | grep -v '{}' | cut -d'/' -f3 | sort -V | tail -n 5
echo -e "\nSecurity advisories (C ecosystem):"
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: C, package: "zlib") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1062
| -DCMAKE_INSTALL_PREFIX=${LIB_INSTALL_PREFIX} | ||
| -DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR} | ||
| -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} | ||
| ${EXTERNAL_PROJECT_C} | ||
| ${EXTERNAL_PROJECT_CXX} | ||
| ${EXTERNAL_PROJECT_CXX_FLAGS} | ||
| ${EXTERNAL_PROJECT_CXX_LINK_FLAGS} | ||
| BUILD_COMMAND ${EXTERNAL_BUILD} -j${CPU_CORE} | ||
| BUILD_BYPRODUCTS ${GTEST_LIBRARIES} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing BUILD_BYPRODUCTS.
The BUILD_BYPRODUCTS should include all built libraries to properly support Ninja build system.
- BUILD_BYPRODUCTS ${GTEST_LIBRARIES}
+ BUILD_BYPRODUCTS ${GTEST_LIBRARIES} ${GTEST_MAIN_LIBRARIES} ${GMOCK_LIBRARIES}📝 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.
| BUILD_BYPRODUCTS ${GTEST_LIBRARIES} | |
| BUILD_BYPRODUCTS ${GTEST_LIBRARIES} ${GTEST_MAIN_LIBRARIES} ${GMOCK_LIBRARIES} |
| @@ -13,14 +13,14 @@ ELSE () | |||
| SET(LIB_PROTOC "libprotoc.a") | |||
| ENDIF () | |||
|
|
|||
| SET(PROTOBUF_SOURCES_DIR "${LIB_SOURCE_DIR}/extern_protobuf" CACHE PATH "Path to protobuf sources") | |||
| SET(PROTOBUF_SOURCES_DIR "${LIB_SOURCE_DIR}/protobuf" CACHE PATH "Path to protobuf sources") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing BUILD_BYPRODUCTS for protoc library.
While the build system improvements look good, the BUILD_BYPRODUCTS should include both the protobuf and protoc libraries since both are built by this target.
Update the BUILD_BYPRODUCTS line:
- BUILD_BYPRODUCTS ${LIB_PROTOBUF}
+ BUILD_BYPRODUCTS ${PROTOBUF_LIBRARY} ${PROTOC_LIBRARY}Also applies to: 23-23, 48-50
| PRIVATE ${ROCKSDB_SOURCES_DIR}/include | ||
| PRIVATE ${LIB_INCLUDE_DIR} | ||
| PRIVATE ${PSTD_INCLUDE_DIR} | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Keep core utility includes PUBLIC in kstd library
The verification confirms that kstd headers are widely used across the codebase. Making these includes PRIVATE would force all dependent targets to duplicate the include paths. The following headers are part of the public interface:
- Core utilities: std_string.h, mutex.h, std_status.h
- Infrastructure: env.h, log.h, memory_file.h
- Concurrency: thread_pool.h, lock_mgr.h, lock_free_ring_buffer.h
🔗 Analysis chain
Consider keeping some include directories PUBLIC.
Since kstd is a core library, other targets might need these include paths when using kstd's headers. Consider keeping headers that are part of kstd's public interface as PUBLIC.
Let's verify which headers are exposed in the public interface:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find headers that might be included by other targets
rg -l "#include.*kstd" . --type cpp
Length of output: 37
Script:
#!/bin/bash
# Find all header files in std directory
fd -e h -e hpp . src/std/
# Check how the include paths are used in other CMake files
rg "include|INCLUDE" "CMakeLists.txt"
Length of output: 1032
Script:
#!/bin/bash
# Search for includes of std headers in other directories
for header in env.h kiwi_slot.h lock_free_ring_buffer.h lock_mgr.h log.h memory_file.h mutex.h std_string.h std_status.h thread_pool.h; do
echo "=== Checking includes of $header ==="
rg -l "include.*${header}" . --type cpp --glob '!src/std/*'
done
Length of output: 5577
| PRIVATE kstd | ||
| PRIVATE ${FMT_LIBRARIES} | ||
| PRIVATE ${ZLIB_LIBRARIES} | ||
| PRIVATE ${LEVELDB_LIBRARIES} | ||
| PRIVATE ${GFLAGS_LIBRARIES} | ||
| PRIVATE ${ROCKSDB_LIBRARIES} | ||
| PRIVATE ${PROTOBUF_LIBRARY} | ||
| PRIVATE ${SPDLOG_LIBRARIES} |
There was a problem hiding this comment.
💡 Codebase verification
Dependencies mismatch: brpc is required but removed
The removal of brpc from dependencies is problematic as it's actively used in the raft implementation (src/raft/raft.h and src/raft/raft.cc). However, the removal of SSL-related dependencies (ssl, crypto) appears intentional as no secure communication features were found in the codebase.
src/raft/raft.h: Uses brpc for server implementationsrc/raft/raft.cc: Initializes brpc server and adds services
🔗 Analysis chain
Verify impact of removed SSL dependencies.
The removal of SSL-related dependencies (ssl, crypto) and network libraries (braft, brpc) might affect secure communication features. Please verify that these features are either not needed or handled differently.
Let's check for SSL usage in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for SSL-related code
rg -l "SSL|TLS|secure|encrypt" . --type cpp
# Check for removed library usage
rg -l "braft|brpc" . --type cpp
Length of output: 220
Script:
#!/bin/bash
# Check SSL context in std_hash
rg "SSL|TLS|secure|encrypt" ./src/std/std_hash.cc -C 3
# Check for security configuration
fd -e conf -e yaml -e yml -e json . --exec rg -l "SSL|TLS|secure|encrypt" {}
# Check comments in raft files for migration/refactoring
rg "(TODO|FIXME|NOTE|XXX|@deprecated).*braft|brpc" ./src/raft/
Length of output: 915
支持 ninja 构建方式, 精简cmake中依赖关系
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
Configuration Updates
Build System Improvements
Dependency Management
Security Enhancements
These changes focus on improving the build system, configuration flexibility, and connection management for the Kiwi project.