Last Comment Bug 661663 - Have to #define XP_WIN when building app with embedded SpiderMonkey JS engine
: Have to #define XP_WIN when building app with embedded SpiderMonkey JS engine
Status: RESOLVED FIXED
wanted-standalone-js, [inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla10
Assigned To: Jacek Caban
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-02 13:47 PDT by Mark Giffin [:markg]
Modified: 2011-10-15 05:41 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't use XP_WIN in jscpucfg.h (1.39 KB, patch)
2011-06-06 03:51 PDT, Jacek Caban
no flags Details | Diff | Splinter Review
fix v1.1 (1.56 KB, patch)
2011-10-13 09:12 PDT, Jacek Caban
benjamin: review+
Details | Diff | Splinter Review

Description Mark Giffin [:markg] 2011-06-02 13:47:46 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.71 Safari/534.24
Build Identifier: SpiderMonkey js185-1.0.0

Should not have to define XP_foo for your platform since about SM 1.8.1. But if I do not add #define XP_WIN to my app that has SM embedded, the build crashes with this error: 
c:\js-1.8.5\js\src\jscpucfg.h(83) : fatal error C1189: #error :  "Must define one of XP_BEOS, XP_OS2, XP_WIN, or XP_UNIX"

Reproducible: Always

Steps to Reproduce:
1. Include jsapi.h in an app to get SpiderMonkey.
2. Do not use #define XP_WIN
3. Build the app in MozillaBuild shell (I used cl.exe from VS 9)
4. Build crashes with the error that is given in Details field ("fatal error C1189" etc)

Actual Results:  
c:\js-1.8.5\js\src\jscpucfg.h(83) : fatal error C1189: #error :  "Must define one of XP_BEOS, XP_OS2, XP_WIN, or XP_UNIX"

Expected Results:  
No build errors

I'm using Windows XP Pro, MozillaBuild bash shell downloaded 4/24/11. 
Full code for my app: http://pastebin.mozilla.org/1240367
Comment 1 Wesley W. Garland 2011-06-02 19:21:58 PDT
Note that this bug affects only Windows, and means that we can't build Win32 embeddings without silly #ifdef hackery!  (Unless you're using a non-MS compiler, anyhow).

Here's why, from jstypes.h:

#ifdef _MSC_VER
# include "jscpucfg.h" /* We can't auto-detect MSVC configuration */
else
# include "jsautocfg.h" /* Use auto-detected configuration */
#endif 

The problem is that jscpucfg.h wants the XP_xxx define, rather than using the autoconf-ery. I don't really understand why.

This was introduced with Bug 475027.
Comment 2 Benjamin Smedberg [:bsmedberg] 2011-06-03 06:16:07 PDT
XP_WIN is defined by configure and ends up in mozilla-config.h. I believe the problem is that this affects the public API, and is therefore supposed to be in js-config.h.

I believe that if you add it to http://mxr.mozilla.org/mozilla-central/source/js/src/js-config.h.in you will be fine.
Comment 3 Jacek Caban 2011-06-06 03:51:31 PDT
Created attachment 537526 [details] [diff] [review]
Don't use XP_WIN in jscpucfg.h

I don't see any reason to use XP_WIN in jscpucfg.h. Checking for _WIN32 and _WIN64 should be enough. The attached (untested!) patch should fix the problem.

BTW, I think that better solution would be fixing generating jsautocfg.h by configure script so that we could get rid of the whole jscpucfg.h.
Comment 4 Wesley W. Garland 2011-10-09 16:24:00 PDT
Jacek, would you be willing to test and r? this patch?
Comment 5 Jacek Caban 2011-10-13 09:12:27 PDT
Created attachment 566857 [details] [diff] [review]
fix v1.1

Sure, but currently my Windows VM is broken, so I can't test standalone build. The fix is, however, very straightforward, so I'm confident about this patch (and may post to try before committing). The attached version cleans up #ifdefs a bit more than previous one. It mostly removes useless #ifdefs that just duplicate the logic.
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-10-13 09:52:39 PDT
Comment on attachment 566857 [details] [diff] [review]
fix v1.1

Needs to pass a tryserver run before landing...
Comment 7 Jacek Caban 2011-10-14 04:20:12 PDT
Thanks for review. Passed on try, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1491a374c1a6
Comment 8 Ed Morley [:emorley] 2011-10-15 05:41:50 PDT
https://hg.mozilla.org/mozilla-central/rev/1491a374c1a6

Note You need to log in before you can comment on or make changes to this bug.