| Message ID | 20211007184716.1187677-1-aclopte@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9ccab75608fb87d82ccec1ab71cc915d7f8dc5a0 |
| Headers | show |
| Series | t/perf: do not run tests in user's $SHELL | expand |
On Thu, Oct 07, 2021 at 08:47:16PM +0200, Johannes Altmanninger wrote: > 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". > --- > > 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
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.
On Fri, Oct 08, 2021 at 07:34:50AM +0200, Johannes Altmanninger wrote: > 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
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_ $*"
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(-)