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

[Andreas Schneider <asn@xxxxxxxxxxxxxx>]

On Thursday 16 January 2014 19:39:48 Raphael Kubo da Costa wrote:
> 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.

asn@magrathea:~/workspace/projects/cmocka> cmake --help-module FindThreads
cmake version 2.8.11.2
  FindThreads
       This module determines the thread library of the system.

       The following variables are set

         CMAKE_THREAD_LIBS_INIT     - the thread library

So we should use CMAKE_THREAD_LIBS_INIT.

> 
> 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.

The add_subdirectory(threads) should be called if any thread library has been 
found and the threads/CMakeLists.txt should then do the right thight for the 
right library. So yes, currently we have only pthreads but we don't have to 
prevent it from entering.

> 
> 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? :-)

I don't want a stricter check in threads/CMakeLists.txt but one which is not 
only for pthreads in src/CMakeLists.txt. Just that in future we do not have to 
touch it :)



	-- andreas


-- 
Andreas Schneider                   GPG-ID: CC014E3D
www.cryptomilk.org                asn@xxxxxxxxxxxxxx