[05/12] selftests/mm: fix invocation of tests that are run via shell scripts

  We cannot depend upon git to reliably retain the executable bit on shell
scripts, or so I was told several years ago while working on this same
run_vmtests.sh script. And sure enough, things such as test_hmm.sh are
lately failing to run, due to lacking execute permissions.

A nice clean way to fix this would have been to use TEST_PROGS instead
of TEST_FILES for the .sh scripts here. That tells the selftest
framework to run these (and emit a warning if the files are not
executable, but still run them anyway).

Unfortunately, run_vmtests.sh has its own run_test() routine, which does
*not* do the right thing for shell scripts.

Fix this by explicitly adding "bash" to each of the shell script
invocations. Leave fixing the overall approach to another day.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/run_vmtests.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

On 02.06.23 03:33, John Hubbard wrote:
> We cannot depend upon git to reliably retain the executable bit on shell
> scripts, or so I was told several years ago while working on this same
> run_vmtests.sh script. And sure enough, things such as test_hmm.sh are
> lately failing to run, due to lacking execute permissions.
> 
> A nice clean way to fix this would have been to use TEST_PROGS instead
> of TEST_FILES for the .sh scripts here. That tells the selftest
> framework to run these (and emit a warning if the files are not
> executable, but still run them anyway).
> 
> Unfortunately, run_vmtests.sh has its own run_test() routine, which does
> *not* do the right thing for shell scripts.
> 
> Fix this by explicitly adding "bash" to each of the shell script
> invocations. Leave fixing the overall approach to another day.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>   tools/testing/selftests/mm/run_vmtests.sh | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> index 4893eb60d96d..8f81432e4bac 100644
> --- a/tools/testing/selftests/mm/run_vmtests.sh
> +++ b/tools/testing/selftests/mm/run_vmtests.sh
> @@ -242,18 +242,18 @@ if [ $VADDR64 -ne 0 ]; then
>   	if [ "$ARCH" == "$ARCH_ARM64" ]; then
>   		echo 6 > /proc/sys/vm/nr_hugepages
>   	fi
> -	CATEGORY="hugevm" run_test ./va_high_addr_switch.sh
> +	CATEGORY="hugevm" run_test bash ./va_high_addr_switch.sh
>   	if [ "$ARCH" == "$ARCH_ARM64" ]; then
>   		echo $prev_nr_hugepages > /proc/sys/vm/nr_hugepages
>   	fi
>   fi # VADDR64
>   
>   # vmalloc stability smoke test
> -CATEGORY="vmalloc" run_test ./test_vmalloc.sh smoke
> +CATEGORY="vmalloc" run_test bash ./test_vmalloc.sh smoke
>   
>   CATEGORY="mremap" run_test ./mremap_dontunmap
>   
> -CATEGORY="hmm" run_test ./test_hmm.sh smoke
> +CATEGORY="hmm" run_test bash ./test_hmm.sh smoke
>   
>   # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests
>   CATEGORY="madv_populate" run_test ./madv_populate

Sounds hacky, but if it gets the job done

Acked-by: David Hildenbrand <david@redhat.com>
  

On Thu, Jun 01, 2023 at 06:33:51PM -0700, John Hubbard wrote:
> We cannot depend upon git to reliably retain the executable bit on shell
> scripts, or so I was told several years ago while working on this same
> run_vmtests.sh script. And sure enough, things such as test_hmm.sh are
> lately failing to run, due to lacking execute permissions.
> 
> A nice clean way to fix this would have been to use TEST_PROGS instead
> of TEST_FILES for the .sh scripts here. That tells the selftest
> framework to run these (and emit a warning if the files are not
> executable, but still run them anyway).
> 
> Unfortunately, run_vmtests.sh has its own run_test() routine, which does
> *not* do the right thing for shell scripts.
> 
> Fix this by explicitly adding "bash" to each of the shell script
> invocations. Leave fixing the overall approach to another day.

Is it possible someone just doesn't have "bash" at all?  I used to only use
"sh" without bash installed I think, but that was not on Linux, so I'm not
sure how much that applies..

Maybe use $(SHELL)?  I saw a bunch of usage in the tree too.
  

On 6/2/23 08:34, Peter Xu wrote:
> On Thu, Jun 01, 2023 at 06:33:51PM -0700, John Hubbard wrote:
>> We cannot depend upon git to reliably retain the executable bit on shell
>> scripts, or so I was told several years ago while working on this same
>> run_vmtests.sh script. And sure enough, things such as test_hmm.sh are
>> lately failing to run, due to lacking execute permissions.
>>
>> A nice clean way to fix this would have been to use TEST_PROGS instead
>> of TEST_FILES for the .sh scripts here. That tells the selftest
>> framework to run these (and emit a warning if the files are not
>> executable, but still run them anyway).
>>
>> Unfortunately, run_vmtests.sh has its own run_test() routine, which does
>> *not* do the right thing for shell scripts.
>>
>> Fix this by explicitly adding "bash" to each of the shell script
>> invocations. Leave fixing the overall approach to another day.
> 
> Is it possible someone just doesn't have "bash" at all?  I used to only use

Well, maybe [1]. But that someone won't be running these tests as-is, because
the tests explicitly require bash, even before this patch.

> "sh" without bash installed I think, but that was not on Linux, so I'm not
> sure how much that applies..

sh invocations are for when you want to express that this script should
avoid using bash-specific things, in order to ensure portability to
other environments.

But here, the run_vmtests.sh file requires bash already, as per the
first line:

     #!/bin/bash

...which is ultimately why I decided to use bash, rather than sh here.

> 
> Maybe use $(SHELL)?  I saw a bunch of usage in the tree too.
> 

That's more of a Makefile construct that you are seeing, and only in a
few odd Makefiles. Recall that in Make, $(SHELL) has the same effect
that ${SHELL} has in bash/sh, by the way: dereferencing a variable.

And Make's "$(shell ...)" command is what is normally used to *run* a
shell command, in the kernel's build system.

Having said all that, I will take a quick look at what it would take
to shift over to the selftest framework's run_test() instead, in
order to avoid this ugly "fix".


[1] https://www.bleepingcomputer.com/news/linux/kali-linux-20204-switches-the-default-shell-from-bash-to-zsh/

thanks,
  

On 6/2/23 03:05, David Hildenbrand wrote:
> On 02.06.23 03:33, John Hubbard wrote:
>> We cannot depend upon git to reliably retain the executable bit on shell
>> scripts, or so I was told several years ago while working on this same
>> run_vmtests.sh script. And sure enough, things such as test_hmm.sh are
>> lately failing to run, due to lacking execute permissions.
>>
>> A nice clean way to fix this would have been to use TEST_PROGS instead
>> of TEST_FILES for the .sh scripts here. That tells the selftest
>> framework to run these (and emit a warning if the files are not
>> executable, but still run them anyway).

Actually, for the record (and I'll update this in v2), the above is
inaccurate, because run_vmtests.sh aspires to be the only TEST_PROGS
item here. And I see that the framework does already work if-and-only-if
invoked via Make, as in "make run_tests".

However,

a) Many people naturally expect to run test scripts without
(unnecessarily!) involving Make, and

b) Based on some experience in building and using various test
frameworks over many years, I'd claim that it's better to use shell
scripts to collect and manage tests and test scripts, rather than
involving Make. Make is a limited, specialized language and is better at
handling builds and dependencies.

So the "make run_tests" is a convenience, but it should not be the only
way to launch a test run. So we still want to fix this up.

>>
>> Unfortunately, run_vmtests.sh has its own run_test() routine, which does
>> *not* do the right thing for shell scripts.
>>
>> Fix this by explicitly adding "bash" to each of the shell script
>> invocations. Leave fixing the overall approach to another day.
>>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   tools/testing/selftests/mm/run_vmtests.sh | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
>> index 4893eb60d96d..8f81432e4bac 100644
>> --- a/tools/testing/selftests/mm/run_vmtests.sh
>> +++ b/tools/testing/selftests/mm/run_vmtests.sh
>> @@ -242,18 +242,18 @@ if [ $VADDR64 -ne 0 ]; then
>>       if [ "$ARCH" == "$ARCH_ARM64" ]; then
>>           echo 6 > /proc/sys/vm/nr_hugepages
>>       fi
>> -    CATEGORY="hugevm" run_test ./va_high_addr_switch.sh
>> +    CATEGORY="hugevm" run_test bash ./va_high_addr_switch.sh
>>       if [ "$ARCH" == "$ARCH_ARM64" ]; then
>>           echo $prev_nr_hugepages > /proc/sys/vm/nr_hugepages
>>       fi
>>   fi # VADDR64
>>   # vmalloc stability smoke test
>> -CATEGORY="vmalloc" run_test ./test_vmalloc.sh smoke
>> +CATEGORY="vmalloc" run_test bash ./test_vmalloc.sh smoke
>>   CATEGORY="mremap" run_test ./mremap_dontunmap
>> -CATEGORY="hmm" run_test ./test_hmm.sh smoke
>> +CATEGORY="hmm" run_test bash ./test_hmm.sh smoke
>>   # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests
>>   CATEGORY="madv_populate" run_test ./madv_populate
> 
> Sounds hacky, but if it gets the job done
> 

Yes. It's also hacky that we can't just invoke shell scripts like normal
programs. This limitation hurts my sense of "things should be more
perfect!". :)

> Acked-by: David Hildenbrand <david@redhat.com>
> 

Thanks for the ack.
  

On Fri, Jun 02, 2023 at 12:19:17PM -0700, John Hubbard wrote:
> On 6/2/23 08:34, Peter Xu wrote:
> > On Thu, Jun 01, 2023 at 06:33:51PM -0700, John Hubbard wrote:
> > > We cannot depend upon git to reliably retain the executable bit on shell
> > > scripts, or so I was told several years ago while working on this same
> > > run_vmtests.sh script. And sure enough, things such as test_hmm.sh are
> > > lately failing to run, due to lacking execute permissions.
> > > 
> > > A nice clean way to fix this would have been to use TEST_PROGS instead
> > > of TEST_FILES for the .sh scripts here. That tells the selftest
> > > framework to run these (and emit a warning if the files are not
> > > executable, but still run them anyway).
> > > 
> > > Unfortunately, run_vmtests.sh has its own run_test() routine, which does
> > > *not* do the right thing for shell scripts.
> > > 
> > > Fix this by explicitly adding "bash" to each of the shell script
> > > invocations. Leave fixing the overall approach to another day.
> > 
> > Is it possible someone just doesn't have "bash" at all?  I used to only use
> 
> Well, maybe [1]. But that someone won't be running these tests as-is, because
> the tests explicitly require bash, even before this patch.
> 
> > "sh" without bash installed I think, but that was not on Linux, so I'm not
> > sure how much that applies..
> 
> sh invocations are for when you want to express that this script should
> avoid using bash-specific things, in order to ensure portability to
> other environments.
> 
> But here, the run_vmtests.sh file requires bash already, as per the
> first line:
> 
>     #!/bin/bash
> 
> ...which is ultimately why I decided to use bash, rather than sh here.

That one can be easily override with $XXX run_vmtests.sh, hard-coded "bash"
in Makefiles can't, afaiu.

> 
> > 
> > Maybe use $(SHELL)?  I saw a bunch of usage in the tree too.
> > 
> 
> That's more of a Makefile construct that you are seeing, and only in a
> few odd Makefiles. Recall that in Make, $(SHELL) has the same effect
> that ${SHELL} has in bash/sh, by the way: dereferencing a variable.
> 
> And Make's "$(shell ...)" command is what is normally used to *run* a
> shell command, in the kernel's build system.
> 
> Having said all that, I will take a quick look at what it would take
> to shift over to the selftest framework's run_test() instead, in
> order to avoid this ugly "fix".

Just to mention that I was not talking about $(shell ...), but the
environment var $(SHELL), or "env | grep SHELL".

Please feel free to have a look at tools/perf/arch/x86/Makefile.

Thanks,
  

On 6/2/23 14:36, Peter Xu wrote:
...
>> But here, the run_vmtests.sh file requires bash already, as per the
>> first line:
>>
>>      #!/bin/bash
>>
>> ...which is ultimately why I decided to use bash, rather than sh here.
> 
> That one can be easily override with $XXX run_vmtests.sh, hard-coded "bash"
> in Makefiles can't, afaiu.

Yes, but then you'd have to deal with the rest of the kernel, and bash
is just completely woven into the whole thing. Just in the selftests
alone, there are dozens or hundreds of direct invocations.

$ git grep -w bash | wc -l
1146

$ cd tools/testing/selftests/

$ git grep -w bash | wc -l
560

$ git grep -w bash | grep -v '/bin' | wc -l
113

That ship really has sailed: it's not practical to expect that kind
of portability here.

...
> Just to mention that I was not talking about $(shell ...), but the
> environment var $(SHELL), or "env | grep SHELL".
> 
> Please feel free to have a look at tools/perf/arch/x86/Makefile.

Yes, but that is a *Makefile*. (And only one out of 145, the others do
not use this.) There is no use of SHELL outside of Makefiles, nor in
fact anywhere in the kernel.


thanks,