Skip to content

Conversation

@burgholzer
Copy link
Member

Description

Picked from #1403.
Improves the portability of the QIR runner by relying on less platform specific functions.

Checklist:

  • The pull request only contains commits that are focused and relevant to this change.
  • 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).
  • The changes follow the project's style guidelines and introduce no new warnings.
  • The changes are fully tested and pass the CI checks.
  • I have reviewed my own code changes.

@burgholzer burgholzer added this to the QIR Support milestone Dec 30, 2025
@burgholzer burgholzer self-assigned this Dec 30, 2025
@burgholzer burgholzer added usability Anything related to usability c++ Anything related to C++ code labels Dec 30, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of file paths containing spaces across different platforms.
    • Enhanced cross-platform system command execution for better compatibility.
  • Chores

    • Updated build configuration for test infrastructure.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

The changes refactor cross-platform compatibility in the QIR test runner by replacing Windows-specific system call macros with uniform std::system invocations and quoted paths. Additionally, the CMake configuration is updated to explicitly set the linker language for generated QIR circuit targets.

Changes

Cohort / File(s) Summary
Cross-platform system call refactoring
test/qir/runner/test_qir_runner.cpp
Eliminates Windows-specific _wsystem macro alias; unifies to std::system across all platforms. Adds path quoting to handle spaces in executable and file paths. Replaces macro-based invocation pattern.
CMake linker configuration
test/qir/runtime/CMakeLists.txt
Adds explicit linker language property (LINKER_LANGUAGE CXX) to targets created by ADD_QIR_CIRCUIT macro via set_target_properties.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A hop through paths both wide and small,
With quotes we wrap them, one and all!
Platform-free, the system calls now run,
And linkers speak CXX—the deed is done! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main objective of improving QIR runner portability by reducing platform-specific functions.
Description check ✅ Passed The description includes the required summary, context about PR #1403, and a completed checklist covering all key areas including tests, documentation, and changelog entries.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f748baa and 5a2dc41.

📒 Files selected for processing (2)
  • test/qir/runner/test_qir_runner.cpp
  • test/qir/runtime/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1403
File: test/qir/runner/test_qir_runner.cpp:35-35
Timestamp: 2025-12-28T17:05:10.588Z
Learning: In the munich-quantum-toolkit/core repository, Windows Unicode support (via _wsystem) is not needed for test utilities like test/qir/runner/test_qir_runner.cpp. Using std::system is acceptable for system calls in tests.
📚 Learning: 2025-12-28T17:05:05.488Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1403
File: test/qir/runner/test_qir_runner.cpp:35-35
Timestamp: 2025-12-28T17:05:05.488Z
Learning: In the munich-quantum-toolkit/core repository, for the test_qir_runner.cpp file under test/qir/runner, use std::system instead of _wsystem. Windows Unicode support is not required in these test utilities, so avoid _wsystem and rely on the standard library's system call. Ensure proper error handling and consider cross-platform portability within tests.

Applied to files:

  • test/qir/runner/test_qir_runner.cpp
📚 Learning: 2025-11-03T23:09:26.881Z
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.

Applied to files:

  • test/qir/runtime/CMakeLists.txt
📚 Learning: 2025-12-28T17:14:53.890Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1403
File: src/qdmi/na/CMakeLists.txt:31-38
Timestamp: 2025-12-28T17:14:53.890Z
Learning: In the munich-quantum-toolkit/core repository, the NA device generator target (mqt-core-qdmi-na-device-gen) is intentionally propagated to MQT_CORE_TARGETS via list(APPEND) because it's publicly linked to the NA device library (the NA device uses a public function from the generator). The SC device generator is not propagated because it has no such dependency with the SC device library.

Applied to files:

  • test/qir/runtime/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
  • GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
  • GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
  • GitHub Check: 🇨‌ Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
  • GitHub Check: 🇨‌ Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
  • GitHub Check: 🇨‌ Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
  • GitHub Check: 🇨‌ Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
  • GitHub Check: 🇨‌ Lint / 🚨 Lint
  • GitHub Check: 🐍 Lint / 🚨 Lint
  • GitHub Check: 🇨‌ Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
  • GitHub Check: 🇨‌ Coverage / 📈 Coverage
🔇 Additional comments (2)
test/qir/runtime/CMakeLists.txt (1)

24-24: LGTM! Explicit linker language specification improves robustness.

Setting the linker language explicitly to CXX is good practice when the target is built from pre-compiled object files, as CMake cannot automatically infer the language. This ensures consistent C++ linkage across platforms.

test/qir/runner/test_qir_runner.cpp (1)

34-35: LGTM! Improved portability with cross-platform std::system usage.

The changes correctly improve cross-platform compatibility by:

  • Using std::system uniformly across all platforms (removing Windows-specific macro)
  • Quoting both the executable path and file path to properly handle spaces

Based on learnings, std::system is appropriate for test utilities without requiring Windows Unicode support.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@burgholzer burgholzer enabled auto-merge (squash) December 30, 2025 19:30
@burgholzer burgholzer merged commit 3bef3a6 into main Dec 30, 2025
34 checks passed
@burgholzer burgholzer deleted the qir-runner branch December 30, 2025 20:09
burgholzer added a commit that referenced this pull request Jan 8, 2026
## Description

Picked from #1403.
Improves the portability of the QIR runner by relying on less platform
specific functions.

## 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.

---------

Signed-off-by: burgholzer <[email protected]>
(cherry picked from commit 3bef3a6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Anything related to C++ code usability Anything related to usability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants