[PATCH] Makefile: fix compilation of test programs under MinGW environment

[PATCH] Makefile: fix compilation of test programs under MinGW environment

[Michael Lukashov]

Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git
into jn/autodep, 2010-01-26) changed Makefile in such a way that
the following error occurs when trying to compile Git under MinGW environment:

  make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.

Under Linux it seems there's no difference between two variants.

This patch applies on top of branch 'next' of git.git repository.

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index b6f097e..498e5e7 100644
--- a/Makefile
+++ b/Makefile
@@ -393,7 +393,7 @@ TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-index-version
 
-TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
+TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
 # List built-in command $C whose implementation cmd_$C() is not in
 # builtin/$C.o but is linked in as part of some other command.
-- 
1.7.0.1556.g5a328

Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment

[Junio C Hamano]

Michael Lukashov <michael.lukashov@gmail.com> writes:

> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git
> into jn/autodep, 2010-01-26) changed Makefile in such a way that
> the following error occurs when trying to compile Git under MinGW environment:
>
>   make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.
>
> Under Linux it seems there's no difference between two variants.

> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))

If the difference were on the RHS of this definition, which does involve
$X that is different between the two platforms, I would understand, but
your patch looks like it is addressing difference between := vs =, and
that is more like a difference of other parts of the Makefile than
difference between Linux and mingw compilation environment.

Does mingw build add other instances of TEST_PROGRAMS definition to the
Makefile, or perhaps have other means (e.g. ./build.sh runs make with
TEST_PROGRAMS set to something else) to affect it?

Or somewhere other than the main makefile, do you have an explicit "make
test-chmtime" (not "make test-chmtime.exe") that tries to make sure that
the build is done?

Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment

[Michael Lukashov]

Hi,

On Sun, Feb 28, 2010 at 12:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Lukashov <michael.lukashov@gmail.com> writes:
>
>> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git
>> into jn/autodep, 2010-01-26) changed Makefile in such a way that
>> the following error occurs when trying to compile Git under MinGW environment:
>>
>>   make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.
>>
>> Under Linux it seems there's no difference between two variants.
>
>> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
>> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
>
> If the difference were on the RHS of this definition, which does involve
> $X that is different between the two platforms, I would understand, but
> your patch looks like it is addressing difference between := vs =, and
> that is more like a difference of other parts of the Makefile than
> difference between Linux and mingw compilation environment.
>
> Does mingw build add other instances of TEST_PROGRAMS definition to the
> Makefile, or perhaps have other means (e.g. ./build.sh runs make with
> TEST_PROGRAMS set to something else) to affect it?
>

No.

> Or somewhere other than the main makefile, do you have an explicit "make
> test-chmtime" (not "make test-chmtime.exe") that tries to make sure that
> the build is done?
>
>

No.

Before commit 225f78c8 definition of TEST_PROGRAMS was:

TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))

After commit 225f78c8 definition of TEST_PROGRAMS changed to

TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))

And it leads to compilation error.

Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael Lukashov <michael.lukashov@gmail.com> writes:
>
>> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git
>> into jn/autodep, 2010-01-26) changed Makefile in such a way that
>> the following error occurs when trying to compile Git under MinGW environment:
>>
>>   make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.
>>
>> Under Linux it seems there's no difference between two variants.
>
>> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
>> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
>
> If the difference were on the RHS of this definition, which does involve
> $X that is different between the two platforms, I would understand, but
> your patch looks like it is addressing difference between := vs =, and
> that is more like a difference of other parts of the Makefile than
> difference between Linux and mingw compilation environment.

Ok, I think I know what happend.

We used to have the definition of TEST_PROGRAMS way later than where we
currently have it, and it was for a reason.  X is to be defined to be .exe
in the platform specific section for MinGW (and probably Cygwin as well).

But because the definition of TEST_PROGRAMS was moved way up, it needs to
be recursively expanded.

TEST_OBJS also uses $X in simple expansion (i.e. sets with := not with =),
so I expect that it has the same issue.  Can you check and verify?

Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment

[Michael Lukashov]

Hi,

On Sun, Feb 28, 2010 at 1:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Michael Lukashov <michael.lukashov@gmail.com> writes:
>>
>>> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git
>>> into jn/autodep, 2010-01-26) changed Makefile in such a way that
>>> the following error occurs when trying to compile Git under MinGW environment:
>>>
>>>   make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.
>>>
>>> Under Linux it seems there's no difference between two variants.
>>
>>> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
>>> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
>>
>> If the difference were on the RHS of this definition, which does involve
>> $X that is different between the two platforms, I would understand, but
>> your patch looks like it is addressing difference between := vs =, and
>> that is more like a difference of other parts of the Makefile than
>> difference between Linux and mingw compilation environment.
>
> Ok, I think I know what happend.
>
> We used to have the definition of TEST_PROGRAMS way later than where we
> currently have it, and it was for a reason.  X is to be defined to be .exe
> in the platform specific section for MinGW (and probably Cygwin as well).
>
> But because the definition of TEST_PROGRAMS was moved way up, it needs to
> be recursively expanded.
>
> TEST_OBJS also uses $X in simple expansion (i.e. sets with := not with =),
> so I expect that it has the same issue.  Can you check and verify?
>
>

It seems there's no difference between

TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))

and

TEST_OBJS = $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))

Both variants seem to work under mingw.

[PATCH 1/2] Makefile: fix definition of $(TEST_PROGRAMS) on Windows

[Jonathan Nieder]

From: Michael Lukashov <michael.lukashov@gmail.com>

Commit ea92519 (build dashless "bin-wrappers" directory similar to
installed bindir, 2009-12-02) replaced the definition of
TEST_PROGRAMS with a macro:

 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))

and commit daa99a9 (Makefile: make sure test helpers are rebuilt when
headers change, 2010-01-26) moved the (unchanged, non-macro)
definition of TEST_PROGRAMS earlier so it could be used in two
different sections of the Makefile.

The merge 225f78 resolving these two changes unfortunately snuck in an
optimization while at it: it replaced the delayed-evaluation =
operator with an immediate := assignment:

 TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))

Such a change would have been safe when TEST_PROGRAMS was defined
towards the bottom of the makefile, but in its new location before
the platform-specific definitions, $(X) is not yet defined.  Thus
the following error occurs when trying to compile Git in Windows:

  make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.

or if X is set to a nonempty value in config.mak.

So change the operator back to =.  This makes TEST_PROGRAMS more
similar to PROGRAMS and the other macros defined with delayed
evaluation in that section.

Thanks to Junio for the analysis.

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for the catch!  Here’s a longer explanation.

 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 93e1a92..b64eec1 100644
--- a/Makefile
+++ b/Makefile
@@ -418,7 +418,7 @@ TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-index-version
 
-TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
+TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
 # List built-in command $C whose implementation cmd_$C() is not in
 # builtin-$C.o but is linked in as part of some other command.
-- 
1.7.0

[PATCH 2/2] Makefile: clarify definition of TEST_OBJS

[Jonathan Nieder]

The definition of TEST_OBJS in commit daa99a91 (Makefile: make sure
test helpers are rebuilt when headers change, 2010-01-26) moved a use
of $X to before the platform-specific section where it gets defined.
There are at least two ways to fix that:

 - Change the definition of TEST_OBJS to use the = delayed
   evaluation operator.  This way, one need not worry about $(X)
   needing to be defined before TEST_OBJS is set.

 - Move the definition of TEST_OBJS to below the definition of $X.

Carry out the second.  The later site of definition makes the code more
readable, since now a reader only has to look down one line to see what
TEST_OBJS is meant to be used for.

Oddly enough, with or without this change the behavior of the Makefile
is the same.  Since TEST_PROGRAMS is defined with delayed evaluation,
the value of

 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))

is independent of the value of $X when it is evaluated: the $X in the
pattern and the $X in $(TEST_PROGRAMS) will simply always cancel out.
Make sure $X has the expected expansion anyway to make the code and
the reader’s sanity more robust in the face of future changes.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Michael Lukashov wrote:

> It seems there's no difference between
> 
> TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
> 
> and
> 
> TEST_OBJS = $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
> 
> Both variants seem to work under mingw.

Yep.  I think the unexpected value of $X is worth fixing regardless just to
keep people from going insane.

Thanks, both.

 Makefile |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index b64eec1..e95c128 100644
--- a/Makefile
+++ b/Makefile
@@ -738,8 +738,6 @@ BUILTIN_OBJS += builtin-verify-pack.o
 BUILTIN_OBJS += builtin-verify-tag.o
 BUILTIN_OBJS += builtin-write-tree.o
 
-TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
-
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
 EXTLIBS =
 
@@ -1686,6 +1684,7 @@ git.o git.spec \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
 
+TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
 GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
 	git.o http.o http-walker.o remote-curl.o
 XDIFF_OBJS = xdiff/xdiffi.o xdiff/xprepare.o xdiff/xutils.o xdiff/xemit.o \
-- 
1.7.0

Re: [PATCH 2/2] Makefile: clarify definition of TEST_OBJS

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Oddly enough, with or without this change the behavior of the Makefile
> is the same.  Since TEST_PROGRAMS is defined with delayed evaluation,
> the value of
>
>  TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
>
> is independent of the value of $X when it is evaluated: the $X in the
> pattern and the $X in $(TEST_PROGRAMS) will simply always cancel out.

Ugh.  That is what I missed.  Thanks for explanation.

The mismerge fixed by [PATCH 1/2] comes from my rerere database, and
thanks to J6t's earlier "rerere forget" work, I managed to fix it in
preparation for the eventual merge to 'master'.  I queued the fix-up
directly on 'next' as well.