{"thread":{"id":"42273","subject":"[PATCH] Windows: only add a no-op pthread_sigmask() when needed","startedAt":"2016-05-10T13:00:35Z","lastAt":"2016-05-11T21:01:17Z","messageCount":5,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"286051","messageId":"26c2fb5560246fc7f980da24a239edc333864527.1462885167.git.johannes.schindelin@gmx.de","threadId":"42273","inReplyTo":null,"subject":"[PATCH] Windows: only add a no-op pthread_sigmask() when needed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-10T13:00:35Z","receivedAt":"2016-05-10T13:00:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In f924b52 (Windows: add pthread_sigmask() that does nothing,\n2016-05-01), we introduced a no-op for Windows. However, this breaks\nbuilding Git in Git for Windows' SDK because pthread_sigmask() is\nalready a no-op there, #define'd in the pthread_signal.h header in\n/mingw64/x86_64-w64-mingw32/include/.\n\nLet's guard the definition of pthread_sigmask() in #ifndef...#endif to\nmake the code compile both with modern MinGW-w64 as well as with the\npreviously common MinGW headers.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tThis patch is obviously based on 'next' (because 'master' does not\n\thave the referenced commit yet).\n\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-sigmask-v1\n compat/win32/pthread.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex d336451..8df702c 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -104,9 +104,11 @@ static inline void *pthread_getspecific(pthread_key_t key)\n \treturn TlsGetValue(key);\n }\n \n+#ifndef pthread_sigmask\n static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)\n {\n \treturn 0;\n }\n+#endif\n \n #endif /* PTHREAD_H */\n-- \n2.8.2.463.g99156ee\n"},{"id":"286088","messageId":"xmqqvb2lpzij.fsf@gitster.mtv.corp.google.com","threadId":"42273","inReplyTo":"26c2fb5560246fc7f980da24a239edc333864527.1462885167.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Windows: only add a no-op pthread_sigmask() when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T17:57:08Z","receivedAt":"2016-05-10T17:57:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In f924b52 (Windows: add pthread_sigmask() that does nothing,\n> 2016-05-01), we introduced a no-op for Windows. However, this breaks\n> building Git in Git for Windows' SDK because pthread_sigmask() is\n> already a no-op there, #define'd in the pthread_signal.h header in\n> /mingw64/x86_64-w64-mingw32/include/.\n>\n> Let's guard the definition of pthread_sigmask() in #ifndef...#endif to\n> make the code compile both with modern MinGW-w64 as well as with the\n> previously common MinGW headers.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n>\n> \tThis patch is obviously based on 'next' (because 'master' does not\n> \thave the referenced commit yet).\n\nOne thing that makes me wonder is what would happen when\n/mingw64/x86_64-w64-mingw32/include/pthread_signal.h changes its\nmind and uses \"static inline\" instead of \"#define\". How much\ncontrol does Git-for-Windows folks have over that header file?\n\nAlso, could you explain \"However\" part a bit? Obviously in _some_\nenvironment other than \"Git for Windows' SDK\", the previous patch\nhelped you compile. And you need to #ifdef it out, because \"Git for\nWindows' SDK\" already has its own support. What is that other\nenvironment that lacks the support (hence we need our own \"static\ninline\") and is there a way to tell \"Git for Windows' SDK\" from that\nother environment?\n\nWhat I am trying to get at is:\n\n (1) If the answer is \"we have total control\", then I am perfectly\n fine with using \"#ifdef pthread_sigmask\" here.\n\n (2) If not, i.e. \"they can change the implementation to 'static\n inline' themselves\", then I do not think it is prudent to use\n \"#ifndef pthread_sigmask\" as the conditional here--using a\n symbol that lets you check for that \"other\" environment and\n doing \"#ifdef THAT_OTHER_ONE_THAT_LACKS_SIGMASK\" would be\n safer.\n\nAlso is https://lists.gnu.org/archive/html/bug-gnulib/2015-04/msg00068.html\nrelevant? Does /mingw64/x86_64-w64-mingw32/include/ implement \"macro only\nwithout function\"?\n\n> Published-As: https://github.com/dscho/git/releases/tag/mingw-sigmask-v1\n> compat/win32/pthread.h | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\n> index d336451..8df702c 100644\n> --- a/compat/win32/pthread.h\n> +++ b/compat/win32/pthread.h\n> @@ -104,9 +104,11 @@ static inline void *pthread_getspecific(pthread_key_t key)\n> \treturn TlsGetValue(key);\n> }\n> \n> +#ifndef pthread_sigmask\n> static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)\n> {\n> \treturn 0;\n> }\n> +#endif\n> \n> #endif /* PTHREAD_H */\n"},{"id":"286270","messageId":"alpine.DEB.2.20.1605111655350.4092@virtualbox","threadId":"42273","inReplyTo":"xmqqvb2lpzij.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Windows: only add a no-op pthread_sigmask() when needed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-11T15:06:26Z","receivedAt":"2016-05-11T15:06:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 10 May 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > In f924b52 (Windows: add pthread_sigmask() that does nothing,\n> > 2016-05-01), we introduced a no-op for Windows. However, this breaks\n> > building Git in Git for Windows' SDK because pthread_sigmask() is\n> > already a no-op there, #define'd in the pthread_signal.h header in\n> > /mingw64/x86_64-w64-mingw32/include/.\n> >\n> > Let's guard the definition of pthread_sigmask() in #ifndef...#endif to\n> > make the code compile both with modern MinGW-w64 as well as with the\n> > previously common MinGW headers.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> >\n> > \tThis patch is obviously based on 'next' (because 'master' does not\n> > \thave the referenced commit yet).\n> \n> One thing that makes me wonder is what would happen when\n> /mingw64/x86_64-w64-mingw32/include/pthread_signal.h changes its\n> mind and uses \"static inline\" instead of \"#define\". How much\n> control does Git-for-Windows folks have over that header file?\n\nWe have no control over this, it is defined by one of the MSYS2 packages.\n(I think those headers come directly from the MinGW-w64 GCC project.)\n\nI can think of two different ways to resolve this, would you please\nindicate your preference?\n\n1. fix the non-POSIX-ness:\n\n\t#ifdef pthread_sigmask\n\t#undef pthread_sigmask\n\t#endif\n\n or even shorter:\n\n\t#undef pthread_sigmask\n\n2. just go with whatever MSYS2 provides:\n\n\t#ifndef __MINGW64_VERSION_MAJOR\n\t[... define the no-op ...]\n\t#endif\n\n> Also, could you explain \"However\" part a bit? Obviously in _some_\n> environment other than \"Git for Windows' SDK\", the previous patch\n> helped you compile.\n\nYes, Hannes uses his own MSys environment. (Which is different from\neverything *I* have, I think, it's not even msysGit.)\n\n> What I am trying to get at is:\n> \n> (1) If the answer is \"we have total control\", then I am perfectly\n> fine with using \"#ifdef pthread_sigmask\" here.\n> \n> (2) If not, i.e. \"they can change the implementation to 'static\n> inline' themselves\", then I do not think it is prudent to use\n> \"#ifndef pthread_sigmask\" as the conditional here--using a\n> symbol that lets you check for that \"other\" environment and\n> doing \"#ifdef THAT_OTHER_ONE_THAT_LACKS_SIGMASK\" would be\n> safer.\n\nSo I guess that you're preferring my 2. above. Going on that assumption, I\nwill send out another iteration.\n\n> Also is\n> https://lists.gnu.org/archive/html/bug-gnulib/2015-04/msg00068.html\n> relevant? Does /mingw64/x86_64-w64-mingw32/include/ implement \"macro\n> only without function\"?\n\nYes, it has that problem. Do we care, really?\n\nCiao,\nDscho\n"},{"id":"286271","messageId":"446e1cd65673e4bcf1aa8ec6e05cd7b361762119.1462979365.git.johannes.schindelin@gmx.de","threadId":"42273","inReplyTo":"26c2fb5560246fc7f980da24a239edc333864527.1462885167.git.johannes.schindelin@gmx.de","subject":"[PATCH v2] Windows: only add a no-op pthread_sigmask() when needed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-11T15:10:30Z","receivedAt":"2016-05-11T15:10:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In f924b52 (Windows: add pthread_sigmask() that does nothing,\n2016-05-01), we introduced a no-op for Windows. However, this breaks\nbuilding Git in Git for Windows' SDK because pthread_sigmask() is\nalready a no-op there, #define'd in the pthread_signal.h header in\n/mingw64/x86_64-w64-mingw32/include/.\n\nLet's wrap the definition of pthread_sigmask() in a guard that skips\nit when compiling with MinGW-w64' headers.\n\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-sigmask-v2\n compat/win32/pthread.h | 2 ++\n 1 file changed, 2 insertions(+)\nInterdiff vs v1:\n\n diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\n index 8df702c..1c16408 100644\n --- a/compat/win32/pthread.h\n +++ b/compat/win32/pthread.h\n @@ -104,7 +104,7 @@ static inline void *pthread_getspecific(pthread_key_t key)\n \treturn TlsGetValue(key);\n }\n \n -#ifndef pthread_sigmask\n +#ifndef __MINGW64_VERSION_MAJOR\n static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)\n {\n \treturn 0;\n\n\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex d336451..1c16408 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -104,9 +104,11 @@ static inline void *pthread_getspecific(pthread_key_t key)\n \treturn TlsGetValue(key);\n }\n \n+#ifndef __MINGW64_VERSION_MAJOR\n static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)\n {\n \treturn 0;\n }\n+#endif\n \n #endif /* PTHREAD_H */\n-- \n2.8.2.465.gb077790\n"},{"id":"286286","messageId":"xmqqvb2kjoma.fsf@gitster.mtv.corp.google.com","threadId":"42273","inReplyTo":"alpine.DEB.2.20.1605111655350.4092@virtualbox","subject":"Re: [PATCH] Windows: only add a no-op pthread_sigmask() when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-11T21:01:17Z","receivedAt":"2016-05-11T21:01:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> So I guess that you're preferring my 2. above. Going on that assumption, I\n> will send out another iteration.\n\nOK.\n\n>> Also is\n>> https://lists.gnu.org/archive/html/bug-gnulib/2015-04/msg00068.html\n>> relevant? Does /mingw64/x86_64-w64-mingw32/include/ implement \"macro\n>> only without function\"?\n>\n> Yes, it has that problem. Do we care, really?\n\nNot really. I was curious how actively this area is evolving;\nknowing that would help me judge pros-and-cons between your 1 & 2\nabove.\n"}]}