On Wed, Nov 09 2022, Taylor Blau wrote:
> 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@xxxxxxxxxxxxxxxxxxxx/
3. https://www.gnu.org/software/make/manual/html_node/Reading-Makefiles.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
---
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
