Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Add runner that builds MSVC target with clang/clang++ #760

Draft
wants to merge 2 commits into
base: dev2
Choose a base branch
from

Conversation

mmuetzel
Copy link
Contributor

@mmuetzel mmuetzel commented Feb 6, 2024

This is just to demonstrate the issue discussed in #750.

The issue probably needs a fix in upstream CMake (not in SuiteSparse). Please, do not merge (until this is fixed upstream and the fix made its way to Visual Studio).

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Feb 6, 2024

The (upstream) issue is that the OpenMP libraries aren't set for some reason by FindOpenMP.cmake:

-- OpenMP C libraries:       
-- OpenMP C include:         
-- OpenMP C flags:          -fopenmp=libomp 

IIUC, they should be set to the same as when building with clang-cl:

-- OpenMP C libraries:      C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.37.32822/lib/x64/libomp.lib 
-- OpenMP C include:         
-- OpenMP C flags:          -Xclang -fopenmp 

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Feb 6, 2024

The following change to the file provided by CMake fixes the issue for me locally:

diff --git a/Modules/FindOpenMP.cmake b/Modules/FindOpenMP.cmake
index 69099f7620..a311b915dd 100644
--- a/Modules/FindOpenMP.cmake
+++ b/Modules/FindOpenMP.cmake
@@ -224,7 +224,8 @@ function(_OPENMP_GET_FLAGS LANG FLAG_MODE OPENMP_FLAG_VAR OPENMP_LIB_NAMES_VAR)
       OUTPUT_VARIABLE OpenMP_TRY_COMPILE_OUTPUT
     )
 
-    if(OpenMP_COMPILE_RESULT_${FLAG_MODE}_${OPENMP_PLAIN_FLAG})
+    if(OpenMP_COMPILE_RESULT_${FLAG_MODE}_${OPENMP_PLAIN_FLAG} AND
+       NOT CMAKE_${LANG}_SIMULATE_ID STREQUAL "MSVC")
       set("${OPENMP_FLAG_VAR}" "${OPENMP_FLAG}" PARENT_SCOPE)
 
       if(CMAKE_${LANG}_VERBOSE_FLAG)

I don't know if this is the correct change though or if this might break something else.

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Feb 6, 2024

#761 would probably fix the build error where it is failing currently.

This is *not* how this should be fixed. Just a test if the other
platforms still work with this change.
@mmuetzel
Copy link
Contributor Author

mmuetzel commented Feb 8, 2024

With the last experiment, the OpenMP libraries are detected correctly also for the runner that uses clang targeting MSVC:
https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/7804679990/job/21287194730#step:15:152

-- OpenMP C libraries:      C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.37.32822/lib/x64/libomp.lib 
-- OpenMP C include:         
-- OpenMP C flags:          -fopenmp=libomp 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant