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

[PATCH v2] 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).

Suppress all error messages, so in case using $PWD works out, we do not
cause the user to worry.  If it does not work, we will end up printing
the following error message anyway:

  check: failed to source common.env (make sure the qemu-iotests are run
  from tests/qemu-iotests in the build tree)

Following that hint (running check from $build_tree/tests/qemu-iotests)
will make it work, and is probably even easier than obtaining a readlink
that understands -f.

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>
---
v2: Suppress stderr (as requested and suggested by Peter)
---
 tests/qemu-iotests/check | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index e14a1f354d..3c9ccc117b 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -44,7 +44,11 @@ then
         _init_error "failed to obtain source tree name from check symlink"
     fi
     source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to enter source tree"
-    build_iotests=$(readlink -f $(dirname "$0"))
+    build_iotests=$(readlink -f $(dirname "$0") 2>/dev/null)
+    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 v2] iotests: Work around failing readlink -f

[Peter Maydell]

On Mon, 14 Sep 2020 at 15:17, 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).
>
> Suppress all error messages, so in case using $PWD works out, we do not
> cause the user to worry.  If it does not work, we will end up printing
> the following error message anyway:
>
>   check: failed to source common.env (make sure the qemu-iotests are run
>   from tests/qemu-iotests in the build tree)
>
> Following that hint (running check from $build_tree/tests/qemu-iotests)
> will make it work, and is probably even easier than obtaining a readlink
> that understands -f.
>
> 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>
> ---
> v2: Suppress stderr (as requested and suggested by Peter)
> ---
>  tests/qemu-iotests/check | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index e14a1f354d..3c9ccc117b 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -44,7 +44,11 @@ then
>          _init_error "failed to obtain source tree name from check symlink"
>      fi
>      source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to enter source tree"
> -    build_iotests=$(readlink -f $(dirname "$0"))
> +    build_iotests=$(readlink -f $(dirname "$0") 2>/dev/null)


Having woken up and actually looked at the context for what
we're doing with readlink here, my usual rune for "give me
the absolute path that this script is in" is

thisdir="$(cd "$(dirname "$0")"; pwd)"

which should be more portable than readlink. It doesn't
give quite the same behaviour if it's run via a path which
is a symlink to a directory, eg if bar/ is a symlink to
foo/ and you run a script as bar/thescript then you'll get
back /path/to/bar/ rather than /path/to/foo/, but do you
really need the path with all the symlinks followed
rather than just some valid absolute path to the build dir?

thanks
-- PMM

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

[Max Reitz]

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

On 14.09.20 16:26, Peter Maydell wrote:
> On Mon, 14 Sep 2020 at 15:17, 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).
>>
>> Suppress all error messages, so in case using $PWD works out, we do not
>> cause the user to worry.  If it does not work, we will end up printing
>> the following error message anyway:
>>
>>   check: failed to source common.env (make sure the qemu-iotests are run
>>   from tests/qemu-iotests in the build tree)
>>
>> Following that hint (running check from $build_tree/tests/qemu-iotests)
>> will make it work, and is probably even easier than obtaining a readlink
>> that understands -f.
>>
>> 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>
>> ---
>> v2: Suppress stderr (as requested and suggested by Peter)
>> ---
>>  tests/qemu-iotests/check | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index e14a1f354d..3c9ccc117b 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -44,7 +44,11 @@ then
>>          _init_error "failed to obtain source tree name from check symlink"
>>      fi
>>      source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to enter source tree"
>> -    build_iotests=$(readlink -f $(dirname "$0"))
>> +    build_iotests=$(readlink -f $(dirname "$0") 2>/dev/null)
> 
> 
> Having woken up and actually looked at the context for what
> we're doing with readlink here, my usual rune for "give me
> the absolute path that this script is in" is
> 
> thisdir="$(cd "$(dirname "$0")"; pwd)"
> 
> which should be more portable than readlink. It doesn't
> give quite the same behaviour if it's run via a path which
> is a symlink to a directory, eg if bar/ is a symlink to
> foo/ and you run a script as bar/thescript then you'll get
> back /path/to/bar/ rather than /path/to/foo/, but do you
> really need the path with all the symlinks followed
> rather than just some valid absolute path to the build dir?

Interesting.  Sounds good.

The only reason we did use readlink here was because realpath wasn’t
available on the BSDs.  We don’t really need the symlink, we just need
the full absolute path to $(dirname $0).

Thanks!

Max


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