threads / patch / 57121

patcht/perf: do not run tests in user's $SHELL

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

## tl;dr

6 messages between Dec 20, 2021 and Dec 25, 2021. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

René Scharfe· Dec 20, 2021, 11:05 UTC · lore
From: Johannes Altmanninger <aclopte@gmail.com>

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>
Acked-by: Jeff King <peff@peff.net>
---
Original submission:
https://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/
 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 780a7402d5..407252bac7 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -161,7 +161,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.34.0
Ævar Arnfjörð Bjarmason· Dec 20, 2021, 11:56 UTC · re: René Scharfe · lore

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

On Mon, Dec 20 2021, René Scharfe wrote:
Show 14 quoted lines
> From: Johannes Altmanninger <aclopte@gmail.com>
>
> 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>
> Acked-by: Jeff King <peff@peff.net>
> ---
> Original submission:
> https://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/
This LGTM & I think it could be picked up as-is.
Just a nit in case af a re-roll. I think it would help to summarize the
history a bit per
https://lore.kernel.org/git/YV+1%2F0b5bN3o6qRG@coredump.intra.peff.net/. I.e. something
like:
    
    In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17)
    when t/perf was introduced the TEST_SHELL_PATH was not part of
    GIT-BUILD-OPTIONS. That was added later in 3f824e91c84 (t/Makefile:
    introduce TEST_SHELL_PATH, 2017-12-08). We will always have that
    available in perf-lib.sh since test-lib.sh will load it before this code
    is executed.
Show 16 quoted lines
>  t/perf/perf-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 780a7402d5..407252bac7 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -161,7 +161,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_ $*"
Johannes Altmanninger· Dec 20, 2021, 13:11 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Mon, Dec 20, 2021 at 12:56:58PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 28 quoted lines
> 
> On Mon, Dec 20 2021, René Scharfe wrote:
> 
> > From: Johannes Altmanninger <aclopte@gmail.com>
> >
> > 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>
> > Acked-by: Jeff King <peff@peff.net>
> > ---
> > Original submission:
> > https://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/
> 
> This LGTM & I think it could be picked up as-is.
> 
> Just a nit in case af a re-roll. I think it would help to summarize the
> history a bit per
> https://lore.kernel.org/git/YV+1%2F0b5bN3o6qRG@coredump.intra.peff.net/. I.e. something
> like:
>     
>     In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17)
>     when t/perf was introduced the TEST_SHELL_PATH was not part of
>     GIT-BUILD-OPTIONS.
(but SHELL_PATH was, which is what we should have used back then)
>     That was added later in 3f824e91c84 (t/Makefile:
>     introduce TEST_SHELL_PATH, 2017-12-08). We will always have that
>     available in perf-lib.sh since test-lib.sh will load it before this code
>     is executed.
yes that's a good thing to point out
Show 18 quoted lines
> 
> >  t/perf/perf-lib.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> > index 780a7402d5..407252bac7 100644
> > --- a/t/perf/perf-lib.sh
> > +++ b/t/perf/perf-lib.sh
> > @@ -161,7 +161,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_ $*"
>
Junio C Hamano· Dec 20, 2021, 21:06 UTC · re: Johannes Altmanninger · lore

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

Johannes Altmanninger <aclopte@gmail.com> writes:
Show 38 quoted lines
> On Mon, Dec 20, 2021 at 12:56:58PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Dec 20 2021, René Scharfe wrote:
>> 
>> > From: Johannes Altmanninger <aclopte@gmail.com>
>> >
>> > 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>
>> > Acked-by: Jeff King <peff@peff.net>
>> > ---
>> > Original submission:
>> > https://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/
>> 
>> This LGTM & I think it could be picked up as-is.
>> 
>> Just a nit in case af a re-roll. I think it would help to summarize the
>> history a bit per
>> https://lore.kernel.org/git/YV+1%2F0b5bN3o6qRG@coredump.intra.peff.net/. I.e. something
>> like:
>>     
>>     In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17)
>>     when t/perf was introduced the TEST_SHELL_PATH was not part of
>>     GIT-BUILD-OPTIONS.
>
> (but SHELL_PATH was, which is what we should have used back then)
>
>>     That was added later in 3f824e91c84 (t/Makefile:
>>     introduce TEST_SHELL_PATH, 2017-12-08). We will always have that
>>     available in perf-lib.sh since test-lib.sh will load it before this code
>>     is executed.
>
> yes that's a good thing to point out
Care to redo the patch in a final form, then?
Thanks.
Show 19 quoted lines
>
>> 
>> >  t/perf/perf-lib.sh | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
>> > index 780a7402d5..407252bac7 100644
>> > --- a/t/perf/perf-lib.sh
>> > +++ b/t/perf/perf-lib.sh
>> > @@ -161,7 +161,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_ $*"
>>
Johannes Altmanninger· Dec 25, 2021, 07:47 UTC · re: Junio C Hamano · lore

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

On Mon, Dec 20, 2021 at 01:06:15PM -0800, Junio C Hamano wrote:
Show 11 quoted lines
> Johannes Altmanninger <aclopte@gmail.com> writes:
> 
> > On Mon, Dec 20, 2021 at 12:56:58PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >>     That was added later in 3f824e91c84 (t/Makefile:
> >>     introduce TEST_SHELL_PATH, 2017-12-08). We will always have that
> >>     available in perf-lib.sh since test-lib.sh will load it before this code
> >>     is executed.
> >
> > yes that's a good thing to point out
> 
> Care to redo the patch in a final form, then?

Of course. I should have acked that earlier, sorry. I waited until I could send my updated version but didn't find a quiet moment until today. Anyway, sending the updated patch now.

Johannes Altmanninger· Dec 25, 2021, 08:16 UTC · re: Junio C Hamano · lore

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

The environment variable $SHELL is usually set to the user's interactive shell. Our build and test scripts never use $SHELL because there are no guarantees about its input language. Instead, we use /bin/sh which should be a POSIX shell.

For systems with a broken /bin/sh, we allow to override that path via SHELL_PATH. To run tests in yet another shell we allow to override SHELL_PATH with TEST_SHELL_PATH.

Perf tests run in $SHELL via a wrapper defined in t/perf/perf-lib.sh, so they break with e.g. SHELL=python. Use TEST_SHELL_PATH like in other tests. TEST_SHELL_PATH is always defined because t/perf/perf-lib.sh includes t/test-lib.sh, which includes GIT-BUILD-OPTIONS.

Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---
 t/perf/perf-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I extended the commit message because in hindsight it was overly terse (judging from both re-reading it and from review comments).

We could add more Acked-bys but one seems enough here.
range-diff to the first version:
    @@ Commit message
         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.
    +    interactive shell. Our build and test scripts never use $SHELL because
    +    there are no guarantees about its input language.  Instead, we use
    +    /bin/sh which should 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.
    +    For systems with a broken /bin/sh, we allow to override that path via
    +    SHELL_PATH.  To run tests in yet another shell we allow to override
    +    SHELL_PATH with TEST_SHELL_PATH.
    +
    +    Perf tests run in $SHELL via a wrapper defined in t/perf/perf-lib.sh,
    +    so they break with e.g. SHELL=python.  Use TEST_SHELL_PATH like
    +    in other tests.  TEST_SHELL_PATH is always defined because
    +    t/perf/perf-lib.sh includes t/test-lib.sh, which includes
    +    GIT-BUILD-OPTIONS.
    +
    +    Acked-by: Jeff King <peff@peff.net>
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 780a7402d5..407252bac7 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -161,7 +161,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.34.1

← back to recent threads