{"thread":{"id":"30962","subject":"[PATCH] Change configure to check if pthreads are usable without any extra flags","startedAt":"2012-07-05T23:03:06Z","lastAt":"2012-07-10T09:52:53Z","messageCount":9,"participants":["Max Horn","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"194683","messageId":"1341529386-11589-1-git-send-email-max@quendi.de","threadId":"30962","inReplyTo":null,"subject":"[PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2012-07-05T23:03:06Z","receivedAt":"2012-07-05T23:03:06Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"The configure script checks whether certain flags / libraries are\nrequired to use pthreads. But so far it did not consider the possibility\nthat no extra compiler flags are needed (as is the case on Mac OS X). As\na result, configure would always add \"-mt\" to the list of flags. This in\nturn triggered a warning in clang about an unknown argument.\nTo solve this, we now first check if pthreads work without extra flags.\n\nSigned-off-by: Max Horn \n---\n configure.ac | 2 +-\n 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)\n\ndiff --git a/configure.ac b/configure.ac\nindex 4e9012f..d767ef3 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -1002,7 +1002,7 @@ if test -n \"$USER_NOPTHREAD\"; then\n # -D_REENTRANT' or some such.\n elif test -z \"$PTHREAD_CFLAGS\"; then\n threads_found=no\n- for opt in -mt -pthread -lpthread; do\n+ for opt in \"\" -mt -pthread -lpthread; do\n old_CFLAGS=\"$CFLAGS\"\n CFLAGS=\"$opt $CFLAGS\"\n AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])\n-- \n1.7.11.1.145.g4722b29.dirty\n"},{"id":"194813","messageId":"7vk3ydkmzq.fsf@alter.siamese.dyndns.org","threadId":"30962","inReplyTo":"1341529386-11589-1-git-send-email-max@quendi.de","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T14:50:01Z","receivedAt":"2012-07-09T14:50:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn writes:\n\n> The configure script checks whether certain flags / libraries are\n> required to use pthreads. But so far it did not consider the possibility\n> that no extra compiler flags are needed (as is the case on Mac OS X). As\n> a result, configure would always add \"-mt\" to the list of flags. This in\n> turn triggered a warning in clang about an unknown argument.\n> To solve this, we now first check if pthreads work without extra flags.\n>\n> Signed-off-by: Max Horn \n> ---\n> configure.ac | 2 +-\n> 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)\n>\n> diff --git a/configure.ac b/configure.ac\n> index 4e9012f..d767ef3 100644\n> --- a/configure.ac\n> +++ b/configure.ac\n> @@ -1002,7 +1002,7 @@ if test -n \"$USER_NOPTHREAD\"; then\n> # -D_REENTRANT' or some such.\n> elif test -z \"$PTHREAD_CFLAGS\"; then\n> threads_found=no\n> - for opt in -mt -pthread -lpthread; do\n> + for opt in \"\" -mt -pthread -lpthread; do\n\nHmph. Would it work to append the new empty string at the end of\nthe existing list, as opposed to prepending it? I'd prefer a\nsolution that is order independent, or if the change is order\ndependent, then a comment to warn others from changing it later.\n\n> old_CFLAGS=\"$CFLAGS\"\n> CFLAGS=\"$opt $CFLAGS\"\n> AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])\n\nPerhaps \"for linking with POSIX Threads\" would make it clearer, as\nCFLAGS (rather, PTHREAD_CFLAGS) has been checked earlier separately.\n"},{"id":"194817","messageId":"C56B4151-8912-4B3A-8A97-E769A878AE68@quendi.de","threadId":"30962","inReplyTo":"7vk3ydkmzq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2012-07-09T15:39:26Z","receivedAt":"2012-07-09T15:39:26Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nOn 09.07.2012, at 16:50, Junio C Hamano wrote:\n\n> Max Horn writes:\n> \n>> The configure script checks whether certain flags / libraries are\n>> required to use pthreads. But so far it did not consider the possibility\n>> that no extra compiler flags are needed (as is the case on Mac OS X). As\n>> a result, configure would always add \"-mt\" to the list of flags. This in\n>> turn triggered a warning in clang about an unknown argument.\n>> To solve this, we now first check if pthreads work without extra flags.\n>> \n>> Signed-off-by: Max Horn \n>> ---\n>> configure.ac | 2 +-\n>> 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)\n>> \n>> diff --git a/configure.ac b/configure.ac\n>> index 4e9012f..d767ef3 100644\n>> --- a/configure.ac\n>> +++ b/configure.ac\n>> @@ -1002,7 +1002,7 @@ if test -n \"$USER_NOPTHREAD\"; then\n>> # -D_REENTRANT' or some such.\n>> elif test -z \"$PTHREAD_CFLAGS\"; then\n>> threads_found=no\n>> - for opt in -mt -pthread -lpthread; do\n>> + for opt in \"\" -mt -pthread -lpthread; do\n> \n> Hmph. Would it work to append the new empty string at the end of\n> the existing list, as opposed to prepending it?\n\nNo, 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. \n\n> I'd prefer a\n> solution that is order independent, or if the change is order\n> dependent, then a comment to warn others from changing it later.\n\nWell, 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 ;).\n\nRegardless 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.)\n\n\n\n> \n>> old_CFLAGS=\"$CFLAGS\"\n>> CFLAGS=\"$opt $CFLAGS\"\n>> AC_MSG_CHECKING([Checking for POSIX Threads with '$opt'])\n> \n> Perhaps \"for linking with POSIX Threads\" would make it clearer, as\n> CFLAGS (rather, PTHREAD_CFLAGS) has been checked earlier separately.\n\nWell, talking about clarity, looking at line 188 it says\n AC_MSG_NOTICE([Will try -pthread then -lpthread to enable POSIX Threads])\n\nwhich 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. \n\nBeyond 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 , which covers many more systems out of the box. \n\nBut 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 ... ;).\n\n\nCheers,\nMax\n"},{"id":"194820","messageId":"7vy5mskewg.fsf@alter.siamese.dyndns.org","threadId":"30962","inReplyTo":"C56B4151-8912-4B3A-8A97-E769A878AE68@quendi.de","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T17:44:47Z","receivedAt":"2012-07-09T17:44:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn writes:\n\n>>> diff --git a/configure.ac b/configure.ac\n>>> index 4e9012f..d767ef3 100644\n>>> --- a/configure.ac\n>>> +++ b/configure.ac\n>>> @@ -1002,7 +1002,7 @@ if test -n \"$USER_NOPTHREAD\"; then\n>>> # -D_REENTRANT' or some such.\n>>> elif test -z \"$PTHREAD_CFLAGS\"; then\n>>> threads_found=no\n>>> - for opt in -mt -pthread -lpthread; do\n>>> + for opt in \"\" -mt -pthread -lpthread; do\n>> \n>> Hmph. Would it work to append the new empty string at the end of\n>> the existing list, as opposed to prepending it?\n>\n> 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. \n\nIf the test in that \"for opt ...; do\" considers the linking \"work\",\nwhy do you even want to tweak it, and instead let \"-mt\" be passed?\n\nIf the warning troubles you, would it be feasible for the purpose of\nthe check to tweak the definition of \"works\" used in the loop so that\nit considers the warning as \"not working\"?\n"},{"id":"194823","messageId":"35825CE5-4411-4C75-830A-6D704ACA1410@quendi.de","threadId":"30962","inReplyTo":"7vy5mskewg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2012-07-09T18:31:04Z","receivedAt":"2012-07-09T18:31:04Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nOn 09.07.2012, at 19:44, Junio C Hamano wrote:\n\n> Max Horn writes:\n> \n>>>> diff --git a/configure.ac b/configure.ac\n>>>> index 4e9012f..d767ef3 100644\n>>>> --- a/configure.ac\n>>>> +++ b/configure.ac\n>>>> @@ -1002,7 +1002,7 @@ if test -n \"$USER_NOPTHREAD\"; then\n>>>> # -D_REENTRANT' or some such.\n>>>> elif test -z \"$PTHREAD_CFLAGS\"; then\n>>>> threads_found=no\n>>>> - for opt in -mt -pthread -lpthread; do\n>>>> + for opt in \"\" -mt -pthread -lpthread; do\n>>> \n>>> Hmph. Would it work to append the new empty string at the end of\n>>> the existing list, as opposed to prepending it?\n>> \n>> 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. \n> \n> If the test in that \"for opt ...; do\" considers the linking \"work\",\n> why do you even want to tweak it, and instead let \"-mt\" be passed?\n> \n> If the warning troubles you,\n\nIt does trouble me, as it means that every single compiled file now triggers a warning, making it impossible to use -Werror, or alternatively much harder to spot legit warning. In fact, I am surprised that it doesn't seem to trouble *you* :-).\n\nMoreover, while compiling and link \"works\" with \"-mt\" present on my system, it is very easy to imagine a setup where this test does break: namely on a system where pthreads are in the C lib, but the C compiler treats unknown compiler options as an error, not a warning. Then none of the three options the test checks right now would work\n\nAs I see it, an autoconf feature test like this should be checking \"which linker flags are *required* in order to use pthreads?\". But what it currently does is to check \"which linker flags do not prevent us from (and possibly help us to) use pthreads?\" and so they come up with flags that are not necessary, and in fact trigger warnings.\n\n\n> would it be feasible for the purpose of\n> the check to tweak the definition of \"works\" used in the loop so that\n> it considers the warning as \"not working\"?\n\nThat would be possible, and probably a good idea. But it is also completely orthogonal to my patch. Indeed, if done without my patch, then as a result, pthreads would not be detected anymore on Mac OS X, since none of the linker flags it tries would work -- as it doesn't try what happens when no linker flags are passed.\n\n\n\nCheers,\nMax"},{"id":"194826","messageId":"7vtxxgkac0.fsf@alter.siamese.dyndns.org","threadId":"30962","inReplyTo":"35825CE5-4411-4C75-830A-6D704ACA1410@quendi.de","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T19:23:27Z","receivedAt":"2012-07-09T19:23:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn writes:\n\n>> would it be feasible for the purpose of\n>> the check to tweak the definition of \"works\" used in the loop so that\n>> it considers the warning as \"not working\"?\n>\n> That would be possible, and probably a good idea. But it is also\n> completely orthogonal to my patch. Indeed, if done without my\n> patch,...\n\nNo, I was suggesting it as a possible way to make the addition of \"\"\norder independent (which you said is impossible in your initial\nreply).\n"},{"id":"194832","messageId":"84D9664E-3249-4D80-980A-C01B02DF5E43@quendi.de","threadId":"30962","inReplyTo":"7vtxxgkac0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2012-07-09T21:44:03Z","receivedAt":"2012-07-09T21:44:03Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nOn 09.07.2012, at 21:23, Junio C Hamano wrote:\n\n> Max Horn writes:\n> \n>>> would it be feasible for the purpose of\n>>> the check to tweak the definition of \"works\" used in the loop so that\n>>> it considers the warning as \"not working\"?\n>> \n>> That would be possible, and probably a good idea. But it is also\n>> completely orthogonal to my patch. Indeed, if done without my\n>> patch,...\n> \n> No, I was suggesting it as a possible way to make the addition of \"\"\n> order independent (which you said is impossible in your initial\n> reply).\n\nThis would make things \"order independent\" for the specific subset of pthread implementations git supports right now. But there are systems where e.g. both -lpthreads and -lpthread work (link correctly, produce no warnings), but only one provides a POSIX compliant pthread implementation. For such systems, order will play a role, no matter what. Granted, git does not yet support such systems (with regards to pthreads, at least) at all. \n\nBut all in all, I don't understand why this order independence seems to be so important? To me it seems merely an illusion anyway, not worth the extra effort.\n"},{"id":"194834","messageId":"7vd344k1ag.fsf@alter.siamese.dyndns.org","threadId":"30962","inReplyTo":"84D9664E-3249-4D80-980A-C01B02DF5E43@quendi.de","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T22:38:47Z","receivedAt":"2012-07-09T22:38:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn writes:\n\n> But all in all, I don't understand why this order independence\n> seems to be so important?\n\nNot so important as long as it is made clear for later people to\npatch that part of the code. I just wanted to make sure that\nsomebody thought hard enough to judge that it is not worth the\neffort to make it independent of the orders with a justification\nbetter than that simply we are too lazy to do so ;-).\n"},{"id":"194863","messageId":"98BD2F91-9204-40CE-AE0A-4F771AFEA984@quendi.de","threadId":"30962","inReplyTo":"7vd344k1ag.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Change configure to check if pthreads are usable without any extra flags","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2012-07-10T09:52:53Z","receivedAt":"2012-07-10T09:52:53Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nOn 10.07.2012, at 00:38, Junio C Hamano wrote:\n\n> Max Horn writes:\n> \n>> But all in all, I don't understand why this order independence\n>> seems to be so important?\n> \n> Not so important as long as it is made clear for later people to\n> patch that part of the code. I just wanted to make sure that\n> somebody thought hard enough to judge that it is not worth the\n> effort to make it independent of the orders with a justification\n> better than that simply we are too lazy to do so ;-).\n\nUnderstood ;). Well, in any case, I'll create a new patch based on your feedback. (And also for the revisions.txt docs, from that other thread), but this week I am extra busy at work, so it might take me a day or two longer, sorry.\n\nMax\n"}]}