Conversation
denialhaag
added
dependencies
📝 WalkthroughWalkthroughThis PR updates CI infrastructure and development tooling across the repository. It upgrades GitHub Actions runners (macOS from versions 14 to 15, Windows from 2022 to 2025), switches several jobs to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
cmake/ExternalDependencies.cmake
Outdated
| # Ensure external shared libraries end up in a common lib layout used by our RUNPATH | ||
| if(TARGET spdlog) | ||
| set_target_properties( | ||
| spdlog | ||
| PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}" | ||
| ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}" | ||
| RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 1) Check where GNUInstallDirs is included
rg -n 'GNUInstallDirs' --type cmakeRepository: munich-quantum-toolkit/qmap
Length of output: 53
🏁 Script executed:
# 2) Check the structure of CMakeLists.txt to understand include order
fd -t f -e cmake | head -20Repository: munich-quantum-toolkit/qmap
Length of output: 122
🏁 Script executed:
# 3) Get context around ExternalDependencies.cmake
cat -n cmake/ExternalDependencies.cmake | sed -n '1,130p'Repository: munich-quantum-toolkit/qmap
Length of output: 5094
🏁 Script executed:
# 4) Check CI workflows for generator/build-type usage
find .github/workflows -type f \( -name '*.yml' -o -name '*.yaml' \) -exec grep -l 'CMAKE' {} \;Repository: munich-quantum-toolkit/qmap
Length of output: 53
🏁 Script executed:
# Find main CMakeLists.txt and search for GNUInstallDirs
cat -n CMakeLists.txt | head -50Repository: munich-quantum-toolkit/qmap
Length of output: 1833
🏁 Script executed:
# Search more broadly for GNUInstallDirs
rg -n 'GNUInstallDirs' --type-listRepository: munich-quantum-toolkit/qmap
Length of output: 460
🏁 Script executed:
# Search for where ExternalDependencies.cmake is included
rg -n 'ExternalDependencies' --type cmakeRepository: munich-quantum-toolkit/qmap
Length of output: 130
🏁 Script executed:
# Check for CI workflows
ls -la .github/workflows/ 2>/dev/null || echo "No workflows found"Repository: munich-quantum-toolkit/qmap
Length of output: 566
🏁 Script executed:
# Continue reading CMakeLists.txt to find GNUInstallDirs
cat -n CMakeLists.txt | sed -n '50,87p'Repository: munich-quantum-toolkit/qmap
Length of output: 1752
🏁 Script executed:
# Search for GNUInstallDirs in all CMakeLists files
rg -n 'GNUInstallDirs'Repository: munich-quantum-toolkit/qmap
Length of output: 53
🏁 Script executed:
# Check ci.yml for CMAKE_GENERATOR or multi-config settings
cat .github/workflows/ci.ymlRepository: munich-quantum-toolkit/qmap
Length of output: 11173
🏁 Script executed:
# Search the entire codebase for any definition or setting of CMAKE_INSTALL_LIBDIR or CMAKE_INSTALL_BINDIR
rg -n 'CMAKE_INSTALL_LIBDIR|CMAKE_INSTALL_BINDIR' --type cmakeRepository: munich-quantum-toolkit/qmap
Length of output: 657
🏁 Script executed:
# Check if there are any issues or failures related to spdlog output directories in git history or issues
git log --oneline --all | head -20Repository: munich-quantum-toolkit/qmap
Length of output: 1134
Remove the multi-config generator guard; instead, verify CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR are defined via GNUInstallDirs.
The code uses CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR without explicitly including GNUInstallDirs, which defines these variables with platform-appropriate defaults. While this hasn't caused CI failures (which use single-config generators only), explicitly including GNUInstallDirs before ExternalDependencies.cmake would ensure these variables are reliably defined and clarify intent.
The original concern about multi-config generators (Visual Studio, Xcode) forcing artifact collisions is not applicable to this project—CI uses only single-config generators (GCC, Clang, MSVC) with separate Debug/Release build directories. The proposed CMAKE_CONFIGURATION_TYPES guard is unnecessary.
Recommended fix: Add include(GNUInstallDirs) in the main CMakeLists.txt before include(cmake/ExternalDependencies.cmake) at line 87 to ensure robustness.
🤖 Prompt for AI Agents
In `@cmake/ExternalDependencies.cmake` around lines 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.
There was a problem hiding this comment.
This has been removed, right?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any CMAKE_CONFIGURATION_TYPES references in the repository
rg -n 'CMAKE_CONFIGURATION_TYPES' --type cmake
# Also check in all files, not just cmake
rg -n 'CMAKE_CONFIGURATION_TYPES'
# Check the entire ExternalDependencies.cmake file
cat cmake/ExternalDependencies.cmake
Length of output: 3797
🏁 Script executed:
#!/bin/bash
# Check if GNUInstallDirs is included in the main CMakeLists.txt
cat CMakeLists.txt | head -90
Length of output: 2935
🏁 Script executed:
#!/bin/bash
# Check if CMAKE_INSTALL_LIBDIR or CMAKE_INSTALL_BINDIR might be defined elsewhere
# First, see if there are any other cmake files that might include GNUInstallDirs
find . -name "*.cmake" -o -name "CMakeLists.txt" | xargs grep -l "GNUInstallDirs" 2>/dev/null || echo "No GNUInstallDirs found in cmake files"
# Also check if these variables are set anywhere
rg "CMAKE_INSTALL_LIBDIR|CMAKE_INSTALL_BINDIR" -A 2 -B 2
Length of output: 546
@denialhaag Yes, correct! There's no CMAKE_CONFIGURATION_TYPES guard in the code—there never was one. My earlier comment mentioned "removing" it, which was confusing since nothing needed to be removed.
However, the underlying issue still applies: CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR are used without including GNUInstallDirs, which defines these variables with platform-appropriate defaults (e.g., lib, lib64, bin).
Currently, these variables are used in:
bindings/CMakeLists.txt(existing code)cmake/ExternalDependencies.cmake(your new code for spdlog)
Without include(GNUInstallDirs), these variables will be empty/undefined unless inherited from a parent project. Since this project can be built standalone, it's safer to add:
include(GNUInstallDirs)in the main CMakeLists.txt before line 87 where include(cmake/ExternalDependencies.cmake) is called. This ensures the variables are always properly defined.
🧠 Learnings used
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qcec PR: 831
File: bindings/register_equivalence_checking_manager.cpp:11-20
Timestamp: 2026-01-26T00:52:09.661Z
Learning: In munich-quantum-toolkit (MQT) projects, Python bindings targets are not built by default in CI workflows. This can cause clang-tidy to miss issues in bindings code (e.g., missing includes, type errors) because clang-tidy requires the code to be compiled to generate a compilation database for analysis. To enable linting of bindings code, add `-DBUILD_MQT_QCEC_BINDINGS=ON` (or the equivalent flag for other MQT packages like `-DBUILD_MQT_CORE_BINDINGS=ON`) to the cmake-args in the linter action workflow.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1356
File: mlir/CMakeLists.txt:13-15
Timestamp: 2026-01-04T23:01:47.734Z
Learning: In the munich-quantum-toolkit/core repository's MLIR directory (mlir/CMakeLists.txt), the MSVC compile definition `_SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING` is necessary to suppress warnings triggered by MLIR/LLVM internal headers, not because the project code uses non-floating complex types. This suppression is needed to avoid warnings from third-party MLIR/LLVM header includes.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.836Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 3
File: lib/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txt:22-25
Timestamp: 2025-12-05T15:57:39.701Z
Learning: The munich-quantum-toolkit projects (core and core-plugins-catalyst) use `file(GLOB_RECURSE ...)` patterns in CMakeLists.txt files to collect header files, following an established convention in the parent repository for consistency across the codebase.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1356
File: src/qdmi/na/CMakeLists.txt:179-184
Timestamp: 2026-01-04T23:54:33.540Z
Learning: In the munich-quantum-toolkit/core repository, MSVC link-time code generation (LTCG) must be disabled for the NA device dynamic library target (mqt-core-qdmi-na-device-dyn) on Windows to avoid link errors and missing symbols. This issue affects both release-only builds and debug/release mixed builds, not just debug/release mixing scenarios.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/ir/register_permutation.cpp:153-171
Timestamp: 2025-12-22T01:25:21.609Z
Learning: In the munich-quantum-toolkit/core repository, when using nanobind iterator factory functions like `make_key_iterator` and `make_iterator`, the unqualified form (without explicit `nb::` prefix) is preferred. The clang-tidy configuration suggests removal of explicit namespace qualification, relying on ADL (Argument-Dependent Lookup) to resolve these functions correctly.
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.
denialhaag
force-pushed
the
update-dependencies
branch
from
abcbbba to
1e2fa51
Compare
1 participant
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).