Refactor CI setup and some CMake updates#238
Conversation
…ns for compatibility with 5.0 cannot use range version because versions < 5 are too strict
…e it's not necessary
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 high |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request introduces a vcpkg.json manifest and CMakePresets.json to manage dependencies and build configurations, while updating Eigen3 version requirements and cleaning up CMake include directories. The feedback recommends correcting an invalid version constraint for boost-config and moving boost-test to an optional feature in vcpkg.json. Additionally, it is advised to make the Eigen3 version checks in CMakeLists.txt and cmake/Config.cmake.in more robust by falling back to EIGEN3_VERSION if Eigen3_VERSION is undefined.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "boost-serialization", | ||
| "boost-spirit", | ||
| "boost-stacktrace", | ||
| "boost-test", |
There was a problem hiding this comment.
boost-test is only required for building and running the unit tests. Declaring it as a top-level mandatory dependency forces all consumers of the cctag library to download and build boost-test. Consider moving boost-test to an optional "tests" feature in vcpkg.json so that it is only built when tests are explicitly enabled.
| if(Eigen3_VERSION VERSION_LESS CCTAG_EIGEN_REQUIRED_VERSION) | ||
| message(FATAL_ERROR "Eigen >= ${CCTAG_EIGEN_REQUIRED_VERSION} is required, found ${Eigen3_VERSION}") | ||
| endif() |
There was a problem hiding this comment.
If Eigen3_VERSION is not set (which can happen with some older or alternative Find modules that set EIGEN3_VERSION or EIGEN3_VERSION_STRING instead), the VERSION_LESS comparison will compare an empty string, resulting in a false positive FATAL_ERROR even if a compatible Eigen3 version is present. To make this check robust, define a fallback variable that checks both Eigen3_VERSION and EIGEN3_VERSION before performing the comparison.
set(_Eigen3_version "${Eigen3_VERSION}")
if(NOT _Eigen3_version AND EIGEN3_VERSION)
set(_Eigen3_version "${EIGEN3_VERSION}")
endif()
if(_Eigen3_version VERSION_LESS CCTAG_EIGEN_REQUIRED_VERSION)
message(FATAL_ERROR "Eigen >= ${CCTAG_EIGEN_REQUIRED_VERSION} is required, found ${_Eigen3_version}")
endif()
| if(Eigen3_VERSION VERSION_LESS "@CCTAG_EIGEN_REQUIRED_VERSION@") | ||
| message(FATAL_ERROR "Eigen3 >= @CCTAG_EIGEN_REQUIRED_VERSION@ is required, found ${Eigen3_VERSION}") | ||
| endif() |
There was a problem hiding this comment.
Similar to the check in CMakeLists.txt, if Eigen3_VERSION is not set by the package finder (e.g., if it sets EIGEN3_VERSION instead), this check will fail with a false positive FATAL_ERROR. Using a fallback variable ensures robustness when CCTag is consumed as a third-party package.
set(_Eigen3_version "${Eigen3_VERSION}")
if(NOT _Eigen3_version AND EIGEN3_VERSION)
set(_Eigen3_version "${EIGEN3_VERSION}")
endif()
if(_Eigen3_version VERSION_LESS "@CCTAG_EIGEN_REQUIRED_VERSION@")
message(FATAL_ERROR "Eigen3 >= @CCTAG_EIGEN_REQUIRED_VERSION@ is required, found ${_Eigen3_version}")
endif()
This pull request removes the previous CI pipeline for Windows (no cache, very slow as it has to build all dependencies every time) in favour of a new pipeline using CMake presets, vcpkg, and caching to speed up the building time. Like the previous pipeline, it does not support CUDA on Windows.
On cache it, the build time goes down from 1h to 8 mins.
Incidentally, it also introduces some minor CMake fixes and a temporary solution to support Eigen 5
Build system and dependency management modernization:
vcpkg.jsonto formally declare project dependencies for vcpkg, simplifying cross-platform dependency management. Thevcpkg.jsonis based on the file of the official port of cctag on vcpkg.CMakePresets.jsonwith presets for building and testing in Debug/Release modes, both with and without vcpkg, enabling more reproducible and maintainable builds.Continuous Integration (CI) improvements:
.github/workflows/build_with_vcpkg_no_cuda.yml) to build and test the project on Windows using vcpkg, with CUDA disabled. This replaces the previous, more manual Windows build job..github/workflows/continuous-integration.ymlto ignore documentation-only changes, add permissions, and remove the legacy Windows build job in favor of the new vcpkg-based workflow. [1] [2] [3]CMake and dependency version enforcement:
CMakeLists.txtto use a minimum Eigen3 version (3.3.9) in any configuration instead of a different version for Windows. This is more maintainable.Target include directories cleanup:
CCTagtarget in both CUDA and non-CUDA builds, removing unnecessary and redundant includes as we are using targets. For one, this avoid CMake errors like[1] [2]