[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] iotests: Work around failing readlink -f
From: |
Max Reitz |
Subject: |
Re: [PATCH] iotests: Work around failing readlink -f |
Date: |
Mon, 14 Sep 2020 16:09:14 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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