Detect availability of OpenMP in CMake and Make files #1001
Conversation
All of the secondary cmakelists.txt files contained code equivalent to: ```cmake target_link_libraries(qsim_decide OpenMP::OpenMP_CXX) ``` Although the use of `link_libraries()` tends to be discouraged in modern CMake usage, this is a case where IMHO it's worth using. I think its use will prevent mistakes and inconsistencies by centralizing the OpenMP setting.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes the OpenMP linking logic by removing individual target_link_libraries calls from subprojects and adding a single link_libraries call in the top-level CMakeLists.txt. While centralizing the logic is a good goal, the use of the obsolete link_libraries command introduces maintainability risks. My review includes a suggestion for a more modern and robust approach using an INTERFACE library to manage this shared dependency.
Gemini Code Assist in code review advocated against using `link_libraries()`. Although I dislike the need to have to put the same command in every cmakelists.txt file, the fact that `link_libraries()` is recommended against is a concern for long-term maintenance and therefore this alternative approach may be be tter in the long run.
mhucka
changed the title
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the OpenMP dependency handling by centralizing the configuration in the top-level CMakeLists.txt and using an INTERFACE library. The changes in the sub-project CMakeLists.txt files are consistent and improve maintainability. However, there is a critical issue in the top-level CMakeLists.txt where a redundant and conflicting find_package(OpenMP REQUIRED) call remains, which will prevent the new conditional OpenMP logic from working correctly.
The CMake files now check if OpenMP can be used. The Makefiles should too.
mhucka
changed the title
1 participant
All of the secondary cmakelists.txt files contained code equivalent to:
target_link_libraries(qsim_decide OpenMP::OpenMP_CXX)This PR updates the form to use the recommended approach of configuring the use in the top-level
CMakeLists.txtand then usingINTERFACEin the calls totarget_link_libraries. The code there and also in the updatedMakefilechecks that OpenMP is supported on the host before adding the link flag.