After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 646369 - Support compilation with standalone mozjs185 release
Support compilation with standalone mozjs185 release
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks: 636977
 
 
Reported: 2011-03-31 18:47 UTC by Colin Walters
Modified: 2011-05-04 19:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Support compilation with standalone mozjs185 release (7.52 KB, patch)
2011-03-31 18:47 UTC, Colin Walters
none Details | Review
Adapt to change in JSScript API from tracemonkey tree (1.79 KB, patch)
2011-04-14 21:43 UTC, Colin Walters
none Details | Review
Support compilation with standalone mozjs185 release (11.23 KB, patch)
2011-04-14 21:43 UTC, Colin Walters
none Details | Review
Support compilation with standalone mozjs185 release (11.73 KB, patch)
2011-05-04 18:19 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2011-03-31 18:47:23 UTC
First, adjust the detection logic so that we look for
mozjs-185.pc first.  If we have this, we can skip all kinds
of insanity.
See https://bugzilla.mozilla.org/show_bug.cgi?id=628723
for the discussion about creating this release.

Also, the JSScript API changed, see:
See https://bugzilla.mozilla.org/show_bug.cgi?id=630209
Comment 1 Colin Walters 2011-03-31 18:47:25 UTC
Created attachment 184810 [details] [review]
Support compilation with standalone mozjs185 release
Comment 2 Christopher Aillon 2011-04-13 01:03:55 UTC
Review of attachment 184810 [details] [review]:

Fedora's standalone js package doesn't provide js-config but instead ships a libjs.pc, which this wouldn't catch.
Comment 3 Owen Taylor 2011-04-13 18:03:10 UTC
Review of attachment 184810 [details] [review]:

Sort of torn here, I understand what you are trying to do here - add another option without disturbing the existing logic which is all magical and about weird old bugs. But I hate the state that this patch leaves the configure.ac:

 - We check for mozjs185 via pkg-config
 - We check for js-config, which we don't know where it comes from
 - We check for mozilla-js via pkg-config
 - We check for xulrunner-js, which is documented in comments to be too old to work via pkg-config
 - We check for firefox-js which we don't even know who uses via pkg-config

I guess it's OK like this - probably we should rewrite when removing support for xulrunner-1.9

::: configure.ac
@@ +96,3 @@
+PKG_CHECK_EXISTS([mozjs185], JS_PACKAGE=mozjs185,)
+if test x$JS_PACKAGE != x; then
+    FIREFOX_JS_LIBDIR=`$PKG_CONFIG --variable=libdir $JS_PACKAGE`

Does the standalone mozjs really install with a weird libdir instead of just being a system library? And shouldn't this patch remove the rpath hacks if it is?

@@ +108,2 @@
 # Look for Spidermonkey. If js-config exists, use that;
 # otherwise we try some pkgconfig files from various distributions.

this comment is now broken by your changees

@@ +117,1 @@
 if test -n "$JS_CONFIG"; then

I think you need to byte the bullet and reindent (M-4 M-x indent-rigidly) - shell code is hard enough to understand without misplaced indentation.

@@ +177,3 @@
 
+if test x$MOZJS_IS_STANDALONE != xyes; then
+    AC_MSG_CHECKING([for mozilla-js >= 1.9.2 ])

Somewhere you need a comment explaining the confusing versioning discrepancy between mozjs and mozilla-js.

@@ +189,3 @@
+AC_CHECK_LIB([$MOZJS_LIB], [JS_GetStringChars], AC_DEFINE([HAVE_JS_GETSTRINGCHARS], [1], [Define if we still have JS_GetStringChars]),, [$JS_LIBS])
+AC_CHECK_LIB([$MOZJS_LIB], [JS_StrictPropertyStub], AC_DEFINE([HAVE_JS_STRICTPROPERTYSTUB], [1], [Define if we have JS_StrictPropertyStub]),, [$JS_LIBS])
+AC_CHECK_LIB([$MOZJS_LIB], [JS_GetGlobalForScopeChain], AC_DEFINE([HAVE_JS_GETGLOBALFORSCOPECHAIN], [1], [Define if we have JS_GetGlobalForScopeChain]),, [$JS_LIBS])

For future reference, the preferrable way to do this is shown in the gnome-shell configure.ac:

saved_CFLAGS=$CFLAGS
saved_LIBS=$LIBS
CFLAGS=$GNOME_SHELL_CFLAGS
LIBS=$GNOME_SHELL_LIBS
AC_CHECK_FUNCS(JS_NewGlobalObject sn_startup_sequence_get_application_id XFixesCreatePointerBarrier)
CFLAGS=$saved_CFLAGS
LIBS=$saved_LIBS

@@ +216,2 @@
 AC_MSG_CHECKING([if SpiderMonkey needs extra compiler flags])
 save_CFLAGS="$CFLAGS"

Again, have to reindent - no choice.

*But* I don't see why you are if'ing this out. It looks like it will work fine, and the extra few seconds of compilation time aren't worth creating untested code paths in the configure.
Comment 4 Colin Walters 2011-04-14 21:00:51 UTC
(In reply to comment #3)
> Review of attachment 184810 [details] [review]:

> I guess it's OK like this - probably we should rewrite when removing support
> for xulrunner-1.9

It will be a lot cleaner when I drop support for compiling against Firefox/XULRunner =)

> Does the standalone mozjs really install with a weird libdir instead of just
> being a system library? And shouldn't this patch remove the rpath hacks if it
> is?

Well...setting the rpath doesn't hurt; build systems can strip it out.  I agree it'd be nice to avoid but it would make a bigger mess of the build system for little gain.

> I think you need to byte the bullet and reindent (M-4 M-x indent-rigidly) -
> shell code is hard enough to understand without misplaced indentation.

Done.

> Somewhere you need a comment explaining the confusing versioning discrepancy
> between mozjs and mozilla-js.

So what I ended up doing with all of this is reordering things so that all the gunk for firefox/xulrunner ends up under one single if, rather than having it scattered.  This works better.
Comment 5 Owen Taylor 2011-04-14 21:32:57 UTC
Did you mean to attach a new version?
Comment 6 Colin Walters 2011-04-14 21:43:00 UTC
Created attachment 185984 [details] [review]
Adapt to change in JSScript API from tracemonkey tree

See https://bugzilla.mozilla.org/show_bug.cgi?id=630209
Comment 7 Colin Walters 2011-04-14 21:43:06 UTC
Created attachment 185985 [details] [review]
Support compilation with standalone mozjs185 release

Adjust the detection logic so that we look for mozjs-185.pc first.  If
we have this, we can skip all kinds of insanity.

See https://bugzilla.mozilla.org/show_bug.cgi?id=628723
for the discussion about creating this release.
Comment 8 Colin Walters 2011-05-04 18:19:37 UTC
Created attachment 187223 [details] [review]
Support compilation with standalone mozjs185 release

Rebased on git master
Comment 9 Colin Walters 2011-05-04 19:42:56 UTC
Attachment 187223 [details] pushed as c3f95ea - Support compilation with standalone mozjs185 release