[PATCH] Makefile: fix cygwin build failure

[PATCH] Makefile: fix cygwin build failure

[Ramsay Jones]

Commit 1c97a5043f (Makefile: define "TEST_{PROGRAM,OBJS}" variables
earlier, 2022-10-31) breaks the cygwin build, like so:

    $ make
    GIT_VERSION = 2.38.1.674.gca75de31c9
        * new build flags
        CC oss-fuzz/fuzz-commit-graph.o
        CC oss-fuzz/fuzz-pack-headers.o
        CC oss-fuzz/fuzz-pack-idx.o
    make: *** No rule to make target 't/helper/test-fake-ssh', needed by 'all'.  Stop.
    $

This is caused by moving an 'all::' target higher in the Makefile,
before the 'include' of the config.mak.* files. This results in
the $X make variable having the default value (empty) rather than
a value suitable for cygwin (ie. '.exe'). Although the value of
this variable is lazily evaluated, the 'all::' target forces an
evaluation prior to it being correctly set.

In order to fix the build, move the 'all::' target lower in the
Makefile (close to where it was originally placed). Although it
could come anywhere after the 'include's, placing it here makes
the diff of the build outputs smaller (placing it directly after
the 'include's causes a change in the order of build products).

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Taylor, Ævar,

I decided to spend some time tonight cleaning up my cygwin git repo (I
have _far_ too many branches scattered across history, some _very_ old,
some half done, some with no commit message etc., etc.). So, before
looking to rebase/re-work some old commits on the current version of
git, I thought it might be an idea to peek at the latest work; I have
not seen any updates to my origin repo, for the last couple of weeks,
since Junio took a break (as you may recall :) ).

So, I set up a 'git' remote:

    $ git remote get-url git
    git@github.com:git/git.git
    $ 

.. to have a peek at the updates to the 'master', 'next' and 'seen'
branches. Also, out of habit, I decided to build all three branches in
the usual way (well, apart from being on a detached HEAD, of course).

First 'git/master' failed to build! ;) It turned out that I had updated
cygwin and installed a bad update to the gettext-devel package, in
particular '/usr/bin/msgfmt.exe' was completely broken. :( (Adam, if you
haven't already run into this, you may appreciate the solution given
at [1] below).

Having fixed my cygwin installation, 'git/master' and 'git/next' built
just fine, 'git/seen' however failed to build (see commit message
above).

I'm not sure what the plans are for the 'ab/make-bin-wrappers' branch,
but if it is going to be re-rolled, could you please squash this into
the patch corresponding to commit 1c97a5043f. (Otherwise, could you
maybe add this to the tip of that branch?).

Note: this patch was created directly on top of 'git/seen'@ca75de31c9,
but I wouldn't anticipate any problem adding it to that branch.

I am a little surprised that it has taken this long to spot this build
failure, since this should also affect a (vanilla) MSYS2 build, along
with a Git-For-Windows Makefile build. Hmm, I have no way of knowing,
but this seems to indicate that nobody builds GFW using the Makefile
these days! :D

If anything in the commit message is unclear, please let me know.

Thanks!

ATB,
Ramsay Jones

[1] After a cygwin update, '/usr/bin/msgfmt.exe' refused to run, saying
that it could not locate the 'cygunistring-5.dll' file. Using cygcheck,
I found that this dll is provided by the 'libunistring5 1.1-1' package.
After installing this package, everything works just fine.

I don't know how package dependencies are specified/updated, but it
would seem the 'gettext-devel' package has a direct or indirect
dependency on the 'libunistring5' package. Looking at my setup.log file
I would guess one-or-more of the following packages needs an update to
note this dependency: 'gettext-devel 0.21.1-1', 'gettext 0.21.1-1',
'libgettextpo0 0.21.1-1', 'libintl-devel 0.21.1-1', 'libintl8 0.21.1-1',
or 'libasprintf0 0.21.1-1'.

Unfortunately, I am not subscribed to the cygwin mailinglist :(

 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index acb02f3882..03ceaf3e79 100644
--- a/Makefile
+++ b/Makefile
@@ -869,7 +869,6 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-tool
 
 TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
-all:: $(TEST_PROGRAMS)
 TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X))
 .PRECIOUS: $(TEST_PROGRAM_OBJS)
 
@@ -3208,7 +3207,7 @@ $(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X)
 endef
 $(eval $(call bin_wrappers_templates))
 
-all:: $(BIN_WRAPPERS)
+all:: $(TEST_PROGRAMS) $(BIN_WRAPPERS)
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems
-- 
2.38.0

Re: [PATCH] Makefile: fix cygwin build failure

[Taylor Blau]

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?

Thanks,
Taylor

Re: [PATCH] Makefile: fix cygwin build failure

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH] Makefile: fix cygwin build failure

[Taylor Blau]

On Thu, Nov 10, 2022 at 12:18:01AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> 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?)

Yes, 'ab/remove--super-prefix' is only in seen for now. So that we don't
break the Cygwin build in the middle of history, could you send a reroll
of that topic that incorporates this patch squashed into the right
location so that each step builds independently?

Thanks,
Taylor

Re: [PATCH] Makefile: fix cygwin build failure

[Taylor Blau]

On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote:
> Yes, 'ab/remove--super-prefix' is only in seen for now.

Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that.

Thanks,
Taylor

Re: [PATCH] Makefile: fix cygwin build failure

[Taylor Blau]

On Wed, Nov 09, 2022 at 09:21:23PM -0500, Taylor Blau wrote:
> On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote:
> > Yes, 'ab/remove--super-prefix' is only in seen for now.
>
> Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that.

...Double oops. Now that I think about it, my notes show that we were
planning on dropping this topic per the discussion beginning at [1].

If that's not the case and you were planning on resending this topic,
then incorporating these changes in would be appreciated. If not, I
think we can drop both of these.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/Y2rPrNz6BD6DlRcO@nand.local/

Re: [PATCH] Makefile: fix cygwin build failure

[Ramsay Jones]

On 10/11/2022 02:22, Taylor Blau wrote:
> On Wed, Nov 09, 2022 at 09:21:23PM -0500, Taylor Blau wrote:
>> On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote:
>>> Yes, 'ab/remove--super-prefix' is only in seen for now.
>>
>> Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that.
> 
> ...Double oops. Now that I think about it, my notes show that we were
> planning on dropping this topic per the discussion beginning at [1].
> 
> If that's not the case and you were planning on resending this topic,
> then incorporating these changes in would be appreciated. If not, I
> think we can drop both of these.
> 
> Thanks,
> Taylor
> 
> [1]: https://lore.kernel.org/git/Y2rPrNz6BD6DlRcO@nand.local/

I noticed my origin server:

    $ git config remote.origin.url
    git://git.kernel.org/pub/scm/git/git.git
    $ 

.. updated tonight, so I fetched, built, and noticed cygwin is
still broken! :(

If the 'ab/make-bin-wrappers' branch is going to stay, could you
please squash my patch[1] into (or put it on top of) this branch.

Thanks!

ATB,
Ramsay Jones

[1] https://lore.kernel.org/git/0dec6e1e-207c-be13-ae95-294d9b1e8831@ramsayjones.plus.com/

Re: [PATCH] Makefile: fix cygwin build failure

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Nov 09, 2022 at 09:21:23PM -0500, Taylor Blau wrote:
>> On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote:
>> > Yes, 'ab/remove--super-prefix' is only in seen for now.
>>
>> Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that.
>
> ...Double oops. Now that I think about it, my notes show that we were
> planning on dropping this topic per the discussion beginning at [1].

OK, let's discard it for now.

Re: [PATCH] Makefile: fix cygwin build failure

[Adam Dinwoodie]

On Wed, 9 Nov 2022 at 22:46, Ramsay Jones wrote:
> <snip>
> [1] After a cygwin update, '/usr/bin/msgfmt.exe' refused to run, saying
> that it could not locate the 'cygunistring-5.dll' file. Using cygcheck,
> I found that this dll is provided by the 'libunistring5 1.1-1' package.
> After installing this package, everything works just fine.
>
> I don't know how package dependencies are specified/updated, but it
> would seem the 'gettext-devel' package has a direct or indirect
> dependency on the 'libunistring5' package. Looking at my setup.log file
> I would guess one-or-more of the following packages needs an update to
> note this dependency: 'gettext-devel 0.21.1-1', 'gettext 0.21.1-1',
> 'libgettextpo0 0.21.1-1', 'libintl-devel 0.21.1-1', 'libintl8 0.21.1-1',
> or 'libasprintf0 0.21.1-1'.
>
> Unfortunately, I am not subscribed to the cygwin mailinglist :(

It looks like this was broken by an update to some of these packages a
couple of weeks ago[0]; I've reproduced the problem and reported it to
the mailing list[1], so the package maintainer should be able to
update the dependency information :)

[0]: https://cygwin.com/pipermail/cygwin-announce/2022-October/010764.html
[1]: https://cygwin.com/pipermail/cygwin/2022-November/252445.html