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