Re: [PATCH] Makefile: fix cygwin build failure

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 9, 2022, 23:18 UTC
Message-ID: <221110.868rkjpty3.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y2wwfQWrs+KYpWNv@nand.local>

On Wed, Nov 09 2022, Taylor Blau wrote:

Show 13 quoted lines

> Hi Ramsay,
>
> On Wed, Nov 09, 2022 at 10:46:05PM +0000, Ramsay Jones wrote:
>> Commit 1c97a5043f (Makefile: define "TEST_{PROGRAM,OBJS}" variables
>> earlier, 2022-10-31) breaks the cygwin build, like so:
>
> It seems reasonable to me, and I'd like to pick it up rather quickly (on
> top of Ævar's branch), especially if this is going to break things
> downstream in Git for Windows.
>
> Ævar: this sort of change is a little tricky to review without more diff
> context ;-). Do you have any objections to me slotting this on top of
> your branch?

Yes, I've reviewed this, sorry about missing this edge case. This fix & analysis looks solid to me (it's still just in "seen", right?)

FWIW I think a more thorough fix for it would be to future-proof this sort of IMMEDIATE expansion by defining such core variables earlier:

-- >8 --
Subject: [PATCH] Makefile & make "uname" and "$X" available earlier

In [1] I broke the build on Cygwin, because by moving "all:: $(TEST_PROGRAMS)" earlier in the file (around line 800) it was declared before around line ~1300, where we include "config.mak.uname", and that's where we'll set "X = .exe" on Cygwin and Windows.

Moving the "all" line down[2] is a more narrow fix for this, but this attempts to make this sort of thing safer in the future. We'll now load a "config.mak.uname-early" (really within the first 100 lines of code, but there's a giant comment at the top).

This ensures that in the future any Makefile rules that have "IMMEDIATE" expansion (e.g. the RHS of a "rule" will work as expected if they use $(X), not just if they use the "DEFERRED" expansion (which e.g. "=" assignment uses). See [3] in the GNU make documentation for details.

1. 1c97a5043f8 (Makefile: define "TEST_{PROGRAM,OBJS}" variables
   earlier, 2022-10-31)
2. https://lore.kernel.org/git/0dec6e1e-207c-be13-ae95-294d9b1e8831@ramsayjones.plus.com/
3. https://www.gnu.org/software/make/manual/html_node/Reading-Makefiles.html

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile               |  9 ++++++---
 config.mak.uname       |  7 -------
 config.mak.uname-early | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.uname-early

diff --git a/Makefile b/Makefile
index 4927379184c..e31678e0547 100644
--- a/Makefile
+++ b/Makefile
@@ -621,6 +621,12 @@ TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 
+# Binary suffix, set to .exe for Windows builds
+X =
+# Make $(uname_*) variables available, and possibly change $X to
+# ".exe" (on Windows)
+include config.mak.uname-early
+
 # Having this variable in your environment would break pipelines because
 # you cause "cd" to echo its destination to stdout.  It can also take
 # scripts to unexpected places.  If you like CDPATH, define it for your
@@ -714,9 +720,6 @@ PROGRAM_OBJS += shell.o
 .PHONY: program-objs
 program-objs: $(PROGRAM_OBJS)
 
-# Binary suffix, set to .exe for Windows builds
-X =
-
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_BUILTINS_OBJS += test-advise.o
diff --git a/config.mak.uname b/config.mak.uname
index d63629fe807..616fa9052e2 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -16,10 +16,6 @@ ifneq ($(findstring MINGW,$(uname_S)),)
 endif
 
 ifdef MSVC
-	# avoid the MingW and Cygwin configuration sections
-	uname_S := Windows
-	uname_O := Windows
-
 	# Generate and include makefile variables that point to the
 	# currently installed set of MSVC command line tools.
 compat/vcbuild/MSVC-DEFS-GEN: compat/vcbuild/find_vs_env.bat
@@ -238,7 +234,6 @@ ifeq ($(uname_O),Cygwin)
 	NEEDS_LIBICONV = YesPlease
 	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
-	X = .exe
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
@@ -523,7 +518,6 @@ ifndef DEBUG
 else
 	BASIC_CFLAGS += -MDd -DDEBUG -D_DEBUG
 endif
-	X = .exe
 
 compat/msvc.o: compat/msvc.c compat/mingw.c GIT-CFLAGS
 endif
@@ -676,7 +670,6 @@ ifeq ($(uname_S),MINGW)
 	PTHREAD_LIBS =
 	RC = windres -O coff
 	NATIVE_CRLF = YesPlease
-	X = .exe
 ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
 	htmldir = doc/git/html/
 	prefix =
diff --git a/config.mak.uname-early b/config.mak.uname-early
new file mode 100644
index 00000000000..000d490a506
--- /dev/null
+++ b/config.mak.uname-early
@@ -0,0 +1,33 @@
+# This is mainly used by config.mak.uname-early, but we load it much
+# earlier to get access to $(X).
+
+uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
+uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
+uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
+uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
+uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
+uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
+
+ifneq ($(findstring MINGW,$(uname_S)),)
+	uname_S := MINGW
+endif
+
+ifdef MSVC
+	# avoid the MingW and Cygwin configuration sections in
+	# config.mak.uname
+	uname_S := Windows
+	uname_O := Windows
+endif
+
+
+ifeq ($(uname_S),MINGW)
+	X = .exe
+else
+ifeq ($(uname_S),Windows)
+	X = .exe
+else
+ifeq ($(uname_O),Cygwin)
+	X = .exe
+endif
+endif
+endif

-- 
2.38.0.1467.g709fbdff1a9