Open
Conversation
| if: matrix.compiler == 'clang' | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y clang |
There was a problem hiding this comment.
Clang version not pinned
apt-get install -y clang installs whatever version Ubuntu 24.04 currently defaults to (currently clang-18, but this can change with future apt updates). This makes the CI non-deterministic — a future Ubuntu package update could silently switch the compiler version and introduce unexpected build failures or behaviour changes. Pinning to a specific version (e.g. clang-18) is more reproducible and consistent with how other tools in this workflow are version-locked (e.g. cmake==3.30.0, pybind11==3.0).
Suggested change
| sudo apt-get install -y clang | |
| sudo apt-get install -y clang-18 | |
| echo "CC=clang-18" >> $GITHUB_ENV | |
| echo "CXX=clang++-18" >> $GITHUB_ENV |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Greptile Summary
This PR extends the CI matrix in
.github/workflows/main.ymlto add a Clang build job (linux-x64-clang) alongside the existing GCC-default jobs, enabling clang-compatibility checks on Linux x64.linux-x64-clang(runs onubuntu-24.04) with acompiler: clangproperty.compiler: defaultproperty to the three pre-existing matrix entries to make the matrix schema consistent.if: matrix.compiler == 'clang') that installs clang viaapt-getand exportsCC=clang/CXX=clang++into$GITHUB_ENV, making the compiler available to all downstream steps (CMake build, C++ tests, C++ examples).$GITHUB_ENV) is the correct GitHub Actions mechanism for propagating values across steps, and both theBuild from source(scikit-build-core/CMake) andRun C++ Examples(standalone CMake) steps will automatically pick upCC/CXX.clangis installed without a pinned version, unlike every other tool in the workflow (cmake, pybind11, ninja, etc.), which could lead to silent version drift.Confidence Score: 4/5
.github/workflows/main.yml.Important Files Changed
linux-x64-clangmatrix entry and a conditional "Install Clang" step. The approach is correct and well-structured, with one minor concern: the clang package is not version-pinned, unlike other dependencies in the workflow.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Push / PR / workflow_dispatch] --> B[lint job\nCode Quality Checks\nubuntu-24.04] B --> C{Matrix: build-and-test} C --> D[macos-arm64\nmacos-15\ncompiler: default] C --> E[linux-arm64\nubuntu-24.04-arm\ncompiler: default] C --> F[linux-x64\nubuntu-24.04\ncompiler: default] C --> G[linux-x64-clang ✨ NEW\nubuntu-24.04\ncompiler: clang] G --> H[Install Clang\napt-get install clang\nCC=clang / CXX=clang++] H --> I[Build from source\npip install via CMake] I --> J[Run C++ Tests] J --> K[Run Python Tests] K --> L[Run C++ Examples] F --> M[Build from source\npip install via CMake\ndefault GCC] M --> N[Run C++ Tests] N --> O[Run Python Tests] O --> P[Run C++ Examples]Last reviewed commit: f7312ee