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
Summary by CodeRabbit
.gitignorefile to prevent tracking of local virtual environment directories.