DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.14k stars 258 forks source link

SuiteSparse_config: Check if linking to librt is necessary #726

Closed mmuetzel closed 7 months ago

mmuetzel commented 7 months ago

On some platform, it is necessary to link to librt when using clock_gettime. Add configure checks to detect if the function is available and whether it needs linking with librt.

That facilitates linking statically to libsuitesparseconfig using the CMake import targets or the pkg-config files. It could also help to avoid issues when linking to the shared library with --as-needed.

The second commit uses the result of the new configure checks to decide whether clock_gettime can be used in the code. That change allows using that function also for non-POSIX targets where it is available (e.g., MinGW).

DrTimothyAldenDavis commented 7 months ago

Looks good, but with one minor downside that's easy to fix. It has a #cmakedefine in SuiteSparse_config.h, and then it looks like the repo is not clean after cmake has configured SuiteSparse_config, if SuiteSparse_config is built with OpenMP or not.

It would give a more stable SuiteSparse_config.h if this part was done all the time, regardless of whether or not OpenMP is in use. In other words, changing this:

# check for librt in case of fallback to "clock_gettime"
if ( NOT SUITESPARSE_CONFIG_USE_OPENMP )
    # librt
    include ( CheckSymbolExists )
    check_symbol_exists ( clock_gettime "time.h" NO_RT )
    if ( NO_RT )
        message ( STATUS "Using clock_gettime without librt" )
        set ( SUITESPARSE_HAVE_CLOCK_GETTIME ON )
    else ( )
        # check if we need to link to librt for that function
        set ( _orig_CMAKE_REQUIRED_LIBRARIES ${CMAKE_REQUIRED_LIBRARIES} )
        list ( APPEND CMAKE_REQUIRED_LIBRARIES "rt" )
        check_symbol_exists ( clock_gettime "time.h" WITH_RT )
        set ( CMAKE_REQUIRED_LIBRARIES ${_orig_CMAKE_REQUIRED_LIBRARIES} )
        if ( WITH_RT )
            message ( STATUS "Using clock_gettime with librt" )
            set ( SUITESPARSE_HAVE_CLOCK_GETTIME ON )
        else ( )
            message ( STATUS "No OpenMP and no clock_gettime available. Timing functions won't work." )
        endif ( )
    endif ( )
endif ( )

by removing the outer if / endif:

if ( NOT SUITESPARSE_CONFIG_USE_OPENMP )

and just always compute SUITESPARSE_HAVE_CLOCK_GETTIME, regardless of whether OpenMP is used or not.

That way, the default SuiteSparse_config.h would be more stable and more often not look like the repo is changed after cmake is done configuring it.

The message:

message ( STATUS "No OpenMP and no clock_gettime available. Timing functions won't work." ) could be put outside after this block of code, in its own test:

if ( NOT SUITESPARSE_CONFIG_USE_OPENMP AND NOT SUITESPARSE_HAVE_CLOCK_GETTIME)
    message ( STATUS "No OpenMP and no clock_gettime available. Timing functions won't work." )
endif ( )

Does that sound OK to you?

I'd like to release a stable 7.5.1 soon, to fix my awful mistake with the SUITESPARSE__VERCODE macro. I can add this fix too. Are there others I should add to 7.5.1?

mmuetzel commented 7 months ago

Thanks for the review. That sounds reasonable. 👍 I made your proposed changes.

I'm currently not aware of other outstanding changes.