Skip to content

Conversation

@burgholzer
Copy link
Member

@burgholzer burgholzer commented Dec 28, 2025

Description

Removes Windows-specific platform guards from dynamic QDMI device library loading infrastructure, adds cross-platform dynamic library loading macros, and updates tests to run uniformly across all platforms.

Fixes #1351

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 QDMI Support milestone Dec 28, 2025
@burgholzer burgholzer self-assigned this Dec 28, 2025
@burgholzer burgholzer added enhancement New feature or request QDMI Anything related to QDMI labels Dec 28, 2025
@burgholzer burgholzer marked this pull request as ready for review December 28, 2025 18:19
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 28, 2025

📝 Walkthrough

Walkthrough

Removes Windows-only guards and makes dynamic QDMI device library support unconditional across platforms, introduces cross-platform dynamic-loading macros in the C++ driver, expands the Python binding add_dynamic_device_library with authentication/config parameters and explicit return type, and updates tests/CMake to run/load dynamic devices on all OSes.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Added entry noting removal of Windows-specific restrictions for dynamic QDMI device library handling and PR link for #1406.
C++ Header
include/mqt-core/qdmi/Driver.hpp
Removed #ifndef _WIN32 guards so DynamicDeviceLibrary and Driver::addDynamicDeviceLibrary are available on all platforms; updated member docs.
C++ Implementation
src/qdmi/Driver.cpp
Added cross-platform macros (DL_OPEN, DL_SYM, DL_CLOSE), replaced dlopen/dlsym/dlclose usage, updated preprocessor branches for Windows vs POSIX, and made addDynamicDeviceLibrary unconditional.
CMake (targets)
src/qdmi/na/CMakeLists.txt, src/qdmi/sc/CMakeLists.txt, test/qdmi/CMakeLists.txt
Removed NOT WIN32 / WIN32 guards so dynamic NA/SC device targets and test compile definitions are created for all platforms.
Python Bindings
bindings/fomac/fomac.cpp
Removed Windows compile guard; expanded add_dynamic_device_library signature to accept optional base_url, token, auth_file, auth_url, username, password, custom1custom5; now returns fomac::Session::Device.
Python Stubs
python/mqt/core/fomac.pyi
Removed docstring note that the function is Windows-unavailable; doc updated to reflect unconditional availability.
Python Tests
test/python/fomac/test_fomac.py
Imported add_dynamic_device_library and removed platform-gated tests; consolidated to unconditional test invocation.
C++ Tests
test/qdmi/test_driver.cpp
Removed Windows-only preprocessor guards; dynamic-device-related tests and DEVICES list now included unconditionally.

Sequence Diagram(s)

mermaid
sequenceDiagram
autonumber
participant Py as Python caller
participant Bind as bindings/fomac
participant Driver as qdmi::Driver
participant Loader as OS loader
participant Device as QDMI Device
Note over Bind,Driver: New params (auth/config) passed through
Py->>Bind: add_dynamic_device_library(path, prefix, auth...)
Bind->>Driver: addDynamicDeviceLibrary(path, prefix, config)
Driver->>Loader: DL_OPEN(library_path)
alt open success
Loader-->>Driver: handle
Driver->>Loader: DL_SYM(handle, factory_symbol)
Loader-->>Driver: symbol_ptr
Driver->>Device: instantiate via symbol_ptr
Driver-->>Bind: Device (wrapped)
Bind-->>Py: fomac::Session::Device
else open/error
Loader-->>Driver: error
Driver-->>Bind: throw/propagate error
Bind-->>Py: exception

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested labels

c++

Suggested reviewers

  • ystade

Poem

🐰 I hopped through guards and patched the seam,
Now Windows and Unix share the same dream.
Libraries load where once they’d part —
One loader, one drive, one beating heart.
🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: removing Windows-specific restrictions for dynamic QDMI device library handling, which is the primary focus of this PR.
Linked Issues check ✅ Passed Changes meet the requirements of issue #1351 by removing Windows guards, adding cross-platform macros, and enabling dynamic library loading on Windows alongside Unix systems.
Out of Scope Changes check ✅ Passed All changes are directly related to removing Windows-specific restrictions and implementing cross-platform dynamic library loading, with no unrelated modifications detected.
Description check ✅ Passed The pull request description includes a summary of changes, references the fixed issue (#1351), includes a completed checklist with all items marked as done, and aligns well with the template structure.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch windows-symbol-loading

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.

@codecov
Copy link

codecov bot commented Dec 28, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/qdmi/Driver.cpp 66.6% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…ary handling

Signed-off-by: burgholzer <burgholzer@me.com>
@burgholzer burgholzer force-pushed the windows-symbol-loading branch from c272cbd to 9136be6 Compare December 28, 2025 18:27
@burgholzer burgholzer merged commit c9036b2 into main Dec 28, 2025
33 of 34 checks passed
@burgholzer burgholzer deleted the windows-symbol-loading branch December 28, 2025 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request QDMI Anything related to QDMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable loading of dynamic QDMI device libraries on Windows

2 participants