From: Ian Jackson <ian.jackson@citrix.com>
To: Roger Pau Monne <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 6/6] osstest: use a locally built pkg repository for FreeBSD
Date: Thu, 23 May 2019 12:06:29 +0100 [thread overview]
Message-ID: <23782.32437.361492.752289@mariner.uk.xensource.com> (raw)
Message-ID: <20190523110629.DpAE5B0PYCUFyyAMsRZBqHxmMz29JSyuyaQLYaY3NUg@z> (raw)
In-Reply-To: <20190220170001.32614-7-roger.pau@citrix.com>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-05-23 11:07 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-20 16:59 [PATCH 0/6] osstest: create a local binary FreeBSD package repository Roger Pau Monne
2019-02-20 16:59 ` [PATCH 1/6] osstest: introduce a helper to stash a whole directory Roger Pau Monne
2019-05-23 9:48 ` Ian Jackson
2019-05-23 9:48 ` [Xen-devel] " Ian Jackson
2019-05-24 9:42 ` Roger Pau Monné
2019-05-24 9:42 ` [Xen-devel] " Roger Pau Monné
2019-02-20 16:59 ` [PATCH 2/6] osstest: introduce a helper to create a weblink to a directory Roger Pau Monne
2019-05-23 9:57 ` Ian Jackson
2019-05-23 9:57 ` [Xen-devel] " Ian Jackson
2019-02-20 16:59 ` [PATCH 3/6] osstest: allow to perform multiple anoints in the same transaction Roger Pau Monne
2019-05-23 10:00 ` Ian Jackson
2019-05-23 10:00 ` [Xen-devel] " Ian Jackson
2019-02-20 16:59 ` [PATCH 4/6] osstest: introduce a helper to get the svn revision of a git commit Roger Pau Monne
2019-05-23 10:03 ` Ian Jackson
2019-05-23 10:03 ` [Xen-devel] " Ian Jackson
2019-05-24 9:57 ` Roger Pau Monné
2019-05-24 9:57 ` [Xen-devel] " Roger Pau Monné
2019-05-24 10:35 ` Ian Jackson
2019-05-24 10:35 ` [Xen-devel] " Ian Jackson
2019-02-20 17:00 ` [PATCH 5/6] osstest: introduce a script to build a FreeBSD package repository Roger Pau Monne
2019-05-23 10:19 ` Ian Jackson
2019-05-23 10:19 ` [Xen-devel] " Ian Jackson
2019-05-23 10:38 ` Ian Jackson
2019-05-23 10:38 ` [Xen-devel] " Ian Jackson
2019-05-24 10:13 ` Roger Pau Monné
2019-05-24 10:13 ` [Xen-devel] " Roger Pau Monné
2019-05-24 11:21 ` Ian Jackson
2019-05-24 11:21 ` [Xen-devel] " Ian Jackson
2019-02-20 17:00 ` [PATCH 6/6] osstest: use a locally built pkg repository for FreeBSD Roger Pau Monne
2019-05-23 11:06 ` Ian Jackson [this message]
2019-05-23 11:06 ` [Xen-devel] " Ian Jackson
2019-05-24 10:45 ` Roger Pau Monné
2019-05-24 10:45 ` [Xen-devel] " Roger Pau Monné
2019-05-24 17:26 ` Ian Jackson
2019-05-24 17:26 ` [Xen-devel] " Ian Jackson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=23782.32437.361492.752289@mariner.uk.xensource.com \
--to=ian.jackson@citrix.com \
--cc=roger.pau@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).