| Message ID | pull.955.git.1621352192238.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | simple-ipc: correct ifdefs when NO_PTHREADS is defined | expand |
"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.
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
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.
diff --git a/Makefile b/Makefile index 3a2d3c80a81a..30df67fd62eb 100644 --- a/Makefile +++ b/Makefile @@ -1687,13 +1687,23 @@ 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. +# (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 endif ifdef NO_ICONV 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 + enum ipc_active_state ipc_get_active_state(const char *path) { enum ipc_active_state state = IPC_STATE__OTHER_ERROR; @@ -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 */ diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c index 8f89c02037e3..958bb562ebb6 100644 --- a/compat/simple-ipc/ipc-win32.c +++ b/compat/simple-ipc/ipc-win32.c @@ -4,6 +4,8 @@ #include "pkt-line.h" #include "thread-utils.h" +#ifdef SUPPORTS_SIMPLE_IPC + #ifndef GIT_WINDOWS_NATIVE #error This file can only be compiled on Windows #endif @@ -749,3 +751,5 @@ void ipc_server_free(struct ipc_server_data *server_data) free(server_data); } + +#endif /* SUPPORTS_SIMPLE_IPC */ 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"