patchMakefile: fix compilation of test programs under MinGW environment

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(-)

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

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?

Hi,

On Sun, Feb 28, 2010 at 12:31 AM, Junio C Hamano <gitster@pobox.com> wrote:

No.

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.

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

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?

Hi,

On Sun, Feb 28, 2010 at 1:15 AM, Junio C Hamano <gitster@pobox.com> 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.

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(-)

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:

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(-)

Jonathan Nieder <jrnieder@gmail.com> writes:

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.