This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Re: Testsuite races
> 2012-08-17 Jeff Law <law@redhat.com>
>
> * intl/Makefile (.../codeset.mo): New Makefile target.
Don't abbreviate target names. Use the full target name exactly as it
appears in the makefile. It's in a makefile so just "New target" is clear.
> (tst-codeset.out): Depend on .../codeset.mo. Remove explicit rule.
Here you can just say, "Depend on that." That's clear when the immediately
preceding line describes a target, and it avoids needing to repeat
something long or use an abbreviation.
> (tst-gettext3.out, tst-gettext5.out): Likewise.
> (tst-codeset-ENV): Define
Needs a period, and I prefer "New variable."
> (tst-codeset.sh): Remove.
> (tst-gettext3.sh, tst-gettext5.sh): Likewise.
These are not targets, rules, or variables in intl/Makefile.
They are files.
> +# Multiple tests use this data. Create it once to avoid racing and
> +# spurious test failures.
> +$(objpfx)domaindir/de_DE/LC_MESSAGES/codeset.mo:
> + msgfmt -o ${objpfx}codeset.mo.$$ tstcodeset.po || exit
Use $(var) not ${var} to be consistent with all our makefile code.
There is no need for "|| exit". A makefile command line that fails
always aborts the whole rule.
> + mkdir -p ${objpfx}domaindir/de_DE/LC_MESSAGES
Use $(@D) rather than repeating part of the target name.
But for this, just use $(make-target-directory) instead.
> + mv -f ${objpfx}codeset.mo.$$ $@
You can't use $$ like this in make rules. Each line runs in its own shell,
so the two expansions aren't the same. There is no need to use a uniquely
named temporary file in this sort of case. When we're considered with
atomic supersede, we use $@T and "mv -f $@T $@".
> +$(objpfx)tst-codeset.out: $(objpfx)tst-codeset $(objpfx)domaindir/de_DE/LC_MESSAGES/codeset.mo
> +$(objpfx)tst-gettext3.out: $(objpfx)tst-gettext3 $(objpfx)domaindir/de_DE/LC_MESSAGES/codeset.mo
> +$(objpfx)tst-gettext5.out: $(objpfx)tst-gettext5 $(objpfx)domaindir/de_DE/LC_MESSAGES/codeset.mo
These lines are too long. Rather than repeating the long file name (with
appropriate wrapping and backslashing), define a variable for it and use it
throughout (including in the target name above).
> +tst-codeset-ENV = LOCPATH=$(common-objpfx)localedata
> +tst-gettext3-ENV = LOCPATH=$(common-objpfx)localedata
> +tst-gettext5-ENV = LOCPATH=$(common-objpfx)localedata
Define a variable for the common setting and make each of these use it.
(We have repetition like this elsehwere, but that doesn't make it wise.)
Thanks,
Roland