fix: set MACOS_CMAKE_ARGS for diff clang versions #49
Conversation
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
.github/workflows/build.yml:96
- The conditional comparison assumes that matrix.clang-version is always numeric. Please ensure that this value is reliably numeric to prevent unexpected failures in the bash condition.
if [ "${{ matrix.clang-version }}" -gt 18 ]; then 
WalkthroughThe pull request introduces modifications to the GitHub Actions build workflow by adding commands to the "Update homebrew" step for macOS builds, specifically appending options to the Changes
Sequence Diagram(s)sequenceDiagram participant Runner as GitHub Actions Runner participant Workflow as Build Workflow participant Shell as Shell Command Executor Runner->>Workflow: Start build process Workflow->>Workflow: Read configuration and environment variables Workflow->>Shell: Execute "Update homebrew" step Shell-->>Workflow: Append options to MACOS_CMAKE_ARGS Workflow->>Shell: Output updated MACOS_CMAKE_ARGS Shell-->>Workflow: Confirm output to environment file Workflow->>Runner: Continue build process Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks).gitignore(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (9, macosx)
- GitHub Check: build (12.0.1, macosx)
- GitHub Check: build (10, windows)
- GitHub Check: build (12.0.1, windows)
- GitHub Check: build (12, windows)
- GitHub Check: build (14, macosx)
- GitHub Check: build (13, macosx)
- GitHub Check: build (13, windows)
- GitHub Check: build (14, windows)
- GitHub Check: build (15, windows)
- GitHub Check: build (18, windows)
- GitHub Check: build (16, windows)
- GitHub Check: build (19, windows)
- GitHub Check: build (20, windows)
🔇 Additional comments (3)
.gitignore (1)
1-2: Ensure Virtual Environments Are IgnoredThe new
.gitignorefile correctly specifies bothvenvand.venv, which will help keep local virtual environment directories out of version control..github/workflows/build.yml (2)
83-85: Improved Readability for COMMON_CMAKE_ARGSThe use of YAML’s multiline (
>) syntax forCOMMON_CMAKE_ARGSimproves clarity and maintainability by clearly laying out the CMake arguments.
86-89: Clear Definition for LINUX_CMAKE_ARGSDefining
LINUX_CMAKE_ARGSwith the updated compiler versions (g++-10 and gcc-10) is clear and aligns with the updated build configuration. The formatted multiline style enhances readability.
.github/workflows/build.yml Outdated
| - name: Set MACOS_CMAKE_ARGS | ||
| if: runner.os == 'macOS' | ||
| run: | | ||
| if [ "${{ matrix.clang-version }}" -gt 18 ]; then | ||
| echo "MACOS_CMAKE_ARGS=-DCMAKE_BUILD_TYPE=MinSizeRel -DCMAKE_CXX_COMPILER=g++-11 -DCMAKE_C_COMPILER=gcc-11 -DZSTD_STATIC_LINKING_ONLY=1 -DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_BUILD_LLVM_DYLIB=ON -DCLANG_LINK_CLANG_DYLIB=ON" >> $GITHUB_ENV | ||
| else | ||
| echo "MACOS_CMAKE_ARGS=-DCMAKE_BUILD_TYPE=MinSizeRel -DCMAKE_CXX_COMPILER=g++-11 -DCMAKE_C_COMPILER=gcc-11 -DZSTD_STATIC_LINKING_ONLY=1 -DLLVM_ENABLE_ASSERTIONS=OFF" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify Numeric Comparison in MACOS_CMAKE_ARGS Step
The new “Set MACOS_CMAKE_ARGS” step is a great addition for customizing macOS builds based on the Clang version. However, the condition:
if [ "${{ matrix.clang-version }}" -gt 18 ]; thenrelies on a numeric comparison that might fail if matrix.clang-version contains non-integer values (e.g., "12.0.1"). To avoid potential errors, consider extracting the major version or using a more robust version comparison. For example:
- if [ "${{ matrix.clang-version }}" -gt 18 ]; then + if [ "$(echo ${{ matrix.clang-version }} | cut -d. -f1)" -gt 18 ]; thenThis change ensures you are comparing only the major version.
Below is a shell script to verify if the current version of the workflow file already implements the suggested major-version extraction:
#!/bin/bash echo "Searching for major version extraction (cut -d. -f1) in .github/workflows/build.yml..." grep "cut -d. -f1" .github/workflows/build.yml || echo "Major version extraction not found"Once you run this, if the output indicates that the extraction isn’t applied, please update the numeric comparison as follows:
Attention: Update Required for MACOS_CMAKE_ARGS Numeric Comparison!
- The step currently compares the full
matrix.clang-versionvalue using-gt 18, which may fail if that value contains non-integer components (e.g.,"12.0.1"). - Update the condition to extract only the major version before the comparison, for example:
- if [ "${{ matrix.clang-version }}" -gt 18 ]; then + if [ "$(echo ${{ matrix.clang-version }} | cut -d. -f1)" -gt 18 ]; thenThis change ensures that only the major version number is compared, avoiding potential numeric errors.
shenxianpeng force-pushed the fix-macos-test-failure branch from 4349692 to d6d60c2 Compare 
shenxianpeng force-pushed the fix-macos-test-failure branch from d6d60c2 to e76454b Compare 1 participant
Summary by CodeRabbit
.gitignorefile to prevent tracking of local virtual environment directories.