{"thread":{"id":"29405","subject":"[PATCH] t/Makefile: Use $(sort ...) explicitly where needed","startedAt":"2012-01-19T20:17:23Z","lastAt":"2012-01-22T19:17:10Z","messageCount":6,"participants":["Kirill Smelkov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"182808","messageId":"1327004244-18892-1-git-send-email-kirr@navytux.spb.ru","threadId":"29405","inReplyTo":null,"subject":"[PATCH] t/Makefile: Use $(sort ...) explicitly where needed","fromName":"Kirill Smelkov","fromEmail":"kirr@navytux.spb.ru","sentAt":"2012-01-19T20:17:23Z","receivedAt":"2012-01-19T20:17:23Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"Starting from GNU Make 3.82 $(wildcard ...) no longer sorts the result\n(from NEWS):\n\n * WARNING: Backward-incompatibility!\n Wildcards were not documented as returning sorted values, but the results\n have been sorted up until this release.. If your makefiles require sorted\n results from wildcard expansions, use the $(sort ...) function to request\n it explicitly.\n\n http://repo.or.cz/w/make.git/commitdiff/2a59dc32aaf0681dec569f32a9d7ab88a379d34f\n\nso we have to sort tests list or else they are executed in seemingly\nrandom order even for -j1.\n\nSigned-off-by: Kirill Smelkov \n---\n t/Makefile | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 9046ec9..66ceefe 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -17,9 +17,9 @@ DEFAULT_TEST_TARGET ?= test\n # Shell quote;\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n \n-T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)\n-TSVN = $(wildcard t91[0-9][0-9]-*.sh)\n-TGITWEB = $(wildcard t95[0-9][0-9]-*.sh)\n+T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n+TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))\n+TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n \n all: $(DEFAULT_TEST_TARGET)\n \n-- \n1.7.9.rc2.124.ge3180\n"},{"id":"182816","messageId":"7v8vl3ic6o.fsf@alter.siamese.dyndns.org","threadId":"29405","inReplyTo":"1327004244-18892-1-git-send-email-kirr@navytux.spb.ru","subject":"Re: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-19T22:01:51Z","receivedAt":"2012-01-19T22:01:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> Starting from GNU Make 3.82 $(wildcard ...) no longer sorts the result\n> (from NEWS):\n>\n> * WARNING: Backward-incompatibility!\n> Wildcards were not documented as returning sorted values, but the results\n> have been sorted up until this release.. If your makefiles require sorted\n> results from wildcard expansions, use the $(sort ...) function to request\n> it explicitly.\n>\n> http://repo.or.cz/w/make.git/commitdiff/2a59dc32aaf0681dec569f32a9d7ab88a379d34f\n>\n> so we have to sort tests list or else they are executed in seemingly\n> random order even for -j1.\n\nI do not necessarily buy your \"so we HAVE TO, OR ELSE\".\n\nEven though I can understand \"We can sort the list of tests _if_ we do not\nwant them executed in seemingly random order when running 'make -j1'\", I\ntend to think that *if* is a big one. Aren't these tests designed not to\ndepend on each other anyway?\n"},{"id":"182826","messageId":"20120120063450.GA15371@mini.zxlink","threadId":"29405","inReplyTo":"7v8vl3ic6o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed","fromName":"Kirill Smelkov","fromEmail":"kirr@navytux.spb.ru","sentAt":"2012-01-20T06:34:50Z","receivedAt":"2012-01-20T06:34:50Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Thu, Jan 19, 2012 at 02:01:51PM -0800, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n> \n> > Starting from GNU Make 3.82 $(wildcard ...) no longer sorts the result\n> > (from NEWS):\n> >\n> > * WARNING: Backward-incompatibility!\n> > Wildcards were not documented as returning sorted values, but the results\n> > have been sorted up until this release.. If your makefiles require sorted\n> > results from wildcard expansions, use the $(sort ...) function to request\n> > it explicitly.\n> >\n> > http://repo.or.cz/w/make.git/commitdiff/2a59dc32aaf0681dec569f32a9d7ab88a379d34f\n> >\n> > so we have to sort tests list or else they are executed in seemingly\n> > random order even for -j1.\n> \n> I do not necessarily buy your \"so we HAVE TO, OR ELSE\".\n> \n> Even though I can understand \"We can sort the list of tests _if_ we do not\n> want them executed in seemingly random order when running 'make -j1'\", I\n> tend to think that *if* is a big one. Aren't these tests designed not to\n> depend on each other anyway?\n\nYes, they don't depend on each other, but what's the point in not\nsorting them? I usually watch test progress visually, and if tests are\nsorted, even with make -j4 they go more or less incrementally by their t\nnumber.\n\nOn my netbook, adding $(sort ...) adds approximately 0.008s to make\nstartup, so imho there is no performance penalty to adding that sort.\n\n\nThanks,\nKirill\n"},{"id":"182833","messageId":"7vbopyhmlx.fsf@alter.siamese.dyndns.org","threadId":"29405","inReplyTo":"20120120063450.GA15371@mini.zxlink","subject":"Re: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-20T07:14:18Z","receivedAt":"2012-01-20T07:14:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n>> I do not necessarily buy your \"so we HAVE TO, OR ELSE\".\n>> \n>> Even though I can understand \"We can sort the list of tests _if_ we do not\n>> want them executed in seemingly random order when running 'make -j1'\", I\n>> tend to think that *if* is a big one. Aren't these tests designed not to\n>> depend on each other anyway?\n>\n> Yes, they don't depend on each other, but what's the point in not\n> sorting them? I usually watch test progress visually, and if tests are\n> sorted, even with make -j4 they go more or less incrementally by their t\n> number.\n>\n> On my netbook, adding $(sort ...) adds approximately 0.008s to make\n> startup, so imho there is no performance penalty to adding that sort.\n\nHeh, who said anything about performance?\n\nI was pointing out that your justification \"we HAVE TO\" was wrong.\n\nIf you are doing this for perceived prettyness and not as a fix for any\ncorrectness issue, I want to see the patch honestly described as such;\nthat's all.\n\nBy the way, if I recall correctly, $(sort) in GNU make not just sorts but\nas a nice side effect removes duplicates. So if we used a(n fictional)\nconstruct in our Makefile like this:\n\n T = $(wildcard *.sh a.*)\n\nthat might produce duplicates (i.e. \"a.sh\" might appear twice), which\nmight leave us two identical pathnames in $T and cause us trouble. Even\nif we do not have such a use currently, rewriting $(wildcard) like your\npatch does using $(sort $(wildcard ...)) may be a good way to future-proof\nour Makefile, and if you justify your patch that way, it would be a\npossible correctness hardening, not just cosmetics, and phrasing it with\n\"HAVE TO\" may be justifiable.\n\nCare to try if $(wildcard *.sh a.*) give you duplicated output with newer\nGNU make? I am lazy but am a bit curious ;-)\n"},{"id":"182835","messageId":"20120120071936.GA22112@mini.zxlink","threadId":"29405","inReplyTo":"7vbopyhmlx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed","fromName":"Kirill Smelkov","fromEmail":"kirr@navytux.spb.ru","sentAt":"2012-01-20T07:19:36Z","receivedAt":"2012-01-20T07:19:36Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Thu, Jan 19, 2012 at 11:14:18PM -0800, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n> \n> >> I do not necessarily buy your \"so we HAVE TO, OR ELSE\".\n> >> \n> >> Even though I can understand \"We can sort the list of tests _if_ we do not\n> >> want them executed in seemingly random order when running 'make -j1'\", I\n> >> tend to think that *if* is a big one. Aren't these tests designed not to\n> >> depend on each other anyway?\n> >\n> > Yes, they don't depend on each other, but what's the point in not\n> > sorting them? I usually watch test progress visually, and if tests are\n> > sorted, even with make -j4 they go more or less incrementally by their t\n> > number.\n> >\n> > On my netbook, adding $(sort ...) adds approximately 0.008s to make\n> > startup, so imho there is no performance penalty to adding that sort.\n> \n> Heh, who said anything about performance?\n> \n> I was pointing out that your justification \"we HAVE TO\" was wrong.\n> \n> If you are doing this for perceived prettyness and not as a fix for any\n> correctness issue, I want to see the patch honestly described as such;\n> that's all.\n\nI agree about rewording.\n\n\n> By the way, if I recall correctly, $(sort) in GNU make not just sorts but\n> as a nice side effect removes duplicates. So if we used a(n fictional)\n> construct in our Makefile like this:\n> \n> T = $(wildcard *.sh a.*)\n> \n> that might produce duplicates (i.e. \"a.sh\" might appear twice), which\n> might leave us two identical pathnames in $T and cause us trouble. Even\n> if we do not have such a use currently, rewriting $(wildcard) like your\n> patch does using $(sort $(wildcard ...)) may be a good way to future-proof\n> our Makefile, and if you justify your patch that way, it would be a\n> possible correctness hardening, not just cosmetics, and phrasing it with\n> \"HAVE TO\" may be justifiable.\n> \n> Care to try if $(wildcard *.sh a.*) give you duplicated output with newer\n> GNU make? I am lazy but am a bit curious ;-)\n\nSure. Please give me time untill evening (GMT+0400), or maybe till the\nweekend.\n\n\nKirill\n"},{"id":"182944","messageId":"20120122191710.GA17366@mini.zxlink","threadId":"29405","inReplyTo":"20120120071936.GA22112@mini.zxlink","subject":"Re: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed","fromName":"Kirill Smelkov","fromEmail":"kirr@navytux.spb.ru","sentAt":"2012-01-22T19:17:10Z","receivedAt":"2012-01-22T19:17:10Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Fri, Jan 20, 2012 at 11:19:36AM +0400, Kirill Smelkov wrote:\n> On Thu, Jan 19, 2012 at 11:14:18PM -0800, Junio C Hamano wrote:\n> > Kirill Smelkov writes:\n> > \n> > >> I do not necessarily buy your \"so we HAVE TO, OR ELSE\".\n> > >> \n> > >> Even though I can understand \"We can sort the list of tests _if_ we do not\n> > >> want them executed in seemingly random order when running 'make -j1'\", I\n> > >> tend to think that *if* is a big one. Aren't these tests designed not to\n> > >> depend on each other anyway?\n> > >\n> > > Yes, they don't depend on each other, but what's the point in not\n> > > sorting them? I usually watch test progress visually, and if tests are\n> > > sorted, even with make -j4 they go more or less incrementally by their t\n> > > number.\n> > >\n> > > On my netbook, adding $(sort ...) adds approximately 0.008s to make\n> > > startup, so imho there is no performance penalty to adding that sort.\n> > \n> > Heh, who said anything about performance?\n> > \n> > I was pointing out that your justification \"we HAVE TO\" was wrong.\n> > \n> > If you are doing this for perceived prettyness and not as a fix for any\n> > correctness issue, I want to see the patch honestly described as such;\n> > that's all.\n> \n> I agree about rewording.\n> \n> \n> > By the way, if I recall correctly, $(sort) in GNU make not just sorts but\n> > as a nice side effect removes duplicates. So if we used a(n fictional)\n> > construct in our Makefile like this:\n> > \n> > T = $(wildcard *.sh a.*)\n> > \n> > that might produce duplicates (i.e. \"a.sh\" might appear twice), which\n> > might leave us two identical pathnames in $T and cause us trouble. Even\n> > if we do not have such a use currently, rewriting $(wildcard) like your\n> > patch does using $(sort $(wildcard ...)) may be a good way to future-proof\n> > our Makefile, and if you justify your patch that way, it would be a\n> > possible correctness hardening, not just cosmetics, and phrasing it with\n> > \"HAVE TO\" may be justifiable.\n> > \n> > Care to try if $(wildcard *.sh a.*) give you duplicated output with newer\n> > GNU make? I am lazy but am a bit curious ;-)\n> \n> Sure. Please give me time untill evening (GMT+0400), or maybe till the\n> weekend.\n\nHello up there again. You are actually right about sort also working as uniq,\ne.g. for the following Makefile\n\n\tT\t:= $(wildcard *.sh a.*)\n\t$(info \"T : $T\")\n\t$(info \"sort(T): $(sort $T)\")\n\t$(error 1)\n\nI'm getting duplicates for a.sh and $(sort) removes it\n\n\t$ ls\n\t0.sh a.sh b.sh c.sh Makefile\n\t\n\t$ make -v |head -1\n\tGNU Make 3.82.90\n\t\n\t$ make\t# v v\n\t\"T : 0.sh c.sh a.sh b.sh a.sh\"\n\t\"sort(T): 0.sh a.sh b.sh c.sh\"\n\tMakefile:4: *** 1. Stop.\n\n\nBUT for older make the duplicate is there too:\n\n\t$ /usr/bin/make -v | head -1\n\tGNU Make 3.81\t\t\t\t# this one has its base from 2006\n\n\t$ /usr/bin/make # v v\n\t\"T : 0.sh a.sh b.sh c.sh a.sh\"\n\t\"sort(T): 0.sh a.sh b.sh c.sh\"\n\tMakefile:4: *** 1. Stop.\n\n\nso yes earlier $(wildcard) sorted the result and no, it sorted it not globally,\nbut separately for each pattern, so in presence of multiple pattern one could\nnot rely on implicit auto-uniq even for older make.\n\nIf we'd like to protect ourselves from duplicates, the sort should be there for\nall makes.\n\n\nUpdated patch follows (sorry for my bad english, I'm too sleepy to get this\ninto shape even by mine standards...)\n\n---- 8< ----\nFrom: Kirill Smelkov \nDate: Sun, 4 Sep 2011 00:41:21 +0400\nSubject: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed\n\nStarting from GNU Make 3.82 $(wildcard ...) no longer sorts the result\n(from NEWS):\n\n * WARNING: Backward-incompatibility!\n Wildcards were not documented as returning sorted values, but the results\n have been sorted up until this release.. If your makefiles require sorted\n results from wildcard expansions, use the $(sort ...) function to request\n it explicitly.\n\n http://repo.or.cz/w/make.git/commitdiff/2a59dc32aaf0681dec569f32a9d7ab88a379d34f\n\nI usually watch test progress visually, and if tests are sorted, even\nwith make -j4 they go more or less incrementally by their t number. On\nthe other side, without sorting, tests are executed in seemingly random\norder even for -j1. Let's please maintain sane tests order for perceived\nprettyness.\n\n\nAnother note is that in GNU Make sort also works as uniq, so after sort\nbeing removed, we might expect e.g. $(wildcard *.sh a.*) to produce\nduplicates for e.g. \"a.sh\". From this point of view, adding sort could\nbe seen as hardening t/Makefile from accidentally introduced dups.\n\nIt turned out that prevous releases of GNU Make did not perform full\nsort in $(wildcard), only sorting results for each pattern, that's why\nexplicit sort-as-uniq is relevant even for older makes.\n\nSigned-off-by: Kirill Smelkov \n---\n t/Makefile | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 9046ec9..66ceefe 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -17,9 +17,9 @@ DEFAULT_TEST_TARGET ?= test\n # Shell quote;\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n \n-T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)\n-TSVN = $(wildcard t91[0-9][0-9]-*.sh)\n-TGITWEB = $(wildcard t95[0-9][0-9]-*.sh)\n+T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n+TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))\n+TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n \n all: $(DEFAULT_TEST_TARGET)\n \n-- \n1.7.9.rc2.124.ge3180\n"}]}