|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 6/6] osstest: use a locally built pkg repository for FreeBSD
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |