Re: [PATCH] Change configure to check if pthreads are usable without any extra flags
On 09.07.2012, at 16:50, Junio C Hamano wrote:
Show 27 quoted lines
> Max Horn <max@quendi.de> writes:
>
>> The configure script checks whether certain flags / libraries are
>> required to use pthreads. But so far it did not consider the possibility
>> that no extra compiler flags are needed (as is the case on Mac OS X). As
>> a result, configure would always add "-mt" to the list of flags. This in
>> turn triggered a warning in clang about an unknown argument.
>> To solve this, we now first check if pthreads work without extra flags.
>>
>> Signed-off-by: Max Horn <max@quendi.de>
>> ---
>> configure.ac | 2 +-
>> 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 4e9012f..d767ef3 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1002,7 +1002,7 @@ if test -n "$USER_NOPTHREAD"; then
>> # -D_REENTRANT' or some such.
>> elif test -z "$PTHREAD_CFLAGS"; then
>> threads_found=no
>> - for opt in -mt -pthread -lpthread; do
>> + for opt in "" -mt -pthread -lpthread; do
>
> Hmph. Would it work to append the new empty string at the end of
> the existing list, as opposed to prepending it?
No, because that loop aborts on the first match that "works". Since no flags are necessary on OS X, but adding "-mt" to the flags "works" in the sense that it does nothing (except triggering a warning about an unknown argument), we need to check the empty string before "-mt" that.
> I'd prefer a
> solution that is order independent, or if the change is order
> dependent, then a comment to warn others from changing it later.
Well, such checks are normally always order dependant, too (looking at many other autoconf tests out there). OK, so we could first test all four possibilities, recording for each whether it works or not. But after doing that, we still need to establish an order, in case more than one "works" according to the linker test. Simply using the order is the easiest way for that and works well in practice. And it avoid unnecessary, time consuming checks ;).
Regardless of all that, of course I can add a comment emphasising that the order here is important. (Although IMHO that should be self-evident for an autoconf test.)
Show 7 quoted lines
>
>> old_CFLAGS="$CFLAGS"
>> CFLAGS="$opt $CFLAGS"
>> AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])
>
> Perhaps "for linking with POSIX Threads" would make it clearer, as
> CFLAGS (rather, PTHREAD_CFLAGS) has been checked earlier separately.
Well, talking about clarity, looking at line 188 it says
AC_MSG_NOTICE([Will try -pthread then -lpthread to enable POSIX Threads])
which is also bad: It is out-of-date (even before my patch, as it doesn't mention -mt), will easily get out of sync with reality again, and in any case contains information that normally shouldn't be printed anyway.
Beyond that, the pthread code checks only for -pthread and -lpthread, thus leaving many systems out. Indeed, it might be worth a thought dropping the custom pthread detection code in git's configure.ac and instead using AX_PTHREAD <http://www.gnu.org/software/autoconf-archive/ax_pthread.html>, which covers many more systems out of the box.
But for now, I really would just want to make this simple trivial fix that avoids tons of pointless warnings when compiling git on Mac OS X 10.7 ... ;).
Cheers, Max
Max Horn