From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Ramsay Jones <ramsay@ramsayjones.plus.com>,
Adam Dinwoodie <git@dinwoodie.org>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
GIT Mailing-list <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] Makefile: fix cygwin build failure
Date: Thu, 10 Nov 2022 00:18:01 +0100 [thread overview]
Message-ID: <221110.868rkjpty3.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Y2wwfQWrs+KYpWNv@nand.local>
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@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
next prev parent reply other threads:[~2022-11-09 23:52 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-09 22:46 [PATCH] Makefile: fix cygwin build failure Ramsay Jones
2022-11-09 22:58 ` Taylor Blau
2022-11-09 23:18 ` Ævar Arnfjörð Bjarmason [this message]
2022-11-10 2:20 ` Taylor Blau
2022-11-10 2:21 ` Taylor Blau
2022-11-10 2:22 ` Taylor Blau
2022-11-22 1:54 ` Ramsay Jones
2022-11-22 2:02 ` Junio C Hamano
2022-11-10 20:35 ` Adam Dinwoodie
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221110.868rkjpty3.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@dinwoodie.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=ramsay@ramsayjones.plus.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).