git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: "Randall S. Becker" <rsbecker@nexbridge.com>
Cc: git@jeffhostetler.com, git@vger.kernel.org,
	"'SZEDER Gábor'" <szeder.dev@gmail.com>,
	"'Johannes Schindelin via GitGitGadget'" <gitgitgadget@gmail.com>,
	jeffhost@microsoft.com
Subject: Re: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)
Date: Tue, 18 May 2021 04:23:49 -0400	[thread overview]
Message-ID: <YKN5lXs4AoK/JFTO@coredump.intra.peff.net> (raw)
In-Reply-To: <009d01d74b44$9efe8a60$dcfb9f20$@nexbridge.com>

On Mon, May 17, 2021 at 01:46:46PM -0400, Randall S. Becker wrote:

> On Wed, 17 Feb 2021 21:48:36, Jeff Hostetler wrote:
> 
> >Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT
> >shutting down the server to make unit tests more predictable on CI servers.
> >(https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev)
> 
> I missed this at the time, but it appears that ipc-unix-socket.c
> forces a dependency on pthreads for Git under Unix-like platforms.
> This is probably not a correct assumption (or likely intended), but
> causes git to no longer build on NonStop x86 and ia64 as of
> 2.32.0-rc0. I am not suggesting undoing this, but amending to make the
> change more sensitive to a lack of pthread support.
> pthread_sigmask() showed up as an undefined external:

Hrm. Usually we do not assume that threads are available. For "async"
stuff via run-command, we allow it to be implemented via fork(), and
insist that the async process talk back to us only over a pipe
descriptor (so it works whether it's a thread or a separate process).
In cases where we use worker threads for performance (like index-pack or
pack-objects), we just run a single "thread" instead, waiting for it to
complete.

In the simple-ipc API, there's an explicit "async" interface. But it's
not clear to me how rich it expects the communication with the caller to
be (i.e., whether we could get away with the fork() trick here). Or if
it would be OK for the threading to remain an implementation detail,
with one "worker" upon whom we wait for completion.

> **** ERROR **** [1210]:
>    libgit.a(ipc-unix-socket.o): In function `thread_block_sigpipe':
>    ipc-unix-socket.o(.text+0xb87): unresolved reference to pthread_sigmask.
> 
> On NonStop, pthread_sigmask is defined in -lput or -lspt, which are not used in our build – and would cause a bunch of other issues
> if referenced. The build does define NO_PTHREADS.

So yeah, you hit that problem because you only have a
sort-of-pthreads-ish case. But it seems like a system which truly has no
pthread support at all and defines NO_PTHREADS to tell us so will have
much more of its compilation broken (because it's also missing obvious
bits like pthread_create()).

We already make simple-ipc compilation conditional on NO_UNIX_SOCKETS. I
think we could probably just do the same for NO_PTHREADS?

Something like:

diff --git a/Makefile b/Makefile
index 3a2d3c80a8..bd7fe0fc24 100644
--- a/Makefile
+++ b/Makefile
@@ -1687,14 +1687,20 @@ ifdef NO_UNIX_SOCKETS
 else
 	LIB_OBJS += unix-socket.o
 	LIB_OBJS += unix-stream-server.o
+endif
+
+# All simple-ipc requires threads, and then individual
+# mechanisms have their own requirements.
+ifndef NO_PTHREADS
+	BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC
 	LIB_OBJS += compat/simple-ipc/ipc-shared.o
+ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o
 endif
-
 ifdef USE_WIN32_IPC
-	LIB_OBJS += compat/simple-ipc/ipc-shared.o
 	LIB_OBJS += compat/simple-ipc/ipc-win32.o
 endif
+endif
 
 ifdef NO_ICONV
 	BASIC_CFLAGS += -DNO_ICONV
diff --git a/simple-ipc.h b/simple-ipc.h
index dc3606e30b..0f58be7945 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -4,11 +4,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"
 

  reply	other threads:[~2021-05-18  8:23 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-17 17:46 Randall S. Becker
2021-05-18  8:23 ` Jeff King [this message]
2021-05-18 11:21   ` Jeff Hostetler
2021-05-18 12:11     ` Jeff King
2021-05-18 13:55       ` Jeff Hostetler
2021-05-18 13:37   ` Randall S. Becker
2021-05-18 13:59     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YKN5lXs4AoK/JFTO@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    --cc=rsbecker@nexbridge.com \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [BUG] Unix Builds Requires Pthread Support (was [PATCH v4 00/12] Simple IPC Mechanism)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).