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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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...

>  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")
etc.

> +    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).

> @@ -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.

> 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.

> +            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 ?

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.

> +        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.


The rest of this looks plausible.

Ian.

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