tests/qemu-iotests/check | 4 ++++ 1 file changed, 4 insertions(+)
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
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
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
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
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
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
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>
© 2016 - 2024 Red Hat, Inc.