mingw pollution of DATADIR vs. configmake.h [was: [libvirt] [PATCH v10 08/19] backup: Parse and output checkpoint XML]

mingw pollution of DATADIR vs. configmake.h [was: [libvirt] [PATCH v10 08/19] backup: Parse and output checkpoint XML]

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2835 bytes --]

[adding bug-gnulib]

On 7/24/19 12:55 AM, Eric Blake wrote:
> Add a new file checkpoint_conf.c that performs the translation to and
> from new XML describing a checkpoint. The code shares a common base
> class with snapshots, since a checkpoint similarly represents the
> domain state at a moment in time. Add some basic testing of round trip
> XML handling through the new code.

> +++ b/src/conf/checkpoint_conf.c

> +#include <config.h>
> +
> +#include "configmake.h"
> +#include "internal.h"
> +#include "virbitmap.h"
> +#include "virbuffer.h"
> +#include "datatypes.h"

This causes a compilation failure on mingw, due to libvirt's
"datatypes.h" including <winsock.h> after the point at which gnulib's
"configmake.h" has already #define'd DATADIR into a string, but mingw's
winsock.h tries to use DATADIR as a data-type tag name:

make  all-am
make[1]: Entering directory '/home/berrange/src/virt/libvirt/src'
  CC       conf/libvirt_conf_la-checkpoint_conf.lo
In file included from
/usr/i686-w64-mingw32/sys-root/mingw/include/objbase.h:66,
                 from
/usr/i686-w64-mingw32/sys-root/mingw/include/ole2.h:17,
                 from
/usr/i686-w64-mingw32/sys-root/mingw/include/wtypes.h:12,
                 from
/usr/i686-w64-mingw32/sys-root/mingw/include/winscard.h:10,
                 from
/usr/i686-w64-mingw32/sys-root/mingw/include/windows.h:97,
                 from
/usr/i686-w64-mingw32/sys-root/mingw/include/winsock2.h:23,
                 from ../gnulib/lib/unistd.h:48,
                 from ./driver.h:24,
                 from ./datatypes.h:26,
                 from conf/checkpoint_conf.c:28:
/usr/i686-w64-mingw32/sys-root/mingw/include/objidl.h:12275:2: error:
expected identifier or '(' before string constant
 } DATADIR;
  ^~~~~~~~~
make[1]: *** [Makefile:10127: conf/libvirt_conf_la-checkpoint_conf.lo]
Error 1



And it's not the first time libvirt has run into this issue; I've found
the following commits in 2015 that worked around it:
https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=976abdf6
https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=bd205a90

Gnulib should work around this: perhaps "configmake.h" should include
<unistd.h> first when built on mingw, as that is sufficient to trigger
enough other headers to be included such that a later inclusion of
<winsock.h> after "configmake.h" no longer runs into an issue with the
DATADIR pollution breaking compilation, or perhaps gnulib can wrap
<winsock.h> in such a way that it no longer depends on a tag name
DATADIR.  In the meantime, I'll push an obvious fix to libvirt to
reorder the header inclusions to work around the problem.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: mingw pollution of DATADIR vs. configmake.h [was: [libvirt] [PATCH v10 08/19] backup: Parse and output checkpoint XML]

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2309 bytes --]

[dropping libvirt list for now...]

On 7/29/19 11:25 AM, Eric Blake wrote:
> [adding bug-gnulib]
> 
> On 7/24/19 12:55 AM, Eric Blake wrote:
>> Add a new file checkpoint_conf.c that performs the translation to and
>> from new XML describing a checkpoint. The code shares a common base
>> class with snapshots, since a checkpoint similarly represents the
>> domain state at a moment in time. Add some basic testing of round trip
>> XML handling through the new code.
> 
>> +++ b/src/conf/checkpoint_conf.c
> 
>> +#include <config.h>
>> +
>> +#include "configmake.h"
>> +#include "internal.h"
>> +#include "virbitmap.h"
>> +#include "virbuffer.h"
>> +#include "datatypes.h"
> 
> This causes a compilation failure on mingw, due to libvirt's
> "datatypes.h" including <winsock.h> after the point at which gnulib's
> "configmake.h" has already #define'd DATADIR into a string, but mingw's
> winsock.h tries to use DATADIR as a data-type tag name:
> 

> Gnulib should work around this: perhaps "configmake.h" should include
> <unistd.h> first when built on mingw, as that is sufficient to trigger
> enough other headers to be included such that a later inclusion of
> <winsock.h> after "configmake.h" no longer runs into an issue with the
> DATADIR pollution breaking compilation, or perhaps gnulib can wrap
> <winsock.h> in such a way that it no longer depends on a tag name
> DATADIR.

Why does including <unistd.h> first for libvirt work? Because
unistd.in.h has:

/* Get all possible declarations of gethostname().  */
#if @GNULIB_GETHOSTNAME@ && @UNISTD_H_HAVE_WINSOCK2_H@ \
  && !defined _GL_INCLUDING_WINSOCK2_H
# define _GL_INCLUDING_WINSOCK2_H
# include <winsock2.h>
# undef _GL_INCLUDING_WINSOCK2_H
#endif

and libvirt is using the gethostname module. But the namespace collision
problem is present whether or not the gethostname module is in use - it
is any mix-and-match of <winsock2.h> with "configmake.h".  So I'm not
immediately sure what the best gnulib patch should be (making the
configmake module depend on the unistd module seems like it is
insufficient if there are other ways that winsock2.h can be in use).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: mingw pollution of DATADIR vs. configmake.h [was: [libvirt] [PATCH v10 08/19] backup: Parse and output checkpoint XML]

[Bruno Haible]

Hi Eric,

> unistd.in.h has:
> 
> /* Get all possible declarations of gethostname().  */
> #if @GNULIB_GETHOSTNAME@ && @UNISTD_H_HAVE_WINSOCK2_H@ \
>   && !defined _GL_INCLUDING_WINSOCK2_H
> # define _GL_INCLUDING_WINSOCK2_H
> # include <winsock2.h>
> # undef _GL_INCLUDING_WINSOCK2_H
> #endif

When the application has requested the gnulib 'gethostname' module, it
means that the application wants to use a POSIX compatible gethostname()
function and not the original Windows gethostname().

Such an override logic does not apply for DATADIR. Asking for socket
functionality does not imply "I don't want to use the DATADIR macro",
and asking for the 'configmake' module does not imply "I want to hide
parts of the native Windows API". So, we cannot resolve the issue this way.

I can see two possible fixes:
  a) Change the 'configmake' module to define MAKEVAR_DATADIR instead of
     DATADIR.
  b) Document that "configmake.h" should be included after all the
     replacements of POSIX header files:
       #include <alloca.h>
       ...
       #include <wctype.h>
       #include "configmake.h"

And in fact, b) is already implemented since 2008! The module description says:

Include:
/* Include only after all system include files.  */
"configmake.h"

I'd suggest to add this recommendation also to doc/configmake.texi.

Bruno