On 14.09.20 14:51, Peter Maydell wrote: > On Mon, 14 Sep 2020 at 13:32, Max Reitz <[email protected]> wrote: >> >> On 14.09.20 14:31, Peter Maydell wrote: >>> On Mon, 14 Sep 2020 at 12:39, Max Reitz <[email protected]> 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 <[email protected]> >>>> Reported-by: Thomas Huth <[email protected]> >>>> Signed-off-by: Max Reitz <[email protected]> >>>> --- >>>> 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
