Have to #define XP_WIN when building app with embedded SpiderMonkey JS engine

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: markg, Assigned: Jacek Caban)

Tracking

Trunk
mozilla10
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: wanted-standalone-js, [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
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

6 years ago
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.

Updated

6 years ago
Whiteboard: wanted-standalone-js
(Assignee)

Comment 3

6 years ago
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

6 years ago
Jacek, would you be willing to test and r? this patch?
(Assignee)

Comment 5

6 years ago
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.
Assignee: general → jacek
Attachment #537526 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #566857 - Flags: review?(benjamin)

Comment 6

6 years ago
Comment on attachment 566857 [details] [diff] [review]
fix v1.1

Needs to pass a tryserver run before landing...
Attachment #566857 - Flags: review?(benjamin) → review+
(Assignee)

Comment 7

6 years ago
Thanks for review. Passed on try, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1491a374c1a6
Whiteboard: wanted-standalone-js → wanted-standalone-js, [inbound]

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1491a374c1a6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.