Fixes qiskit_cext library not found by CMake on Windows #15389
Conversation
qiskit-bot
added
the
Community PR
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
Correcting the paths here looks like a good thing, thanks.
I'm not actually sure that qiskit_cext should be needed at all; can you check if make ctest works for you without it?
Just checking: we don't use cmake as the complete driver here. When you're running tests, are you using make ctest and letting that call cmake for you?
CMakeLists.txt
Outdated
|
|
||
| include_directories (${CMAKE_CURRENT_SOURCE_DIR}/dist/c/include) | ||
| # The remaining CMake configuration is done inside of the C API test suite. | ||
| add_subdirectory (test/c) |
There was a problem hiding this comment.
Should this also have a root-path correction?
There was a problem hiding this comment.
added that as well, in case its run from a different directory
CMakeLists.txt
Outdated
| # On Windows, Rust creates "qiskit_cext.dll" + "qiskit_cext.dll.lib" (import lib). | ||
| # CMake doesn't know about ".dll.lib" by default, so add it. | ||
| if(WIN32) | ||
| set(CMAKE_FIND_LIBRARY_SUFFIXES ".dll.lib" ${CMAKE_FIND_LIBRARY_SUFFIXES}) | ||
| endif() | ||
|
|
||
| # The C API is provided by the qiskit_cext Rust crate. Its header gets | ||
| # generated by cbindgen (see `make cheader`). | ||
| find_library (qiskit qiskit_cext PATHS target/debug REQUIRED) | ||
| find_library (qiskit qiskit_cext PATHS ${CMAKE_CURRENT_SOURCE_DIR}/target/debug REQUIRED) |
There was a problem hiding this comment.
We have CI that runs on Windows, and includes using this CMakeLists file - it's been doing that successfully since the file was introduced. Can you explain why this change is needed, given we'd been using the file successfully on Windows previously?
There was a problem hiding this comment.
Without knowing how the CI is set up, I suspect the difference is between GNU and MSVC toolchains. I am using MSVC cl, etc. on my system. The makefile was configured to use gnu tools so I suspect it relies on gcc, etc. I made local modifications so I could run the makefile on my system without e.g. MSYS2
There was a problem hiding this comment.
However I believe it is irrelevant, make ctest works without the library import so the conditional and import can be removed.
There was a problem hiding this comment.
We only support MSVC on Windows, and that's what's tested in CI - it doesn't/shouldn't rely on the GNU tool chain. You can see the MSVC-specific options getting set in some other CMake file iirc.
There was a problem hiding this comment.
Considering tests are failing, and its building fine on my system, I suspect some kind of configuration issue. I will look into this more.
|
I changed from |
3 participants
Summary
A small change,
.dll.libfiles are not always found by default in a Windows environment. This pull request adds a conditional flag to theCMakeLists.txtscript adding it to the list of valid CMake library suffixes on Windows systems. Additionally, it fixes some path issues by prepending the paths with theCMakeLists.txtroot directory (base qiskit folder)