[Fuego] [PATCH] need_check: add linux distro automatic detection

[Fuego] [PATCH] need_check: add linux distro automatic detection

[Daniel Sangorrin]

This function can be used by tests that require different
commands depending on the target board distribution.
The automatic detection can be disabled by manually
defining the value for BOARD_DISTRO_ID and
BOARD_DISTRO_VERSION_ID.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 engine/scripts/need_check.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/engine/scripts/need_check.sh b/engine/scripts/need_check.sh
index 1fd6d6d..a6e5182 100755
--- a/engine/scripts/need_check.sh
+++ b/engine/scripts/need_check.sh
@@ -123,6 +123,33 @@ function get_free_storage {
   echo $result
 }
 
+# This function detects the Linux distribution used on the target board and
+# exports the following variables:
+#    BOARD_DISTRO_ID (e.g. ubuntu, debian, centos)
+#    BOARD_DISTRO_VERSION_ID (e.g. "16.04", "8", "7")
+#
+# If the distribution cannot be automatically detected it will return 1.
+# Defining BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID on the board file
+# disables automatic detection.
+function check_distro {
+    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ]; then
+        if cmd "test -f /etc/os-release" ; then
+            os_release_tmpfile=$(mktemp)
+            get "/etc/os-release" $os_release_tmpfile
+            export BOARD_DISTRO_ID="$(awk -F  "=" '/^ID=/ {print $2}' $os_release_tmpfile)"
+            export BOARD_DISTRO_VERSION_ID="$(awk -F  "=" '/^VERSION_ID=/ {print $2}' $os_release_tmpfile)"
+        fi
+    fi
+
+    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ]; then
+            echo "ERROR: could not detect the distribution running on the board."
+            echo "Define BOARD_DISTRO_(ID|VERSION_ID) on your board file."
+            return 1
+    else
+        echo "Board distribution is $BOARD_DISTRO_ID $BOARD_DISTRO_VERSION_ID"
+    fi
+}
+
 # $1 has a single string holding a space-separated args:
 #   arg1 = size
 #   arg2 = directory to check
-- 
2.7.4

Re: [Fuego] [PATCH] need_check: add linux distro automatic detection

[Daniel Sangorrin]

Hi Tim,

If the patch is OK, could you please add these variables to the documentation as well?

Thanks,
Daniel

> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-bounces@lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Thursday, January 11, 2018 1:13 PM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH] need_check: add linux distro automatic detection
> 
> This function can be used by tests that require different
> commands depending on the target board distribution.
> The automatic detection can be disabled by manually
> defining the value for BOARD_DISTRO_ID and
> BOARD_DISTRO_VERSION_ID.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
>  engine/scripts/need_check.sh | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/engine/scripts/need_check.sh b/engine/scripts/need_check.sh
> index 1fd6d6d..a6e5182 100755
> --- a/engine/scripts/need_check.sh
> +++ b/engine/scripts/need_check.sh
> @@ -123,6 +123,33 @@ function get_free_storage {
>    echo $result
>  }
> 
> +# This function detects the Linux distribution used on the target board and
> +# exports the following variables:
> +#    BOARD_DISTRO_ID (e.g. ubuntu, debian, centos)
> +#    BOARD_DISTRO_VERSION_ID (e.g. "16.04", "8", "7")
> +#
> +# If the distribution cannot be automatically detected it will return 1.
> +# Defining BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID on the board file
> +# disables automatic detection.
> +function check_distro {
> +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ]; then
> +        if cmd "test -f /etc/os-release" ; then
> +            os_release_tmpfile=$(mktemp)
> +            get "/etc/os-release" $os_release_tmpfile
> +            export BOARD_DISTRO_ID="$(awk -F  "=" '/^ID=/ {print $2}' $os_release_tmpfile)"
> +            export BOARD_DISTRO_VERSION_ID="$(awk -F  "=" '/^VERSION_ID=/ {print $2}' $os_release_tmpfile)"
> +        fi
> +    fi
> +
> +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ]; then
> +            echo "ERROR: could not detect the distribution running on the board."
> +            echo "Define BOARD_DISTRO_(ID|VERSION_ID) on your board file."
> +            return 1
> +    else
> +        echo "Board distribution is $BOARD_DISTRO_ID $BOARD_DISTRO_VERSION_ID"
> +    fi
> +}
> +
>  # $1 has a single string holding a space-separated args:
>  #   arg1 = size
>  #   arg2 = directory to check
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH] need_check: add linux distro automatic detection

[Bird, Timothy]

> -----Original Message-----
> From: Daniel Sangorrin on Wednesday, January 10, 2018 8:13 PM
> This function can be used by tests that require different
> commands depending on the target board distribution.
> The automatic detection can be disabled by manually
> defining the value for BOARD_DISTRO_ID and
> BOARD_DISTRO_VERSION_ID.
> 
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>

OK - this is an interesting function.  I was unaware of /etc/os-release.
It looks like it's pretty useful.  I'm a little anxious that test scripts will
test these variables, instead of testing for individual distro features that they
are really checking for.  I think the latter is much preferred, as it is less
likely to be version dependent.

So, I'm going to accept this, but keep a careful eye on any use of the
BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID in future patche
submissions, and try to avoid using these when a more direct test of
distro functionality would (IMO) be a better solution.

Comments inline below...

> ---
>  engine/scripts/need_check.sh | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/engine/scripts/need_check.sh b/engine/scripts/need_check.sh
> index 1fd6d6d..a6e5182 100755
> --- a/engine/scripts/need_check.sh
> +++ b/engine/scripts/need_check.sh
> @@ -123,6 +123,33 @@ function get_free_storage {
>    echo $result
>  }
> 
> +# This function detects the Linux distribution used on the target board and
> +# exports the following variables:
> +#    BOARD_DISTRO_ID (e.g. ubuntu, debian, centos)
> +#    BOARD_DISTRO_VERSION_ID (e.g. "16.04", "8", "7")
> +#
> +# If the distribution cannot be automatically detected it will return 1.
> +# Defining BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID on the
> board file
> +# disables automatic detection.
> +function check_distro {
> +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ];
> then
> +        if cmd "test -f /etc/os-release" ; then
> +            os_release_tmpfile=$(mktemp)
> +            get "/etc/os-release" $os_release_tmpfile

This does 2 operations on the board (the cmd and the get).
It is possible to just do the 'get' and detect the presence or absence of /etc/os-release
from that?

Lately, I've been trying to cut down on the number of round-trips to the client board.

> +            export BOARD_DISTRO_ID="$(awk -F  "=" '/^ID=/ {print $2}'
> $os_release_tmpfile)"
> +            export BOARD_DISTRO_VERSION_ID="$(awk -F  "="
> '/^VERSION_ID=/ {print $2}' $os_release_tmpfile)"
> +        fi

This is a minor quibble, but I find awk scripts hard to read.  Here is a substitute that
I find easier to read (at the cost of two sub-command invocations rather than one):
export BOARD_DISTRO="$(grep ^ID= $os_release_tmpfile | cut -b 4-)"
export BOARD_DISTRO_VERSION_ID="$(grep ^VERSION_ID= $os_release_tmpfile | cut -b12-)"
 
Another possibility is this:
    .  $os_release_tempfile
    export BOARD_DISTRO_ID="$ID"
    export BOARD_DISTRO_VERSION_ID="$VERSION_ID"

... But that has some potential nasty security issues - never mind.

I don't have strong feelings on the parsing.  I just have a nagging feeling like
bash should be able to parse this without using an external command.

I could let the 'awk' thing go.  (I'm just an 'awk' disliker.)

> +    fi
This seems to leak $os_release_tmpfile.  I think you need to remove it somewhere.

> +
> +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ];
> then
> +            echo "ERROR: could not detect the distribution running on the board."
> +            echo "Define BOARD_DISTRO_(ID|VERSION_ID) on your board file."
on-> in

I would avoid a regex in the error string.

Please use something like "Define BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID in your board file"

> +            return 1
> +    else
> +        echo "Board distribution is $BOARD_DISTRO_ID
> $BOARD_DISTRO_VERSION_ID"
> +    fi
> +}
> +
>  # $1 has a single string holding a space-separated args:
>  #   arg1 = size
>  #   arg2 = directory to check
> --
> 2.7.4
> 
> 
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH] need_check: add linux distro automatic detection

[Bird, Timothy]

> -----Original Message-----
> From: Daniel Sangorrin on Wednesday, January 10, 2018 8:30 PM
> If the patch is OK, could you please add these variables to the documentation
> as well?

I'd be happy to, after the dust settles.  See my feedback on the patch.
 -- Tim

Re: [Fuego] [PATCH] need_check: add linux distro automatic detection

[Bird, Timothy]

OK - my intro comment says I accepted it.
However, can you please respond to my comments, and/or resubmit
the patch.  I have NOT applied it.
 
Thanks,
 -- Tim

> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> bounces@lists.linuxfoundation.org] On Behalf Of Bird, Timothy
> Sent: Thursday, January 11, 2018 11:33 AM
> To: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>;
> fuego@lists.linuxfoundation.org
> Subject: Re: [Fuego] [PATCH] need_check: add linux distro automatic
> detection
> 
> > -----Original Message-----
> > From: Daniel Sangorrin on Wednesday, January 10, 2018 8:13 PM
> > This function can be used by tests that require different
> > commands depending on the target board distribution.
> > The automatic detection can be disabled by manually
> > defining the value for BOARD_DISTRO_ID and
> > BOARD_DISTRO_VERSION_ID.
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> 
> OK - this is an interesting function.  I was unaware of /etc/os-release.
> It looks like it's pretty useful.  I'm a little anxious that test scripts will
> test these variables, instead of testing for individual distro features that they
> are really checking for.  I think the latter is much preferred, as it is less
> likely to be version dependent.
> 
> So, I'm going to accept this, but keep a careful eye on any use of the
> BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID in future patche
> submissions, and try to avoid using these when a more direct test of
> distro functionality would (IMO) be a better solution.
> 
> Comments inline below...
> 
> > ---
> >  engine/scripts/need_check.sh | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/engine/scripts/need_check.sh b/engine/scripts/need_check.sh
> > index 1fd6d6d..a6e5182 100755
> > --- a/engine/scripts/need_check.sh
> > +++ b/engine/scripts/need_check.sh
> > @@ -123,6 +123,33 @@ function get_free_storage {
> >    echo $result
> >  }
> >
> > +# This function detects the Linux distribution used on the target board and
> > +# exports the following variables:
> > +#    BOARD_DISTRO_ID (e.g. ubuntu, debian, centos)
> > +#    BOARD_DISTRO_VERSION_ID (e.g. "16.04", "8", "7")
> > +#
> > +# If the distribution cannot be automatically detected it will return 1.
> > +# Defining BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID on the
> > board file
> > +# disables automatic detection.
> > +function check_distro {
> > +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ];
> > then
> > +        if cmd "test -f /etc/os-release" ; then
> > +            os_release_tmpfile=$(mktemp)
> > +            get "/etc/os-release" $os_release_tmpfile
> 
> This does 2 operations on the board (the cmd and the get).
> It is possible to just do the 'get' and detect the presence or absence of
> /etc/os-release
> from that?
> 
> Lately, I've been trying to cut down on the number of round-trips to the
> client board.
> 
> > +            export BOARD_DISTRO_ID="$(awk -F  "=" '/^ID=/ {print $2}'
> > $os_release_tmpfile)"
> > +            export BOARD_DISTRO_VERSION_ID="$(awk -F  "="
> > '/^VERSION_ID=/ {print $2}' $os_release_tmpfile)"
> > +        fi
> 
> This is a minor quibble, but I find awk scripts hard to read.  Here is a
> substitute that
> I find easier to read (at the cost of two sub-command invocations rather than
> one):
> export BOARD_DISTRO="$(grep ^ID= $os_release_tmpfile | cut -b 4-)"
> export BOARD_DISTRO_VERSION_ID="$(grep ^VERSION_ID=
> $os_release_tmpfile | cut -b12-)"
> 
> Another possibility is this:
>     .  $os_release_tempfile
>     export BOARD_DISTRO_ID="$ID"
>     export BOARD_DISTRO_VERSION_ID="$VERSION_ID"
> 
> ... But that has some potential nasty security issues - never mind.
> 
> I don't have strong feelings on the parsing.  I just have a nagging feeling like
> bash should be able to parse this without using an external command.
> 
> I could let the 'awk' thing go.  (I'm just an 'awk' disliker.)
> 
> > +    fi
> This seems to leak $os_release_tmpfile.  I think you need to remove it
> somewhere.
> 
> > +
> > +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ];
> > then
> > +            echo "ERROR: could not detect the distribution running on the
> board."
> > +            echo "Define BOARD_DISTRO_(ID|VERSION_ID) on your board file."
> on-> in
> 
> I would avoid a regex in the error string.
> 
> Please use something like "Define BOARD_DISTRO_ID and
> BOARD_DISTRO_VERSION_ID in your board file"
> 
> > +            return 1
> > +    else
> > +        echo "Board distribution is $BOARD_DISTRO_ID
> > $BOARD_DISTRO_VERSION_ID"
> > +    fi
> > +}
> > +
> >  # $1 has a single string holding a space-separated args:
> >  #   arg1 = size
> >  #   arg2 = directory to check
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego

Re: [Fuego] [PATCH] need_check: add linux distro automatic detection

[Daniel Sangorrin]

Hi Tim,

> -----Original Message-----
> From: Bird, Timothy [mailto:Tim.Bird@sony.com]
> Sent: Friday, January 12, 2018 4:42 AM
> To: Bird, Timothy; Daniel Sangorrin; fuego@lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH] need_check: add linux distro automatic detection
> 
> OK - my intro comment says I accepted it.
> However, can you please respond to my comments, and/or resubmit
> the patch.  I have NOT applied it.

Thanks for the review. I just resubmitted the patch.
I used cut -c instead of cut -b (even though both work) for clarity, but you can change it
to -b if you prefer.

Thanks,
Daniel


> 
> Thanks,
>  -- Tim
> 
> > -----Original Message-----
> > From: fuego-bounces@lists.linuxfoundation.org [mailto:fuego-
> > bounces@lists.linuxfoundation.org] On Behalf Of Bird, Timothy
> > Sent: Thursday, January 11, 2018 11:33 AM
> > To: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>;
> > fuego@lists.linuxfoundation.org
> > Subject: Re: [Fuego] [PATCH] need_check: add linux distro automatic
> > detection
> >
> > > -----Original Message-----
> > > From: Daniel Sangorrin on Wednesday, January 10, 2018 8:13 PM
> > > This function can be used by tests that require different
> > > commands depending on the target board distribution.
> > > The automatic detection can be disabled by manually
> > > defining the value for BOARD_DISTRO_ID and
> > > BOARD_DISTRO_VERSION_ID.
> > >
> > > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> >
> > OK - this is an interesting function.  I was unaware of /etc/os-release.
> > It looks like it's pretty useful.  I'm a little anxious that test scripts will
> > test these variables, instead of testing for individual distro features that they
> > are really checking for.  I think the latter is much preferred, as it is less
> > likely to be version dependent.
> >
> > So, I'm going to accept this, but keep a careful eye on any use of the
> > BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID in future patche
> > submissions, and try to avoid using these when a more direct test of
> > distro functionality would (IMO) be a better solution.
> >
> > Comments inline below...
> >
> > > ---
> > >  engine/scripts/need_check.sh | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/engine/scripts/need_check.sh b/engine/scripts/need_check.sh
> > > index 1fd6d6d..a6e5182 100755
> > > --- a/engine/scripts/need_check.sh
> > > +++ b/engine/scripts/need_check.sh
> > > @@ -123,6 +123,33 @@ function get_free_storage {
> > >    echo $result
> > >  }
> > >
> > > +# This function detects the Linux distribution used on the target board and
> > > +# exports the following variables:
> > > +#    BOARD_DISTRO_ID (e.g. ubuntu, debian, centos)
> > > +#    BOARD_DISTRO_VERSION_ID (e.g. "16.04", "8", "7")
> > > +#
> > > +# If the distribution cannot be automatically detected it will return 1.
> > > +# Defining BOARD_DISTRO_ID and BOARD_DISTRO_VERSION_ID on the
> > > board file
> > > +# disables automatic detection.
> > > +function check_distro {
> > > +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ];
> > > then
> > > +        if cmd "test -f /etc/os-release" ; then
> > > +            os_release_tmpfile=$(mktemp)
> > > +            get "/etc/os-release" $os_release_tmpfile
> >
> > This does 2 operations on the board (the cmd and the get).
> > It is possible to just do the 'get' and detect the presence or absence of
> > /etc/os-release
> > from that?
> >
> > Lately, I've been trying to cut down on the number of round-trips to the
> > client board.
> >
> > > +            export BOARD_DISTRO_ID="$(awk -F  "=" '/^ID=/ {print $2}'
> > > $os_release_tmpfile)"
> > > +            export BOARD_DISTRO_VERSION_ID="$(awk -F  "="
> > > '/^VERSION_ID=/ {print $2}' $os_release_tmpfile)"
> > > +        fi
> >
> > This is a minor quibble, but I find awk scripts hard to read.  Here is a
> > substitute that
> > I find easier to read (at the cost of two sub-command invocations rather than
> > one):
> > export BOARD_DISTRO="$(grep ^ID= $os_release_tmpfile | cut -b 4-)"
> > export BOARD_DISTRO_VERSION_ID="$(grep ^VERSION_ID=
> > $os_release_tmpfile | cut -b12-)"
> >
> > Another possibility is this:
> >     .  $os_release_tempfile
> >     export BOARD_DISTRO_ID="$ID"
> >     export BOARD_DISTRO_VERSION_ID="$VERSION_ID"
> >
> > ... But that has some potential nasty security issues - never mind.
> >
> > I don't have strong feelings on the parsing.  I just have a nagging feeling like
> > bash should be able to parse this without using an external command.
> >
> > I could let the 'awk' thing go.  (I'm just an 'awk' disliker.)
> >
> > > +    fi
> > This seems to leak $os_release_tmpfile.  I think you need to remove it
> > somewhere.
> >
> > > +
> > > +    if [ -z "$BOARD_DISTRO_ID" ] || [ -z "$BOARD_DISTRO_VERSION_ID" ];
> > > then
> > > +            echo "ERROR: could not detect the distribution running on the
> > board."
> > > +            echo "Define BOARD_DISTRO_(ID|VERSION_ID) on your board file."
> > on-> in
> >
> > I would avoid a regex in the error string.
> >
> > Please use something like "Define BOARD_DISTRO_ID and
> > BOARD_DISTRO_VERSION_ID in your board file"
> >
> > > +            return 1
> > > +    else
> > > +        echo "Board distribution is $BOARD_DISTRO_ID
> > > $BOARD_DISTRO_VERSION_ID"
> > > +    fi
> > > +}
> > > +
> > >  # $1 has a single string holding a space-separated args:
> > >  #   arg1 = size
> > >  #   arg2 = directory to check
> > > --
> > > 2.7.4
> > >
> > >
> > > _______________________________________________
> > > Fuego mailing list
> > > Fuego@lists.linuxfoundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/fuego
> > _______________________________________________
> > Fuego mailing list
> > Fuego@lists.linuxfoundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/fuego