835551 - Build failure due to missing define of UINT32_MAX

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[darkxst]

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?

Comment 1

[Jeff Walden [:Waldo]]

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.

Comment 2

[darkxst]

-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.

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch

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.

Comment 5

[Jeff Walden [:Waldo]]

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 6

[darkxst]

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.

Comment 7

[Masatoshi Kimura [:emk]]

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...)

Comment 8

[Jeff Walden [:Waldo]]

(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.

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Jeff Walden [:Waldo]]

Sean, see comment 9.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/38e92a7b5bf4

Comment 12

[Sean Stangl [:sstangl]]

(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.