[Fuego] kselftest: minor fixes

[Fuego] kselftest: minor fixes

[Daniel Sangorrin]

Hi Tim,

In response to your comments I send the next patches.

[PATCH 1/2] kselftest: avoid pushd popd bashisms
[PATCH 2/2] kselftest: remove cip spec use latest linux instead

Thanks,
Daniel

[Fuego] [PATCH 1/2] kselftest: avoid pushd popd bashisms

[Daniel Sangorrin]

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/tests/Functional.kselftest/fuego_test.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/engine/tests/Functional.kselftest/fuego_test.sh b/engine/tests/Functional.kselftest/fuego_test.sh
index 81f7a4c..a3c62e9 100755
--- a/engine/tests/Functional.kselftest/fuego_test.sh
+++ b/engine/tests/Functional.kselftest/fuego_test.sh
@@ -12,7 +12,7 @@ function test_build {
 }
 
 function test_deploy {
-    pushd tools/testing/selftests
+    cd tools/testing/selftests
     mkdir fuego
     # kselftest targets (see tools/testing/selftests/Makefile)
     if [ ! -z ${FUNCTIONAL_KSELFTEST_TARGETS+x} ]; then
@@ -24,7 +24,7 @@ function test_deploy {
     fi
     put ./fuego/* $BOARD_TESTDIR/fuego.$TESTDIR/
     rm -rf fuego
-    popd
+    cd ../../../
 }
 
 function test_run {
-- 
2.7.4

[Fuego] [PATCH 2/2] kselftest: remove cip spec use latest linux instead

[Daniel Sangorrin]

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/tests/Functional.kselftest/spec.json | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/engine/tests/Functional.kselftest/spec.json b/engine/tests/Functional.kselftest/spec.json
index f6026f1..d250fef 100644
--- a/engine/tests/Functional.kselftest/spec.json
+++ b/engine/tests/Functional.kselftest/spec.json
@@ -9,9 +9,6 @@
             "targets": "exec kcmp timers",
             "fail_count": "3"
         },
-        "cip": {
-            "gitrepo": "https://github.com/cip-project/linux-cip.git"
-        },
         "template": {
             "gitrepo": "https://xxx/yyy.git",
             "gitref": "my_tag_branch_or_commit_id",
-- 
2.7.4

Re: [Fuego] [PATCH 1/2] kselftest: avoid pushd popd bashisms

[Bird, Timothy]

Applied - thanks.

I checked and pushd and popd are actually bash builtins.  So there was
no danger this would not be available on the host (we are guaranteed to
be running the fuego_test scripts on bash inside the docker container).

I was torn about accepting this patch, even though I brought the issue up.
The pushd and popd are a bit "cleaner" looking, IMHO.
However, in the end I decided to apply it since I think in general it might
be confusing to mix bash and posix shell code (the only kind supported
by Fuego on target boards) in the fuego_test.sh files.

I think it's probably good to avoid having bashisms creep into fuego_test.sh
for this reason, even though they are supported on host.
(Note - this doesn't apply to non-fuego_test.sh code (e.g. functions.sh), as
that is handled by "experts" who know what they're doing. :-)

In any event, thanks for the patch!
 -- Tim

> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Monday, November 13, 2017 10:56 PM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 1/2] kselftest: avoid pushd popd bashisms
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/tests/Functional.kselftest/fuego_test.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/engine/tests/Functional.kselftest/fuego_test.sh
> b/engine/tests/Functional.kselftest/fuego_test.sh
> index 81f7a4c..a3c62e9 100755
> --- a/engine/tests/Functional.kselftest/fuego_test.sh
> +++ b/engine/tests/Functional.kselftest/fuego_test.sh
> @@ -12,7 +12,7 @@ function test_build {
>  }
> 
>  function test_deploy {
> -    pushd tools/testing/selftests
> +    cd tools/testing/selftests
>      mkdir fuego
>      # kselftest targets (see tools/testing/selftests/Makefile)
>      if [ ! -z ${FUNCTIONAL_KSELFTEST_TARGETS+x} ]; then
> @@ -24,7 +24,7 @@ function test_deploy {
>      fi
>      put ./fuego/* $BOARD_TESTDIR/fuego.$TESTDIR/
>      rm -rf fuego
> -    popd
> +    cd ../../../
>  }
> 
>  function test_run {
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 2/2] kselftest: remove cip spec use latest linux instead

[Bird, Timothy]

Applied - thanks!
 -- Tim

> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Monday, November 13, 2017 10:56 PM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH 2/2] kselftest: remove cip spec use latest linux
> instead
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/tests/Functional.kselftest/spec.json | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/engine/tests/Functional.kselftest/spec.json
> b/engine/tests/Functional.kselftest/spec.json
> index f6026f1..d250fef 100644
> --- a/engine/tests/Functional.kselftest/spec.json
> +++ b/engine/tests/Functional.kselftest/spec.json
> @@ -9,9 +9,6 @@
>              "targets": "exec kcmp timers",
>              "fail_count": "3"
>          },
> -        "cip": {
> -            "gitrepo": "https://github.com/cip-project/linux-cip.git"
> -        },
>          "template": {
>              "gitrepo": "https://xxx/yyy.git",
>              "gitref": "my_tag_branch_or_commit_id",
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 1/2] kselftest: avoid pushd popd bashisms

[Daniel Sangorrin]

> -----Original Message-----
> From: Bird, Timothy [mailto:Tim.Bird@sony.com]
> Sent: Wednesday, November 15, 2017 1:25 PM
> To: Daniel Sangorrin; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 1/2] kselftest: avoid pushd popd bashisms
> 
> Applied - thanks.
> 
> I checked and pushd and popd are actually bash builtins.  So there was
> no danger this would not be available on the host (we are guaranteed to
> be running the fuego_test scripts on bash inside the docker container).
> 
> I was torn about accepting this patch, even though I brought the issue up.
> The pushd and popd are a bit "cleaner" looking, IMHO.
> However, in the end I decided to apply it since I think in general it might
> be confusing to mix bash and posix shell code (the only kind supported
> by Fuego on target boards) in the fuego_test.sh files.
> 
> I think it's probably good to avoid having bashisms creep into fuego_test.sh
> for this reason, even though they are supported on host.
> (Note - this doesn't apply to non-fuego_test.sh code (e.g. functions.sh), as
> that is handled by "experts" who know what they're doing. :-)
> 
> In any event, thanks for the patch!
>  -- Tim

Thanks Tim.

If you install the "devscripts" package on Debian or Ubuntu, there is a software
that allows checking for bashisms: "checkbashisms"

This software detected the pushd and popd as bashisms for example. I have checked
other scripts and I found other bashisms.
For example, in LTP there are conditions that use double square brackets that are possibly bashisms.
There is also a "possible bashism" warning about the use of the "function" keyword (saying that it is useless).
I'm not an expert on bashisms, but I think this could be a good task candidate for the next Hackathon. 

Having said that, are you sure we need to avoid bashisms in the whole fuego_test.sh?
I agree that code inside a "report" or "cmd" sentence needs to avoid bashisms at all, but that's just
a tiny piece of fuego_test.sh. Most of the code in fuego_test.sh will be executed inside docker
where we can assume to be using bash.

Thanks
Daniel

P.S.: Another task candidate would be to fix the shell's code style. I found this interesting reference:
https://google.github.io/styleguide/shell.xml
Maybe we can take it as the base coding style, and then add exceptions. 
For example: indent with 4 spaces, not 2
We would still need to differentiate code that must not have bashisms from code that may.
For example: the use of [ instead of [[ depends on that.

















> 
> > -----Original Message-----
> > From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> > bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> > Sent: Monday, November 13, 2017 10:56 PM
> > To: fuego@lists.linuxfoundation.org
> > Subject: [Fuego] [PATCH 1/2] kselftest: avoid pushd popd bashisms
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> >  engine/tests/Functional.kselftest/fuego_test.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/engine/tests/Functional.kselftest/fuego_test.sh
> > b/engine/tests/Functional.kselftest/fuego_test.sh
> > index 81f7a4c..a3c62e9 100755
> > --- a/engine/tests/Functional.kselftest/fuego_test.sh
> > +++ b/engine/tests/Functional.kselftest/fuego_test.sh
> > @@ -12,7 +12,7 @@ function test_build {
> >  }
> >
> >  function test_deploy {
> > -    pushd tools/testing/selftests
> > +    cd tools/testing/selftests
> >      mkdir fuego
> >      # kselftest targets (see tools/testing/selftests/Makefile)
> >      if [ ! -z ${FUNCTIONAL_KSELFTEST_TARGETS+x} ]; then
> > @@ -24,7 +24,7 @@ function test_deploy {
> >      fi
> >      put ./fuego/* $BOARD_TESTDIR/fuego.$TESTDIR/
> >      rm -rf fuego
> > -    popd
> > +    cd ../../../
> >  }
> >
> >  function test_run {
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH 1/2] kselftest: avoid pushd popd bashisms

[Bird, Timothy]

> -----Original Message-----
> From:  Daniel Sangorrin on Tuesday, November 14, 2017 8:49 PM
> > -----Original Message-----
> > From: Bird, Timothy [mailto:Tim.Bird@sony.com]
....
> 
> Thanks Tim.
> 
> If you install the "devscripts" package on Debian or Ubuntu, there is a
> software
> that allows checking for bashisms: "checkbashisms"

I checked it out.  It looks pretty useful.  I think it would be good
to add a self-test for Fuego that runs this on scripts.

Should I put it into the Dockerfile, and write a fuego release
test based on it?

We'd have to come up with a criteria file to ignore errors until
we get things fixed up.

> This software detected the pushd and popd as bashisms for example. I have
> checked
> other scripts and I found other bashisms.
> For example, in LTP there are conditions that use double square brackets
> that are possibly bashisms.

LTP has a bunch of scripts that have  "#!/bin/bash" in their header.
(This is not to say they are necessarily invoked with bash, but it's likely
for most of them.)  Some scripts require other oddball interpreters.

Here are some interesting greps (in an /opt/ltp directory):
$ grep -r ^#!  * | grep bin/sh | wc -l
1917 
$ grep -r ^#! * | grep bin/bash | wc -l
372
$ grep -r ^#! * | grep bin/python | wc -l
10
$ grep -r ^#! * | grep bin/expect | wc -l
5

> There is also a "possible bashism" warning about the use of the "function"
> keyword (saying that it is useless).
> I'm not an expert on bashisms, but I think this could be a good task candidate
> for the next Hackathon.
That might be worth looking at.  Can you add it as an idea on this page:
http://fuegotest.org/wiki/Japan_Fuego_Hackathon_2017

> 
> Having said that, are you sure we need to avoid bashisms in the whole
> fuego_test.sh?
> I agree that code inside a "report" or "cmd" sentence needs to avoid
> bashisms at all, but that's just
> a tiny piece of fuego_test.sh. Most of the code in fuego_test.sh will be
> executed inside docker
> where we can assume to be using bash.
That's true. However, my preference would be to avoid mixing code.  I'd
like to avoid having too much tricky stuff in fuego_test.sh.  I'd have to
see an example where the bashism was really needed, to say.

That said, some of the fuego release tests I envision might require
a bit trickier operations that most other tests (as they'll be doing
most of their work on the host side).
 -- Tim

> P.S.: Another task candidate would be to fix the shell's code style. I found
> this interesting reference:
> https://google.github.io/styleguide/shell.xml
> Maybe we can take it as the base coding style, and then add exceptions.
> For example: indent with 4 spaces, not 2
> We would still need to differentiate code that must not have bashisms from
> code that may.
> For example: the use of [ instead of [[ depends on that.
I'd be happy to adopt that, with a few exceptions.  Should we start 
a styleguide for Fuego?  Where do we put it?

 -- Tim