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

Bird, Timothy Tim.Bird at sony.com
Thu Jan 11 19:41:43 UTC 2018


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 at lists.linuxfoundation.org [mailto:fuego-
> bounces at lists.linuxfoundation.org] On Behalf Of Bird, Timothy
> Sent: Thursday, January 11, 2018 11:33 AM
> To: Daniel Sangorrin <daniel.sangorrin at toshiba.co.jp>;
> fuego at 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 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
> _______________________________________________
> Fuego mailing list
> Fuego at lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego


More information about the Fuego mailing list