[ISSUE #5006]♻️Refactor CI workflow: unify system dependencies, streamline caching, and simplify build/test matrix for all platforms #5007
Conversation
…mline caching, and simplify build/test matrix for all platforms
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
WalkthroughRefactored CI workflow to unify OS dependency installation, caching, and workspace-wide Clippy/build/test/coverage steps; added RocksDB jemalloc toggle and propagated LLVM/clang envs; pinned Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant GH as GitHub Actions
participant Runner as Runner OS
participant Cache as Action Cache
participant Toolchain as LLVM/Clang + System PKGs
participant Cargo as Cargo (clippy/build/test/coverage)
participant Codecov as Codecov
Dev->>GH: push / pull_request
GH->>Runner: provision runner (ubuntu/macos/windows)
Runner->>Toolchain: install OS-specific deps (clang/llvm, cmake, protoc, snappy/lz4/zstd...)
Runner->>Runner: export envs (CC, CXX, LIBCLANG_PATH, ROCKSDB_DISABLE_JEMALLOC, CARGO_TARGET_DIR)
GH->>Cache: restore caches (key includes envs)
GH->>Cargo: run `cargo clippy --workspace --all-features`
GH->>Cargo: run `cargo build/test --workspace --all-features`
GH->>Cargo: run `cargo llvm-cov --workspace --all-features` -> generate lcov
Cargo->>Codecov: upload lcov.info
Codecov-->>GH: report result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![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 (3)
.github/workflows/ci.yaml (3)
45-46: Consider more deterministic LIBCLANG_PATH resolution.Using
head -n 1to select the LLVM directory may not reliably pick the correct version if multiple LLVM installations exist. While GitHub-hosted Ubuntu runners typically have one version, this could become fragile.🔎 More robust alternative
- LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | head -n 1) - echo "LIBCLANG_PATH=$LIBCLANG_DIR" >> $GITHUB_ENV + # Use the clang version being used to determine LIBCLANG_PATH + CLANG_VERSION=$(clang --version | grep -oP 'clang version \K[0-9]+' | head -n 1) + if [ -d "/usr/lib/llvm-${CLANG_VERSION}/lib" ]; then + echo "LIBCLANG_PATH=/usr/lib/llvm-${CLANG_VERSION}/lib" >> $GITHUB_ENV + else + # Fallback to first available + LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | sort -V | tail -n 1) + echo "LIBCLANG_PATH=$LIBCLANG_DIR" >> $GITHUB_ENV + fiThis matches the installed clang version or falls back to the highest version available.
108-109: Remove unnecessarybrew updateto speed up CI.GitHub-hosted macOS runners already have up-to-date Homebrew. Running
brew updateadds 30-60 seconds without benefit and occasionally causes transient failures.🔎 Proposed fix
- brew update brew install llvm cmake protobuf snappy lz4 zstd
158-170: Consider extracting Ubuntu dependency installation to a composite action.The Ubuntu dependency installation is duplicated across three jobs (lines 33-46, 88-103, 158-170). While this works correctly, extracting it to a reusable composite action would improve maintainability and reduce the risk of drift between jobs.
This is a larger refactor that could be deferred, but would involve:
- Creating
.github/actions/setup-ubuntu-deps/action.yml- Replacing the duplicated steps with
uses: ./.github/actions/setup-ubuntu-depsThe current approach is acceptable for now.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
🔇 Additional comments (6)
.github/workflows/ci.yaml (6)
17-19: LGTM: Global build environment variables.The addition of
CARGO_TARGET_DIRandROCKSDB_DISABLE_JEMALLOCas global environment variables ensures consistent build behavior across all jobs and platforms. These are correctly propagated to cache configurations.
53-63: LGTM: Cache configuration includes RocksDB environment variables.The cache configuration correctly includes compiler and RocksDB-related environment variables, ensuring cache invalidation when these settings change. This aligns with the PR objective to streamline caching.
68-69: LGTM: Consolidated Clippy check.The unified Clippy check across the entire workspace with all features simplifies the CI workflow while maintaining comprehensive coverage. This aligns with the PR objective to simplify the build/test matrix.
140-144: LGTM: Consolidated build and test steps.The unified build and test commands with
--all-featuressimplify the CI workflow while ensuring comprehensive coverage across all packages. The OS-specific cache keys properly handle platform-specific artifacts.
190-191: LGTM: Consolidated coverage generation.The unified coverage generation with
--all-featuresensures comprehensive code coverage metrics. The lcov output format is correctly configured for Codecov upload.
123-123: The current LIBCLANG_PATH configuration is correct. For LLVM installed via Chocolatey on Windows,libclang.dllis located in thebindirectory (C:\Program Files\LLVM\bin\libclang.dll), andLIBCLANG_PATHshould point to this directory. The code as written is the proper configuration and requires no changes.
…for improved reproducibility
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rocketmq-controller/Cargo.toml (2)
30-33: Consider pinningopenraftto a specific revision for consistency.While
raftis now pinned to a specific revision for reproducibility (line 30),openraftstill tracks therelease-0.10branch (line 33). Since the project is migrating toopenraftand the PR aims to improve CI reproducibility, consider also pinningopenraftto a specific commit or tag to ensure fully deterministic builds across all platforms.🔎 Example of how to pin openraft
-openraft = { git = "https://github.com/databendlabs/openraft.git", branch = "release-0.10", features = ["serde", "tokio-rt"] } +openraft = { git = "https://github.com/databendlabs/openraft.git", rev = "<specific-commit-hash>", features = ["serde", "tokio-rt"] }You can find the latest commit on the
release-0.10branch and use its hash for therevfield.
30-30: Consider pinningopenraftto a specific revision for consistency.Pinning the
raftdependency to revision1fd05e0is a good practice that improves build reproducibility. However,openrafton line 33 still usesbranch = "release-0.10", which can introduce non-deterministic builds. For consistency and to align with reproducibility goals, consider pinningopenraftto a specific revision as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
rocketmq-controller/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5007 +/- ##
==========================================
+ Coverage 33.20% 33.26% +0.05%
==========================================
Files 745 746 +1
Lines 109501 109682 +181
==========================================
+ Hits 36361 36485 +124
- Misses 73140 73197 +57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (2)
45-46: Consider using a more deterministic LLVM path resolution.The glob
/usr/lib/llvm-*/libwithhead -n 1selects alphabetically, which may not always be the latest version (e.g.,llvm-14would sort beforellvm-9). On ubuntu-latest this is typically fine since only one version is installed, but for robustness you could use numeric sorting.🔎 Optional improvement for deterministic version selection
- LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | head -n 1) + LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | sort -V | tail -n 1)
88-102: Consider extracting duplicated dependency installation into a composite action.The Ubuntu dependency installation block is duplicated across three jobs (check, build-test, coverage). While functional, this increases maintenance burden—any change requires updates in multiple places.
For better maintainability, consider creating a composite action in
.github/actions/install-deps/action.ymlthat handles platform-specific dependency installation. This would reduce duplication and ensure consistency.Also applies to: 104-114
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: auto-approve
🔇 Additional comments (6)
.github/workflows/ci.yaml (6)
1-20: LGTM!The workflow metadata and environment variables are well-structured. Setting
ROCKSDB_DISABLE_JEMALLOC: 1globally is a sensible choice for CI builds to avoid jemalloc-related build complications across platforms.
53-69: LGTM!The cache configuration properly includes environment variables that affect the build (CC, CXX, LIBCLANG_PATH, ROCKSDB_DISABLE_JEMALLOC), ensuring cache invalidation when these change. The Clippy step correctly targets the entire workspace with all features.
116-120: Windows is missing LLVM/clang setup and compression libraries.Unlike Ubuntu and macOS, the Windows step doesn't install LLVM/clang or set
CC,CXX,LIBCLANG_PATHenvironment variables. This may cause:
- Build failures if
libclangis required by any dependency (e.g.,bindgen)- Different compiler behavior (MSVC vs Clang) compared to other platforms
- Missing compression libraries that Ubuntu/macOS install (snappy, lz4, zstd)
If MSVC is intentional for Windows, this is fine—but the cache
env-varslist includesCC,CXX,LIBCLANG_PATHwhich will be empty on Windows, potentially causing cache key inconsistencies.Please verify:
- Does the RocksDB build (or any bindgen-based crate) require
LIBCLANG_PATHon Windows?- Is using MSVC instead of Clang intentional for Windows builds?
137-141: LGTM!The unified build and test steps using
--workspace --all-featuresalign well with the PR objective to simplify the build/test matrix.
172-182: Cache key differs from build-test Ubuntu job—potential missed cache reuse.The coverage job uses
shared-key: rocketmq-rust-coveragewhile the build-test job usesshared-key: rocketmq-rust-ubuntu-latest. Since both run on Ubuntu with identical dependencies, they could potentially share the cache.However, if coverage instrumentation produces different build artifacts, keeping separate caches is correct to avoid cache pollution.
Is the separate cache key intentional to isolate coverage-instrumented builds, or could this be changed to
rocketmq-rust-ubuntu-latestto improve cache hit rates?
187-196: LGTM!The coverage generation and upload configuration is well-structured:
- Uses
--workspace --all-featuresfor comprehensive coveragefail_ci_if_error: falseprevents flaky Codecov uploads from blocking CIverbose: trueaids debugging
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/ci.yaml (3)
18-19: Clarify the necessity of these environment variables.
CARGO_TARGET_DIR: targetsets the target directory to its default value, which appears redundant. Consider removing it unless there's a specific caching or build strategy reason.ROCKSDB_DISABLE_JEMALLOC: 1disables jemalloc in RocksDB builds. This can impact performance but may be necessary for compatibility. Please document the rationale (e.g., avoiding linking conflicts, simplifying dependencies, or addressing platform-specific issues).</review_comment_end>
45-46: LIBCLANG_PATH detection is fragile.The command
ls -d /usr/lib/llvm-*/lib | head -n 1picks the first LLVM directory alphabetically. If multiple LLVM versions are installed, this may not select the intended or latest version, potentially causing build inconsistencies.🔎 Proposed more robust detection
- LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | head -n 1) + LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | sort -V | tail -n 1) echo "LIBCLANG_PATH=$LIBCLANG_DIR" >> $GITHUB_ENVAlternatively, consider pinning a specific LLVM version:
- LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | head -n 1) + echo "LIBCLANG_PATH=/usr/lib/llvm-18/lib" >> $GITHUB_ENV
88-103: Duplicated LIBCLANG_PATH detection logic.This is the same fragile detection pattern flagged in the check job (lines 45-46). Consider extracting this to a reusable composite action or shared script to maintain consistency and ease updates.
</review_comment_end>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
🔇 Additional comments (4)
.github/workflows/ci.yaml (4)
182-183: Excellent consolidation of coverage generation.The move from per-package coverage to a single workspace-wide
--all-featuresinvocation simplifies the workflow and ensures comprehensive coverage reporting.</review_comment_end>
132-136: Good simplification of build and test steps.Consolidating to workspace-wide
--all-featuresbuilds and tests reduces complexity and ensures consistent testing across all packages. This aligns well with the PR objectives.</review_comment_end>
68-69: Clippy consolidation improves workflow efficiency.Running Clippy once across the entire workspace with
--all-featuresis more efficient than per-package checks and ensures consistent linting standards.</review_comment_end>
112-115: Windows should align with Ubuntu/macOS for compression library dependencies.Ubuntu (line 97) and macOS (line 109) install
snappy,lz4, andzstdlibraries, but Windows (line 115) does not. While modern-syscrates (zstd-sys,lz4-sys) typically include vendored source code and can compile without pre-installed system libraries on Windows, this inconsistency should be resolved for consistency and to avoid potential platform-specific build issues.Consider aligning Windows to install the same compression libraries via Chocolatey:
- name: Install system dependencies (Windows) if: matrix.os == 'windows-latest' run: | choco install cmake ninja protoc -y + choco install zstd -yAlternatively, if verification confirms Windows builds succeed without these libraries, document why they are not needed on Windows.
| - name: Install system dependencies (macOS) | ||
| if: matrix.os == 'macos-latest' | ||
| run: | | ||
| FILENAME="protoc-3.20.3-osx-x86_64.zip" | ||
| URL=https://github.com/protocolbuffers/protobuf/releases/download/v3.20.3/protoc-3.20.3-osx-x86_64.zip | ||
| echo "Detected arch: $ARCH, downloading $FILENAME" | ||
| curl -LO "$URL" | ||
| sudo unzip -o "$FILENAME" -d /usr/local | ||
| protoc --version | ||
| - if: matrix.os == 'windows-latest' | ||
| name: Install dependencies (windows-latest) | ||
| brew update | ||
| brew install cmake protobuf snappy lz4 zstd |
There was a problem hiding this comment.
Missing compiler and LIBCLANG_PATH configuration on macOS.
Unlike Ubuntu (lines 99-102), the macOS step does not set CC, CXX, or LIBCLANG_PATH environment variables. This creates inconsistency across platforms and may lead to:
- Different compilers being used (system default vs. explicitly specified)
- Potential build failures if bindgen or other tools expect
LIBCLANG_PATH
🔎 Suggested fix to align macOS configuration
- name: Install system dependencies (macOS)
if: matrix.os == 'macos-latest'
run: |
brew update
brew install cmake protobuf snappy lz4 zstd
+
+ echo "CC=clang" >> $GITHUB_ENV
+ echo "CXX=clang++" >> $GITHUB_ENV
+ # macOS ships with LLVM/clang in a standard location
+ echo "LIBCLANG_PATH=/usr/local/opt/llvm/lib" >> $GITHUB_ENVNote: Verify the actual LLVM path on macos-latest runners if using Homebrew LLVM.
| - name: Cache Rust + RocksDB | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| shared-key: rust-ci | ||
|
|
||
| - name: Build (other packages) | ||
| run: cargo build --verbose --workspace --exclude rocketmq-controller --all-features | ||
|
|
||
| - name: Build (controller without rocksdb) | ||
| run: cargo build --verbose -p rocketmq-controller --no-default-features --features storage-file,metrics,debug | ||
|
|
||
| - name: Run tests (other packages) | ||
| run: cargo test --verbose --workspace --exclude rocketmq-controller --all-features | ||
|
|
||
| - name: Run tests (controller without rocksdb) | ||
| run: cargo test --verbose -p rocketmq-controller --no-default-features --features storage-file,metrics,debug | ||
|
|
||
| # Code coverage (only on Ubuntu) | ||
| shared-key: rocketmq-rust-${{ matrix.os }} | ||
| workspaces: | | ||
| . | ||
| env-vars: | | ||
| CC | ||
| CXX | ||
| LIBCLANG_PATH | ||
| ROCKSDB_DISABLE_JEMALLOC |
There was a problem hiding this comment.
Cache env-vars include variables not set on all platforms.
The cache configuration lists CC, CXX, and LIBCLANG_PATH in env-vars (lines 127-129), but:
- These variables are only set on Ubuntu (lines 99-102)
- macOS does not set them (lines 105-109)
- Windows does not set them (lines 112-115)
This can lead to suboptimal cache behavior, as the cache key will differ when these variables are unset vs. set, potentially causing cache misses or inconsistencies across platforms.
Recommended fix: Either set these variables consistently across all platforms, or make the cache env-vars platform-specific. Alternatively, if these variables are not relevant for non-Ubuntu platforms, document why they're included.
</review_comment_end>
🤖 Prompt for AI Agents
.github/workflows/ci.yaml lines 120-130: the cache step currently lists CC, CXX,
and LIBCLANG_PATH in env-vars even though those are only defined on Ubuntu,
causing inconsistent cache keys across macOS/Windows; fix by making the cached
env-vars platform-specific (e.g., move CC/CXX/LIBCLANG_PATH into the Ubuntu
job's cache step or use a conditional matrix key so only Ubuntu includes them),
or alternatively ensure the variables are defined (set sensible defaults or
empty values) on macOS/Windows before the cache step so the key is stable across
runners.
| - name: Install system dependencies (Ubuntu) | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y \ | ||
| clang llvm libclang-dev \ | ||
| cmake make ninja-build pkg-config \ | ||
| protobuf-compiler \ | ||
| libsnappy-dev liblz4-dev libzstd-dev zlib1g-dev | ||
| echo "CC=clang" >> $GITHUB_ENV | ||
| echo "CXX=clang++" >> $GITHUB_ENV | ||
| LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | head -n 1) | ||
| echo "LIBCLANG_PATH=$LIBCLANG_DIR" >> $GITHUB_ENV |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Third instance of duplicated dependency installation logic.
This Ubuntu dependency installation block is identical to the ones in the check job (lines 33-46) and build-test job (lines 88-103). Consider extracting this into a reusable composite action to reduce duplication and ensure consistency.
Benefits:
- Single source of truth for dependency versions and configuration
- Easier maintenance when updating LLVM versions or adding new dependencies
- Consistent LIBCLANG_PATH detection logic across all jobs
Example composite action structure:
Create .github/actions/setup-ubuntu-deps/action.yml:
name: 'Setup Ubuntu Dependencies'
description: 'Install system dependencies for RocketMQ Rust on Ubuntu'
runs:
using: 'composite'
steps:
- name: Install system dependencies
shell: bash
run: |
sudo apt-get update
sudo apt-get install -y \
clang llvm libclang-dev \
cmake make ninja-build pkg-config \
protobuf-compiler \
libsnappy-dev liblz4-dev libzstd-dev zlib1g-dev
echo "CC=clang" >> $GITHUB_ENV
echo "CXX=clang++" >> $GITHUB_ENV
LIBCLANG_DIR=$(ls -d /usr/lib/llvm-*/lib | sort -V | tail -n 1)
echo "LIBCLANG_PATH=$LIBCLANG_DIR" >> $GITHUB_ENVThen replace each Ubuntu installation block with:
- name: Setup Ubuntu dependencies
uses: ./.github/actions/setup-ubuntu-deps</review_comment_end>
🤖 Prompt for AI Agents
.github/workflows/ci.yaml around lines 150 to 162: this Ubuntu dependency
installation block is duplicated across three jobs; extract it into a reusable
composite action (e.g., .github/actions/setup-ubuntu-deps/action.yml) that runs
the apt-get update/install commands and sets CC/CXX and LIBCLANG_PATH, then
replace each inline block in the check, build-test, and this job with a single
step that uses the composite action (uses: ./.github/actions/setup-ubuntu-deps)
to centralize maintenance and ensure consistent LIBCLANG_PATH detection.
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #5006
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.