[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 at 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 at lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego