simple-ipc: correct ifdefs when NO_PTHREADS is defined by jeffhostetler · Pull Request #955 · gitgitgadget/git

jeffhostetler · 2021-05-18T15:11:23Z

Here is V3 of this fixup. I've added fixups for the CMake builds.

Jeff

cc: Jeff Hostetler git@jeffhostetler.com

jeffhostetler · 2021-05-18T15:35:30Z

/submit

gitgitgadget · 2021-05-18T15:36:47Z

Submitted as pull.955.git.1621352192238.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v1

To fetch this version to local tag pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v1

gitgitgadget · 2021-05-19T00:50:37Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +# Simple-ipc requires threads and platform-specific IPC support.
> +# (We group all Unix variants in the top-level else because Windows
> +# also defines NO_UNIX_SOCKETS.)
>  ifdef USE_WIN32_IPC
> +	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
>  	LIB_OBJS += compat/simple-ipc/ipc-shared.o
>  	LIB_OBJS += compat/simple-ipc/ipc-win32.o
> +else
> +ifndef NO_PTHREADS
> +ifndef NO_UNIX_SOCKETS
> +	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
> +	LIB_OBJS += compat/simple-ipc/ipc-shared.o
> +	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
> +endif
> +endif

OK, so "!defined(NO_PTHREADS) && !defined(NO_UNIX_SOCKETS)" is the
requirement for SIMPLE_IPC unless you are on Windows.

> diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
> index 38689b278df3..c23b17973983 100644
> --- a/compat/simple-ipc/ipc-unix-socket.c
> +++ b/compat/simple-ipc/ipc-unix-socket.c
> @@ -6,10 +6,16 @@
>  #include "unix-socket.h"
>  #include "unix-stream-server.h"
>  
> +#ifdef SUPPORTS_SIMPLE_IPC
> +
>  #ifdef NO_UNIX_SOCKETS
>  #error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets
>  #endif
>  
> +#ifdef NO_PTHREADS
> +#error compat/simple-ipc/ipc-unix-socket.c requires pthreads
> +#endif
> +

Do we want to duplicate the requirement here and risk them drifting
apart?

> @@ -997,3 +1003,5 @@ void ipc_server_free(struct ipc_server_data *server_data)
>  	free(server_data->fifo_fds);
>  	free(server_data);
>  }
> +
> +#endif /* SUPPORTS_SIMPLE_IPC */

If anything, I do not think we want a huge #ifdef/#endif around the
whole file.  Feeding this source to your compiler when these three C
proprocessor macros do not agree with its use is an error, so perhaps
lose all of these #ifdef/#endif around the three macros and refer human
readers to the top-level Makefile with a comment, e.g.

/*
 * Non Windows platforms need !NO_UNIX_SOCKETS and !NO_PTHREADS
 * to compile and use this file.  See the top-level Makefile.
 */

if we really wanted to have a way to help builders identify the
reason why their build is failing.

gitgitgadget · 2021-05-19T10:27:51Z

This branch is now known as jh/simple-ipc-sans-pthread.

gitgitgadget · 2021-05-19T10:27:52Z

This patch series was integrated into seen via git@a76cfe5.

gitgitgadget · 2021-05-19T14:33:02Z

On the Git mailing list, Jeff Hostetler wrote (reply to this):



On 5/18/21 8:48 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +# Simple-ipc requires threads and platform-specific IPC support.
>> +# (We group all Unix variants in the top-level else because Windows
>> +# also defines NO_UNIX_SOCKETS.)
>>   ifdef USE_WIN32_IPC
>> +	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
>>   	LIB_OBJS += compat/simple-ipc/ipc-shared.o
>>   	LIB_OBJS += compat/simple-ipc/ipc-win32.o
>> +else
>> +ifndef NO_PTHREADS
>> +ifndef NO_UNIX_SOCKETS
>> +	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
>> +	LIB_OBJS += compat/simple-ipc/ipc-shared.o
>> +	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
>> +endif
>> +endif
> 
> OK, so "!defined(NO_PTHREADS) && !defined(NO_UNIX_SOCKETS)" is the
> requirement for SIMPLE_IPC unless you are on Windows.
> 
>> diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
>> index 38689b278df3..c23b17973983 100644
>> --- a/compat/simple-ipc/ipc-unix-socket.c
>> +++ b/compat/simple-ipc/ipc-unix-socket.c
>> @@ -6,10 +6,16 @@
>>   #include "unix-socket.h"
>>   #include "unix-stream-server.h"
>>   
>> +#ifdef SUPPORTS_SIMPLE_IPC
>> +
>>   #ifdef NO_UNIX_SOCKETS
>>   #error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets
>>   #endif
>>   
>> +#ifdef NO_PTHREADS
>> +#error compat/simple-ipc/ipc-unix-socket.c requires pthreads
>> +#endif
>> +
> 
> Do we want to duplicate the requirement here and risk them drifting
> apart?
> 
>> @@ -997,3 +1003,5 @@ void ipc_server_free(struct ipc_server_data *server_data)
>>   	free(server_data->fifo_fds);
>>   	free(server_data);
>>   }
>> +
>> +#endif /* SUPPORTS_SIMPLE_IPC */
> 
> If anything, I do not think we want a huge #ifdef/#endif around the
> whole file.  Feeding this source to your compiler when these three C
> proprocessor macros do not agree with its use is an error, so perhaps
> lose all of these #ifdef/#endif around the three macros and refer human
> readers to the top-level Makefile with a comment, e.g.
> 
> /*
>   * Non Windows platforms need !NO_UNIX_SOCKETS and !NO_PTHREADS
>   * to compile and use this file.  See the top-level Makefile.
>   */
> 
> if we really wanted to have a way to help builders identify the
> reason why their build is failing.
> 

Would it be better to just have something like the following at the
top of the source files and leave the details to the Makefile:


#ifndef SUPPORTS_SIMPLE_IPC
/*
  * This source file should only be included when Simple IPC
  * is supported.  See the top-level Makefile.
  */
#error SUPPORTS_SIMPLE_IPC not defined
#endif

gitgitgadget · 2021-05-19T14:33:05Z

User Jeff Hostetler <git@jeffhostetler.com> has been added to the cc: list.

gitgitgadget · 2021-05-19T22:06:50Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff Hostetler <git@jeffhostetler.com> writes:

>>>   #ifdef NO_UNIX_SOCKETS
>>>   #error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets
>>>   #endif
>>>   +#ifdef NO_PTHREADS
>>> +#error compat/simple-ipc/ipc-unix-socket.c requires pthreads
>>> +#endif
>>> +
>> Do we want to duplicate the requirement here and risk them drifting
>> apart?
>>  ...
> Would it be better to just have something like the following at the
> top of the source files and leave the details to the Makefile:
>
>
> #ifndef SUPPORTS_SIMPLE_IPC
> /*
>  * This source file should only be included when Simple IPC
>  * is supported.  See the top-level Makefile.
>  */
> #error SUPPORTS_SIMPLE_IPC not defined
> #endif

Yeah, that is a much better message, with even less duplication,
than what I sent.

I do not think #ifndef/#error/#endif adds much value, though.  After
all, the Makefile does not even tell us to feed this file to the
compiler when the C preprocessor macro is not defined, so presumably
whoever hits the #error knows s/he is doing something not supported,
and the point of the new message is to help those who we failed by
leaving the rest of the source file unbuildable even when we defined
the C preprocessor macro in the Makefile (like the mistaken
dependency on pthreads that we missed).

Thanks.

gitgitgadget · 2021-05-20T03:33:28Z

This patch series was integrated into seen via git@14bdd74.

gitgitgadget · 2021-05-20T03:54:49Z

There was a status update in the "New Topics" section about the branch jh/simple-ipc-sans-pthread on the Git mailing list:

The "simple-ipc" did not compile without pthreads support, but the
build procedure was not properly account for it.

Expecting a reroll.

gitgitgadget · 2021-05-20T07:08:16Z

This patch series was integrated into seen via git@607320e.

jeffhostetler · 2021-05-20T14:21:17Z

/submit

gitgitgadget · 2021-05-20T14:22:43Z

Submitted as pull.955.v2.git.1621520547726.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v2

To fetch this version to local tag pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v2

gitgitgadget · 2021-05-20T15:12:37Z

On the Git mailing list, Jeff Hostetler wrote (reply to this):



On 5/20/21 10:22 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Simple IPC always requires threads (in addition to various
> platform-specific IPC support).  Fix the ifdefs in the Makefile
> to define SUPPORTS_SIMPLE_IPC when appropriate.
> 

I got in a hurry this morning and forgot to update the CMake script.
I'll look at that now.

Jeff

gitgitgadget · 2021-05-20T15:12:39Z

User Jeff Hostetler <git@jeffhostetler.com> has been added to the cc: list.

Simple IPC always requires threads (in addition to various platform-specific IPC support). Fix the ifdefs in the Makefile to define SUPPORTS_SIMPLE_IPC when appropriate. Previously, the Unix version of the code would only verify that Unix domain sockets were available. This problem was reported here: https://lore.kernel.org/git/YKN5lXs4AoK%2FJFTO@coredump.intra.peff.net/T/#m08be8f1942ea8a2c36cfee0e51cdf06489fdeafc Reported-by: Randall S. Becker <rsbecker@nexbridge.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>

jeffhostetler · 2021-05-20T18:26:53Z

/submit

gitgitgadget · 2021-05-20T18:28:51Z

Submitted as pull.955.v3.git.1621535291406.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v3

To fetch this version to local tag pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v3

gitgitgadget · 2021-05-20T23:05:17Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  Makefile                            | 22 ++++++++++++++++++++--
>  compat/simple-ipc/ipc-shared.c      | 10 +++++++---
>  compat/simple-ipc/ipc-unix-socket.c |  8 ++++++--
>  compat/simple-ipc/ipc-win32.c       |  8 ++++++--
>  contrib/buildsystems/CMakeLists.txt | 10 +++++++++-
>  simple-ipc.h                        |  4 ----
>  6 files changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 3a2d3c80a81a..ea4c0a77604d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1687,13 +1687,31 @@ ifdef NO_UNIX_SOCKETS
>  else
>  	LIB_OBJS += unix-socket.o
>  	LIB_OBJS += unix-stream-server.o
> -	LIB_OBJS += compat/simple-ipc/ipc-shared.o
> -	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
>  endif
>  
> +# Simple IPC requires threads and platform-specific IPC support.
> +# Only platforms that have both should include these source files
> +# in the build.
> +#
> +# On Windows-based systems, Simple IPC requires threads and Windows
> +# Named Pipes.  These are always available, so Simple IPC support
> +# is optional.

The last part for windows feels funny in that even if they were not
always available, the builder can still choose to compile Simple IPC
support out, hence it is optional (this is true for both Windows and
others).  In other words, "prereqs are always satisified" does not
lead to "hence it is optional".

But let's leave it as-is.  We could rewrite it to "..., so Simple
IPC is always enabled", though.

> +#
> +# On Unix-based systems, Simple IPC requires pthreads and Unix
> +# domain sockets.  So support is only enabled when both are present.

Other than that, looks good to me.

Will queue.

gitgitgadget · 2021-05-21T03:52:17Z

This patch series was integrated into seen via git@75a2135.

gitgitgadget · 2021-05-21T07:51:51Z

There was a status update in the "Cooking" section about the branch jh/simple-ipc-sans-pthread on the Git mailing list:

The "simple-ipc" did not compile without pthreads support, but the
build procedure was not properly account for it.

Will merge to 'next' and then to 'master'.

gitgitgadget · 2021-05-22T09:33:41Z

This patch series was integrated into seen via git@df0afee.

gitgitgadget · 2021-05-22T09:33:42Z

This patch series was integrated into next via git@4332dd2.

gitgitgadget · 2021-05-22T12:21:12Z

This patch series was integrated into seen via git@6aae0e2.

gitgitgadget · 2021-05-22T12:21:13Z

This patch series was integrated into next via git@6aae0e2.

gitgitgadget · 2021-05-22T12:21:14Z

This patch series was integrated into master via git@6aae0e2.

gitgitgadget · 2021-05-22T12:21:16Z

Closed via 6aae0e2.

Users were noticing some cases of infinite loops with the error message: ``` fatal: recursion detected in die handler ``` It's only happening for repos with a post-command hook, but it's not deterministic. I'm not sure what is triggering the problem, but the post-command hook is definitely able to recurse with its existing logic around the `run_post_hook` variable. Move this earlier to avoid a potential double-call. It's a fixup to the introduction of the method, which may cause conflicts with later adjustments to this method still in the branch thicket (but maybe those should be squashed, too). See #955 for the version on 2.54.0.

gitgitgadget Bot added the seen label May 19, 2021

jeffhostetler force-pushed the fixup-simple-ipc-no-pthreads branch from 4adcf35 to 119412f Compare May 20, 2021 14:13

jeffhostetler force-pushed the fixup-simple-ipc-no-pthreads branch from b5127fb to d4f4170 Compare May 20, 2021 17:41

gitgitgadget Bot added the next label May 22, 2021

gitgitgadget Bot added the master label May 22, 2021

gitgitgadget Bot closed this May 22, 2021

jeffhostetler deleted the fixup-simple-ipc-no-pthreads branch June 21, 2023 20:32

Uh oh!

Conversation

jeffhostetler commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffhostetler commented May 18, 2021

Uh oh!

gitgitgadget Bot commented May 18, 2021

Uh oh!

gitgitgadget Bot commented May 19, 2021

Uh oh!

gitgitgadget Bot commented May 19, 2021

Uh oh!

gitgitgadget Bot commented May 19, 2021

Uh oh!

gitgitgadget Bot commented May 19, 2021

Uh oh!

gitgitgadget Bot commented May 19, 2021

Uh oh!

gitgitgadget Bot commented May 19, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

jeffhostetler commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

jeffhostetler commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 20, 2021

Uh oh!

gitgitgadget Bot commented May 21, 2021

Uh oh!

gitgitgadget Bot commented May 21, 2021

Uh oh!

gitgitgadget Bot commented May 22, 2021

Uh oh!

gitgitgadget Bot commented May 22, 2021

Uh oh!

gitgitgadget Bot commented May 22, 2021

Uh oh!

gitgitgadget Bot commented May 22, 2021

Uh oh!

gitgitgadget Bot commented May 22, 2021

Uh oh!

gitgitgadget Bot commented May 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jeffhostetler commented May 18, 2021 •

edited

Loading