threads / patch / 56626

patchperf: fix test_export with SHELL=zsh

Subject: [PATCH] perf: fix test_export with SHELL=zsh

## tl;dr

7 messages between Oct 2, 2021 and Oct 8, 2021. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

René Scharfe· Oct 2, 2021, 09:40 UTC · lore

Unlike other shells, zsh doesn't do word-splitting on variables. This is documented in https://zsh.sourceforge.io/FAQ/zshfaq03.html#31. That breaks the perf function test_export because it uses a space-separated variable as a poor man's array, and as a consequence p0000 fails with "not ok 3 - test_export works". Pass the value through an unquoted command substitution to force word-splitting even in zsh.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/perf/perf-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to t/perf/perf-lib.sh +1 −2

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index f5ed092ee5..f74cfd35d6 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -165,7 +165,7 @@ test_export () {
 '"$1"'
 ret=$?
 needles=
-for v in $test_export_
+for v in $(echo "$test_export_")
 do
 	needles="$needles;s/^$v=/export $v=/p"
 done
--
2.33.0

Johannes Altmanninger· Oct 2, 2021, 11:26 UTC · re: René Scharfe · lore

Re: [PATCH] perf: fix test_export with SHELL=zsh

On Sat, Oct 02, 2021 at 11:40:02AM +0200, René Scharfe wrote:

> Pass the value through an unquoted command substitution to force
> word-splitting even in zsh.

This seems like the wrong direction. There are probably other places where we rely on such POSIX features. Also it doesn't fix the case where users use other non-POSIX shells, say

	SHELL=/bin/fish t/perf/run p0000-perf-lib-sanity.sh

We already have solutions in place - our top-level Makefile has

	# Define SHELL_PATH to a POSIX shell if your /bin/sh is broken.
	...
	ifndef SHELL_PATH
		SHELL_PATH = /bin/sh
	endif
	...
	SHELL = $(SHELL_PATH)

Meanwhile t/Makefile uses

	SHELL_PATH ?= $(SHELL)
	TEST_SHELL_PATH ?= $(SHELL_PATH)

A related fix is to make those two consistent. I would prefer the version in the top-level Makefile, because t/Makefile gives the false illusion that $SHELL is honored:

	SHELL=garbage make -C t t4211-line-log.sh # succeeds because make sets SHELL=/bin/sh
	make SHELL=garbage -C t t4211-line-log.sh # fails

---

Anyway, for this issue we should just have t/perf/perf-lib.sh follow our convention of using ${SHELL_PATH:-/bin/sh} instead of $SHELL.

Looks like t/interop/Makefile has the same problem.

I'll prepare the patches later unless someone beats me to it :)

Johannes Altmanninger· Oct 7, 2021, 18:47 UTC · re: Johannes Altmanninger · lore

[PATCH] t/perf: do not run tests in user's $SHELL

The environment variable $SHELL is usually set to the user's interactive shell. We never use that shell for build and test scripts because it might not be a POSIX shell.

Perf tests are run inside $SHELL via a wrapper defined in t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.

Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---

Regarding the inconsistency around $(SHELL) in Makefiles: we could do something like

	-SHELL_PATH ?= $(SHELL)
	+SHELL_PATH ?= /bin/sh
	+SHELL = $(SHELL_PATH)

in some Makefiles. Though the upside (consistency & slightly easier to build with broken /bin/sh) seems fairly low, so I'd leave it be.

 t/perf/perf-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to t/perf/perf-lib.sh +1 −1

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index f5ed092ee5..e6b59ecbf7 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -157,7 +157,7 @@ test_run_perf_ () {
 	test_cleanup=:
 	test_export_="test_cleanup"
 	export test_cleanup test_export_
-	"$GTIME" -f "%E %U %S" -o test_time.$i "$SHELL" -c '
+	"$GTIME" -f "%E %U %S" -o test_time.$i "$TEST_SHELL_PATH" -c '
 . '"$TEST_DIRECTORY"/test-lib-functions.sh'
 test_export () {
 	test_export_="$test_export_ $*"

-- 
2.33.0.rc2.dirty

Jeff King· Oct 8, 2021, 03:07 UTC · re: Johannes Altmanninger · lore

Re: [PATCH] t/perf: do not run tests in user's $SHELL

On Thu, Oct 07, 2021 at 08:47:16PM +0200, Johannes Altmanninger wrote:

Show 6 quoted lines

> The environment variable $SHELL is usually set to the user's
> interactive shell. We never use that shell for build and test scripts
> because it might not be a POSIX shell.
> 
> Perf tests are run inside $SHELL via a wrapper defined in
> t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.

Yes, I think this is the right thing to do. We didn't have $TEST_SHELL_PATH back when this code was added, but I think it should have been $SHELL_PATH from the start.

I wondered if we would always have TEST_SHELL_PATH set, but we should: it is put unconditionally into GIT-BUILD-OPTIONS, and we will always load that via test-lib.sh, even if the test is run outside of "make".

Show 11 quoted lines

> ---
> 
> Regarding the inconsistency around $(SHELL) in Makefiles: we could do
> something like
> 
> 	-SHELL_PATH ?= $(SHELL)
> 	+SHELL_PATH ?= /bin/sh
> 	+SHELL = $(SHELL_PATH)
> 
> in some Makefiles. Though the upside (consistency & slightly easier to build
> with broken /bin/sh) seems fairly low, so I'd leave it be.

In general assuming that $SHELL is a valid /bin/sh replacement is questionable (e.g., if your login shell is zsh or god forbid tcsh). But I think GNU make will set SHELL=/bin/sh, rather than pick it up from the environment (probably for this exact reason).

-Peff

Johannes Altmanninger· Oct 8, 2021, 05:34 UTC · re: Jeff King · lore

Re: [PATCH] t/perf: do not run tests in user's $SHELL

On Thu, Oct 07, 2021 at 11:07:43PM -0400, Jeff King wrote:

> I wondered if we would always have TEST_SHELL_PATH set, but we should:
> it is put unconditionally into GIT-BUILD-OPTIONS, and we will always
> load that via test-lib.sh, even if the test is run outside of "make".

Yeah I was gonna add this to the commit message but didn't because it's fairly easy to check. Probably still better to include it. I can resend if that helps.

Jeff King· Oct 8, 2021, 05:41 UTC · re: Johannes Altmanninger · lore

Re: [PATCH] t/perf: do not run tests in user's $SHELL

On Fri, Oct 08, 2021 at 07:34:50AM +0200, Johannes Altmanninger wrote:

Show 8 quoted lines

> On Thu, Oct 07, 2021 at 11:07:43PM -0400, Jeff King wrote:
> 
> > I wondered if we would always have TEST_SHELL_PATH set, but we should:
> > it is put unconditionally into GIT-BUILD-OPTIONS, and we will always
> > load that via test-lib.sh, even if the test is run outside of "make".
> 
> Yeah I was gonna add this to the commit message but didn't because it's fairly
> easy to check. Probably still better to include it. I can resend if that helps.

I think it's probably fine as-is. Mostly I was talking to myself out loud to let people follow along my review process. :)

-Peff

brian m. carlson· Oct 2, 2021, 21:02 UTC · re: René Scharfe · lore

Re: [PATCH] perf: fix test_export with SHELL=zsh

On 2021-10-02 at 09:40:02, René Scharfe wrote:

Show 6 quoted lines

> Unlike other shells, zsh doesn't do word-splitting on variables.  This
> is documented in https://zsh.sourceforge.io/FAQ/zshfaq03.html#31.  That
> breaks the perf function test_export because it uses a space-separated
> variable as a poor man's array, and as a consequence p0000 fails with
> "not ok 3 - test_export works".  Pass the value through an unquoted
> command substitution to force word-splitting even in zsh.

There are a variety of places in our testsuite where zsh is broken in zsh mode. I recently sent a patch to make it do the right thing for subshells in sh mode which has not yet been released, and as far as I'm aware, with that patch, our testsuite is fully functional with zsh when it's run in sh mode.

Note that in sh mode, zsh enables the SH_WORD_SPLIT option, and this should work just fine. The easiest way to do that is to create a symlink to zsh called "sh" and invoke that.

-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA