| Message ID | 1468490266-3468-2-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote: > The rather random list of check-qtest-xxx entries caused some > confusion in the past, where to use "=" and where to use "+=" > (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da > for example). > Sorting the check-qtest-xxx entries by architecure instead and > using some empty lines inbetween should help to ease this > situation a little bit, so that it is hopefully now obvious > that new tests should be added with "+=" instead of "=". > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/Makefile.include | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 2010b11..b7784d3 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF) > check-qtest-x86_64-y += $(check-qtest-i386-y) > gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c > gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y)) > + Hm, seems to me like there's still potential for confusion in the mixing of check-qtest lines and gcov-files lines. > check-qtest-mips-y = tests/endianness-test$(EXESUF) > check-qtest-mips64-y = tests/endianness-test$(EXESUF) > check-qtest-mips64el-y = tests/endianness-test$(EXESUF) > + > check-qtest-ppc-y = tests/endianness-test$(EXESUF) > +check-qtest-ppc-y += tests/boot-order-test$(EXESUF) > +check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > + > check-qtest-ppc64-y = tests/endianness-test$(EXESUF) > +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) > +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) > +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c For example, AFAICT this is the first line setting gcov-files-ppc64-y, so shouldn't it be '=' instead of '+='? > +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) > + > check-qtest-sh4-y = tests/endianness-test$(EXESUF) > check-qtest-sh4eb-y = tests/endianness-test$(EXESUF) Should be another blank line between sh4 and sh4eb, no? > + > +check-qtest-sparc-y = tests/prom-env-test$(EXESUF) > +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF) > +#gcov-files-sparc-y = hw/timer/m48t59.c You've added a comment here, haven't you? Which changes something real, not just order. > + > check-qtest-sparc64-y = tests/endianness-test$(EXESUF) > -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF) > #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF) > -gcov-files-sparc-y += hw/timer/m48t59.c > -gcov-files-sparc64-y += hw/timer/m48t59.c > +#gcov-files-sparc64-y += hw/timer/m48t59.c And here? > +#Disabled for now, triggers a TCG bug on 32-bit hosts > +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > + > check-qtest-arm-y = tests/tmp105-test$(EXESUF) > check-qtest-arm-y += tests/ds1338-test$(EXESUF) > gcov-files-arm-y += hw/misc/tmp105.c > check-qtest-arm-y += tests/virtio-blk-test$(EXESUF) > gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c > -check-qtest-ppc-y += tests/boot-order-test$(EXESUF) > -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) > -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) > -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c > -check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) > -check-qtest-sparc-y += tests/prom-env-test$(EXESUF) > -#Disabled for now, triggers a TCG bug on 32-bit hosts > -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > + > check-qtest-microblazeel-y = $(check-qtest-microblaze-y) > check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) And there should be a blank betweem microblazeel and xtensaeb too, no?
On 15.07.2016 05:12, David Gibson wrote: > On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote: >> The rather random list of check-qtest-xxx entries caused some >> confusion in the past, where to use "=" and where to use "+=" >> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da >> for example). >> Sorting the check-qtest-xxx entries by architecure instead and >> using some empty lines inbetween should help to ease this >> situation a little bit, so that it is hopefully now obvious >> that new tests should be added with "+=" instead of "=". >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> tests/Makefile.include | 32 ++++++++++++++++++++------------ >> 1 file changed, 20 insertions(+), 12 deletions(-) >> >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index 2010b11..b7784d3 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF) >> check-qtest-x86_64-y += $(check-qtest-i386-y) >> gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c >> gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y)) >> + > > Hm, seems to me like there's still potential for confusion in the > mixing of check-qtest lines and gcov-files lines. > >> check-qtest-mips-y = tests/endianness-test$(EXESUF) >> check-qtest-mips64-y = tests/endianness-test$(EXESUF) >> check-qtest-mips64el-y = tests/endianness-test$(EXESUF) >> + >> check-qtest-ppc-y = tests/endianness-test$(EXESUF) >> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF) >> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF) >> + >> check-qtest-ppc64-y = tests/endianness-test$(EXESUF) >> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) >> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) >> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c > > For example, AFAICT this is the first line setting gcov-files-ppc64-y, > so shouldn't it be '=' instead of '+='? True. It currently doesn't hurt because the variable is empty initially, but it's likely cleaner to start with "=" here, too, for the first entry... I'll change it in v2. >> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) >> + >> check-qtest-sh4-y = tests/endianness-test$(EXESUF) >> check-qtest-sh4eb-y = tests/endianness-test$(EXESUF) > > Should be another blank line between sh4 and sh4eb, no? I didn't mind it here yet since there is currently only one test for these architectures, but yes, it's likely cleaner to also add a space here right from the start, too. >> + >> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF) >> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF) >> +#gcov-files-sparc-y = hw/timer/m48t59.c > > You've added a comment here, haven't you? Which changes something > real, not just order. Yeah, I've commented it out since the test is disabled, ... I guess I should either leave it uncommented or write a proper sentence about this in the commit description... >> + >> check-qtest-sparc64-y = tests/endianness-test$(EXESUF) >> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF) >> #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF) >> -gcov-files-sparc-y += hw/timer/m48t59.c >> -gcov-files-sparc64-y += hw/timer/m48t59.c >> +#gcov-files-sparc64-y += hw/timer/m48t59.c > > And here? > >> +#Disabled for now, triggers a TCG bug on 32-bit hosts >> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) >> + >> check-qtest-arm-y = tests/tmp105-test$(EXESUF) >> check-qtest-arm-y += tests/ds1338-test$(EXESUF) >> gcov-files-arm-y += hw/misc/tmp105.c >> check-qtest-arm-y += tests/virtio-blk-test$(EXESUF) >> gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c >> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF) >> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) >> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) >> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c >> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF) >> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) >> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF) >> -#Disabled for now, triggers a TCG bug on 32-bit hosts >> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) >> + >> check-qtest-microblazeel-y = $(check-qtest-microblaze-y) >> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) > > And there should be a blank betweem microblazeel and xtensaeb too, no? Yes, I'll change in v2. Thanks for the review! Thomas
On Fri, Jul 15, 2016 at 08:46:47AM +0200, Thomas Huth wrote: > On 15.07.2016 05:12, David Gibson wrote: > > On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote: > >> The rather random list of check-qtest-xxx entries caused some > >> confusion in the past, where to use "=" and where to use "+=" > >> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da > >> for example). > >> Sorting the check-qtest-xxx entries by architecure instead and > >> using some empty lines inbetween should help to ease this > >> situation a little bit, so that it is hopefully now obvious > >> that new tests should be added with "+=" instead of "=". > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> tests/Makefile.include | 32 ++++++++++++++++++++------------ > >> 1 file changed, 20 insertions(+), 12 deletions(-) > >> > >> diff --git a/tests/Makefile.include b/tests/Makefile.include > >> index 2010b11..b7784d3 100644 > >> --- a/tests/Makefile.include > >> +++ b/tests/Makefile.include > >> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF) > >> check-qtest-x86_64-y += $(check-qtest-i386-y) > >> gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c > >> gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y)) > >> + > > > > Hm, seems to me like there's still potential for confusion in the > > mixing of check-qtest lines and gcov-files lines. > > > >> check-qtest-mips-y = tests/endianness-test$(EXESUF) > >> check-qtest-mips64-y = tests/endianness-test$(EXESUF) > >> check-qtest-mips64el-y = tests/endianness-test$(EXESUF) > >> + > >> check-qtest-ppc-y = tests/endianness-test$(EXESUF) > >> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF) > >> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > >> + > >> check-qtest-ppc64-y = tests/endianness-test$(EXESUF) > >> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) > >> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) > >> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c > > > > For example, AFAICT this is the first line setting gcov-files-ppc64-y, > > so shouldn't it be '=' instead of '+='? > > True. It currently doesn't hurt because the variable is empty initially, > but it's likely cleaner to start with "=" here, too, for the first > entry... I'll change it in v2. Hm, given that += on a previously undeclared variable treats it as empty, it seems like it would be safer to just use += everywhere. I don't believe there are any cases where we're trying to remove tests > > >> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) > >> + > >> check-qtest-sh4-y = tests/endianness-test$(EXESUF) > >> check-qtest-sh4eb-y = tests/endianness-test$(EXESUF) > > > > Should be another blank line between sh4 and sh4eb, no? > > I didn't mind it here yet since there is currently only one test for > these architectures, but yes, it's likely cleaner to also add a space > here right from the start, too. > > >> + > >> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF) > >> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF) > >> +#gcov-files-sparc-y = hw/timer/m48t59.c > > > > You've added a comment here, haven't you? Which changes something > > real, not just order. > > Yeah, I've commented it out since the test is disabled, ... I guess I > should either leave it uncommented or write a proper sentence about this > in the commit description... Ah.. yes, I see. I hadn't connected the coverage to the fact the corresponding test is disabled. Yeah, I think that needs a mention in the description. > > >> + > >> check-qtest-sparc64-y = tests/endianness-test$(EXESUF) > >> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF) > >> #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF) > >> -gcov-files-sparc-y += hw/timer/m48t59.c > >> -gcov-files-sparc64-y += hw/timer/m48t59.c > >> +#gcov-files-sparc64-y += hw/timer/m48t59.c > > > > And here? > > > >> +#Disabled for now, triggers a TCG bug on 32-bit hosts > >> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > >> + > >> check-qtest-arm-y = tests/tmp105-test$(EXESUF) > >> check-qtest-arm-y += tests/ds1338-test$(EXESUF) > >> gcov-files-arm-y += hw/misc/tmp105.c > >> check-qtest-arm-y += tests/virtio-blk-test$(EXESUF) > >> gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c > >> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF) > >> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) > >> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) > >> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c > >> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF) > >> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) > >> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF) > >> -#Disabled for now, triggers a TCG bug on 32-bit hosts > >> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) > >> + > >> check-qtest-microblazeel-y = $(check-qtest-microblaze-y) > >> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y) > > > > And there should be a blank betweem microblazeel and xtensaeb too, no? > > Yes, I'll change in v2. > > Thanks for the review! > > Thomas > > >
diff --git a/tests/Makefile.include b/tests/Makefile.include index 2010b11..b7784d3 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF) check-qtest-x86_64-y += $(check-qtest-i386-y) gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y)) + check-qtest-mips-y = tests/endianness-test$(EXESUF) check-qtest-mips64-y = tests/endianness-test$(EXESUF) check-qtest-mips64el-y = tests/endianness-test$(EXESUF) + check-qtest-ppc-y = tests/endianness-test$(EXESUF) +check-qtest-ppc-y += tests/boot-order-test$(EXESUF) +check-qtest-ppc-y += tests/prom-env-test$(EXESUF) + check-qtest-ppc64-y = tests/endianness-test$(EXESUF) +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) + check-qtest-sh4-y = tests/endianness-test$(EXESUF) check-qtest-sh4eb-y = tests/endianness-test$(EXESUF) + +check-qtest-sparc-y = tests/prom-env-test$(EXESUF) +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF) +#gcov-files-sparc-y = hw/timer/m48t59.c + check-qtest-sparc64-y = tests/endianness-test$(EXESUF) -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF) #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF) -gcov-files-sparc-y += hw/timer/m48t59.c -gcov-files-sparc64-y += hw/timer/m48t59.c +#gcov-files-sparc64-y += hw/timer/m48t59.c +#Disabled for now, triggers a TCG bug on 32-bit hosts +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) + check-qtest-arm-y = tests/tmp105-test$(EXESUF) check-qtest-arm-y += tests/ds1338-test$(EXESUF) gcov-files-arm-y += hw/misc/tmp105.c check-qtest-arm-y += tests/virtio-blk-test$(EXESUF) gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c -check-qtest-ppc-y += tests/boot-order-test$(EXESUF) -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF) -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c -check-qtest-ppc-y += tests/prom-env-test$(EXESUF) -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF) -check-qtest-sparc-y += tests/prom-env-test$(EXESUF) -#Disabled for now, triggers a TCG bug on 32-bit hosts -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF) + check-qtest-microblazeel-y = $(check-qtest-microblaze-y) check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
The rather random list of check-qtest-xxx entries caused some confusion in the past, where to use "=" and where to use "+=" (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da for example). Sorting the check-qtest-xxx entries by architecure instead and using some empty lines inbetween should help to ease this situation a little bit, so that it is hopefully now obvious that new tests should be added with "+=" instead of "=". Signed-off-by: Thomas Huth <thuth@redhat.com> --- tests/Makefile.include | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)