[PATCH 1/1] Improve missing mods error message and make shell script executable

[PATCH 1/1] Improve missing mods error message and make shell script executable

[David Hunter]

Make the test executable. Currently, tests in this shell script are not
executable, so the scipt file is skipped entirely.

Also, the error message descirbing the required modules is inaccurate.
Currently, only  "SKIP: Need act_mirred module" is shown. As a result,
users might only that module; however, three modules are actually
required and if any of them are missing, the build will fail with the
same message.

Fix the error message to show any/all modules needed for the script file
upon failure.

Signed-off-by: David Hunter <david.hunter.linux@gmail.com>
---
 .../testing/selftests/net/test_ingress_egress_chaining.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 tools/testing/selftests/net/test_ingress_egress_chaining.sh

diff --git a/tools/testing/selftests/net/test_ingress_egress_chaining.sh b/tools/testing/selftests/net/test_ingress_egress_chaining.sh
old mode 100644
new mode 100755
index 08adff6bb3b6..b1a3d68e0ec2
--- a/tools/testing/selftests/net/test_ingress_egress_chaining.sh
+++ b/tools/testing/selftests/net/test_ingress_egress_chaining.sh
@@ -13,8 +13,14 @@ if [ "$(id -u)" -ne 0 ];then
 fi
 
 needed_mods="act_mirred cls_flower sch_ingress"
+mods_missing=""
+
+for mod in $needed_mods; do 
+	modinfo $mod &>/dev/null || mods_missing="$mods_missing$mod "
+done
+
 for mod in $needed_mods; do
-	modinfo $mod &>/dev/null || { echo "SKIP: Need act_mirred module"; exit $ksft_skip; }
+	modinfo $mod &>/dev/null || { echo "SKIP: modules needed: $mods_missing"; exit $ksft_skip; }
 done
 
 ns="ns$((RANDOM%899+100))"
-- 
2.43.0

Re: [PATCH 1/1] Improve missing mods error message and make shell script executable

[Jakub Kicinski]

On Tue, 20 Aug 2024 16:21:16 -0400 David Hunter wrote:
> Subject: [PATCH 1/1] Improve missing mods error message and make shell script executable

Would be good to add a prefix to the subject:

selftests: net:

> Make the test executable. Currently, tests in this shell script are not
> executable, so the scipt file is skipped entirely.

Could you clarify how it gets skipped? We use make [...] run_tests
in our CI and it does seem to run.

> Also, 

If you say "also" there's a good chance the commit should be split into
two..

> the error message descirbing the required modules is inaccurate.
> Currently, only  "SKIP: Need act_mirred module" is shown. As a result,
> users might only that module; however, three modules are actually
> required and if any of them are missing, the build will fail with the
> same message.
> 
> Fix the error message to show any/all modules needed for the script file
> upon failure.
> 
> Signed-off-by: David Hunter <david.hunter.linux@gmail.com>
> ---
>  .../testing/selftests/net/test_ingress_egress_chaining.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>  mode change 100644 => 100755 tools/testing/selftests/net/test_ingress_egress_chaining.sh
> 
> diff --git a/tools/testing/selftests/net/test_ingress_egress_chaining.sh b/tools/testing/selftests/net/test_ingress_egress_chaining.sh
> old mode 100644
> new mode 100755
> index 08adff6bb3b6..b1a3d68e0ec2
> --- a/tools/testing/selftests/net/test_ingress_egress_chaining.sh
> +++ b/tools/testing/selftests/net/test_ingress_egress_chaining.sh
> @@ -13,8 +13,14 @@ if [ "$(id -u)" -ne 0 ];then
>  fi
>  
>  needed_mods="act_mirred cls_flower sch_ingress"
> +mods_missing=""
> +
> +for mod in $needed_mods; do 
> +	modinfo $mod &>/dev/null || mods_missing="$mods_missing$mod "
> +done
> +
>  for mod in $needed_mods; do

Do you have to loop again? Maybe just check if mods_missing is empty?

> -	modinfo $mod &>/dev/null || { echo "SKIP: Need act_mirred module"; exit $ksft_skip; }
> +	modinfo $mod &>/dev/null || { echo "SKIP: modules needed: $mods_missing"; exit $ksft_skip; }
>  done
-- 
pw-bot: cr

Re: Re: [PATCH 1/1] Improve missing mods error message and make shell script executable

[David Hunter]

> If you say "also" there's a good chance the commit should be split into two..

I am splitting original patch into 2 separate patches. I forgot to do the reply all command on kernel lore.  Here is the link to version 2 for the improving the missing modules error message:

https://lore.kernel.org/all/20240823054833.144612-1-david.hunter.linux@gmail.com/
Subject: [PATCH 1/1 V2] Selftests: net: Improve missing modules error message

> Could you clarify how it gets skipped? We use make [...] run_tests in our CI and it does seem to run.

Here is my set up:

$ uname -a 

- Linux dshunter-HP-Laptop-15-dy5xxx 6.11.0-rc2+ #2 SMP PREEMPT_DYNAMIC Tue Aug 20 14:31:34 EDT 2024 x86_64 x86_64 x86_64 GNU/Linux

Steps I took to produce the error:
	- use git clone to get the mainline source
	- run make -C tools/testing/selftests
	- make summary=1 -C tools/testing/selftests TARGETS=net run_tests

Output: 
# selftests: net: test_ingress_egress_chaining.sh
# Warning: file test_ingress_egress_chaining.sh is not executable

After running chmod +x on the shell script. The tests were able to be run.

Thanks, 
David 

[PATCH 1/1 V2] Selftests: net: Set executable bit for shell script

[David Hunter]

Turn on the execution bit for the shell script file. The test is skipped
when downloaded from the linux_mainline source files.

Signed-off-by: David Hunter <david.hunter.linux@gmail.com>
---
V1 --> V2 
	- Split the patch into two separate patches (one for each issue)
	- Included subject prefixes
---
 tools/testing/selftests/net/test_ingress_egress_chaining.sh | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 tools/testing/selftests/net/test_ingress_egress_chaining.sh

diff --git a/tools/testing/selftests/net/test_ingress_egress_chaining.sh b/tools/testing/selftests/net/test_ingress_egress_chaining.sh
old mode 100644
new mode 100755
-- 
2.43.0

Re: [PATCH 1/1 V2] Selftests: net: Set executable bit for shell script

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Sat, 24 Aug 2024 10:38:37 -0400 you wrote:
> Turn on the execution bit for the shell script file. The test is skipped
> when downloaded from the linux_mainline source files.
> 
> Signed-off-by: David Hunter <david.hunter.linux@gmail.com>
> ---
> V1 --> V2
> 	- Split the patch into two separate patches (one for each issue)
> 	- Included subject prefixes
> 
> [...]

Here is the summary with links:
  - [1/1,V2] Selftests: net: Set executable bit for shell script
    https://git.kernel.org/netdev/net-next/c/39e8111ce5ce

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 1/1 V2] Selftests: net: Set executable bit for shell script

[Jakub Kicinski]

On Sat, 24 Aug 2024 10:38:37 -0400 David Hunter wrote:
> Subject: [PATCH 1/1 V2] Selftests: net: Set executable bit for shell script

no need to capitalize Selftests

> Turn on the execution bit for the shell script file. The test is skipped
> when downloaded from the linux_mainline source files.

Change makes sense but I don't understand the commit message.
What is linux_mainline and how does one download from it?
I see an Arch package with similar name is that what you mean?

BTW ignore the pw-bot it gets confused by mode changes
-- 
pw-bot: cr

Re: [PATCH 1/1 V2] Selftests: net: Set executable bit for shell script

[David Hunter]

From: david.hunter.linux@gmail.com

On  Mon, 26 Aug 2024 14:40:22 -0700 Jakub Kicinski wrote:
> What is linux_mainline and how does one download from it?

The Linux Mainline source files can be downloaded using the following command:

git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git linux_mainline

I meant to say "Linux Mainline". In my computer, I put the title for the directory as "linux_mainline", so I have a habit of putting the underscore when I should not. 

Re: [PATCH 1/1 V2] Selftests: net: Set executable bit for shell script

[Jakub Kicinski]

On Tue, 27 Aug 2024 17:37:21 -0400 David Hunter wrote:
> On  Mon, 26 Aug 2024 14:40:22 -0700 Jakub Kicinski wrote:
> > What is linux_mainline and how does one download from it?  
> 
> The Linux Mainline source files can be downloaded using the following command:
> 
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git linux_mainline
> 
> I meant to say "Linux Mainline". In my computer, I put the title for
> the directory as "linux_mainline", so I have a habit of putting the
> underscore when I should not. 

I see. But going back to the commit message - why would the files be
skipped? At some point you shared a warning which gets printed:

 # Warning: file test_ingress_egress_chaining.sh is not executable

that's a better thing to put in the commit message, than talking 
about downloading.

Keep in mind you need to put a space or two before the # otherwise
git will think it's a comment.