diff mbox series

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

Message ID pull.955.v3.git.1621535291406.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 4332dd20125dfe425f4f7cfc250e7613606f8e1e
Headers show
Series [v3] simple-ipc: correct ifdefs when NO_PTHREADS is defined | expand

Commit Message

Jeff Hostetler May 20, 2021, 6:28 p.m. UTC 
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.

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>
---
    simple-ipc: correct ifdefs when NO_PTHREADS is defined
    
    Here is V3 of this fixup. I've added fixups for the CMake builds.
    
    Jeff

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-955%2Fjeffhostetler%2Ffixup-simple-ipc-no-pthreads-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-955/jeffhostetler/fixup-simple-ipc-no-pthreads-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/955

Range-diff vs v2:

 1:  119412f52ff5 ! 1:  d4f4170414e3 simple-ipc: correct ifdefs when NO_PTHREADS is defined
     @@ compat/simple-ipc/ipc-win32.c
       
       static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)
      
     + ## contrib/buildsystems/CMakeLists.txt ##
     +@@ contrib/buildsystems/CMakeLists.txt: endif()
     + 
     + if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
     + 	list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-win32.c)
     ++	add_compile_definitions(SUPPORTS_SIMPLE_IPC)
     ++	set(SUPPORTS_SIMPLE_IPC 1)
     + else()
     +-	list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-unix-socket.c)
     ++	# Simple IPC requires both Unix sockets and pthreads on Unix-based systems.
     ++	if(NOT NO_UNIX_SOCKETS AND NOT NO_PTHREADS)
     ++		list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-unix-socket.c)
     ++		add_compile_definitions(SUPPORTS_SIMPLE_IPC)
     ++		set(SUPPORTS_SIMPLE_IPC 1)
     ++	endif()
     + endif()
     + 
     + set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX})
     +@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "X='${EXE_EXTENSION}'\n")
     + file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n")
     + file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n")
     + file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
     ++file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
     + if(WIN32)
     + 	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
     + endif()
     +
       ## simple-ipc.h ##
      @@
        * See Documentation/technical/api-simple-ipc.txt


 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(-)


base-commit: bf949ade81106fbda068c1fdb2c6fd1cb1babe7e

Comments

Junio C Hamano May 20, 2021, 11:01 p.m. UTC | #1 
"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.
diff mbox series

Patch

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.
+#
+# On Unix-based systems, Simple IPC requires pthreads and Unix
+# domain sockets.  So support is only enabled when both are present.
+#
 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
 endif
 
 ifdef NO_ICONV
diff --git a/compat/simple-ipc/ipc-shared.c b/compat/simple-ipc/ipc-shared.c
index 1edec8159532..1b9d359ab681 100644
--- a/compat/simple-ipc/ipc-shared.c
+++ b/compat/simple-ipc/ipc-shared.c
@@ -4,7 +4,13 @@ 
 #include "pkt-line.h"
 #include "thread-utils.h"
 
-#ifdef SUPPORTS_SIMPLE_IPC
+#ifndef SUPPORTS_SIMPLE_IPC
+/*
+ * This source file should only be compiled when Simple IPC is supported.
+ * See the top-level Makefile.
+ */
+#error SUPPORTS_SIMPLE_IPC not defined
+#endif
 
 int ipc_server_run(const char *path, const struct ipc_server_opts *opts,
 		   ipc_server_application_cb *application_cb,
@@ -24,5 +30,3 @@  int ipc_server_run(const char *path, const struct ipc_server_opts *opts,
 
 	return ret;
 }
-
-#endif /* SUPPORTS_SIMPLE_IPC */
diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
index 38689b278df3..1927e6ef4bca 100644
--- a/compat/simple-ipc/ipc-unix-socket.c
+++ b/compat/simple-ipc/ipc-unix-socket.c
@@ -6,8 +6,12 @@ 
 #include "unix-socket.h"
 #include "unix-stream-server.h"
 
-#ifdef NO_UNIX_SOCKETS
-#error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets
+#ifndef SUPPORTS_SIMPLE_IPC
+/*
+ * This source file should only be compiled when Simple IPC is supported.
+ * See the top-level Makefile.
+ */
+#error SUPPORTS_SIMPLE_IPC not defined
 #endif
 
 enum ipc_active_state ipc_get_active_state(const char *path)
diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 8f89c02037e3..8dc7bda087da 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -4,8 +4,12 @@ 
 #include "pkt-line.h"
 #include "thread-utils.h"
 
-#ifndef GIT_WINDOWS_NATIVE
-#error This file can only be compiled on Windows
+#ifndef SUPPORTS_SIMPLE_IPC
+/*
+ * This source file should only be compiled when Simple IPC is supported.
+ * See the top-level Makefile.
+ */
+#error SUPPORTS_SIMPLE_IPC not defined
 #endif
 
 static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 75ed198a6a36..a87841340e6a 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -252,8 +252,15 @@  endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
 	list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-win32.c)
+	add_compile_definitions(SUPPORTS_SIMPLE_IPC)
+	set(SUPPORTS_SIMPLE_IPC 1)
 else()
-	list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-unix-socket.c)
+	# Simple IPC requires both Unix sockets and pthreads on Unix-based systems.
+	if(NOT NO_UNIX_SOCKETS AND NOT NO_PTHREADS)
+		list(APPEND compat_SOURCES compat/simple-ipc/ipc-shared.c compat/simple-ipc/ipc-unix-socket.c)
+		add_compile_definitions(SUPPORTS_SIMPLE_IPC)
+		set(SUPPORTS_SIMPLE_IPC 1)
+	endif()
 endif()
 
 set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX})
@@ -974,6 +981,7 @@  file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "X='${EXE_EXTENSION}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
+file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
 if(WIN32)
 	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
 endif()
diff --git a/simple-ipc.h b/simple-ipc.h
index dc3606e30bd6..2c48a5ee0047 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -5,10 +5,6 @@ 
  * See Documentation/technical/api-simple-ipc.txt
  */
 
-#if defined(GIT_WINDOWS_NATIVE) || !defined(NO_UNIX_SOCKETS)
-#define SUPPORTS_SIMPLE_IPC
-#endif
-
 #ifdef SUPPORTS_SIMPLE_IPC
 #include "pkt-line.h"