User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:188.8.131.52) Gecko/20071008 Firefox/184.108.40.206 Build Identifier: jsapi.h head 3.162 jsapi.h now depends on jsutil.h. jsutil.h is not published to dist/include during make export. This breaks all embeddings which do not -I include the SpiderMonkey source directory in their CPPFLAGS. I believe the last version which did not depend on jsutil.h was 3.142. 3.143 was checked in to fix bug 375270. (2007 04 17) The reason jsutil.h was added seems to be to provide the JS_STATIC_ASSERT macro. Use of this macro is not limited (at least in the current jsapi.h) to conditional code. Reproducible: Always Steps to Reproduce: 1. Embed spidermonkey in a C program 2. Try to compile your program 3. Observe that it does not compile, but rather, complains that it cannot find jsutil.h Expected Results: Build with error. Suggested possible fixes: 1 - Add jsutil.h to the export list in Makefile.ref / Makefile.in 2 - Add JS_STATIC_ASSERT macro to jsapi.h 3 - Remove references to JS_STATIC_ASSERT from jsapi.h I have worked around this problem using both methods 1 and 2 without issue. Until then, new embedders pulling from CVS will be unable to build SpiderMonkey.
I've whinged before about not putting JS_STATIC_ASSERTS in header files -- it's a modularity killer. Igor, can you land the patch? /be
Status: UNCONFIRMED → NEW
Ever confirmed: true
I mean take this bug and patch it, of course. Preemptive r+a=me. On the right possible fix, 1 is out for now (we liberally export but I do not think jsutil.h should be nested in jsapi.h); 2 is possible but breaks symmetry with file housing JS_ASSERT; 3 is what we've done elsewhere: put JS_STATIC_ASSERT calls in a canonical (for that assertion) .c file. /be
Created attachment 298738 [details] [diff] [review] Proposed patch which fixes jsapi.h and dependencies Here is a proposed patch against a recent trunk checkout which fixes the problem for me. The changes I made were to remove jsutil.h from jsapi.h, then move some static asserts out of jsapi.h and jsatom.h to allow Spidermonkey and trivial embeddings to build without publishing jsutil.h. (i.e., option 3 in my initial bug report). The following JS_STATIC_ASSERTs were moved: - JS_TRACE_KIND from jsapi.h to jsgc.c - sizeof(JSAtom *) from jsatom.h into jsatom.c - sizeof(JSHashNumber) removed from jsatom.h, already in jsscope.c - ATOM_OFFSET_START and ATOM_OFFSET_LIMIT from jsatom.h into jsatom.c - offsetof(JSAtomState, typeAtoms) and booleanAtoms from jsatom.h into jsatom.c Note: Related to this bug, the following files in dist/include still depend on jsutil.h: jsbit.h, jscntxt.h, jsgc.h, jsopcode.h, jsotypes.h. Note 2: Related to this bug, the following files in dist/include still have JS_STATIC_ASSERT statements: jsbit.h, jscntxt.h, jsgc.h, jsopcode.h Open question -- should those files also be static-assertion-cleaned at a later date? Or should they be removed from dist/include? (They do not appear, at a casual glance, to be dependencies of jsapi.h)
Assignee: general → wes
Status: NEW → ASSIGNED
Attachment #298738 - Flags: review?
Attachment #298738 - Flags: review? → review?(igor)
Comment on attachment 298738 [details] [diff] [review] Proposed patch which fixes jsapi.h and dependencies >Index: jsatom.c ... >+JS_STATIC_ASSERT(1 * sizeof(JSAtom *) == >+ offsetof(JSAtomState, typeAtoms) - ATOM_OFFSET_START); >+JS_STATIC_ASSERT((1 + JSTYPE_LIMIT) * sizeof(JSAtom *) == >+ offsetof(JSAtomState, booleanAtoms) - ATOM_OFFSET_START); >+ >+JS_STATIC_ASSERT(sizeof(JSAtom *) == JS_BYTES_PER_WORD); > JS_STATIC_ASSERT(JS_ARRAY_LENGTH(js_common_atom_names) * sizeof(JSAtom *) == > LAZY_ATOM_OFFSET_START - ATOM_OFFSET_START); > These four asses require a comment about their relation with js_common_atom_names. >Index: jsatom.h >@@ -282,21 +275,16 @@ extern const char *const js_common_atom_ > /* > * Macros to access C strings for JSType and boolean literals together with > * checks that type names and booleans starts from index 1 and 1+JSTYPE_LIMIT > * correspondingly. > */ > #define JS_TYPE_STR(type) (js_common_atom_names[1 + (type)]) > #define JS_BOOLEAN_STR(type) (js_common_atom_names[1 + JSTYPE_LIMIT + (type)]) The comments should refer to the static checks in jsatom.c.
"[12:54] <jimb> Wes: jsutil.h is installed these days." New build system fixes this -- bug 97954
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
fix for 1.9.0 SpiderMonkey 1.8.0
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Version: unspecified → 1.9.0 Branch
Created attachment 362586 [details] [diff] [review] v1 This is the one-line patch to put a copy of jsutil.h in dist/include when the user does `make -f Makefile.ref export`. r?brendan because Brendan didn't want to fix it this way originally. We can fix it in a different way for 1.8.1. If desired, I can specifically release-note that JS_STATIC_ASSERT is not an API and will go away in 1.8.1. I just want to do a 1.8 release embedders can use.
Attachment #362586 - Flags: review?(brendan) → review+
Comment on attachment 362586 [details] [diff] [review] v1 Requesting approval220.127.116.11. This is NPOTB and the bug blocks the official SpiderMonkey 1.8 source release.
Attachment #362586 - Flags: approval18.104.22.168?
Comment on attachment 362586 [details] [diff] [review] v1 Approved for 22.214.171.124, a=dveditz for release-drivers
Attachment #362586 - Flags: approval126.96.36.199? → approval188.8.131.52+
attachment 362586 [details] [diff] [review] checked into 1.9.0 /cvsroot/mozilla/js/src/Makefile.ref,v <-- Makefile.ref new revision: 3.54; previous revision: 3.53
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago → 9 years ago
Resolution: --- → FIXED
v 1.9.0 dist/include/js/jsutil.h now included.
Status: RESOLVED → VERIFIED
Keywords: fixed184.108.40.206 → verified220.127.116.11
You need to log in before you can comment on or make changes to this bug.