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

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jacek, Assigned: jacek)

Tracking

Trunk
mozilla30
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch fix (obsolete) — Splinter Review
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.
Attachment #8373464 - Flags: review?(ehsan)
Do you have a link to the contents of the initguid.h file?
Flags: needinfo?(jacek)
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).
Flags: needinfo?(jacek)
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.
Attachment #8373464 - Flags: review?(ehsan) → review-
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
Attachment #8373464 - Attachment is obsolete: true
Attachment #8374085 - Flags: review?(ehsan)
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 //
Attachment #8374085 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/d6473daed318
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 977279
You need to log in before you can comment on or make changes to this bug.