Closed
Bug 835551
Opened 12 years ago
Closed 12 years ago
Build failure due to missing define of UINT32_MAX
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: darkxst, Assigned: Waldo)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
ted
:
review+
darkxst
:
review+
|
Details | Diff | Splinter Review |
Building gjs/gnome-shell against an esr17 snapshot I get the following build failure.
make all-am
make[1]: Entering directory `/usr/local/src/jhbuild-checkout/source/gjs'
CXX libgjs_la-jsapi-private.lo
In file included from /opt/gnome/include/js/jsprvtd.h:28:0,
from /opt/gnome/include/js/jsdbgapi.h:14,
from ./gjs/compat.h:36,
from gjs/jsapi-util.h:31,
from gjs/jsapi-private.cpp:30:
/opt/gnome/include/js/HashTable.h: In static member function 'static void js::detail::HashTable<T, HashPolicy, AllocPolicy>::staticAsserts()':
/opt/gnome/include/js/HashTable.h:293:9: error: 'UINT32_MAX' was not declared in this scope
make[1]: *** [libgjs_la-jsapi-private.lo] Error 1
make[1]: Leaving directory `/usr/local/src/jhbuild-checkout/source/gjs'
make: *** [all] Error 2
As a hacky workaround, I just added a "#define UINT32_MAX 4294967295U" to HashTable.h, which gets things building, but I figure there is probably a better way to fix this?
Assignee | ||
Comment 1•12 years ago
|
||
SpiderMonkey's build processes include a command-line -D__STDC_LIMIT_MACROS (well, I think it might be -include fileThatIncludesThatDefine.h, but whatever) so that these macros are always available.
If we're using those macros in headers now -- and it looks like we are -- we may have silently started requiring the same of our users. (Hmm, and I guess we're requiring the same thing for -Wno-c++0x-extensions wrt using the Attributes.h macros like MOZ_OVERRIDE and such.)
I'm not sure what the exact right solution, to fix everyone, and to accommodate our adding more such auto-#defines like this, would be. Maybe you have some ideas.
In the meantime, adding -D__STDC_LIMIT_MACROS to your compile lines should fix things here.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: sm-embedding
-D__STDC_LIMIT_MACROS is defined in js-confdefs.h, however this header is not installed, additionally it contains a lot of autogenerated defines, that wouldnt want to be installed into a public/internal header.
Waldo suggested on IRC to split out the stuff that is required for an embedded build, into a separate header.
Assignee | ||
Comment 3•12 years ago
|
||
Just talked to ted in-person about this, sounds like he's good with this plan of having two separate -includes for the configury stuff and for the constant #defines.
Assignee | ||
Comment 4•12 years ago
|
||
I think this'll do the trick for JS. It seems to compile a browser far enough that I don't think it's buggy on that front, and shell builds seem equally good.
Interestingly I realized Mozilla-as-embedder is weird yet again, in that the only reason this stuff works for us is that our mozilla-config.h just *happens* to define the same things that RequiredDefines.h defines. I'm not sure of a way around this. But it doesn't matter for this immediate bug, or for the present.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #711138 -
Flags: review?(ted)
Attachment #711138 -
Flags: review?(darkxst)
Assignee | ||
Comment 5•12 years ago
|
||
And of course with that patch, you'd add -include js/RequiredDefines.h instead, and you'd pick up other #define requirements we'll grow in the future (like __STDC_FORMAT_MACROS at some point soon).
Comment on attachment 711138 [details] [diff] [review]
Patch
Review of attachment 711138 [details] [diff] [review]:
-----------------------------------------------------------------
sure, that works for me.
I added the -include via the package config file (js.pc.in), which works fine for gjs.
Attachment #711138 -
Flags: review?(darkxst) → review+
Comment 7•12 years ago
|
||
Comment on attachment 711138 [details] [diff] [review]
Patch
>+#ifndef js_RequiredDefines_h___
>+#define js_RequiredDefines_h___
Don't use consecutive underscores in a new header.
(I'm inclined to file a bug to cleanup these spec violations...)
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #7)
> Don't use consecutive underscores in a new header.
> (I'm inclined to file a bug to cleanup these spec violations...)
Fair enough. It does happen to be the existing style, for better or worse. And yes, file the bug.
Updated•12 years ago
|
Attachment #711138 -
Flags: review?(ted) → review+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/38e92a7b5bf4
This is one of the patches we'll want to backport for a 17-based release, Sean. We'll also need to tell embedders to add js/RequiredDefines.h to their command lines, although the plan is for pkgconfig or whatever to spit it out as part of js-cflags somehow, so hopefully many won't need to take special steps here.
The consecutive-underscores thing I left for the followup bug, at least partly because I thought I'd changed it in the patch already when I pushed, but now I look back and see I didn't change it. Shouldn't be any real harm, probably, and the followup bug can handle it easily enough.
Target Milestone: --- → mozilla21
Assignee | ||
Comment 10•12 years ago
|
||
Sean, see comment 9.
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 12•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> This is one of the patches we'll want to backport for a 17-based release,
> Sean. We'll also need to tell embedders to add js/RequiredDefines.h to
> their command lines, although the plan is for pkgconfig or whatever to spit
> it out as part of js-cflags somehow, so hopefully many won't need to take
> special steps here.
Just for note, the pkgconfig changes are being tracked as part of Bug 812265, which is in a state of convoluted mess at the moment.
You need to log in
before you can comment on or make changes to this bug.
Description
•