Re: [PATCH 2/2] threads: Use the right CMake check to decide whether to build the library.

[Raphael Kubo da Costa <rakuco@xxxxxxxxxxx>]

Hi,

Andreas Schneider <asn@xxxxxxxxxxxxxx> writes:
>> -if (CMAKE_HAVE_THREADS_LIBRARY)
>> +if (CMAKE_USE_PTHREADS_INIT)
>>      add_subdirectory(threads)
>> -endif (CMAKE_HAVE_THREADS_LIBRARY)
>> +endif (CMAKE_USE_PTHREADS_INIT)
>
> I think this should be
>
> if (CMAKE_HAVE_THREADS_LIBRARY OR CMAKE_USE_PTHREADS_INIT)
>
> or will break other threading libraries than pthread

The problem with this form is that CMAKE_HAVE_THREADS_LIBRARY is a
variable that's internal to CMake's FindThreads.cmake (it's not
documented as being for public consumption) and may even go away in the
future. Additionally, the way FindThreads.cmake works
CMAKE_HAVE_THREADS_LIBRARY is set in a subset of the cases where
CMAKE_USE_PTHREADS_INIT is set, so checking for both isn't more
inclusive.

How about just checking for Threads_FOUND? It has its own drawbacks,
though, see below:

> We currently only have a pthread backend here, but we would like to encourage
> other to contribute. You should not assume that we have support only pthread
> cause this might not be the case in future :)

This has a problem of its own mentioned in the commit message:

>> (this was already the case before, since if CMAKE_USE_PTHREADS_INIT
>> was false add_library() would not build any source file at all).

If we end up building threads/ when we have no pthreads, we will
essentially call add_library(ssh_threads) and CMake will fail because we
are not passing any source files to it.

So we can either make the check more inclusive with Threads_FOUND and
let the build fail at configure-time if the system does not have/use
pthreads or maintain the stricter check I proposed at the expense of
having to change it when another backend is written. Any preference? :-)