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

From: Jeff King <peff@peff.net>
Date: Oct 8, 2021, 03:07 UTC
Message-ID: <YV+1/0b5bN3o6qRG@coredump.intra.peff.net>
In-Reply-To: <20211007184716.1187677-1-aclopte@gmail.com>

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