SuiteSparse icon indicating copy to clipboard operation
SuiteSparse copied to clipboard

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

Open mmuetzel opened this issue 1 year ago • 4 comments

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 avatar Feb 06 '24 08:02 mmuetzel

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 avatar Feb 06 '24 10:02 mmuetzel

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 avatar Feb 06 '24 11:02 mmuetzel

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

mmuetzel avatar Feb 06 '24 17:02 mmuetzel

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 

mmuetzel avatar Feb 08 '24 17:02 mmuetzel

Rebased to check if this is fixed in the CMake version ~~included in MSVC 19.40~~ that is used on the latest version of the GitHub runner images.

mmuetzel avatar Jun 06 '24 14:06 mmuetzel

It looks like this is working with the versions of MSVC, Clang, and CMake on the current GitHub runner images: https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/9403462108/job/25906182377?pr=760#step:15:134

-- Found OpenMP_C: -fopenmp=libomp (found version "5.1")
-- Found OpenMP: TRUE (found version "5.1") found components: C
-- SuiteSparse_config has OpenMP: ON
[...]
-- OpenMP C libraries:      C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.40.33807/lib/x64/libomp.lib 
-- OpenMP C include:         
-- OpenMP C flags:          -fopenmp=libomp 

Converted from draft to "ready for review".

mmuetzel avatar Jun 07 '24 06:06 mmuetzel