⬆️ Update QDMI to latest version from stable v1.2.x branch#1593
⬆️ Update QDMI to latest version from stable v1.2.x branch#1593burgholzer merged 4 commits intomainfrom
v1.2.x branch#1593Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR updates the QDMI dependency from version 1.2.1 to 1.2.2, adjusts CMake device library configurations to remove direct qdmi::qdmi dependencies and add export macros with hidden visibility settings, updates test includes to use mqt_ddsim_qdmi headers, and improves source file documentation comments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qdmi/devices/na/CMakeLists.txt`:
- Around line 189-198: The target compile definitions and visibility properties
are being applied to ${TARGET_NAME} instead of the dynamic library target,
leaving ${DYN_TARGET_NAME} without export macros or hidden visibility; update
the block so that target_compile_definitions(...) uses ${DYN_TARGET_NAME} with
the dynamic export macro (e.g., MQT_NA_DYN_QDMI_device_EXPORTS) and
set_target_properties(...) applies
C_VISIBILITY_PRESET/CXX_VISIBILITY_PRESET/CXX_VISIBILITY_INLINES_HIDDEN to
${DYN_TARGET_NAME} rather than ${TARGET_NAME}, keeping this change inside the
existing if(NOT TARGET ${DYN_TARGET_NAME}) scope.
In `@src/qdmi/devices/sc/CMakeLists.txt`:
- Around line 186-195: The compile definitions and visibility properties are
being applied to ${TARGET_NAME} but should be applied to the dynamic library
target ${DYN_TARGET_NAME}; update the two calls that reference ${TARGET_NAME}
(the target_compile_definitions(...) and set_target_properties(...)) to use
${DYN_TARGET_NAME} so the dynamic library gets the export macro and hidden
visibility settings (these lines are the ones inside the if(NOT TARGET
${DYN_TARGET_NAME}) block).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4eb90655-dfb4-45bb-9c43-9da82a64f299
📒 Files selected for processing (3)
src/qdmi/devices/dd/CMakeLists.txtsrc/qdmi/devices/na/CMakeLists.txtsrc/qdmi/devices/sc/CMakeLists.txt
a2e428d to
dd74c2f
Compare
dd74c2f to
42b86f4
Compare
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qdmi/devices/dd/CMakeLists.txt`:
- Around line 60-65: Replace the incorrect CMake property name
CXX_VISIBILITY_INLINES_HIDDEN with the correct VISIBILITY_INLINES_HIDDEN in the
set_target_properties call for ${TARGET_NAME}; keep the value (1) and the
surrounding visibility presets (C_VISIBILITY_PRESET and CXX_VISIBILITY_PRESET)
unchanged so inline symbol hiding is actually applied.
In `@src/qdmi/devices/na/CMakeLists.txt`:
- Around line 142-147: Replace the non-existent CMake property
CXX_VISIBILITY_INLINES_HIDDEN with the documented VISIBILITY_INLINES_HIDDEN in
the set_target_properties call for ${TARGET_NAME}; update both visibility blocks
where CXX_VISIBILITY_INLINES_HIDDEN is used so the project uses
VISIBILITY_PRESET hidden / C_VISIBILITY_PRESET hidden and
VISIBILITY_INLINES_HIDDEN 1 to ensure inline symbols are actually hidden across
languages.
In `@src/qdmi/devices/sc/CMakeLists.txt`:
- Around line 139-144: The CMake visibility blocks setting target properties for
${TARGET_NAME} use the undocumented property CXX_VISIBILITY_INLINES_HIDDEN which
is ignored; replace that property with the correct, language-agnostic
VISIBILITY_INLINES_HIDDEN in both visibility blocks where set_target_properties
is used (the block that sets C_VISIBILITY_PRESET and CXX_VISIBILITY_PRESET and
the second analogous block), keeping the rest of the properties intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1c43c31-1c42-4d45-ac6b-85cb6bf1aada
📒 Files selected for processing (3)
src/qdmi/devices/dd/CMakeLists.txtsrc/qdmi/devices/na/CMakeLists.txtsrc/qdmi/devices/sc/CMakeLists.txt
…ibility Signed-off-by: burgholzer <burgholzer@me.com>
42b86f4 to
7039bdf
Compare
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/ExternalDependencies.cmake`:
- Around line 96-99: The cached variables QDMI_VERSION and QDMI_REV are set
without FORCE so reconfigures won’t update them; change the set(...) calls for
QDMI_VERSION and QDMI_REV to use the CACHE STRING ... FORCE form (i.e., add
FORCE to the set invocations that define QDMI_VERSION and QDMI_REV) so that
updating the pinned version/revision will overwrite the existing CMake cache
entry and FetchContent will fetch the new commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3afe7063-b381-4e9f-882d-6b9f69c5c7fd
📒 Files selected for processing (26)
CHANGELOG.mdcmake/ExternalDependencies.cmakeinclude/mqt-core/qdmi/common/Common.hppsrc/qdmi/devices/dd/CMakeLists.txtsrc/qdmi/devices/dd/Device.cppsrc/qdmi/devices/na/App.cppsrc/qdmi/devices/na/CMakeLists.txtsrc/qdmi/devices/na/Device.cppsrc/qdmi/devices/na/DynDevice.cppsrc/qdmi/devices/na/Generator.cppsrc/qdmi/devices/sc/App.cppsrc/qdmi/devices/sc/CMakeLists.txtsrc/qdmi/devices/sc/Device.cppsrc/qdmi/devices/sc/DynDevice.cppsrc/qdmi/devices/sc/Generator.cpptest/qdmi/devices/dd/concurrency_test.cpptest/qdmi/devices/dd/device_properties_test.cpptest/qdmi/devices/dd/device_status_test.cpptest/qdmi/devices/dd/error_handling_test.cpptest/qdmi/devices/dd/helpers/test_utils.cpptest/qdmi/devices/dd/job_lifecycle_test.cpptest/qdmi/devices/dd/job_parameters_test.cpptest/qdmi/devices/dd/results_probabilities_test.cpptest/qdmi/devices/dd/results_sampling_test.cpptest/qdmi/devices/dd/results_statevector_test.cpptest/qdmi/devices/dd/session_lifecycle_test.cpp
💤 Files with no reviewable changes (1)
- include/mqt-core/qdmi/common/Common.hpp
## Description This PR updates QDMI to the latest version from the stable 1.2.x branch, which should be fairly close to the actual 1.2.2 release. The biggest change here is that device now no longer need to directly link against `qdmi::qdmi`, but are rather self-contained. ## Checklist <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [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. **If PR contains AI-assisted content:** - [x] I have disclosed the use of AI tools in the PR description as per our [AI Usage Guidelines](https://github.com/munich-quantum-toolkit/core/blob/main/docs/ai_usage.md). - [x] AI-assisted commits include an `Assisted-by: [Model Name] via [Tool Name]` footer. - [x] I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it. --------- Signed-off-by: burgholzer <burgholzer@me.com> (cherry picked from commit bd9f9cb) # Conflicts: # CHANGELOG.md # include/mqt-core/qdmi/common/Common.hpp # test/qdmi/devices/dd/concurrency_test.cpp # test/qdmi/devices/dd/device_properties_test.cpp # test/qdmi/devices/dd/device_status_test.cpp # test/qdmi/devices/dd/helpers/test_utils.cpp # test/qdmi/devices/dd/job_parameters_test.cpp # test/qdmi/devices/dd/results_probabilities_test.cpp # test/qdmi/devices/dd/results_sampling_test.cpp # test/qdmi/devices/dd/results_statevector_test.cpp
|
@Mergifyio backport v3.x |
✅ Backports have been createdDetails
Cherry-pick of bd9f9cb has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
## Description This PR updates QDMI to the latest version from the stable 1.2.x branch, which should be fairly close to the actual 1.2.2 release. The biggest change here is that device now no longer need to directly link against `qdmi::qdmi`, but are rather self-contained. ## Checklist <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [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. **If PR contains AI-assisted content:** - [x] I have disclosed the use of AI tools in the PR description as per our [AI Usage Guidelines](https://github.com/munich-quantum-toolkit/core/blob/main/docs/ai_usage.md). - [x] AI-assisted commits include an `Assisted-by: [Model Name] via [Tool Name]` footer. - [x] I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it. --------- Signed-off-by: burgholzer <burgholzer@me.com> (cherry picked from commit bd9f9cb) # Conflicts: # CHANGELOG.md
## Description This PR updates QDMI to the latest version from the stable 1.2.x branch, which should be fairly close to the actual 1.2.2 release. The biggest change here is that device now no longer need to directly link against `qdmi::qdmi`, but are rather self-contained. ## Checklist <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [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. **If PR contains AI-assisted content:** - [x] I have disclosed the use of AI tools in the PR description as per our [AI Usage Guidelines](https://github.com/munich-quantum-toolkit/core/blob/main/docs/ai_usage.md). - [x] AI-assisted commits include an `Assisted-by: [Model Name] via [Tool Name]` footer. - [x] I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it. --------- Signed-off-by: burgholzer <burgholzer@me.com> (cherry picked from commit bd9f9cb) Signed-off-by: burgholzer <burgholzer@me.com> # Conflicts: # CHANGELOG.md
…#1593) (#1597) ## Description This PR updates QDMI to the latest version from the stable 1.2.x branch, which should be fairly close to the actual 1.2.2 release. The biggest change here is that device now no longer need to directly link against `qdmi::qdmi`, but are rather self-contained. ## 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. **If PR contains AI-assisted content:** - [x] I have disclosed the use of AI tools in the PR description as per our [AI Usage Guidelines](https://github.com/munich-quantum-toolkit/core/blob/main/docs/ai_usage.md). - [x] AI-assisted commits include an `Assisted-by: [Model Name] via [Tool Name]` footer. - [x] I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it. <hr>This is an automatic backport of pull request #1593 done by [Mergify](https://mergify.com). Co-authored-by: Lukas Burgholzer <burgholzer@me.com>
Description
This PR updates QDMI to the latest version from the stable 1.2.x branch, which should be fairly close to the actual 1.2.2 release.
The biggest change here is that device now no longer need to directly link against
qdmi::qdmi, but are rather self-contained.Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.