[PATCH] t: use portable wrapper for readlink(1)

[PATCH] t: use portable wrapper for readlink(1)

[Jeff King]

On Thu, Jun 03, 2021 at 03:39:53PM -0400, Jeff King wrote:

> > FWIW this broke tests on AIX because we can't assume readlink(1) exists
> > at all. See d2addc3b96 (t7800: readlink may not be available,
> > 2016-05-31) for a workaround.
> 
> Hmm. So obviously we can use a fix similar to the one in t7800 (though
> it's sufficiently complicated that I'd be tempted to wrap it in a helper
> function). There are a few other calls that could be changed, too.

Here's a patch to do that. Can you confirm that it fixes your test
failure?

-- >8 --
Subject: [PATCH] t: use portable wrapper for readlink(1)

Not all systems have a readlink program available for use by the shell.
This causes t3210 to fail on at least AIX. Let's provide a perl
one-liner to do the same thing, and use it there.

I also updated calls in t9802. Nobody reported failure there, but it's
the same issue. Presumably nobody actually tests with p4 on AIX in the
first place (if it is even available there).

I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
valgrind isn't available on exotic platforms anyway (and I didn't want
to increase dependencies between test-lib.sh and test-lib-functions.sh).

There's one other curious case. Commit d2addc3b96 (t7800: readlink may
not be available, 2016-05-31) fixed a similar case. We can't use our
wrapper function there, though, as it's inside a sub-script triggered by
Git. It uses a slightly different technique ("ls" piped to "sed"). I
chose not to use that here as it gives confusing "ls -l" output if the
file is unexpectedly not a symlink (which is OK for its limited use, but
potentially confusing for general use within the test suite). The perl
version emits the empty string.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3210-pack-refs.sh       | 2 +-
 t/t9802-git-p4-filetype.sh | 4 ++--
 t/test-lib-functions.sh    | 6 ++++++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 3b7cdc56ec..577f32dc71 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
 	git for-each-ref >all-refs-packed &&
 	test_cmp all-refs-before all-refs-packed &&
 	test -h .git/packed-refs &&
-	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
+	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
 '
 
 test_done
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 94edebe272..19073c6e9f 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
 	(
 		cd "$git" &&
 		test -L symlink &&
-		test $(readlink symlink) = symlink-target
+		test $(test_readlink symlink) = symlink-target
 	)
 '
 
@@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
 	git p4 clone --dest="$git" //depot@all &&
 	(
 		cd "$git" &&
-		test $(readlink empty-symlink) = target2
+		test $(test_readlink empty-symlink) = target2
 	)
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b823c14027..661f376077 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1692,3 +1692,9 @@ test_region () {
 
 	return 0
 }
+
+# Print the destination of symlink(s) provided as arguments. Basically
+# the same as the readlink command, but it's not available everywhere.
+test_readlink () {
+	perl -le 'print readlink($_) for @ARGV' "$@"
+}
-- 
2.32.0.rc3.525.gfa939c0632

Re: [PATCH] t: use portable wrapper for readlink(1)

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 03 2021, Jeff King wrote:

> On Thu, Jun 03, 2021 at 03:39:53PM -0400, Jeff King wrote:
>
>> > FWIW this broke tests on AIX because we can't assume readlink(1) exists
>> > at all. See d2addc3b96 (t7800: readlink may not be available,
>> > 2016-05-31) for a workaround.
>> 
>> Hmm. So obviously we can use a fix similar to the one in t7800 (though
>> it's sufficiently complicated that I'd be tempted to wrap it in a helper
>> function). There are a few other calls that could be changed, too.
>
> Here's a patch to do that. Can you confirm that it fixes your test
> failure?

It does, thanks, t3210*.sh is broken on current master on AIX, fixed
with this patch. I don't have p4 on that machine (or any box I hack on),
so I can't test t9802*.sh.

> -- >8 --
> Subject: [PATCH] t: use portable wrapper for readlink(1)
>
> Not all systems have a readlink program available for use by the shell.
> This causes t3210 to fail on at least AIX. Let's provide a perl
> one-liner to do the same thing, and use it there.
>
> I also updated calls in t9802. Nobody reported failure there, but it's
> the same issue. Presumably nobody actually tests with p4 on AIX in the
> first place (if it is even available there).
>
> I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
> valgrind isn't available on exotic platforms anyway (and I didn't want
> to increase dependencies between test-lib.sh and test-lib-functions.sh).
>
> There's one other curious case. Commit d2addc3b96 (t7800: readlink may
> not be available, 2016-05-31) fixed a similar case. We can't use our
> wrapper function there, though, as it's inside a sub-script triggered by
> Git. It uses a slightly different technique ("ls" piped to "sed"). I
> chose not to use that here as it gives confusing "ls -l" output if the
> file is unexpectedly not a symlink (which is OK for its limited use, but
> potentially confusing for general use within the test suite). The perl
> version emits the empty string.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t3210-pack-refs.sh       | 2 +-
>  t/t9802-git-p4-filetype.sh | 4 ++--
>  t/test-lib-functions.sh    | 6 ++++++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index 3b7cdc56ec..577f32dc71 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
>  	git for-each-ref >all-refs-packed &&
>  	test_cmp all-refs-before all-refs-packed &&
>  	test -h .git/packed-refs &&
> -	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> +	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
>  '
>  
>  test_done
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> index 94edebe272..19073c6e9f 100755
> --- a/t/t9802-git-p4-filetype.sh
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
>  	(
>  		cd "$git" &&
>  		test -L symlink &&
> -		test $(readlink symlink) = symlink-target
> +		test $(test_readlink symlink) = symlink-target
>  	)
>  '
>  
> @@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
>  	git p4 clone --dest="$git" //depot@all &&
>  	(
>  		cd "$git" &&
> -		test $(readlink empty-symlink) = target2
> +		test $(test_readlink empty-symlink) = target2
>  	)
>  '
>  
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index b823c14027..661f376077 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1692,3 +1692,9 @@ test_region () {
>  
>  	return 0
>  }
> +
> +# Print the destination of symlink(s) provided as arguments. Basically
> +# the same as the readlink command, but it's not available everywhere.
> +test_readlink () {
> +	perl -le 'print readlink($_) for @ARGV' "$@"
> +}

[PATCH] t: use portable wrapper for readlink(1)

[Jeff King]

Not all systems have a readlink program available for use by the shell.
This causes t3210 to fail on at least AIX. Let's provide a perl
one-liner to do the same thing, and use it there.

I also updated calls in t9802. Nobody reported failure there, but it's
the same issue. Presumably nobody actually tests with p4 on AIX in the
first place (if it is even available there).

I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
valgrind isn't available on exotic platforms anyway (and I didn't want
to increase dependencies between test-lib.sh and test-lib-functions.sh).

There's one other curious case. Commit d2addc3b96 (t7800: readlink may
not be available, 2016-05-31) fixed a similar case. We can't use our
wrapper function there, though, as it's inside a sub-script triggered by
Git. It uses a slightly different technique ("ls" piped to "sed"). I
chose not to use that here as it gives confusing "ls -l" output if the
file is unexpectedly not a symlink (which is OK for its limited use, but
potentially confusing for general use within the test suite). The perl
version emits the empty string.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
This is a re-post that doesn't seem to have made it into "seen"; the
original[1] was buried in a thread, but Ævar reported there that it
fixes t3210 on his AIX build.

[1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/

 t/t3210-pack-refs.sh       | 2 +-
 t/t9802-git-p4-filetype.sh | 4 ++--
 t/test-lib-functions.sh    | 6 ++++++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 3b7cdc56ec..577f32dc71 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
 	git for-each-ref >all-refs-packed &&
 	test_cmp all-refs-before all-refs-packed &&
 	test -h .git/packed-refs &&
-	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
+	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
 '
 
 test_done
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 94edebe272..19073c6e9f 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
 	(
 		cd "$git" &&
 		test -L symlink &&
-		test $(readlink symlink) = symlink-target
+		test $(test_readlink symlink) = symlink-target
 	)
 '
 
@@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
 	git p4 clone --dest="$git" //depot@all &&
 	(
 		cd "$git" &&
-		test $(readlink empty-symlink) = target2
+		test $(test_readlink empty-symlink) = target2
 	)
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f0448daa74..b2810478a2 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1708,3 +1708,9 @@ test_region () {
 
 	return 0
 }
+
+# Print the destination of symlink(s) provided as arguments. Basically
+# the same as the readlink command, but it's not available everywhere.
+test_readlink () {
+	perl -le 'print readlink($_) for @ARGV' "$@"
+}
-- 
2.32.0.352.gff02c21e72

Re: [PATCH] t: use portable wrapper for readlink(1)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

On 2021-06-18 at 16:32:22, Jeff King wrote:
> Not all systems have a readlink program available for use by the shell.
> This causes t3210 to fail on at least AIX. Let's provide a perl
> one-liner to do the same thing, and use it there.
> 
> I also updated calls in t9802. Nobody reported failure there, but it's
> the same issue. Presumably nobody actually tests with p4 on AIX in the
> first place (if it is even available there).
> 
> I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
> valgrind isn't available on exotic platforms anyway (and I didn't want
> to increase dependencies between test-lib.sh and test-lib-functions.sh).
> 
> There's one other curious case. Commit d2addc3b96 (t7800: readlink may
> not be available, 2016-05-31) fixed a similar case. We can't use our
> wrapper function there, though, as it's inside a sub-script triggered by
> Git. It uses a slightly different technique ("ls" piped to "sed"). I
> chose not to use that here as it gives confusing "ls -l" output if the
> file is unexpectedly not a symlink (which is OK for its limited use, but
> potentially confusing for general use within the test suite). The perl
> version emits the empty string.
> 
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is a re-post that doesn't seem to have made it into "seen"; the
> original[1] was buried in a thread, but Ævar reported there that it
> fixes t3210 on his AIX build.
>
> [1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/

In case I didn't say it up in the previous post, this looks fine to me.
Using Perl here seems like a fine solution.  If we needed to in the
future, we could add this to test-tool and use the real readlink(2), but
we can hold off until we decide we need to.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH] t: use portable wrapper for readlink(1)

[Jeff King]

On Fri, Jun 18, 2021 at 07:13:31PM +0000, brian m. carlson wrote:

> > This is a re-post that doesn't seem to have made it into "seen"; the
> > original[1] was buried in a thread, but Ævar reported there that it
> > fixes t3210 on his AIX build.
> >
> > [1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/
> 
> In case I didn't say it up in the previous post, this looks fine to me.
> Using Perl here seems like a fine solution.  If we needed to in the
> future, we could add this to test-tool and use the real readlink(2), but
> we can hold off until we decide we need to.

Agreed on all counts (if we do want to drop perl from the test suite,
there are a lot of these little one-liners that would need converting).

Thanks.

-Peff

Re: [PATCH] t: use portable wrapper for readlink(1)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Not all systems have a readlink program available for use by the shell.
> This causes t3210 to fail on at least AIX. Let's provide a perl
> one-liner to do the same thing, and use it there.
>
> I also updated calls in t9802. Nobody reported failure there, but it's
> the same issue. Presumably nobody actually tests with p4 on AIX in the
> first place (if it is even available there).
>
> I left the use of readlink in the "--valgrind" setup in test-lib.sh, as
> valgrind isn't available on exotic platforms anyway (and I didn't want
> to increase dependencies between test-lib.sh and test-lib-functions.sh).
>
> There's one other curious case. Commit d2addc3b96 (t7800: readlink may
> not be available, 2016-05-31) fixed a similar case. We can't use our
> wrapper function there, though, as it's inside a sub-script triggered by
> Git. It uses a slightly different technique ("ls" piped to "sed"). I
> chose not to use that here as it gives confusing "ls -l" output if the
> file is unexpectedly not a symlink (which is OK for its limited use, but
> potentially confusing for general use within the test suite). The perl
> version emits the empty string.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is a re-post that doesn't seem to have made it into "seen"; the
> original[1] was buried in a thread, but Ævar reported there that it
> fixes t3210 on his AIX build.

Yeah, I think I've seen this one and thought I have queued it.
Thanks for resurrecting.

By the way, I'll be mostly offline next week and won't be back to
full speed til the end of the month (it's time to migrate in the
other direction).

> [1] https://lore.kernel.org/git/YLk0Zm2J6VOA%2Flks@coredump.intra.peff.net/
>
>  t/t3210-pack-refs.sh       | 2 +-
>  t/t9802-git-p4-filetype.sh | 4 ++--
>  t/test-lib-functions.sh    | 6 ++++++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index 3b7cdc56ec..577f32dc71 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '
>  	git for-each-ref >all-refs-packed &&
>  	test_cmp all-refs-before all-refs-packed &&
>  	test -h .git/packed-refs &&
> -	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> +	test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs"
>  '
>  
>  test_done
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> index 94edebe272..19073c6e9f 100755
> --- a/t/t9802-git-p4-filetype.sh
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
>  	(
>  		cd "$git" &&
>  		test -L symlink &&
> -		test $(readlink symlink) = symlink-target
> +		test $(test_readlink symlink) = symlink-target
>  	)
>  '
>  
> @@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '
>  	git p4 clone --dest="$git" //depot@all &&
>  	(
>  		cd "$git" &&
> -		test $(readlink empty-symlink) = target2
> +		test $(test_readlink empty-symlink) = target2
>  	)
>  '
>  
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index f0448daa74..b2810478a2 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1708,3 +1708,9 @@ test_region () {
>  
>  	return 0
>  }
> +
> +# Print the destination of symlink(s) provided as arguments. Basically
> +# the same as the readlink command, but it's not available everywhere.
> +test_readlink () {
> +	perl -le 'print readlink($_) for @ARGV' "$@"
> +}