On 09/04/2012 06:03 PM, Jim Meyering wrote:
> Stefano Lattarini wrote:
>> On 09/04/2012 05:25 PM, Jim Meyering wrote:
>>>
>>> Hi Stefano,
>>>
>>> I'm done reviewing.
>>> Here's the 17-cset series I'm ready to push.
>
> [For anyone wondering about the message to which Stefano has
> just replied, it was delayed due to size >100KiB, then rejected, since
> the only one who really needed it was Stefano, and he was a recipient. ]
>
>> I'll (re-)review it this evening, if you can wait. Otherwise, feel
>> free to just push it -- I trust you enough anyway :-)
>
> I can wait, but do not expect to get back to it until tomorrow.
> I've re-inserted this patch:
>
> tests: more resilient about tainted absolute srcdir path
>
Find my final nits below.
Thanks,
Stefano
-*-*-*-
> [PATCH 01/17] build: use 'check-local' to extend the 'check' target
>
> * tests/Makefile.am (check-local): Here, by making this depend
> on 'vc_exe_in_TESTS' ...
> (check): ... rather than this. While the old usage worked, it relied
> on an implementation detail rather than on documented behavior.
> * src/local.mk (check-local): Similarly, make this depend on
> 'check-README' and 'check-duplicate-no-install' ...
> (check): ... rather than on this.
>
s/rather than this/rather than making this depend on them/
> [PATCH 02/17] maint: avoid parsing of Makefile.am from vc_exe_in_TESTS
>
> * tests/Makefile.am (TESTS): Rename ...
> (all_tests): ... like this, so that we'll still be able to know the
> complete list of our tests even if the user override
>
s/override/overrides/
> TESTS from the command line (which he's allowed to do by the test
> harness API).
>
> [SNIP]
>
> +# Indirections required so that we'll still be able to know the
> +# complete list of our tests even if the user override TESTS from the
>
s/override TESTS/overrides TESTS/
> +# command line (which he's allowed to do by the test harness API).
> +TESTS = $(all_tests)
> +root_tests = $(all_root_tests)
> +
>
> -# Ensure that all version-controlled executable files are listed
> +# Ensure that all version-controlled filestable files are listed
> # in $(all_tests).
>
Uh? I've introduced a botched comment. It should read:
Ensure that all version-controlled files in 'tests/' that have a
suffix specified in $(TEST_EXTENSIONS) are listed in $(all_tests).
Sorry for the confusion.
> [PATCH 06/17] tests: remove the unused 'root-hint' target
>
> * tests/Makefile.am (root-hint): Here. The interested user can see
> the reasons why some tests are skipped by looking at the messages
> they display on the console; here's an excerpt:
>
> ..
>
"...", not merely ".."
> PASS: misc/id-groups.sh
> id-setgid.sh: skipped test: must be run as root
> SKIP: misc/id-setgid.sh
> PASS: misc/md5sum.pl
> ...
> PASS: df/total-verify.sh
> 2g.sh: skipped test: very expensive: disabled by default
> SKIP: du/2g.sh
> ...
>
> Clear enough, and more specific and precise that a generic "some tests
> might need to be run as root" message. An if that user is interested
>
s/An if/And if/.
> in making those tests run anyway, he'll just take a look to the README
> files to look for info. So there's no reason to pollute the stdout
> with another "hint" that is subsumed by those messages, and that might
> go unnoticed anyway.
>
> Moreover, and possibly more importantly, that hint wasn't being
> displayed anyway, even before this change! That's because the
> 'root-hint' target was listed as prerequisite for the 'check-recursive'
> target, which however was not a dependency of the 'check' target in
> 'tests/Makefile.am' (because that makefile contains no $(SUBDIRS)
> definition).
I'd replace the last two lines with this:
'tests/Makefile.am', because that file contains no $(SUBDIRS)
definition.
OK?
> Subject: [PATCH 08/17] maint: remove anachronistic syntax-check
>
> * cfg.mk (sc_no_exec_perl_coreutils): This. Our new testsuite
> layout (perl tests having '.pl' suffix, shell tests having '.sh'
> suffix) makes it basically impossible to run into the issues this
>
s/issues/issue/
> check guarded against.
> [PATCH 15/17] maint: fix syntax checks 'sc_root_tests'
>
> * cfg.mk: Don't work by trying to parse the (now gone) file
> 'tests/Makefile.am'; rather, use the contents of the new make
> variable $(all_root_tests).
>
Rather than:
of the new make variable $(all_root_tests)
I think it would be better to use:
of the make variable $(all_root_tests), introduced few commits ago
WDYT?
