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

Daniel Sangorrin daniel.sangorrin at toshiba.co.jp
Fri Jan 12 01:59:47 UTC 2018


Hi Tim,

> -----Original Message-----
> From: Bird, Timothy [mailto:Tim.Bird at sony.com]
> Sent: Friday, January 12, 2018 4:42 AM
> To: Bird, Timothy; Daniel Sangorrin; fuego at 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 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