Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates GitHub Actions CI/CD workflows to use newer macOS (macos-15) and Windows (windows-2025) runners, standardizes some jobs to ubuntu-slim, updates the spdlog dependency from 1.15.3 to 1.17.0, and modifies build wheel exclusions to reflect the new dependency version. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 315: The test-command currently uses a relative path "pytest test/python"
which will fail under cibuildwheel's temporary working directory; either change
the test-command to use the project-root placeholder (e.g. "pytest
{project}/test/python") or ensure you add a test-sources entry that copies your
tests into test/python in the build environment so the relative path is valid;
update the pyproject.toml test-command value accordingly or add the matching
test-sources configuration to guarantee the tests are found when cibuildwheel
runs.
dbaa623 to
e391dc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmake/ExternalDependencies.cmake`:
- Around line 110-116: Remove the multi-config generator guard and ensure
GNUInstallDirs is included so CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR are
defined: add include(GNUInstallDirs) in the top-level CMakeLists before
include(cmake/ExternalDependencies.cmake) and drop the CMAKE_CONFIGURATION_TYPES
guard logic so the block that sets target properties for spdlog can reliably use
CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR.
5354689 to
abcbbba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Line 200: The python-tests-extensive job's runner matrix is inconsistent with
the python-tests job: update the runs-on entries for the python-tests-extensive
job (symbol: python-tests-extensive) from "macos-14" and "windows-2022" to
"macos-15" and "windows-2025" to match the PR objective, or if you intentionally
want to exercise older platforms, add a brief inline comment above the
python-tests-extensive job explaining that it intentionally tests on
macos-14/windows-2022 for broader compatibility so reviewers know the
discrepancy is deliberate.
## Description This PR updates `.pre-commit-config.yml` to make use of `prek`'s priority feature. The commits were initially part of #921, which is blocked due to a CD issue. ## Checklist: - [x] The pull request only contains commits that are focused and relevant to this change. - [x] ~I have added appropriate tests that cover the new/changed functionality.~ - [x] ~I have updated the documentation to reflect these changes.~ - [x] ~I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.~ - [x] ~I have added migration instructions to the upgrade guide (if needed).~ - [x] The changes follow the project's style guidelines and introduce no new warnings. - [x] The changes are fully tested and pass the CI checks. - [x] I have reviewed my own code changes.
abcbbba to
1e2fa51
Compare
456f303 to
6660379
Compare
There was a problem hiding this comment.
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1).github/workflows/ci.yml (1)
62-81:⚠️ Potential issue | 🟡 Minor
cpp-tests-windowswas not updated towindows-2025, inconsistent withcpp-tests-macos.
cpp-tests-macoswas updated tomacos-15/macos-15-intel, butcpp-tests-windowsstill targetswindows-2022. The extensive test (cpp-tests-extensive-windowsat line 128) already covers bothwindows-2022andwindows-2025, matching the pattern where extensive tests provide backward-compat coverage while regular tests run on current runners.If the omission is intentional (e.g., a known MSVC issue on
windows-2025), please add an inline comment explaining the intent.🔧 Proposed fix
matrix: - runs-on: [windows-2022, windows-11-arm] + runs-on: [windows-2025, windows-11-arm] compiler: [msvc] config: [Release] include: - - runs-on: windows-2022 + - runs-on: windows-2025 compiler: msvc config: Debug🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 62 - 81, The cpp-tests-windows job currently targets windows-2022 in its matrix (matrix.runs-on and the include entry) while cpp-tests-macos was moved to macos-15; update the windows job (cpp-tests-windows) to use the current runner by replacing occurrences of "windows-2022" with "windows-2025" in the matrix.runs-on array and in the include block (and ensure the Debug include maps to the same new runner), and if this change is intentionally omitted due to a known MSVC/windows-2025 issue, instead add an inline comment next to the matrix or include explaining that intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 62-81: The cpp-tests-windows job currently targets windows-2022 in
its matrix (matrix.runs-on and the include entry) while cpp-tests-macos was
moved to macos-15; update the windows job (cpp-tests-windows) to use the current
runner by replacing occurrences of "windows-2022" with "windows-2025" in the
matrix.runs-on array and in the include block (and ensure the Debug include maps
to the same new runner), and if this change is intentionally omitted due to a
known MSVC/windows-2025 issue, instead add an inline comment next to the matrix
or include explaining that intent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/cd.yml.github/workflows/ci.yml.github/workflows/release-drafter.yml.github/workflows/templating.yml.github/workflows/upstream.ymlCHANGELOG.mdcmake/ExternalDependencies.cmakepyproject.toml
burgholzer
left a comment
There was a problem hiding this comment.
LGTM 👍🏻 thanks for the fixes
## Description This PR updates `.pre-commit-config.yml` to make use of `prek`'s priority feature. The commits were initially part of munich-quantum-toolkit#921, which is blocked due to a CD issue. ## Checklist: - [x] The pull request only contains commits that are focused and relevant to this change. - [x] ~I have added appropriate tests that cover the new/changed functionality.~ - [x] ~I have updated the documentation to reflect these changes.~ - [x] ~I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.~ - [x] ~I have added migration instructions to the upgrade guide (if needed).~ - [x] The changes follow the project's style guidelines and introduce no new warnings. - [x] The changes are fully tested and pass the CI checks. - [x] I have reviewed my own code changes.
Description
This PR updates the CI to use
macos-15instead ofmacos-14andwindows-2025instead ofwindows-2022.Checklist:
I have added appropriate tests that cover the new/changed functionality.I have updated the documentation to reflect these changes.I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.I have added migration instructions to the upgrade guide (if needed).