| Message ID | 1452263399-14094-5-git-send-email-ian.campbell@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ian Campbell writes ("[PATCH RFC OSSTEST v1 05/12] make-*flight: Abolish $defsuite and $guestdefsuite"):
> Instead have mfi-common set $suite or $guestsuite if it is unset. When
> doing so move the use of local to this point, using local at the top
> of the function would shadow any attempt to set a global value, while
> restricting it only to when setting the default means it doesn't leak.
> NB "local" scopes the variable to the containing function, not the
> scope of the block where it is written (i.e. the if body in this
> case).
I'm not really sure this approach is right.
If we were to decide that some of the tests resulting from
test_matrix_iterate ought to have different suite values, we would
have to (re)introduce a layer of indirection.
Perhaps it would be better to retain defsuite and defguestsuite;
move the copy from those to suite and guestsuite closer to use site;
and unconditionally set the runvar ?
Ian.
On Tue, 2016-01-12 at 14:18 +0000, Ian Jackson wrote: > Ian Campbell writes ("[PATCH RFC OSSTEST v1 05/12] make-*flight: Abolish > $defsuite and $guestdefsuite"): > > Instead have mfi-common set $suite or $guestsuite if it is unset. When > > doing so move the use of local to this point, using local at the top > > of the function would shadow any attempt to set a global value, while > > restricting it only to when setting the default means it doesn't leak. > > NB "local" scopes the variable to the containing function, not the > > scope of the block where it is written (i.e. the if body in this > > case). > > I'm not really sure this approach is right. > > If we were to decide that some of the tests resulting from > test_matrix_iterate ought to have different suite values, we would > have to (re)introduce a layer of indirection. > > Perhaps it would be better to retain defsuite and defguestsuite; > move the copy from those to suite and guestsuite closer to use site; > and unconditionally set the runvar ? Actually, I think only this one hunk is the useful bit, corresponding to the last line: @@ -431,10 +434,10 @@ test_matrix_iterate () { arch_runvars=\"\$ARCH_RUNVARS_$dom0arch\" " - debian_runvars="debian_kernkind=$kernkind debian_arch=$dom0arch" - if [ $guestsuite != $defguestsuite ] ; then - debian_runvars="$debian_runvars debian_suite=$guestsuite" - fi + debian_runvars="debian_kernkind=$kernkind \ + debian_arch=$dom0arch \ + debian_suite=$guestsuite \ + " most_hostflags="arch-$dom0arch,arch-xen-$xenarch,suite-$suite,purpose-test" if [ "x$min_linux_hostflag" != "x" ] ; then I'm not sure about moving the copy from $deffoo to $foo from where it is without this patch, they are already quite close to the use site I think. WRT tests resulting from test-matrix_iterate having different $suite values I think it is sufficient that the foo=$defoo is in the body of hte loop and not outside. I'll respin with just that one hunk. Ian.
diff --git a/make-distros-flight b/make-distros-flight index 9d04d3b..a11ce84 100755 --- a/make-distros-flight +++ b/make-distros-flight @@ -30,9 +30,6 @@ flight=`./cs-flight-create $blessing $branch` . ./ap-common . ./mfi-common -defsuite=`getconfig DebianSuite` -defguestsuite=`getconfig GuestDebianSuite` - case $branch in distros-debian-*) debian_suite=${branch#distros-debian-} ;; *) echo $branch >&2; exit 1 ;; diff --git a/make-flight b/make-flight index 6b2b3ea..50abf97 100755 --- a/make-flight +++ b/make-flight @@ -31,9 +31,6 @@ flight=`./cs-flight-create $blessing $branch` . ./ap-common . ./mfi-common -defsuite=`getconfig DebianSuite` -defguestsuite=`getconfig GuestDebianSuite` - case "$branch" in xen-unstable-smoke) global_runvars+=" hostalloc_maxbonus_variation~=0 " diff --git a/mfi-common b/mfi-common index 0e2b64f..44e7b3e 100644 --- a/mfi-common +++ b/mfi-common @@ -80,7 +80,7 @@ create_build_jobs () { local arch local pvops_kernel pvops_kconfig_overrides - local suite hostos_runvars + local hostos_runvars local want_xend build_defxend build_extraxend local enable_ovmf local build_hostflags @@ -119,9 +119,9 @@ create_build_jobs () { ;; esac - case "$arch" in - *) suite=$defsuite;; - esac + if [ -z "$suite" ] ; then + local suite=`getconfig DebianSuite` + fi hostos_runvars="all_host_suite=$suite" @@ -401,9 +401,12 @@ test_matrix_iterate () { ;; esac - case "$xenarch" in - *) suite=$defsuite; guestsuite=$defguestsuite;; - esac + if [ -z "$suite" ] ; then + local suite=`getconfig DebianSuite` + fi + if [ -z "$guestsuite" ] ; then + local guestsuite=`getconfig GuestDebianSuite` + fi hostos_runvars="all_host_suite=$suite" @@ -431,10 +434,10 @@ test_matrix_iterate () { arch_runvars=\"\$ARCH_RUNVARS_$dom0arch\" " - debian_runvars="debian_kernkind=$kernkind debian_arch=$dom0arch" - if [ $guestsuite != $defguestsuite ] ; then - debian_runvars="$debian_runvars debian_suite=$guestsuite" - fi + debian_runvars="debian_kernkind=$kernkind \ + debian_arch=$dom0arch \ + debian_suite=$guestsuite \ + " most_hostflags="arch-$dom0arch,arch-xen-$xenarch,suite-$suite,purpose-test" if [ "x$min_linux_hostflag" != "x" ] ; then
Instead have mfi-common set $suite or $guestsuite if it is unset. When doing so move the use of local to this point, using local at the top of the function would shadow any attempt to set a global value, while restricting it only to when setting the default means it doesn't leak. NB "local" scopes the variable to the containing function, not the scope of the block where it is written (i.e. the if body in this case). This adds an explicit debian_suite to some jobs which didn't already have one, meaning that those jobs will remain the same when cloned for a bisect and run in a tree where $c{DebianGuestSuite} has changed since the original construction. No expected semantic change. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- The use of local in the first paragraph seems a bit "icky". Using local suite=$suite at the top of the function might be a less gross alternative? @@ -444,13 +444,42 @@ xen-unstable test-amd64-amd64-xl-pvh-intel debian_pvh 1 xen-unstable test-amd64-amd64-amd64-pvgrub debian_suite jessie xen-unstable test-amd64-amd64-i386-pvgrub debian_suite jessie +xen-unstable test-amd64-amd64-libvirt debian_suite jessie +xen-unstable test-amd64-amd64-libvirt-pair debian_suite jessie xen-unstable test-amd64-amd64-libvirt-vhd debian_suite jessie +xen-unstable test-amd64-amd64-libvirt-xsm debian_suite jessie +xen-unstable test-amd64-amd64-migrupgrade debian_suite jessie +xen-unstable test-amd64-amd64-pair debian_suite jessie xen-unstable test-amd64-amd64-pygrub debian_suite jessie +xen-unstable test-amd64-amd64-xl debian_suite jessie +xen-unstable test-amd64-amd64-xl-credit2 debian_suite jessie +xen-unstable test-amd64-amd64-xl-multivcpu debian_suite jessie +xen-unstable test-amd64-amd64-xl-pvh-amd debian_suite jessie +xen-unstable test-amd64-amd64-xl-pvh-intel debian_suite jessie xen-unstable test-amd64-amd64-xl-qcow2 debian_suite jessie +xen-unstable test-amd64-amd64-xl-rtds debian_suite jessie +xen-unstable test-amd64-amd64-xl-xsm debian_suite jessie +xen-unstable test-amd64-i386-libvirt debian_suite jessie +xen-unstable test-amd64-i386-libvirt-pair debian_suite jessie +xen-unstable test-amd64-i386-libvirt-xsm debian_suite jessie +xen-unstable test-amd64-i386-migrupgrade debian_suite jessie +xen-unstable test-amd64-i386-pair debian_suite jessie +xen-unstable test-amd64-i386-xl debian_suite jessie xen-unstable test-amd64-i386-xl-raw debian_suite jessie +xen-unstable test-amd64-i386-xl-xsm debian_suite jessie +xen-unstable test-armhf-armhf-libvirt debian_suite jessie xen-unstable test-armhf-armhf-libvirt-qcow2 debian_suite jessie xen-unstable test-armhf-armhf-libvirt-raw debian_suite jessie +xen-unstable test-armhf-armhf-libvirt-xsm debian_suite jessie +xen-unstable test-armhf-armhf-xl debian_suite jessie +xen-unstable test-armhf-armhf-xl-arndale debian_suite jessie +xen-unstable test-armhf-armhf-xl-credit2 debian_suite jessie +xen-unstable test-armhf-armhf-xl-cubietruck debian_suite jessie +xen-unstable test-armhf-armhf-xl-midway debian_suite jessie +xen-unstable test-armhf-armhf-xl-multivcpu debian_suite jessie +xen-unstable test-armhf-armhf-xl-rtds debian_suite jessie xen-unstable test-armhf-armhf-xl-vhd debian_suite jessie +xen-unstable test-armhf-armhf-xl-xsm debian_suite jessie xen-unstable test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm debianhvm_image debian-8.2.0-amd64-CD-1.iso xen-unstable test-amd64-amd64-xl-qemut-debianhvm-amd64 debianhvm_image debian-8.2.0-amd64-CD-1.iso xen-unstable test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm debianhvm_image debian-8.2.0-amd64-CD-1.iso --- make-distros-flight | 3 --- make-flight | 3 --- mfi-common | 25 ++++++++++++++----------- 3 files changed, 14 insertions(+), 17 deletions(-)