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

SuiteSparse_config: Check if linking to librt is necessary #726

Merged

Conversation

mmuetzel
Copy link
Contributor

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
Copy link
Owner

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?

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 also helps to avoid
issues when linking to the shared library with `--as-needed`.
…time

Use result of feature test to decide whether `clock_gettime` can be used.
@mmuetzel
Copy link
Contributor Author

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

I'm currently not aware of other outstanding changes.

@DrTimothyAldenDavis DrTimothyAldenDavis merged commit 3b2647f into DrTimothyAldenDavis:dev2 Jan 14, 2024
24 checks passed
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.

2 participants