Closed
Bug 970429
Opened 7 years ago
Closed 7 years ago
Limit scope of initguid.h usage to avoid problems in unified sources builds.
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: jacek, Assigned: jacek)
References
Details
Attachments
(1 file, 1 obsolete file)
15.07 KB,
patch
|
ehsan
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•7 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ce9ae79f6f11
Comment 2•7 years ago
|
||
Do you have a link to the contents of the initguid.h file?
Flags: needinfo?(jacek)
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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-
Assignee | ||
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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+
Assignee | ||
Comment 7•7 years ago
|
||
Thanks for the review. https://hg.mozilla.org/integration/mozilla-inbound/rev/d6473daed318
Comment 8•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d6473daed318
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•