fix: cmake issues and support ninja #230
Conversation
WalkthroughThis pull request updates multiple CMake configuration files to add or modify the Changes
Sequence Diagram(s)sequenceDiagram
participant CMake as CMake Config
participant EP as ExternalProject_Add
participant Build as Build Process
CMake->>EP: Invoke ExternalProject_Add with parameters
EP-->>CMake: Process GIT_SHALLOW & UPDATE_COMMAND settings
CMake->>Build: Continue configuration and build steps
sequenceDiagram
participant CI as GitHub Actions Runner
participant Script as build.sh
participant CMake as CMake
CI->>Script: Execute build.sh with --ninja --verbose
Script->>CMake: Configure project with "-G Ninja" and flags
CMake->>Script: Process external dependencies and build targets
Script-->>CI: Return build outcome
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (15)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (
|
artikell
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
cmake/gtest.cmake (1)
18-18: Remove redundant GIT_SHALLOW parameter.This
GIT_SHALLOW truedeclaration is redundant as it's already specified at line 16. Duplicate parameters in CMake'sExternalProject_Addcan cause confusion and potential issues in future maintenance.- GIT_SHALLOW truecmake/braft.cmake (1)
27-27: Remove redundant GIT_SHALLOW parameter.This
GIT_SHALLOW truedeclaration is redundant as it's already specified at line 25. Duplicate parameters in CMake'sExternalProject_Addcan cause confusion and potential issues in future maintenance.- GIT_SHALLOW truecmake/zlib.cmake (2)
17-17: Remove redundant GIT_SHALLOW parameter.This
GIT_SHALLOW truedeclaration is redundant as it's already specified at line 15. Duplicate parameters in CMake'sExternalProject_Addcan cause confusion.- GIT_SHALLOW true
18-18: Note possible redundancy with UPDATE_COMMAND.While adding
UPDATE_DISCONNECTED 1is generally beneficial, note that line 30 already setsUPDATE_COMMAND ""which effectively disables updates completely. Consider whether both parameters are necessary in this case.etc/script/build.sh (1)
76-76: Consider making Ninja generator optionalThe script now enforces Ninja as the CMake generator, but it doesn't check if Ninja is installed on the system. This could lead to build failures if Ninja isn't available.
Consider either:
- Adding a check for Ninja availability
- Making the generator configurable via a command-line option
- ${CMAKE_FLAGS} -G Ninja -S . -B ${PREFIX} + ${CMAKE_FLAGS} ${CMAKE_GENERATOR:+-G $CMAKE_GENERATOR} -S . -B ${PREFIX}And then add a new option in the script:
+ --ninja) + CMAKE_GENERATOR="Ninja" + ;;cmake/brpc.cmake (1)
26-27: Remove redundant GIT_SHALLOW parameterThe
GIT_SHALLOW trueparameter is only applicable when usingGIT_REPOSITORYandGIT_TAGfor Git-based downloads. Since this project uses URL-based download (lines 20-24), theGIT_SHALLOWparameter has no effect and should be removed to avoid confusion.- GIT_SHALLOW true UPDATE_DISCONNECTED 1The
UPDATE_DISCONNECTED 1parameter is appropriate as it allows builds to proceed when offline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmake/braft.cmake(1 hunks)cmake/brpc.cmake(1 hunks)cmake/fmt.cmake(1 hunks)cmake/gflags.cmake(1 hunks)cmake/gtest.cmake(2 hunks)cmake/leveldb.cmake(1 hunks)cmake/lz4.cmake(1 hunks)cmake/protobuf.cmake(2 hunks)cmake/rocksdb.cmake(1 hunks)cmake/snappy.cmake(1 hunks)cmake/spdlog.cmake(1 hunks)cmake/zlib.cmake(1 hunks)cmake/zstd.cmake(1 hunks)etc/script/build.sh(1 hunks)src/raft/CMakeLists.txt(2 hunks)
🔇 Additional comments (6)
cmake/gtest.cmake (2)
19-19: LGTM! Adding UPDATE_DISCONNECTED is beneficial.The
UPDATE_DISCONNECTED 1parameter allows builds to proceed without network connectivity after the initial download, which is great for CI environments or offline development.
30-30: LGTM! Proper declaration of all build byproducts.Including both
${GTEST_LIBRARIES}and${GTEST_MAIN_LIBRARIES}inBUILD_BYPRODUCTScorrectly informs the build system about all artifacts generated by this target. This is particularly important for Ninja and other generators that need to know all outputs in advance.cmake/protobuf.cmake (2)
37-37: LGTM! Adding UPDATE_DISCONNECTED is beneficial.The
UPDATE_DISCONNECTED 1parameter allows builds to proceed without network connectivity after the initial download.
52-52: LGTM! Improved variable consistency in BUILD_BYPRODUCTS.Using
${PROTOBUF_LIBRARY}inBUILD_BYPRODUCTSis better than using the direct path as it maintains consistency with the variable defined at line 18.cmake/braft.cmake (1)
28-28: LGTM! Adding UPDATE_DISCONNECTED is beneficial.The
UPDATE_DISCONNECTED 1parameter allows builds to proceed without network connectivity after the initial download, which is useful for CI environments or offline development.src/raft/CMakeLists.txt (1)
53-53: LGTM: Dependencies properly addedAdding
braftandbrpcas dependencies for theraftlibrary is appropriate if the RAFT functionality depends on these components.
cmake/gflags.cmake
Outdated
| GIT_SHALLOW true | ||
| UPDATE_DISCONNECTED 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Misplaced GIT_SHALLOW parameter for URL-based download
The GIT_SHALLOW true parameter is only applicable for git-based downloads, but this project is using URL-based download (not git clone). This parameter will have no effect here and should be removed.
The UPDATE_DISCONNECTED 1 parameter is beneficial for offline builds, but note that it may conflict with the existing UPDATE_COMMAND "" at line 44, which already effectively disables the update step.
DOWNLOAD_DIR "${CMAKE_CURRENT_SOURCE_DIR}/download"
DOWNLOAD_NAME "gflags-2.2.2.zip"
SOURCE_DIR ${GFLAGS_SOURCES_DIR}
- GIT_SHALLOW true
UPDATE_DISCONNECTED 1📝 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.
| GIT_SHALLOW true | |
| UPDATE_DISCONNECTED 1 | |
| DOWNLOAD_DIR "${CMAKE_CURRENT_SOURCE_DIR}/download" | |
| DOWNLOAD_NAME "gflags-2.2.2.zip" | |
| SOURCE_DIR ${GFLAGS_SOURCES_DIR} | |
| UPDATE_DISCONNECTED 1 |
cmake/spdlog.cmake
Outdated
| GIT_SHALLOW true | ||
| UPDATE_DISCONNECTED 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Misplaced GIT_SHALLOW parameter for URL-based download
The GIT_SHALLOW true parameter is only applicable for git-based downloads, but this project is using URL-based download as shown by the URL and URL_HASH parameters (not git clone). This parameter will have no effect here and should be removed.
The UPDATE_DISCONNECTED 1 parameter is beneficial for offline builds where network connectivity isn't available during build time.
SOURCE_DIR ${SPDLOG_SOURCES_DIR}
DOWNLOAD_NO_PROGRESS 1
- GIT_SHALLOW true
UPDATE_DISCONNECTED 1📝 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.
| GIT_SHALLOW true | |
| UPDATE_DISCONNECTED 1 | |
| SOURCE_DIR ${SPDLOG_SOURCES_DIR} | |
| DOWNLOAD_NO_PROGRESS 1 | |
| UPDATE_DISCONNECTED 1 |
cmake/zstd.cmake
Outdated
| GIT_SHALLOW true | ||
| UPDATE_DISCONNECTED 1 |
There was a problem hiding this comment.
GIT_SHALLOW parameter is not applicable with URL downloads
The GIT_SHALLOW true parameter is only effective when using GIT_REPOSITORY to clone a Git repository. Since this ExternalProject is configured with URL to download a tarball (not a Git repository), the GIT_SHALLOW parameter will have no effect.
Additionally, the UPDATE_DISCONNECTED 1 parameter is redundant since the configuration already includes UPDATE_COMMAND "" on line 35, which prevents any updates from occurring.
- GIT_SHALLOW true
- UPDATE_DISCONNECTED 1
+ UPDATE_DISCONNECTED 1Committable suggestion skipped: line range outside the PR's diff.
cmake/lz4.cmake
Outdated
| GIT_SHALLOW true | ||
| UPDATE_DISCONNECTED 1 |
There was a problem hiding this comment.
GIT_SHALLOW parameter is not applicable with URL downloads
The GIT_SHALLOW true parameter is only effective when using GIT_REPOSITORY to clone a Git repository. Since this ExternalProject is configured with URL to download a tarball (not a Git repository), the GIT_SHALLOW parameter will have no effect.
Additionally, the UPDATE_DISCONNECTED 1 parameter is redundant since the configuration already includes UPDATE_COMMAND "" on line 35, which prevents any updates from occurring.
- GIT_SHALLOW true
- UPDATE_DISCONNECTED 1
+ UPDATE_DISCONNECTED 1Committable suggestion skipped: line range outside the PR's diff.
cmake/protobuf.cmake
Outdated
| DOWNLOAD_NAME "protobuf-3.18.0.tar.gz" | ||
| SOURCE_DIR ${PROTOBUF_SOURCES_DIR} | ||
| DOWNLOAD_NO_PROGRESS 1 | ||
| GIT_SHALLOW true |
There was a problem hiding this comment.
Remove inappropriate GIT_SHALLOW parameter.
The GIT_SHALLOW true parameter has no effect here and should be removed as this project is downloaded via URL (lines 30-33) rather than Git. This parameter only applies to repositories fetched with GIT_REPOSITORY.
- GIT_SHALLOW true📝 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.
| GIT_SHALLOW true |
src/raft/CMakeLists.txt
Outdated
| PRIVATE ${GFLAGS_LIBRARY} | ||
| PRIVATE ${ROCKSDB_LIBRARIES} | ||
| PRIVATE z | ||
| PRIVATE ${BRAFT_INCLUDE_DIR} |
There was a problem hiding this comment.
Fix incorrect linking against include directory
${BRAFT_INCLUDE_DIR} appears to be an include directory path (based on its name), not a library. It should not be included in TARGET_LINK_LIBRARIES. Include directories should be specified with TARGET_INCLUDE_DIRECTORIES instead.
- PRIVATE ${BRAFT_INCLUDE_DIR}If you meant to link against a specific BRAFT library, please use the correct library variable instead.
📝 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.
| PRIVATE ${BRAFT_INCLUDE_DIR} |
artikell
force-pushed
the
cmake_support_ninja
branch
2 times, most recently
from
9ab5278 to
324ee0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/kiwidb.yml (1)
109-112: Ninja installation for Ubuntu environment is implemented correctly, with a minor enhancement suggested.
The new "Install Ninja" step usingsudo apt install -y ninja-buildcorrectly ensures that Ninja is available in the Ubuntu CI environment. Consider adding anapt updatecommand (e.g.,sudo apt update) before the installation command to ensure the package index is current, which can help avoid potential package installation issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/kiwidb.yml(2 hunks)cmake/braft.cmake(1 hunks)cmake/brpc.cmake(1 hunks)cmake/fmt.cmake(1 hunks)cmake/gflags.cmake(1 hunks)cmake/gtest.cmake(2 hunks)cmake/leveldb.cmake(1 hunks)cmake/lz4.cmake(1 hunks)cmake/protobuf.cmake(2 hunks)cmake/rocksdb.cmake(1 hunks)cmake/snappy.cmake(1 hunks)cmake/spdlog.cmake(1 hunks)cmake/zlib.cmake(1 hunks)cmake/zstd.cmake(1 hunks)etc/script/build.sh(2 hunks)src/raft/CMakeLists.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- cmake/zstd.cmake
- cmake/braft.cmake
- cmake/spdlog.cmake
- cmake/gflags.cmake
- cmake/snappy.cmake
- etc/script/build.sh
- cmake/leveldb.cmake
- cmake/lz4.cmake
- cmake/zlib.cmake
- cmake/protobuf.cmake
- cmake/brpc.cmake
- cmake/gtest.cmake
- cmake/rocksdb.cmake
- src/raft/CMakeLists.txt
- cmake/fmt.cmake
🔇 Additional comments (1)
.github/workflows/kiwidb.yml (1)
52-52: Ninja installation for MacOS environment is added correctly.
The addition ofbrew install ninjain the "build_on_macos" job ensures that the Ninja build system is available on macOS, which aligns with the overall PR objective to support Ninja.
artikell
force-pushed
the
cmake_support_ninja
branch
4 times, most recently
from
1e44531 to
7e0d323
Compare
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 (16)
.github/workflows/kiwidb.yml(3 hunks)cmake/braft.cmake(1 hunks)cmake/brpc.cmake(1 hunks)cmake/fmt.cmake(1 hunks)cmake/gflags.cmake(1 hunks)cmake/gtest.cmake(2 hunks)cmake/leveldb.cmake(1 hunks)cmake/lz4.cmake(1 hunks)cmake/protobuf.cmake(2 hunks)cmake/rocksdb.cmake(1 hunks)cmake/snappy.cmake(1 hunks)cmake/spdlog.cmake(1 hunks)cmake/zlib.cmake(1 hunks)cmake/zstd.cmake(1 hunks)etc/script/build.sh(2 hunks)src/raft/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- cmake/zstd.cmake
- cmake/fmt.cmake
- cmake/gflags.cmake
- cmake/spdlog.cmake
- cmake/braft.cmake
- cmake/snappy.cmake
- cmake/zlib.cmake
- cmake/leveldb.cmake
- cmake/protobuf.cmake
- cmake/lz4.cmake
- cmake/brpc.cmake
- src/raft/CMakeLists.txt
- cmake/rocksdb.cmake
- etc/script/build.sh
- .github/workflows/kiwidb.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (1)
cmake/gtest.cmake (1)
30-30: LGTM! Improved Ninja compatibility with complete BUILD_BYPRODUCTS.The addition of
${GTEST_MAIN_LIBRARIES}to theBUILD_BYPRODUCTSparameter is essential for proper dependency tracking, especially when using generators like Ninja. This ensures all build artifacts are correctly tracked during parallel builds, which directly supports the PR objective of adding Ninja compatibility.
artikell
force-pushed
the
cmake_support_ninja
branch
from
7e0d323 to
b5b40f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmake/findTools.cmake (1)
50-56: Good addition to improve build performance with CCache.Adding CCache support is a great way to speed up builds, especially for repeated builds in development and CI environments. This implementation correctly sets up both compilation and linking to use CCache when available.
For a minor improvement, consider adding a status message to inform users when CCache is found and activated, similar to how the other tools are reported:
if(CCACHE_FOUND) set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE ccache) set_property(GLOBAL PROPERTY RULE_LAUNCH_LINK ccache) + MESSAGE(STATUS "found ccache at ${CCACHE_FOUND}, build acceleration enabled") endif(CCACHE_FOUND)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmake/braft.cmake(1 hunks)cmake/brpc.cmake(1 hunks)cmake/findTools.cmake(1 hunks)cmake/fmt.cmake(1 hunks)cmake/gflags.cmake(1 hunks)cmake/gtest.cmake(2 hunks)cmake/leveldb.cmake(1 hunks)cmake/lz4.cmake(1 hunks)cmake/protobuf.cmake(2 hunks)cmake/rocksdb.cmake(1 hunks)cmake/snappy.cmake(1 hunks)cmake/spdlog.cmake(1 hunks)cmake/zlib.cmake(1 hunks)cmake/zstd.cmake(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- cmake/lz4.cmake
- cmake/leveldb.cmake
- cmake/spdlog.cmake
- cmake/zstd.cmake
- cmake/zlib.cmake
- cmake/gflags.cmake
- cmake/fmt.cmake
- cmake/snappy.cmake
- cmake/braft.cmake
- cmake/rocksdb.cmake
- cmake/brpc.cmake
- cmake/protobuf.cmake
- cmake/gtest.cmake
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
.github/workflows/kiwidb.yml
Outdated
| export LIBRARY_PATH=$(xcrun --show-sdk-path)/usr/lib | ||
| export DYLD_LIBRARY_PATH=$(xcrun --show-sdk-path)/usr/lib | ||
| bash ./etc/script/build.sh --verbose | ||
| bash ./etc/script/build.sh --clear |
There was a problem hiding this comment.
bash ./etc/script/build.sh --clear 这里不应该 clear, 否则缓存的文件会失效吧
There was a problem hiding this comment.
俩个阶段本来就不一样，下面是ninja编译的，就索性直接清理掉了。
There was a problem hiding this comment.
我的意思是, 如果clear, 这里使用 GitHub 的缓存会失效吧, 上面恢复的缓存文件, 这里被 clear了
artikell
force-pushed
the
cmake_support_ninja
branch
from
7f8a981 to
519b2a8
Compare
3 participants
Summary by CodeRabbit