fix: add centos CI in github action #2942
Conversation
WalkthroughThe pull request introduces significant changes to the CI workflow defined in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
github-actions
bot
added
☢️ Bug
![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
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
142-212: Document the transition from Rocky to CentOS.The Rocky Linux job is commented out rather than removed. While this preserves history, it clutters the workflow file.
Consider:
- Removing the commented code as it's preserved in Git history
- Adding a comment explaining why CentOS was chosen over Rocky
- Documenting the transition in the repository's documentation
Line range hint
87-212: Overall architectural considerations for CI changes.The transition to CentOS raises several architectural points:
Testing Matrix:
- Consider using matrix strategy to test against multiple OS versions
- This would reduce duplication between Ubuntu, CentOS, and macOS jobs
Dependency Management:
- The custom Docker image obscures dependency management
- Consider making dependencies explicit in the workflow
Version Consistency:
- Ensure consistent versions of actions across jobs
- Document version requirements for tools and dependencies
Consider implementing a matrix strategy:
strategy: matrix: os: [ubuntu-latest, centos7] include: - os: ubuntu-latest container: ubuntu:latest - os: centos7 container: cheniujh/pika-ci-centos7:v1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🔇 Additional comments (3)
.github/workflows/pika.yml (3)
94-98: Explain the rationale for downgrading checkout action.
The PR downgrades actions/checkout from v4 to v3 without explanation. This might impact security and performance as v4 includes important updates.
Let's check if there are any compatibility issues with v4:
#!/bin/bash
# Description: Search for checkout action version usage across workflows
# Expected: Find consistent version usage or documented reasons for differences
rg "uses: actions/checkout@v" .github/workflows/99-111: Review toolchain version and caching strategy.
Several observations about the build configuration:
- Switching from gcc-toolset-13 to devtoolset-10 is a significant downgrade
- Cache keys have been updated appropriately for CentOS
- The CMake configuration remains consistent
Let's verify the toolchain requirements:
#!/bin/bash
# Description: Check minimum compiler requirements in CMake files
# Expected: Find compiler version requirements or dependencies
rg -A 3 "CMAKE_CXX_COMPILER|CMAKE_C_COMPILER|gcc|g\+\+" CMakeLists.txtConsider:
- Documenting the reason for using an older toolchain
- Adding compiler version checks in CMake
Line range hint 113-139: Verify test coverage and execution.
Changes in the test execution:
- Removed the cleanup step present in Rocky job
- Changed
./pikatests.sh all cleanto./pikatests.sh all
Let's check the impact of these changes:
Consider:
- Maintaining consistent cleanup steps across all jobs
- Documenting why cleanup is handled differently in CentOS
✅ Verification successful
Cleanup step removal is safe and intentional
After analyzing the test scripts, I can confirm:
-
The cleanup function in
pikatests.shis called in three scenarios:- At the start of every test run
- When tests fail
- Optionally at the end if
cleanparameter is provided
-
The removal of the cleanup step is safe because:
- Initial cleanup is always performed before tests start
- Failed tests trigger cleanup automatically
- The optional final cleanup was just an additional safety measure
-
The integration test scripts (
start_master_and_slave.sh,start_codis.sh, andintegrate_test.sh) manage their own test environments independently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze test script behavior
# Expected: Understand the difference between 'all' and 'all clean' modes
rg -A 5 "function.*clean" pikatests.sh
Length of output: 245
Script:
#!/bin/bash
# Let's first check if the file exists and its content
fd "pikatests.sh" --type f --exec cat {}
# Also check the integration test scripts to understand the cleanup context
fd "start_master_and_slave.sh|start_codis.sh|integrate_test.sh" --type f --exec cat {}
Length of output: 10980
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/pika.yml
98-98: input "node-version" is not defined in action "actions/checkout@v3". available inputs are "clean", "fetch-depth", "fetch-tags", "github-server-url", "lfs", "path", "persist-credentials", "ref", "repository", "set-safe-directory", "sparse-checkout", "sparse-checkout-cone-mode", "ssh-key", "ssh-known-hosts", "ssh-strict", "submodules", "token"
(action)
🔇 Additional comments (4)
.github/workflows/pika.yml (4)
104-112: LGTM: Cache configuration properly updated
The cache keys have been correctly updated to reflect the CentOS context while maintaining the same cache structure.
143-213: Document CI environment changes
The complete removal of the Rocky Linux CI environment in favor of CentOS should be documented. Please:
- Update relevant documentation to reflect this change
- Document the rationale for switching from Rocky Linux 9 to CentOS 7
Let's check for CI documentation that needs updating:
#!/bin/bash
# Search for CI documentation
fd -g "*CI*.md" docs/
fd -g "*BUILD*.md" docs/
fd -g "*CONTRIBUTING*.md" .124-124: Verify unit test command modification
The unit test command has been simplified from ./pikatests.sh all clean to ./pikatests.sh all. Please confirm if removing the clean parameter is intentional and won't affect test isolation.
Let's check the test script implementation:
#!/bin/bash
# Check pikatests.sh implementation to understand the 'clean' parameter
rg -A 5 "clean" pikatests.sh102-103: Document toolchain downgrade rationale
The change from gcc-toolset-13 to devtoolset-10 represents a significant toolchain downgrade. Please document the reasons for this change, including any compatibility requirements or constraints that necessitated using an older toolset.
Let's check if there are any known compatibility issues with gcc-toolset-13:
cheniujh
removed
✏️ Feature
github-actions
bot
added
☢️ Bug
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
107-108: Consider creating a composite action for toolset setupThe
source /opt/rh/devtoolset-10/enablecommand is used multiple times. Consider creating a composite action to encapsulate the toolset setup for better maintainability.Example composite action:
# .github/actions/setup-centos-toolset/action.yml name: 'Setup CentOS Toolset' description: 'Sets up the CentOS development toolset' runs: using: 'composite' steps: - shell: bash run: source /opt/rh/devtoolset-10/enable
148-218: Remove commented out Rocky Linux jobInstead of keeping the commented-out job configuration in the workflow file, consider:
- Removing it completely since it's tracked in Git history
- Or, if needed for reference, moving it to documentation
This will improve the workflow file's readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 97-97: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🔇 Additional comments (4)
.github/workflows/pika.yml (4)
88-91: Skip comment as security concerns are already addressed in past reviews.
99-102: Skip comment as devtoolset version issue is already addressed.
103-111: LGTM: Cache configuration properly updated for CentOS
The cache keys have been correctly updated to reflect the CentOS environment while maintaining the same cache paths.
Line range hint 113-139: Skip comment as test command issue is already addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
101-102: Ensure consistent CMake flags across jobsThe CMake configuration in the CentOS job is missing compiler launcher flags (
CMAKE_C_COMPILER_LAUNCHERandCMAKE_CXX_COMPILER_LAUNCHER) that are present in Ubuntu and macOS jobs. This inconsistency could lead to slower builds on CentOS.Apply this diff to align with other jobs:
- cmake -B build -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DUSE_PIKA_TOOLS=ON -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address + cmake -B build -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DUSE_PIKA_TOOLS=ON -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache
142-212: Document the rationale for migrating from Rocky to CentOSWhile the commented Rocky Linux configuration should be removed as suggested in previous reviews, it would be valuable to document why CentOS was chosen over Rocky Linux. Consider adding a comment in the workflow file or updating the repository's CI documentation to explain this architectural decision.
Add a comment before the CentOS job explaining the rationale:
+ # CentOS 7 is used instead of Rocky Linux 9 due to better compatibility with legacy dependencies + # and reduced pull failures in CI environment. See PR #2942 for detailed discussion. build_on_centos:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
101-102: Add compiler launcher flags for better build performanceThe CMake configuration should include compiler launcher flags to optimize build performance with ccache.
- cmake -B build -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DUSE_PIKA_TOOLS=ON -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address + cmake -B build -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DUSE_PIKA_TOOLS=ON -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
121-121: Remove trailing whitespace- rm -rf ./buildtrees + rm -rf ./buildtrees🧰 Tools
🪛 yamllint
[error] 121-121: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 121-121: trailing spaces
(trailing-spaces)
cheniujh
removed
✏️ Feature
github-actions
bot
added
☢️ Bug
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
111-112: Remove trailing whitespaceRemove the trailing whitespace on line 112:
- rm -rf ./buildtrees + rm -rf ./buildtrees🧰 Tools
🪛 yamllint
[error] 112-112: trailing spaces
(trailing-spaces)
Line range hint
88-138: Consider using job matrix for Linux buildsThe Ubuntu and CentOS jobs share similar steps and could be consolidated using a matrix strategy:
jobs: build_on_linux: strategy: matrix: os: [ubuntu, centos] include: - os: ubuntu container: ubuntu:latest - os: centos container: pikadb/pika-ci-centos7:v5 runs-on: ubuntu-latest container: ${{ matrix.container }}This would:
- Reduce workflow maintenance by eliminating duplicated job definitions
- Make it easier to add more Linux distributions in the future
- Ensure consistent steps across Linux builds
Would you like me to help create a proof of concept for this refactor?
🧰 Tools
🪛 yamllint
[error] 112-112: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 112-112: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/pika.yml (1)
94-98: The previous review comment about upgrading the checkout action from v1 to v4 is still valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
101-102: Add compiler launcher flags for better build performanceThe CMake configuration is missing the compiler launcher flags that are present in Ubuntu and macOS jobs. Add these flags to improve build performance:
- cmake -B build -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DUSE_PIKA_TOOLS=ON -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address + cmake -B build -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DUSE_PIKA_TOOLS=ON -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache
112-112: Remove trailing spacesFix the formatting by removing trailing spaces from this line.
- rm -rf ./buildtrees + rm -rf ./buildtrees🧰 Tools
🪛 yamllint
[error] 112-112: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 112-112: trailing spaces
(trailing-spaces)
* add centos CI instead of rockylinux
add back centos CI by using customized docker image to avoid pull dependency fail
Summary by CodeRabbit
build_on_rockyjob with a newbuild_on_centosjob.These changes enhance the build process, ensuring a smoother integration and testing experience for users.