Issues with the testsuite idiom ". ./defs || Exit 1" (was: Re: [Automake-NG] [PATCH 0/7] Move detection of possible typos in _SOURCES etc. at make runtime)

[Stefano Lattarini]

[adding automake-patches]

Hi Akim.

On 06/06/2012 11:47 AM, Akim Demaille wrote:
> 
> Le 6 juin 2012 à 00:21, Stefano Lattarini a écrit :
> 
>> Stefano Lattarini (7):
>>  [ng] coverage: conditional defn of lib_LIBRARIES and lib_LTLIBRARIES
>>  [ng] automake: new global variable '%known_ltlibraries'
>>  [ng] refactor: new make variables am__all_libs and am__all_ltlibs
>>  [ng] warns: typos in _SOURCES etc. reported at make runtime
>>  [ng] warns: typos in '_DEPENDENCIES' variables are now reported
>>  [ng] warns: also report typos for 'LOG_DEPENDENCIES' variables
>>  [ng] cleanup: unused variable in the automake script removed
>>
>> Makefile.am                          |    1 +
>> automake.in                          |   67 ++++++++-----------
>> lib/am/check-typos.am                |   84 ++++++++++++++++++++++++
>> lib/am/header-vars.am                |    8 +++
>> lib/am/parallel-tests.am             |    3 +
>> t/{all-progs.sh => all-prog-libs.sh} |   33 ++++++++--
>> t/cond30.sh                          |   35 ++++++++--
>> t/spell.sh                           |   29 +++++++-
>> t/spell2.sh                          |   33 ++++++++--
>> t/vartypo2.sh                        |   63 ------------------
>> t/vartypos-deps.sh                   |  103 +++++++++++++++++++++++++++++
>> t/vartypos.sh                        |  120 
>> +++++++++++++++++++++-------------
>> t/warnopts.sh                        |   27 ++++----
>> 13 files changed, 429 insertions(+), 177 deletions(-)
> 
> Hi Stefano!
> 
> It looks good.
> 
> Maybe you should add failing cases for ancillary
> functions such as errgrep to make sure they do
> their job?  Say
> 
> ! errgrep FAILURE
>
That would seem really overkill to me (and if we were going for it anyway,
there would be dozens of similar usages in other test cases that would
require a similar "sanity check").  A similar sanity check is only warranted
for more complex usages IMHO, not for a simple grep.

> Also, is it ./defs that set -e?
>
Absolutely.  If it weren't, 99% of the tests would be prone to false
negatives (brrr!)

> Otherwise I don't see how errgrep really checks something.  Or
> is it just for the logs?
> 

> I don't understand well
> 
> . ./defs || Exit 1
> 
> I suppose it is defs that defines Exit, so this will
> not work properly if one fails to find ./defs.  If the
> point is to make sure that ./defs did not fail inside,
> why wouldn't Exit be called from inside?
>
I have no idea :-)  I was just copying the pre-existing idiom used in
all the testcase since a long time (at least, since I've been around
here).

Still, I don't know of any POSIX shell that does not bail out (at least
in non-interactive mode) when trying to source a non-existing file with
the '.' built-in.

Moreover, since './defs' calls 'set -e' early, any unexpected failure in
there should cause the test case to exit with error.

So simply using '. ./defs' in our test cases should be enough after all.

If we go for such a change, it should be done for mainline Automake
first, like this:

  - in 'maint', write a a new maintainer check that warns about an
    usages like ". ./defs || ..."; commit it.
  - in maint, edit all the test cases to switch from the older idiom
    ". ./defs || Exit 1" to the new, simpler one ". ./defs"; commit.
  - merge maint into master;
  - in master, catch any still-existing ". ./defs || Exit 1" usage,
    using the new maintainer check (such usages might still be present
    because some tests present in master are not also into maint); fix
    them, commit.

Then we can proceed with Automake-NG:

  - merge master into ng/master;
  - in ng/master, catch any still-existing ". ./defs || Exit 1" usage,
    again using the new maintainer check (there will be several such
    usages because of the tests present in ng/master that are not also
    in master); fix them, commit.

Care to write a patch series implementing the tests above? ;-)

Regards,
  Stefano