Re: [PATCH] iotests: Work around failing readlink -f

[Max Reitz]

On 14.09.20 14:51, Peter Maydell wrote:
> On Mon, 14 Sep 2020 at 13:32, Max Reitz <mreitz@redhat.com> wrote:
>>
>> On 14.09.20 14:31, Peter Maydell wrote:
>>> On Mon, 14 Sep 2020 at 12:39, Max Reitz <mreitz@redhat.com> wrote:
>>>>
>>>> On macOS, (out of the box) readlink does not have -f.  If the recent
>>>> "readlink -f" call introduced by b1cbc33a397 fails, just fall back to
>>>> the old behavior (which means you can run the iotests only from the
>>>> build tree, but that worked fine for six years, so it should be fine
>>>> still).
>>>>
>>>> Keep any potential error message on stderr.  If users want to run the
>>>> iotests from outside the build tree, this may point them to what's wrong
>>>> (with their system).
>>>>
>>>> Fixes: b1cbc33a3971b6bb005d5ac3569feae35a71de0f
>>>>        ("iotests: Allow running from different directory")
>>>> Reported-by: Claudio Fontana <cfontana@suse.de>
>>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>> Hi Thomas,
>>>>
>>>> I thought this would be quicker than writing a witty response on whether
>>>> you or me should write this patch. O:)
>>>> ---
>>>>  tests/qemu-iotests/check | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>>> index e14a1f354d..75675e1a18 100755
>>>> --- a/tests/qemu-iotests/check
>>>> +++ b/tests/qemu-iotests/check
>>>> @@ -45,6 +45,10 @@ then
>>>>      fi
>>>>      source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to 
>>>> enter source tree"
>>>>      build_iotests=$(readlink -f $(dirname "$0"))
>>>> +    if [ "$?" -ne 0 ]; then
>>>> +        # Perhaps -f is unsupported, revert to pre-b1cbc33a397 behavior
>>>> +        build_iotests=$PWD
>>>> +    fi
>>>>  else
>>>
>>> This still prints
>>>   readlink: illegal option -- f
>>>   usage: readlink [-n] [file ...]
>>>
>>> (you can see it in the build log that Thomas links to).
>>>
>>>    build_iotests=$(readlink -f $(dirname "$0") 2>/dev/null)
>>>
>>> should avoid that, I think.
>>
>> I mentioned in the commit message that I find this useful and desirable
>> behavior.  Something isn’t working that perhaps users are expecting to
>> work (because it will work on other systems), so I don’t think the error
>> message should be suppressed.
> 
> I disagree. Either iotests can handle readlink not having '-f',
> in which case it should not let readlink spew junk to the log,
> or it can't, in which case it should bail out.

I find this a bit complicated, because we can’t exactly handle readlink
without -f.  We can fall back to the old behavior on such systems, which
I think is good enough and I assume you think, too.

> If you want to tell the user something, you should catch the
> failure and print something yourself, because then you can do
> so with a message that will make sense to somebody trying to
> run the test suite and point them in the direction of what
> they can do to deal with the problem, eg something like
>  "readlink version doesn't support '-f'. Assuming that iotests
>   are being run from the build tree. If this isn't true then
>   we will fail later on: you can either run from the build tree,
>   or install a newer readlink."

Doesn’t sound bad, it isn’t “bail out”, though, so I don’t fully
understand how this relates to your paragraph above.  (Because it seems
like you suggest printing this unconditionally.  I think it would be
better to print it only if readlink -f failed, and then check finds $PWD
isn’t $build_tree/tests/qemu-iotests.  But...)

> Printing "readlink: illegal option" is just going to cause
> people to assume QEMU's scripts are broken and send us bug
> reports, so please don't do that.

(That’s absolutely true.)

...given everything, I think the best is then to indeed just suppress
readlink’s error message.  If readlink doesn’t work, and build_iotests
defaults to $PWD, and $PWD is not the build tree, then you’ll end up
with the six-year-old error message “(make sure the qemu-iotests are run
from tests/qemu-iotests in the build tree)”.  Because doing so is
probably easier anyway than trying to find a readlink that works.

Max

signature.asc
Description: OpenPGP digital signature