[PATCH] iotests: Work around failing readlink -f

[PATCH] iotests: Work around failing readlink -f

[Max Reitz]

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
     # called from the source tree
     source_iotests=$PWD
-- 
2.26.2

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

[Alex Bennée]

Max Reitz <mreitz@redhat.com> writes:

> 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>

Queued to testing/next, thanks.

-- 
Alex Bennée

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

[Thomas Huth]

On 14/09/2020 13.38, Max Reitz 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
>      # called from the source tree
>      source_iotests=$PWD

Thanks, the macOS build seems to work again:

 https://cirrus-ci.com/task/5431828267925504?command=main#L7033

Tested-by: Thomas Huth <thuth@redhat.com>

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2164 bytes --]

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.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Peter Maydell]

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.

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."
(fix up the details to suit, or throw away entirely in favour
of some other text if you like).

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.

thanks
-- PMM

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4302 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]