970429 - Limit scope of initguid.h usage to avoid problems in unified sources builds.

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jacek Caban]

Attachment: fix

initguid.h is dangerous in unified builds, because it affects all subsequent includes that contain DEFINE_GUID. I have observed problems in widget/windows mingw builds so far, but we may want to take a look at other cases and perhaps even find a more generic solution. For the problem I've seen (multiple declarations), the attached patch is enough. initguid.h is not needed in nsToolkit.cpp AFAICT (I will push to try to make sure) and we may restore original DEFINE_GUID variant in nsTextStore.h.

Comment 1

[Jacek Caban]

https://tbpl.mozilla.org/?tree=Try&rev=ce9ae79f6f11

Comment 2

[:ehsan akhgari]

Do you have a link to the contents of the initguid.h file?

Comment 3

[Jacek Caban]

Not for MSVC version, that wouldn't be legal... But I may confirm that it does the same thing as mingw-w64 version:

http://repo.or.cz/w/mingw-w64/jacek.git/blob/f442bbe2ce894297df4f1c753e581bb978b40232:/mingw-w64-headers/include/initguid.h

So it's just two lines:
#define INITGUID
#include <guiddef.h>

The real work is done by guiddef.h:

http://repo.or.cz/w/mingw-w64/jacek.git/blob/f442bbe2ce894297df4f1c753e581bb978b40232:/mingw-w64-headers/include/guiddef.h#l50

Note that DEFINE_GUID is in a part of file that is not guarded, so multiple includes will override each other (that's what my patch uses).

Comment 4

[:ehsan akhgari]

Comment on attachment 8373464 [details] [diff] [review]
fix

Review of attachment 8373464 [details] [diff] [review]:
-----------------------------------------------------------------

I see, thanks for the explanation.

I think instead of doing a spot fix like this, you want to modify the include template here: <http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#654> and fail the build if a source file defines INITGUID.  Then based on the build failures resulting from that patch, take the source files which rely on that macro out of unified builds.

Comment 5

[Jacek Caban]

Attachment: Don't allow using INITGUID in unified sources.patch.diff

OK, here is a patch implementing that. It came out that INITGUID was quite abused in the tree. gfx/2d is a winner for enabling it in moz.build. Here is a try build (still running):

https://tbpl.mozilla.org/?tree=Try&rev=1413f85fc3d1

Comment 6

[:ehsan akhgari]

Comment on attachment 8374085 [details] [diff] [review]
Don't allow using INITGUID in unified sources.patch.diff

Review of attachment 8374085 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +661,5 @@
>                      'so it cannot be built in unified mode."\n'
>                      '#undef FORCE_PR_LOG\n'
> +                    '#endif\n'
> +                    '#ifdef INITGUID\n'
> +                    '#error "%(cppfile)s forces defines INITGUID, '

Nit: s/forces //

Comment 7

[Jacek Caban]

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6473daed318

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6473daed318