Re: [PATCH] simple-ipc: correct ifdefs when NO_PTHREADS is defined

"Jeff Hostetler via GitGitGadget" [off-list ref] 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.

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

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.

On 5/18/21 8:48 PM, Junio C Hamano wrote:

"Jeff Hostetler via GitGitGadget" [off-list ref] writes:

quoted

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

quoted

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?

quoted

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