On 01/15/2015 03:53 PM, Gary R Hook wrote:
>>> +storagepoolxml2xmltest_CFLAGS = $(AM_CFLAGS)
>>> +if WITH_STORAGE_ZFS
>>> +storagepoolxml2xmltest_CFLAGS += -DWITH_STORAGE_ZFS
>>> +endif
>>
>> This shouldn't be needed. Our config.h should already be defining all
>> the appropriate WITH_* feature macros. If it is not, that's a bug in
>> configure.ac.
>
> I'm not adding a #define. I'm just passing one along to the compiler.
> Turning a configure parameter in to a compiler parameter.
Adding a -D to Makefile.am is the same as adding a #define to the
source; my point was that the source should already have the #define
without the need for a -D in CFLAGS.
>
> If you have a suggestion as to how to #ifdef the source code (below) in
> a way that keeps it from being compiled when ZFS isn't in use, I'm all
> ears. I thought this was pretty clean.
After a quick search, I note that src/storage/storage_backend.c already
uses WITH_STORAGE_ZFS without having to add a -D to the CFLAGS, because
<config.h> already defines it. So your testcase should do the same.
You shouldn't have to modify Makefile.am; just blindly use
WITH_STORAGE_ZFS in the test file, which already includes <config.h>.
>
>>>
>>> nodedevxml2xmltest_SOURCES = \
>>> nodedevxml2xmltest.c \
>>> diff --git a/tests/storagepoolxml2xmltest.c
>>> b/tests/storagepoolxml2xmltest.c
>>> index 52e2193..270f75d 100644
>>> --- a/tests/storagepoolxml2xmltest.c
>>> +++ b/tests/storagepoolxml2xmltest.c
>>> @@ -106,8 +106,10 @@ mymain(void)
>>> DO_TEST("pool-gluster");
>>> DO_TEST("pool-gluster-sub");
>>> DO_TEST("pool-scsi-type-scsi-host-stable");
>>> +#ifdef WITH_STORAGE_ZFS
>>> DO_TEST("pool-zfs");
>>> DO_TEST("pool-zfs-sourcedev");
>>> +#endif
>>
>> Why is only this test conditional? Shouldn't other things like gluster
>> also be conditional, based on whether those features were built into
>> libvirt?
>
> You have a very good point. I wasn't trying to fix any/all problems,
> just the one that bit me, and that is the most recent of this type of
> mistake. I might conclude that any older conditional tests haven't been
> exposed as problematic, although clearly every test should be properly
> vetted against the actual environment.
Your solution seems okay to me, except that we probably ought to make
all of the tests honor the proper conditionals, and not just the new zfs
test.
>
> Really appreciate the quick notice and response, however. Thank you.
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list