Re: [PATCH] test-driver: optionally run in test directory

[Stefano Lattarini]

On 10/29/2013 07:30 AM, Philipp A. Hartmann wrote:
> Hi Stefano,
> 
> On 29/10/13 01:10, Stefano Lattarini wrote:
>>
>> On 10/25/2013 10:34 AM, Philipp A. Hartmann wrote:
>>> ...
>>> This patch adds an option '--change-dir={yes|no}' to the
>>> test-driver to change to the sub-directory of the given
>>> test script (directory component taken from the script
>>> name, if any) before running the script.
>>>
>> Honestly, I'm very reluctant to add more code to the test-driver
>> script for a feature that, as you noticed, is very easy to implement
>> (in an even more controlled and granular fashion) with environment
>> variables and/or simple LOG_COMPILER wrapper script.  In addition,
>> the custom test drivers support in automake (since 1.12) means that
>> you *can* easily use your own version of the script in your package,
>> without any need to patch automake directly.
> 
> I agree (based on your review below) that this functionality may not be
> fit for the core script.  Adding the needed change to the driver just
> was the quickest way for me to solve my local problem.  Then I thought
> it might be useful for others.
> 
>> Let me still do a review of your patch though; hopefully you can
>> benefit from it in case you decide to keep using the patched
>> script (instead of environment variables) in your project.
> 
> Thanks for the review.  :-)
> 
> [...]
>>> +# Optionally switch to directory of test script before running it.
>>> +if test $change_dir = yes; then
>>> +  tstbin=`basename "$@"`
>>>
>> 'basename' only take a single argument, so you should just use "$1"
>> here.  (Note that this is more a theoretical issue, since at the
>> moment there is no way the test-driver script will be passed more
>> than one non-option argument by the automake generated Makefile
>> rules).
> 
> Agreed.
> 
>>> +  # Prefer dirname, but fall back on a substitute if dirname fails.
>>>
>> If you assume 'basename' exists, you could as well just assume
>> 'dirname' exists as well.  And I don't think any system lacking
>> basename or dirname is worth supporting or worrying about today.
> 
> Hmm, I borrowed this part from install-sh, which does the same
> (unconditionally calling basename and adding dirname emulation).
>
Nice catch!  And a good occasion to remove some legacy code.
I will soon post a follow-up patch.

> [...]
>>> +  # Test script is run here (in the directory of the script).
>>> +  (cd "$tstdir" ; "./$tstbin") >$log_file 2>&1
>>>
>> This will break in VPATH rules, since it will run in the source
>> directory rather than in the build directory -- and in VPATH
>> builds, the source directory is allowed to be read-only (this
>> actually happens in "make distcheck").  I don't know how much
>> adding proper VPATH support would complicate the implementation;
>> nor have I given it much thought honestly, since personally
>> I'd just go with environment variables (this is what Automake's
>> own testsuite does).
> 
> Here you got me.  In our case, the TESTS are all compiled/generated and
> therefore not present in the source tree.
>
Oh, I didn't think about a situation where all the TESTS are both
generated and not distributed.  Then your tweaked script is legitimate
in your setup (and that explains why you didn't notice any failure in
"make distcheck")

> But of course in case of preexisting test (scripts), the
> "change dir" functionality is just plain wrong.
> 
> Thanks again for your feedback!
> 
> Greetings from Oldenburg,
>    Philipp
> 
> [...]
> 

Thanks, and best regards,
  Stefano