{"thread":{"id":"22849","subject":"[PATCH] Makefile: fix compilation of test programs under MinGW environment","startedAt":"2010-02-27T21:09:29Z","lastAt":"2010-02-28T21:17:21Z","messageCount":8,"participants":["Michael Lukashov","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"135851","messageId":"1267304969-1924-1-git-send-email-michael.lukashov@gmail.com","threadId":"22849","inReplyTo":null,"subject":"[PATCH] Makefile: fix compilation of test programs under MinGW environment","fromName":"Michael Lukashov","fromEmail":"michael.lukashov@gmail.com","sentAt":"2010-02-27T21:09:29Z","receivedAt":"2010-02-27T21:09:29Z","isPatch":true,"sender":{"key":"michael.lukashov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/890439?v=4"},"body":"Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git\ninto jn/autodep, 2010-01-26) changed Makefile in such a way that\nthe following error occurs when trying to compile Git under MinGW environment:\n\n make: *** No rule to make target `test-chmtime', needed by `all'. Stop.\n\nUnder Linux it seems there's no difference between two variants.\n\nThis patch applies on top of branch 'next' of git.git repository.\n\nSigned-off-by: Michael Lukashov \n---\n Makefile | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex b6f097e..498e5e7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -393,7 +393,7 @@ TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\n TEST_PROGRAMS_NEED_X += test-index-version\n \n-TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n+TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n \n # List built-in command $C whose implementation cmd_$C() is not in\n # builtin/$C.o but is linked in as part of some other command.\n-- \n1.7.0.1556.g5a328\n"},{"id":"135854","messageId":"7vy6ietlf7.fsf@alter.siamese.dyndns.org","threadId":"22849","inReplyTo":"1267304969-1924-1-git-send-email-michael.lukashov@gmail.com","subject":"Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-27T21:31:56Z","receivedAt":"2010-02-27T21:31:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lukashov writes:\n\n> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git\n> into jn/autodep, 2010-01-26) changed Makefile in such a way that\n> the following error occurs when trying to compile Git under MinGW environment:\n>\n> make: *** No rule to make target `test-chmtime', needed by `all'. Stop.\n>\n> Under Linux it seems there's no difference between two variants.\n\n> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n\nIf the difference were on the RHS of this definition, which does involve\n$X that is different between the two platforms, I would understand, but\nyour patch looks like it is addressing difference between := vs =, and\nthat is more like a difference of other parts of the Makefile than\ndifference between Linux and mingw compilation environment.\n\nDoes mingw build add other instances of TEST_PROGRAMS definition to the\nMakefile, or perhaps have other means (e.g. ./build.sh runs make with\nTEST_PROGRAMS set to something else) to affect it?\n\nOr somewhere other than the main makefile, do you have an explicit \"make\ntest-chmtime\" (not \"make test-chmtime.exe\") that tries to make sure that\nthe build is done?\n"},{"id":"135856","messageId":"63cde7731002271340k5c26e064r8b7cc4a53b435e95@mail.gmail.com","threadId":"22849","inReplyTo":"7vy6ietlf7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment","fromName":"Michael Lukashov","fromEmail":"michael.lukashov@gmail.com","sentAt":"2010-02-27T21:40:41Z","receivedAt":"2010-02-27T21:40:41Z","isPatch":true,"sender":{"key":"michael.lukashov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/890439?v=4"},"body":"Hi,\n\nOn Sun, Feb 28, 2010 at 12:31 AM, Junio C Hamano wrote:\n> Michael Lukashov writes:\n>\n>> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git\n>> into jn/autodep, 2010-01-26) changed Makefile in such a way that\n>> the following error occurs when trying to compile Git under MinGW environment:\n>>\n>>   make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.\n>>\n>> Under Linux it seems there's no difference between two variants.\n>\n>> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n>> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n>\n> If the difference were on the RHS of this definition, which does involve\n> $X that is different between the two platforms, I would understand, but\n> your patch looks like it is addressing difference between := vs =, and\n> that is more like a difference of other parts of the Makefile than\n> difference between Linux and mingw compilation environment.\n>\n> Does mingw build add other instances of TEST_PROGRAMS definition to the\n> Makefile, or perhaps have other means (e.g. ./build.sh runs make with\n> TEST_PROGRAMS set to something else) to affect it?\n>\n\nNo.\n\n> Or somewhere other than the main makefile, do you have an explicit \"make\n> test-chmtime\" (not \"make test-chmtime.exe\") that tries to make sure that\n> the build is done?\n>\n>\n\nNo.\n\nBefore commit 225f78c8 definition of TEST_PROGRAMS was:\n\nTEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n\nAfter commit 225f78c8 definition of TEST_PROGRAMS changed to\n\nTEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n\nAnd it leads to compilation error.\n"},{"id":"135859","messageId":"7vmxyupbpa.fsf@alter.siamese.dyndns.org","threadId":"22849","inReplyTo":"7vy6ietlf7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-27T22:15:29Z","receivedAt":"2010-02-27T22:15:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael Lukashov writes:\n>\n>> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git\n>> into jn/autodep, 2010-01-26) changed Makefile in such a way that\n>> the following error occurs when trying to compile Git under MinGW environment:\n>>\n>> make: *** No rule to make target `test-chmtime', needed by `all'. Stop.\n>>\n>> Under Linux it seems there's no difference between two variants.\n>\n>> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n>> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n>\n> If the difference were on the RHS of this definition, which does involve\n> $X that is different between the two platforms, I would understand, but\n> your patch looks like it is addressing difference between := vs =, and\n> that is more like a difference of other parts of the Makefile than\n> difference between Linux and mingw compilation environment.\n\nOk, I think I know what happend.\n\nWe used to have the definition of TEST_PROGRAMS way later than where we\ncurrently have it, and it was for a reason. X is to be defined to be .exe\nin the platform specific section for MinGW (and probably Cygwin as well).\n\nBut because the definition of TEST_PROGRAMS was moved way up, it needs to\nbe recursively expanded.\n\nTEST_OBJS also uses $X in simple expansion (i.e. sets with := not with =),\nso I expect that it has the same issue. Can you check and verify?\n"},{"id":"135862","messageId":"63cde7731002271503oac53237ubed6d318b46042e9@mail.gmail.com","threadId":"22849","inReplyTo":"7vmxyupbpa.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Makefile: fix compilation of test programs under MinGW environment","fromName":"Michael Lukashov","fromEmail":"michael.lukashov@gmail.com","sentAt":"2010-02-27T23:03:35Z","receivedAt":"2010-02-27T23:03:35Z","isPatch":true,"sender":{"key":"michael.lukashov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/890439?v=4"},"body":"Hi,\n\nOn Sun, Feb 28, 2010 at 1:15 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Michael Lukashov writes:\n>>\n>>> Commit 225f78c8 (Merge branch 'master' of git://repo.or.cz/alt-git\n>>> into jn/autodep, 2010-01-26) changed Makefile in such a way that\n>>> the following error occurs when trying to compile Git under MinGW environment:\n>>>\n>>>   make: *** No rule to make target `test-chmtime', needed by `all'.  Stop.\n>>>\n>>> Under Linux it seems there's no difference between two variants.\n>>\n>>> -TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n>>> +TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n>>\n>> If the difference were on the RHS of this definition, which does involve\n>> $X that is different between the two platforms, I would understand, but\n>> your patch looks like it is addressing difference between := vs =, and\n>> that is more like a difference of other parts of the Makefile than\n>> difference between Linux and mingw compilation environment.\n>\n> Ok, I think I know what happend.\n>\n> We used to have the definition of TEST_PROGRAMS way later than where we\n> currently have it, and it was for a reason.  X is to be defined to be .exe\n> in the platform specific section for MinGW (and probably Cygwin as well).\n>\n> But because the definition of TEST_PROGRAMS was moved way up, it needs to\n> be recursively expanded.\n>\n> TEST_OBJS also uses $X in simple expansion (i.e. sets with := not with =),\n> so I expect that it has the same issue.  Can you check and verify?\n>\n>\n\nIt seems there's no difference between\n\nTEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n\nand\n\nTEST_OBJS = $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n\nBoth variants seem to work under mingw.\n"},{"id":"135868","messageId":"20100228090311.GA30143@progeny.tock","threadId":"22849","inReplyTo":"63cde7731002271503oac53237ubed6d318b46042e9@mail.gmail.com","subject":"[PATCH 1/2] Makefile: fix definition of $(TEST_PROGRAMS) on Windows","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-28T09:03:12Z","receivedAt":"2010-02-28T09:03:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Michael Lukashov \n\nCommit ea92519 (build dashless \"bin-wrappers\" directory similar to\ninstalled bindir, 2009-12-02) replaced the definition of\nTEST_PROGRAMS with a macro:\n\n TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n\nand commit daa99a9 (Makefile: make sure test helpers are rebuilt when\nheaders change, 2010-01-26) moved the (unchanged, non-macro)\ndefinition of TEST_PROGRAMS earlier so it could be used in two\ndifferent sections of the Makefile.\n\nThe merge 225f78 resolving these two changes unfortunately snuck in an\noptimization while at it: it replaced the delayed-evaluation =\noperator with an immediate := assignment:\n\n TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n\nSuch a change would have been safe when TEST_PROGRAMS was defined\ntowards the bottom of the makefile, but in its new location before\nthe platform-specific definitions, $(X) is not yet defined. Thus\nthe following error occurs when trying to compile Git in Windows:\n\n make: *** No rule to make target `test-chmtime', needed by `all'. Stop.\n\nor if X is set to a nonempty value in config.mak.\n\nSo change the operator back to =. This makes TEST_PROGRAMS more\nsimilar to PROGRAMS and the other macros defined with delayed\nevaluation in that section.\n\nThanks to Junio for the analysis.\n\nSigned-off-by: Michael Lukashov \nSigned-off-by: Jonathan Nieder \n---\nThanks for the catch! Here’s a longer explanation.\n\n Makefile | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 93e1a92..b64eec1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -418,7 +418,7 @@ TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\n TEST_PROGRAMS_NEED_X += test-index-version\n \n-TEST_PROGRAMS := $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n+TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n \n # List built-in command $C whose implementation cmd_$C() is not in\n # builtin-$C.o but is linked in as part of some other command.\n-- \n1.7.0\n"},{"id":"135869","messageId":"20100228091155.GB30143@progeny.tock","threadId":"22849","inReplyTo":"63cde7731002271503oac53237ubed6d318b46042e9@mail.gmail.com","subject":"[PATCH 2/2] Makefile: clarify definition of TEST_OBJS","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-02-28T09:11:55Z","receivedAt":"2010-02-28T09:11:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The definition of TEST_OBJS in commit daa99a91 (Makefile: make sure\ntest helpers are rebuilt when headers change, 2010-01-26) moved a use\nof $X to before the platform-specific section where it gets defined.\nThere are at least two ways to fix that:\n\n - Change the definition of TEST_OBJS to use the = delayed\n evaluation operator. This way, one need not worry about $(X)\n needing to be defined before TEST_OBJS is set.\n\n - Move the definition of TEST_OBJS to below the definition of $X.\n\nCarry out the second. The later site of definition makes the code more\nreadable, since now a reader only has to look down one line to see what\nTEST_OBJS is meant to be used for.\n\nOddly enough, with or without this change the behavior of the Makefile\nis the same. Since TEST_PROGRAMS is defined with delayed evaluation,\nthe value of\n\n TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n\nis independent of the value of $X when it is evaluated: the $X in the\npattern and the $X in $(TEST_PROGRAMS) will simply always cancel out.\nMake sure $X has the expected expansion anyway to make the code and\nthe reader’s sanity more robust in the face of future changes.\n\nSigned-off-by: Jonathan Nieder \n---\nMichael Lukashov wrote:\n\n> It seems there's no difference between\n> \n> TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n> \n> and\n> \n> TEST_OBJS = $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n> \n> Both variants seem to work under mingw.\n\nYep. I think the unexpected value of $X is worth fixing regardless just to\nkeep people from going insane.\n\nThanks, both.\n\n Makefile | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex b64eec1..e95c128 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -738,8 +738,6 @@ BUILTIN_OBJS += builtin-verify-pack.o\n BUILTIN_OBJS += builtin-verify-tag.o\n BUILTIN_OBJS += builtin-write-tree.o\n \n-TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n-\n GITLIBS = $(LIB_FILE) $(XDIFF_LIB)\n EXTLIBS =\n \n@@ -1686,6 +1684,7 @@ git.o git.spec \\\n \t$(patsubst %.perl,%,$(SCRIPT_PERL)) \\\n \t: GIT-VERSION-FILE\n \n+TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \\\n \tgit.o http.o http-walker.o remote-curl.o\n XDIFF_OBJS = xdiff/xdiffi.o xdiff/xprepare.o xdiff/xutils.o xdiff/xemit.o \\\n-- \n1.7.0\n"},{"id":"135897","messageId":"7vhbp1vz4u.fsf@alter.siamese.dyndns.org","threadId":"22849","inReplyTo":"20100228091155.GB30143@progeny.tock","subject":"Re: [PATCH 2/2] Makefile: clarify definition of TEST_OBJS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-28T21:17:21Z","receivedAt":"2010-02-28T21:17:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Oddly enough, with or without this change the behavior of the Makefile\n> is the same. Since TEST_PROGRAMS is defined with delayed evaluation,\n> the value of\n>\n> TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))\n>\n> is independent of the value of $X when it is evaluated: the $X in the\n> pattern and the $X in $(TEST_PROGRAMS) will simply always cancel out.\n\nUgh. That is what I missed. Thanks for explanation.\n\nThe mismerge fixed by [PATCH 1/2] comes from my rerere database, and\nthanks to J6t's earlier \"rerere forget\" work, I managed to fix it in\npreparation for the eventual merge to 'master'. I queued the fix-up\ndirectly on 'next' as well.\n"}]}