Re: [PATCH 6/6] osstest: use a locally built pkg repository for FreeBSD

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, May 23, 2019 at 12:06:29PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[Xen-devel] [PATCH 6/6] osstest: use a locally built pkg repository for FreeBSD"):
> > This removes the dependency on the official pkg repository, which is
> > dangerous when not testing stable branches, since the ABI of the
> > development branch is not stable, and thus it's easy for packages to
> > get out of sync, or for osstest to test an old FreeBSD version that
> > has an ABI different than the one used to build the official
> > packages.
> 
> I realise this is a bit late to be saying this, but had you
> considered making the packages build a different step in the same
> job ?  That might make a lot of this go away...

Do you mean to build the packages in build-prep instead of relying on
having a custom binary repository?

I could do that, but it's going to take some time. Also doing the
package build in build-prep would require fetching the svn ports
repository for each build-prep instance.

IIRC the package building job takes a non-trivial amount of time (2-3h
IIRC), because it has to build gcc (for SeaBIOS) and python, perl...

> >  IFS=$'\n'
> > +count=0
> >  for anointed in \
> > -    `./mg-anoint list-prepared "freebsd build $freebsd_branch *"`; do
> > +    `./mg-anoint list-prepared "freebsd* build $freebsd_branch *"`; do
> >      # Retrieve previous successful FreeBSD build for each arch.
> >      freebsd_arch=${anointed##* }
> > -    freebsd_envvar="FREEBSD_${freebsd_arch^^}_BUILDJOB"
> > +    freebsd_name=${anointed%% *}
> > +    freebsd_name=${freebsd_name/-/_}
> > +    freebsd_envvar="${freebsd_name^^}_${freebsd_arch^^}_BUILDJOB"
> >      if [ "x${!freebsd_envvar}" = "x" ]; then
> > -        flight_job=`./mg-anoint retrieve "$anointed"`
> > -        export ${freebsd_envvar}=${flight_job/ /.}
> > +	envvars[$count]="$freebsd_envvar"
> > +	refkeys[$count]="$anointed"
> > +	count=$((count+1))
> 
> You don't need this counter.  You can just say
>    envvars=()
>    ...
>    envvars+=("$freebsd_envvar")

Oh, thanks!

> > +    fi
> > +done
> > +count=0
> > +for flight_job in `./mg-anoint retrieve ${refkeys[@]}`; do
> > +    if [ "$flight_job" != "ERROR" ]; then
> > +	export ${envvars[$count]}=${flight_job/ /.}
> >      fi
> > +    count=$((count+1))
> 
> I think you do need count here, if you do this as two loops.  But:
> 
> Why not do this retrieve, and set the env vars, inside the first
> loop ?  I think that would avoid having to accumulate a data structure
> full of information in shell variables at all (and shell is not very
> good at this kind of thing).

Yes, I think I can do it as you suggest.

> > @@ -542,17 +553,23 @@ freebsd-*)
> >         [ "x$OSSTEST_BLESSING" == "xreal" ]; then
> >          IFS=$'\n'
> >          for anointed in `./mg-anoint list-prepared \
> > -                                     "freebsd build $freebsd_branch *"`; do
> > +                                     "freebsd* build $freebsd_branch *"`; do
> >              # Update anointed versions
> >              # NB: failure to update an anointed build for a specific arch
> >              # should not be fatal, and it's not an issue if one of the
> >              # arches gets slightly out of sync with the other ones.
> >              freebsd_arch=${anointed##* }
> > -            if ./mg-anoint anoint "$anointed" \
> > -                           $flight build-$freebsd_arch-freebsd; then
> > -                echo "Anointed artifacts from build-$freebsd_arch-freebsd"
> > -            fi
> > +            freebsd_name=${anointed%% *}
> > +	    # Rely on the fact that the job suffix is the same as the
> > +	    # anointment refkey. Ie:
> 
> I don't think you mean "Rely on the fact".  Rather, "by definition,
> from the way the flight is constructed, the intended..." ?
> 
> > +	    # refkey: freebsd          job: build-<arch>-freebsd
> > +	    # refkey: freebsd-packages job: build-<arch>-freebsd-packages
> > +            anoint="$anoint \"$anointed\" $flight \
> > +                    build-$freebsd_arch-$freebsd_name"
> 
> Maybe use an array variable for anount, and then you can avoid the
> shell \" quoting.

Please bear with me, but can you elaborate on this?

> > diff --git a/mfi-common b/mfi-common
> > index 83d3c713..12cde85f 100644
> > --- a/mfi-common
> > +++ b/mfi-common
> > @@ -156,7 +156,6 @@ set_freebsd_runvars () {
> ...
> > +    # Check if the packages are provided externally, or else assume they
> > +    # are provided by the same flight as the installer binaries.
> > +    local pkgpath=`getconfig "FreeBSDPackages"`
> > +    counter=0
> > +    IFS=$'\n'
> > +    for flightjob in `./mg-anoint retrieve --tolerate-unprepared \
> > +                      "freebsd build master $arch" \
> > +                      "freebsd-packages build master $arch"`; do
> > +        if [ $counter -eq 0 ]; then
> > +            # Anointed FreeBSD installer
> 
> I don't much like this code, but I'm having trouble saying what I
> think should be done instead.
> 
> I don't much like the $counter -eq 0 approach.  Maybe some of it
> should go into a function ?
>     ./mg-anoint retrieve ... >tmpfile
>     if freebsd_want_anointed <tmpfile '' Dist ...
> but not sure what the function should be.

Yes, the counter stuff is nasty. I guess I could change the output of
mg-anoint so it signals which output line belongs to which input
parameter, but there's always going to be multiple lines, and I wanted
to avoid having to change much of mg-anoint.

> 
> > +            local envvar="FREEBSD_${arch^^}_BUILDJOB"
> > +            local distpath=`getconfig "FreeBSDDist"`
> > +            if [ -n "${!envvar}" ]; then
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsdbuildjob=${!envvar}"
> > +            elif [ -n "$FREEBSD_DIST" ] && [ -n "$FREEBSD_VERSION" ]; then
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsd_distpath=$FREEBSD_DIST/$arch \
> > +                                 freebsd_version=$FREEBSD_VERSION"
> > +            elif [ -n "$distpath" ]; then
> > +                local version=`getconfig "FreeBSDVersion"`
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsd_distpath=$distpath/$arch \
> > +                                 freebsd_version=$version"
> > +            elif [ "$flightjob" != "ERROR" ]; then
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsdbuildjob=${flightjob/ /.}"
> 
> There seems like a lot of repetition here.  For example, FREEBSD_DIST
> overrides FreeBSDDist but /$arch is appended in two places.  Maybe
> ${FREEBSD_DIST- ... something ... } would be better ?

OK, let me try to remove some of the duplication here.

> It is difficult to see the wood for the trees, particularly with the
> constant repetition of
>    freebsd_runvars="$freebsd_runvars \
> which could be avoided by having this fragment set a local variable
> containing the things to be added.

Hm, indeed. Using a local variable and then appending it when done
would make it clearer.

> > +        elif [ $counter -eq 1 ]; then
> > +            # Anointed package repository
> > +            local envvar="FREEBSD_PACKAGES_${arch^^}_BUILDJOB"
> > +            local pkgpath=`getconfig "FreeBSDPackages"`
> > +            if [ -n "${!envvar}" ]; then
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsdpackagesbuildjob=${!envvar}"
> > +            elif [ -n "$FREEBSD_PACKAGES" ]; then
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsd_packages=$FREEBSD_PACKAGES/$arch"
> > +            elif [ -n "$pkgpath" ]; then
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsd_packages=$pkgpath/$arch"
> > +            elif [ "$flightjob" != "ERROR" ]; then
> > +                freebsd_runvars="$freebsd_runvars \
> > +                                 freebsdpackagesbuildjob=${flightjob/ /.}"
> 
> This feels very similar to the code above, although it lacks the
> special handling for the version.

Maybe I can see about factoring some of this into a helper, but there
are slight differences in both if branches that I'm not sure can be
factored out.

Why I don't start by fixing the repetition of:
freebsd_runvars="$freebsd_runvars \... and we take it from there?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel