Re: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed
On Fri, Jan 20, 2012 at 11:19:36AM +0400, Kirill Smelkov wrote:
Show 48 quoted lines
> On Thu, Jan 19, 2012 at 11:14:18PM -0800, Junio C Hamano wrote:
> > Kirill Smelkov <kirr@navytux.spb.ru> writes:
> >
> > >> I do not necessarily buy your "so we HAVE TO, OR ELSE".
> > >>
> > >> Even though I can understand "We can sort the list of tests _if_ we do not
> > >> want them executed in seemingly random order when running 'make -j1'", I
> > >> tend to think that *if* is a big one. Aren't these tests designed not to
> > >> depend on each other anyway?
> > >
> > > Yes, they don't depend on each other, but what's the point in not
> > > sorting them? I usually watch test progress visually, and if tests are
> > > sorted, even with make -j4 they go more or less incrementally by their t
> > > number.
> > >
> > > On my netbook, adding $(sort ...) adds approximately 0.008s to make
> > > startup, so imho there is no performance penalty to adding that sort.
> >
> > Heh, who said anything about performance?
> >
> > I was pointing out that your justification "we HAVE TO" was wrong.
> >
> > If you are doing this for perceived prettyness and not as a fix for any
> > correctness issue, I want to see the patch honestly described as such;
> > that's all.
>
> I agree about rewording.
>
>
> > By the way, if I recall correctly, $(sort) in GNU make not just sorts but
> > as a nice side effect removes duplicates. So if we used a(n fictional)
> > construct in our Makefile like this:
> >
> > T = $(wildcard *.sh a.*)
> >
> > that might produce duplicates (i.e. "a.sh" might appear twice), which
> > might leave us two identical pathnames in $T and cause us trouble. Even
> > if we do not have such a use currently, rewriting $(wildcard) like your
> > patch does using $(sort $(wildcard ...)) may be a good way to future-proof
> > our Makefile, and if you justify your patch that way, it would be a
> > possible correctness hardening, not just cosmetics, and phrasing it with
> > "HAVE TO" may be justifiable.
> >
> > Care to try if $(wildcard *.sh a.*) give you duplicated output with newer
> > GNU make? I am lazy but am a bit curious ;-)
>
> Sure. Please give me time untill evening (GMT+0400), or maybe till the
> weekend.
Hello up there again. You are actually right about sort also working as uniq, e.g. for the following Makefile
T := $(wildcard *.sh a.*)
$(info "T : $T")
$(info "sort(T): $(sort $T)")
$(error 1)
I'm getting duplicates for a.sh and $(sort) removes it
$ ls
0.sh a.sh b.sh c.sh Makefile
$ make -v |head -1
GNU Make 3.82.90
$ make # v v
"T : 0.sh c.sh a.sh b.sh a.sh"
"sort(T): 0.sh a.sh b.sh c.sh"
Makefile:4: *** 1. Stop.
BUT for older make the duplicate is there too:
$ /usr/bin/make -v | head -1
GNU Make 3.81 # this one has its base from 2006
$ /usr/bin/make # v v
"T : 0.sh a.sh b.sh c.sh a.sh"
"sort(T): 0.sh a.sh b.sh c.sh"
Makefile:4: *** 1. Stop.
so yes earlier $(wildcard) sorted the result and no, it sorted it not globally, but separately for each pattern, so in presence of multiple pattern one could not rely on implicit auto-uniq even for older make.
If we'd like to protect ourselves from duplicates, the sort should be there for all makes.
Updated patch follows (sorry for my bad english, I'm too sleepy to get this into shape even by mine standards...)
---- 8< ----
From: Kirill Smelkov <kirr@navytux.spb.ru>
Date: Sun, 4 Sep 2011 00:41:21 +0400
Subject: [PATCH] t/Makefile: Use $(sort ...) explicitly where needed
Starting from GNU Make 3.82 $(wildcard ...) no longer sorts the result (from NEWS):
* WARNING: Backward-incompatibility!
Wildcards were not documented as returning sorted values, but the results
have been sorted up until this release.. If your makefiles require sorted
results from wildcard expansions, use the $(sort ...) function to request
it explicitly. http://repo.or.cz/w/make.git/commitdiff/2a59dc32aaf0681dec569f32a9d7ab88a379d34f
I usually watch test progress visually, and if tests are sorted, even with make -j4 they go more or less incrementally by their t number. On the other side, without sorting, tests are executed in seemingly random order even for -j1. Let's please maintain sane tests order for perceived prettyness.
Another note is that in GNU Make sort also works as uniq, so after sort being removed, we might expect e.g. $(wildcard *.sh a.*) to produce duplicates for e.g. "a.sh". From this point of view, adding sort could be seen as hardening t/Makefile from accidentally introduced dups.
It turned out that prevous releases of GNU Make did not perform full sort in $(wildcard), only sorting results for each pattern, that's why explicit sort-as-uniq is relevant even for older makes.
Signed-off-by: Kirill Smelkov <kirr@navytux.spb.ru>
---
t/Makefile | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to t/Makefile +3 −3
diff --git a/t/Makefile b/t/Makefile
index 9046ec9..66ceefe 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -17,9 +17,9 @@ DEFAULT_TEST_TARGET ?= test
# Shell quote;
SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
-TSVN = $(wildcard t91[0-9][0-9]-*.sh)
-TGITWEB = $(wildcard t95[0-9][0-9]-*.sh)
+T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
+TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))
+TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
all: $(DEFAULT_TEST_TARGET)
--
1.7.9.rc2.124.ge3180
Kirill Smelkov