Conversation
f006fc5 to
37dab8b
Compare
776b599 to
5e953af
Compare
|
I think the token in the GL_API_ACCESS_TOKEN Secret expired. We'd have to save a new valid token. |
5e953af to
9d0774f
Compare
|
Actually I'm not sure if I interpret the CMake docs correctly and if CMP0074 should still be set (to NEW). |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the CMake configuration to use CMake 3.23+ features, including the FILE_SET mechanism for managing headers, target_compile_features for C++ standard requirements, and improved target exports. The changes also namespace all CMake targets and add support for building docs and Python bindings only when the project is the top-level project, addressing issue #67.
Changes:
- Upgraded minimum CMake version requirement from 3.9 to 3.23 and migrated to modern CMake practices using FILE_SET for headers and target_compile_features for C++ standard
- Namespaced all CMake targets (library, documentation, Python bindings) and made docs/Python bindings conditional on being the top-level project to prevent conflicts when used as a subdirectory
- Simplified CI configurations to automatically fetch Catch2 via FetchContent when submodules are not used, removed manual Catch2 installation steps, and updated dependency versions
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| {{cookiecutter.project_slug}}/CMakeLists.txt | Updated to CMake 3.23, replaced CMAKE_CXX_STANDARD with target_compile_features, added PROJECT_IS_TOP_LEVEL checks for app/tests/docs/Python, modernized install rules with FILE_SET |
| {{cookiecutter.project_slug}}/src/CMakeLists.txt | Migrated to target_sources with FILE_SET HEADERS, replaced target_include_directories with BASE_DIRS/FILES, added target_compile_features |
| {{cookiecutter.project_slug}}/tests/CMakeLists.txt | Renamed test target with project prefix, improved FetchContent handling for Catch2, added TEST_PREFIX for test discovery |
| {{cookiecutter.project_slug}}/app/CMakeLists.txt | Modernized to use target_sources pattern |
| {{cookiecutter.project_slug}}/doc/CMakeLists.txt | Namespaced doxygen and sphinx-doc targets with project prefix, scoped documentation paths |
| {{cookiecutter.project_slug}}/cmake/{{cookiecutter.project_slug}}Config.cmake.in | Moved from root to cmake/ subdirectory, simplified using @PACKAGE_INIT@ and modern CMake patterns |
| {{cookiecutter.project_slug}}/{{cookiecutter.project_slug}}Config.cmake.in | Removed old config file from root directory |
| {{cookiecutter.project_slug}}/.github/workflows/ci.yml | Removed manual Catch2 installation, added Doxygen installation step, updated Python version and option names |
| {{cookiecutter.project_slug}}/.gitlab-ci.yml | Removed manual Catch2 installation, added Doxygen installation, updated cmake command syntax |
| {{cookiecutter.project_slug}}/.pre-commit-config.yaml | Removed cmake-format and cmake-lint hooks |
| {{cookiecutter.project_slug}}/TODO.md | Updated CMake version requirement and config file path references |
| .github/workflows/ci.yml | Removed manual Catch2 installation, updated Python versions and checkout action |
| README.md | Updated CMake version requirement and C++ standard support documentation |
| cookiecutter.json | Bumped Catch2 to 3.12.0 and cibuildwheel to 3.3.1 |
| hooks/post_gen_project.py | Removed conditional removal of old Config.cmake.in file |
| tests/test_bake_project.py | Updated test targets to use namespaced names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2923bc7 to
c5cd7a6
Compare
…dule, we do not have to install it manually in CI
0b365c1 to
84685c4
Compare
…ne between cmake commands
…set python version to 3.14
153f67f to
7ca662f
Compare
lkeegan
left a comment
There was a problem hiding this comment.
thanks @thomasisensee lgtm! two minor suggestions
Co-authored-by: Liam Keegan <liam@keegan.ch>
f0cc9ce to
dfd7c9b
Compare
target_include_directoriesusage and migrate totarget_sources(FILE_SET)for headerstarget_compile_featuresfor C++ standard instead of global variables (set(CMAKE_CXX_STANDARD))FILE_SETdefaults and modern target exportsCMP0074policy override (now NEW by default)The commit a2841d0 also removes the
cmake-formatpre-commit hook since their output is hard to read and diverges from the CMake documentation style. I think manual formatting is sufficient for CMake files that probably don't change as often as actual code.